chr4-cookbooks / iptables-ng

Cookbook to maintain iptables rules and policies on different platforms, respecting the way the os handles these settings.
GNU General Public License v3.0
38 stars 33 forks source link

Run systemctl daemon-reload on RHEL 7+ #53

Closed jboeshart closed 9 years ago

jboeshart commented 9 years ago

On some images (specifically saw it on the Centos7 AMI) I've seen a failure to start up iptables when systemd is in use. The packages get installed but systemd needs to pick up the changes, which a systemctl daemon-reload will take care of. Added in a check for the platform family and version to run systemctl daemon-reload if we're running RHEL 7+.

I'm not entirely sure what all might be needed from a testing standpoint (I'm somewhat new to cooking with Chef) so please let me know if you need specifics around test scenarios.

jboeshart commented 9 years ago

Not quite sure why the build is failing, I didn't modify anything in the attributes file, nor did I make any chances in the rubocop config. It doesn't seem to like the alignment in the default['iptables-ng']['packages'] assignment.

attributes/default.rb:59:1: W: end at 59, 0 is not aligned with case at 45, 37.
chr4 commented 9 years ago

Thanks for spotting! I think this is a good idea, I wonder though, whether we should just check for systemctl and then run it (instead of checking for a specific OS), as archlinux and newer Ubuntu/ Debian Systems should/ might also be affected.

I'd suggest a not_if in the execute resource, like this

not_if 'which systemctl' or not_if 'test -e /usr/bin/systemctl'

nathwill commented 9 years ago
only_if { IO.read('/proc/1/comm').chomp == 'systemd' }

^ that's how i do it, works nice :)

jboeshart commented 9 years ago

That's a better way to handle it, though a slight modification on your suggestion. If use a not_if or only_if in the execute resource, it will run systemctl daemon-reload on every chef-client run. To prevent this I replaced the platform check with @nathwill's check for systemd, that way we only run the execute resource if we actually install a package.

Array(node['iptables-ng']['packages']).each do |pkg|
  # Check to see if we're using systemd so we run systemctl daemon-reload to pick up new services
  if IO.read('/proc/1/comm').chomp == 'systemd'
    package pkg do
      notifies :run, 'execute[systemctl daemon-reload]', :immediately
    end
  else
    package pkg
  end
end

execute 'systemctl daemon-reload' do
  action :nothing
end
nathwill commented 9 years ago

/me shrugs.

that works, but you can avoid the nesting and needing to define the package twice by just letting the notify happen everywhere, and only running the action on systemd-based platforms. i think that

package pkg do
  notifes :run, 'execute[systemd-reload]', :immediately
end

execute 'systemctl daemon-reload' do
  only_if { IO.read('/proc/1/comm').chomp == 'systemd' }
end

i guess which one's preferable is just a matter of preference.

jboeshart commented 9 years ago

Agreed, that looks much cleaner, thanks for the feedback. I'll test it out tomorrow and if everything works will update the pull request.

chr4 commented 9 years ago

Dont' forget the action :nothing, otherwise daemon-reload will still be called all the time. Besides that I think the pattern @nathwill is the one we should go with.

Thanks for testing @jboeshart! Looking forward for the new PR :)

jboeshart commented 9 years ago

Ok, tested locally with a CentOS 6.6 and a CentOS 7.1 server and it looked good. Updated the recipe with the recommendations and updated the PR, let me know if there's anything else we want to include/improve on this. Thanks again for the feedback, my Ruby-fu is still green belt so definitely appreciate the feedback on how to make it better!

chr4 commented 9 years ago

This looks great! Merging. Will release in a bit, maybe together with other changes.