Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wild mixtures of questions #249

Open
a7b81a9086 opened this issue Apr 12, 2022 · 5 comments
Open

Wild mixtures of questions #249

a7b81a9086 opened this issue Apr 12, 2022 · 5 comments

Comments

@a7b81a9086
Copy link
Contributor

This might become a little messy, but I have the feeling that I now have sufficient understanding of this cookbook, to ask some questions:

  1. Why are there two separate providers for ubuntu <= 14.04 and ubuntu > 14.04? They online differ in the service name (netfilter-persistent vs iptables-persistent). This could be fixed easily by adding a method iptables_servicename to libraries/helpers_iptables which does the distinction based on the method value_for_platform.
  2. Could the same be done for libraries/provider_firewall_iptables.rb and libraries/provider_firewall_iptables_ubuntu.rb? This will require more work, but it should still be possible.
  3. In general all the resources and providers would be way more readable if they were rewritten as "new" custom resources using the edit_resource methods.
  4. Why do firewall_rule-resource search the resource_collection for firewall['default'], which then again searches the resource_collection for all firewall_rules to accumulate all the rules? This should be possible by adding all the rules to the firewall resource from the firewall_rules directly without needing to search the resource-collection again. However this would be quite a lot of work.
  5. The firewalld-cookbook can be adopted. It offers many features that could be used in this cookbook, and it would be a better way to handle firewalld with this cookbook. Right now, if one were to use this cookbook to configure firewalld on a more recent distribution, this cookbook would generate iptables-rules via firewalld direct interface, which would result in iptables-rules on the OS which would then be converted to nftables.
@Stromweld
Copy link
Contributor

This is an old cookbook that needs a lot of updating to be modernized per resource cookbook driven designs. With that old OSs' like Ubuntu 14.04 that are EOL support could be dropped further simplifying this cookbook. There has been talks about whether this cookbook should be updated or split up into seperate cookbooks based on the firewall type. 1 cookbook for all firewalls is very ambitious and difficult to support. At the very least separating out each firewall into separate resources will make it easier to manage and maintain and moves onus as to which firewall resource to use on to the users wrapper cookbook and out of this cookbooks logic.

@a7b81a9086
Copy link
Contributor Author

Ok, that is useful information. It also leads to many more questions:

  1. I am strongly in favor of removing support for ubuntu 14.04. Is it ok to just make a PR to remove it, or should a deprecation warning come first?
  2. Splitting up support to separate cookbooks would also mean:
    1. Remove support for firewalld, as there already is a better cookbook for that
    2. ufw support also does not fit in, since it is another firewall that offers significantly more features than plain iptables/nftables and will thus be difficult to support.
    3. I haven't looked at windows. Maybe it can also be moved to a separate cookbook.
    4. This cookbook could be renamed to netfilter, as it then supports only iptables and nftables. nftables and iptables practically only differ in syntax, which means that supporting them in the same cookbook should be possible.
  3. What do you mean by separating out each firewall into separate resources? The firewall resources are already separated, and I am not sure if there is any benefit for doing the same for firewall_rule resources. The dance which firewall resource to use is not very funny right now. From a user perspective it would be way simpler to just choose the firewall by e.g. node['firewall']['iptables'] instead of node['firewall']['iptables_ubuntu'] but only for versions <= 14.04, so I would try to support iptables on any requested platform with just one firewall-resource. Or drop iptables support all together, as most many distributions moved to nftables as default.

Again, feedback on my thoughts is very welcome.

@Stromweld
Copy link
Contributor

  1. I wouldn't open a PR to solely remove it if it's working with current code, but if you do other updates and or refactors it'd make sense to me to remove it then.
  2. Essentially if we split this into multiple cookbooks for each firewall type named after the firewall type this one would get deprecated and moved to the boneyard.
  3. Proper resource driven cookbooks don't have attributes, recipes, and everything isn't in the libraries folder. This one they prefixed several libraries with resource_ but they aren't the new resource design standard for cookbooks that came years after the CB was created. See https://github.com/sous-chefs/java for good examples of resource driven cookbook design.

@a7b81a9086
Copy link
Contributor Author

Thanks for your patience, it is all starting to make sense now. Since the first version of #242, I had already familiarized myself with the new resource design. So I just updated #242:

  1. Use the new resource design
  2. Use separate resource names, namely nftables and nftables_rule
  3. Avoid the use of attributes. This made things a lot easier and allowed for a lot of cleanup.

Once again, I would very much welcome any feedback on whether I actually got it right.

@Stromweld
Copy link
Contributor

looks good just have to fix the failing pipeline jobs and should be ready to be approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants