debops / ansible-ferm

Manage iptables firewall using ferm
GNU General Public License v3.0
33 stars 20 forks source link

Restart ferm only when configuration changes are made #76

Closed logan2211 closed 8 years ago

logan2211 commented 8 years ago

Currently the role restarts ferm on every single run. This does an iptables-save/restore and ferm does not restore counters, so every iptables counter will be reset to 0.

This commit just uses the handler that is already in place to restart ferm upon changes, instead of performing a restart every time the role runs regardless of change states.

drybjed commented 8 years ago

The problem with removing that task is that you might want to have configured firewall before the current play ends and handlers are fired - for example you need a port opened on a remote host.

The idea is nice though. I suppose there could be a few register variables, each one for each task that modifies the firewall. Then the restart task could check if any of the variables has var.changed|bool and restart the firewall in that case.

logan2211 commented 8 years ago

In the role there is only 1 handler and it is the restart handler. From the restart task I removed until the end of the role I did not see anything that would have changed depending on whether the service was restarted. However, if there are tasks there that depend on ferm having been restarted if necessary, then flushing the handlers at the same point where the unconditional restart occurred would work.

drybjed commented 8 years ago

Good idea with the flushing! Just keep the same when condition and it just might work.

drybjed commented 8 years ago

@Logan2211 Ah, you mean adding the flush handlers in each role that would need to have firewall set up? Actually I thought about replacing the task you removed with a flush handlers task which should do the same thing, so that adding them to all the other roles won't be needed. Do you think that will do fine?

logan2211 commented 8 years ago

Added the tag to the handler flush. The conditional is implemented here already: https://github.com/debops/ansible-ferm/blob/master/handlers/main.yml#L7

drybjed commented 8 years ago

:+1: