[WIP] Add optional "safe-reload" mode

[nathwill]

related to #34

[nathwill]

ran out of time to work on this tonight, but the only thing left is some testing, as this appears to be working correctly now. will hopefully wrap it up this weekend

[chr4]

Thanks for the pull-request! I added some questions/ notes to the commits, would welcome feedback!

[chr4]

Thanks for the rebase. Let me know once you're finished with testing and I can review it.
Most available tests should check for the basic functionality, what's missing would be a negative test - Break the chef run and then verify that the rules are not applied.
Running the tests is a pain currently, though, as it requires dozens of virtual machine spawns/ runs.

[chr4]

This doesn't work, as node is not available when running the tests here.

NameError: undefined local variable or method `node' for #<#<Class:0x000000050b92f8>:0x000000050b91e0>
/tmp/kitchen/cache/cookbooks/iptables-ng/recipes/manage.rb:45:in `block (2 levels) in from_file'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dsl.rb:53:in `instance_exec'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dsl.rb:53:in `block in on'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dispatcher.rb:38:in `call'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dispatcher.rb:38:in `block in call_subscribers'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dispatcher.rb:29:in `each'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/event_dispatch/dispatcher.rb:29:in `call_subscribers'
(eval):2:in `converge_complete'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/client.rb:654:in `block in converge'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/client.rb:648:in `catch'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/client.rb:648:in `converge'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/client.rb:687:in `converge_and_save'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/client.rb:269:in `run'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:270:in `block in fork_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:258:in `fork'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:258:in `fork_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:224:in `block in run_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/local_mode.rb:44:in `with_server_connectivity'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:212:in `run_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application/client.rb:408:in `block in interval_run_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application/client.rb:398:in `loop'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application/client.rb:398:in `interval_run_chef_client'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application/client.rb:388:in `run_application'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/lib/chef/application.rb:60:in `run'
/opt/chef/embedded/lib/ruby/gems/2.1.0/gems/chef-12.5.1/bin/chef-client:26:in `<top (required)>'
/opt/chef/bin/chef-client:54:in `load'
/opt/chef/bin/chef-client:54:in `<main>'

[nathwill]

thanks! just now able to get back to this, i'll get it fixed up.

[nathwill]

Running the tests is a pain currently, though, as it requires dozens of virtual machine spawns/ runs.

I've got some free time in the coming weeks (yay EoY PTO burn-down..), and I'd love to help make this better. Are you attached to minitest-handler, or would some test suite rewrites that kicked things over to chefspec and serverspec be something you'd be interested in?

[nathwill]

Break the chef run and then verify that the rules are not applied.

This would sure be nice, but as far as I can see (and folks on IRC confirm) you can't actually do this yet 😭. Looks like test-kitchen has a feature-request open for it, so hopefully this'll eventually be doable.

FWIW, the logic here is pretty straightforward, if an exception is raised during converge, chef-client never reaches #converge_complete and runs #converge_failed in the rescue instead. I'm fairly sure they won't change that, and even if they decided to move #converge_complete into an ensure block, it should just revert to the previous "always run" behavior, and not introduce any new/weird failures.

[nathwill]

OK, this seems to be working as expected at this point; the one thing I think could still be improved is to carry some kind of indicator over between runs when a chain/rule resource has been updated, but the create_rules or restart_iptables actions haven't been run yet; this way we'd catch iptables-resource updates from a previous failed run and be able to apply them on the next successful converge.

Does that sound reasonable? If you think this sounds alright, I'll add something in.

[chr4]

I'm not at all attached to minitest! serverspec simply wasn't available at the time of writing. The main issue is, though, that I think it's a good habit to start of with a fresh setup on each test, and that iptables can't be tested using the LXC backend, as it requires, well iptables :)

Thanks for your work! I'll review it in a bit.
You're right about the indicator. I'm not a fan of taining the node attributes for this kind of things, but maybe that's a path that we could go. Comparing the saved rulesets with the currently active rules is probably pretty complicated and not very straightforward. But not having the rules applied (in case a cookbook is fixed/ rerun) might result in unsafe servers.

Well, we could also decide to opt-in the feature for a while, until we find a way howto take care of this, if we can't come up with a clean solution...

[nathwill]

Yeah, comparing saved with current is definitely something of a nightmare (and also probably not desirable i think... definitely had to manually add temporary rules on iptables-ng-managed systems before...), even if only due to the counters...

I agree about tainting node attributes, was thinking about something like an /etc/iptables.d/reload-required file getting created as part of the rule/chain providers and having the reload action clean it up at the end of a reload? not terribly elegant, but i think it would at least be effective :)

[chr4]

Tought of a simple file as well, seems straightforward to me.

[nathwill]

cool, want that in a separate PR, or would you rather it be part of this change-set?

[nathwill]

hmmm... trying to run the test suites, looks like something may still need tweaking.

[chr4]

If possible, put it in this PR, as without this, users with e.g. a glitch in the chef code will be left without any iptables rules..

[nathwill]

sorry for going dark,on this... holidays ended up busier than anticipated.

anyways, as to current status of this PR, it's (hilariously) not going to be able to be merged before overhauling the test suites, as far as i can see. since minispec runs its tests within the converge, the tests are actually getting run before the converge_complete handler is triggered, and the failure of the test resources in the chef run results in the converge_complete action not being called... so... yay! for now i'll just leave this here, and work on finding some time to go dig into the test suites, with intention of rebasing this onto the serverspec-based version once that's been merged.

[chr4]

Thanks for looking into this. I always found running the test suite to be a pain, so I didn't dig too deep into the errors yet.
So I guess migrating to Serverspec seems to be a good idea anyway. Let me know once you're workingon something, I might also have a look when I find the time!

Thanks for your effords so far, apprechiating it!