Add possibility to disable the reload or restore of iptables

[stevedomin]

At the end of a chef run.

This is useful in some situations where another service is adding rules to iptables and you actually want to control when the rules added by Chef are actually applied on the server (if you are using Docker for instance).

[chr4]

Thanks for your contribution!

The pull request looks good to me, I'm not entirely sure though whether it's a good idea to have two applications maintain iptables rules. I encountered your problem myself before when using Docker. It feels a little hacky though, that Docker changes the rules on runtime and Chef just doesn't care when rules are changed (this seems to me to be against the Chef principle of having a consistent state).

This could be a useful workaround, leaving the decision to the (hopefully reasonable) admin. Any feedback from someone else on this?

[stevedomin]

Hi Chris,

I'm entirely sure it's not a good idea to have two applications maintaining iptables :)

We've found a workflow that will work for us while we figure out a better solution.

[chr4]

Good to hear you found a solution! Would you mind sharing it, if applicable?

[stevedomin]

I wouldn't really call it a solution.

When Chef runs it creates the new rules file. After having migrated all the docker containers away from that node (we use Mesos and Marathon so it's fairly easy) we stop docker, apply the new iptables config using the rule file generated by Chef and then restart docker again.

It's really a workaround but we don't change iptables rules that often so it works.

[nathwill]

this seems like a reasonable hook for folks with special needs. we also suffered with docker for a while, but eventually disabled docker iptables management and set up our own rules. it was definitely painful having to chain docker service restarts to iptables reloads/restarts before we finally took over.

since the PR doesn't change the more reasonable default behavior, and just adds some flexibility, i'm :+1: on it.

[stevedomin]

@chr4 do you want more feedback before merging this?

[chr4]

The more feedback the better!

My current consideration is to merge this but not officially documenting it in the README. Maybe this is a way to allow users to use the feature, but prevent users from accidently messing around with their servers firewall configuration? We can later document it more openly, in case needed.

[chr4]

Maybe adding a "be careful, might leave your servers in an unconsistent state, only use it if you know what you're doing" kinda note to the CHANGELOG or as a code comment in attributes/defaults.rb...

[stevedomin]

Just added a comment to the attributes file that gives more context around this change.

[stevedomin]

I'm happy with it not being documented, people who really need it will find it pretty easily anyway.

[chr4]

I agree. Ok, will merge this. If there's any serious trouble in the future, we can just revert it. In the meantime this might be helping to workaround issues with Docker.

Thanks again!

[chr4]

(Once we decide to document the feature, we should also include a test)