make cleanup an opt-in

[nathwill]

i'm all for this new behavior, but it's a bit contrary to how Chef normally behaves, and has a strong probability of catching people off-guard. this would make the new behavior an opt-in.

[chr4]

Hi! I agree, auto-purging kind of is not the default Chef behaviour. I personally assemble iptables rules over multiple cookbooks (e.g some base rules, and then specific nginx rules in a cookbook that configures nginx), and as far as I can see, the "new behaviour" would crash that. I won't release the new version until we tested things like that.

I can also understand though, that having old rules in /etc/iptables.d is error prone and probably not what people usually want.

I'd like to bring this into discussion, so please share your thoughts! My current feeling is using an 'opt-in' with a strong disclaimer in the README.

[nathwill]

i think if we want to achieve the underlying goal (not having legacy rules still applied because the files are left behind), it would be less error prone to change how the cookbook currently works.

i.e. instead of globbing files from /etc/iptables.d, it could crawl the resource list at the end of a run in order to build the combined rules file (e.g. /etc/sysconfig/iptables).

i did something similar with many resources combining to generate a conglomerate resource with the haproxy-ng cookbook that might apply.

[chr4]

Referencing: https://github.com/chr4-cookbooks/iptables-ng/pull/45 @chewi I invite you to join, as the patch was your idea.

[chr4]

@nathwill can you point us to the implementation in your cookbook?

[nathwill]

@chr4 yeah, no problem. these three spots are the main part of the implementation that i was referencing. i think for the "many cookbooks combining to form a conglomerate rule-set" thing you mentioned (i do it this way too), it'd probably be better to do a full search of the resource list rather than what the haproxy-ng cookbook does (have the contributory resources be specifically enumerated). there's some other reasons to do that for haproxy, such as having the same proxy be part of multiple instances... but those reasons aren't relevant here.

https://github.com/nathwill/chef-haproxy-ng/blob/master/recipes/default.rb#L21-L29 https://github.com/nathwill/chef-haproxy-ng/blob/master/libraries/chef_haproxy_instance.rb#L136-L154 https://github.com/nathwill/chef-haproxy-ng/blob/master/libraries/haproxy_helpers.rb#L28-L42

if this sounds reasonable to you, i'd be happy to start working on a PR (though probably not this weekend, got a wedding to go to :dancer: ).

[chewi]

I was prepared to make it opt-in if anyone asked. This patch looks good to me. I think you may be misunderstanding how the change works though. It shouldn't matter whether you assemble the rules over multiple cookbooks or not. This only affects attribute-based rules, as @chr4 helpfully added a suffix to those, and those rules are only processed once per Chef client run. I'll review the above now to see whether that might be safer somehow.

[chewi]

i.e. instead of globbing files from /etc/iptables.d, it could crawl the resource list at the end of a run in order to build the combined rules file (e.g. /etc/sysconfig/iptables).

I believe this is what @chr4 said he didn't want to do in #41 as you would end up rewriting the whole file every single time instead of only when something changes.

[nathwill]

@chewi ah, good ref, i'd missed that conversation.

no way so far to cleanly check whether the iptables-restore files are up-to-date without having to concatinate the files in /etc/iptables.d/ over and over again on each Chef run.

pretty sure i could manage that by pulling the resources out of the resource_collection instead of concatenating, but code speaks louder than guesses, so i'll go ahead and test it out to see if i can pull it off, and then we'll be talking about an actual implementation instead of abstractions.

[chewi]

@nathwill, I've reread the above and now understand that the globbing you mentioned was in building the files, not deleting them. Still, looking at the resource_collection would only tell you what the rules should be now and not what they were before so if you don't check for the presence of individual rule files, the only way to tell if the combined file is up-to-date is to generate a new one. It wouldn't necessarily have to write to disk as the file would be constructed in memory, just as it is now, but it still seems a tad inefficient. You could prove me wrong but I'm not yet convinced that there's a problem with my approach. Chef will only execute a recipe once per run.

[nathwill]

@chewi I agree, your approach is a good way without overhauling everything, but I do think it should be an opt-in.

[chr4]

@chewi Ah, I indeed missed that your patch only takes care of rules created from attributes. Would you mind adding some documentation to the README on howto activate (I'm currently favoring the opt-in approach) and what implications and dangers might be involved?

[chr4]

A suggestion: Wouldn't it make it more clear if we name the opt-in-attribute in a way that makes it clear that only attribute rules are purged? e.g. something like default['iptables-ng']['cleanup_old_attribute_rules']

[chewi]

I was thinking something a little more catchy like auto_prune but clear is good!

[nathwill]

good idea! updated the attr name

[chr4]

I like auto_prune_attribute_rules. If you change this, I'll merge this or now, so in case someone installs this cookbook from git, it won't mess up things. We need some documentation later for this feature, though.

[chewi]

Yep, I'll get something written up as soon as I can.

[nathwill]

attr name updated