Add reset action.

[nmische]

This is an example of what a :reset action may look like.

[rtkrruvinskiy]

Thanks for the pull request, Nathan!

Can you elaborate a little bit on your motivation here? I looked through our previous discussion and through your code change to use persistent attributes from last year, and I didn't really see that. Is this just to make sure that notifications work properly? Or are there other reasons?

Another problem is that, as I mentioned previously, the way things are currently implemented, if persistent attributes are used, changing an existing rule resource in a recipe does not have the desired effect (depending on what you change, the new rule can be appended rather than replace the old rule, or nothing can happen at all). I think that issue needs to be addressed before bringing back the option to use persistent attributes.

[nmische]

First let me say I understand your desire to reset IP tables on every run, but I don't think it is a good idea to force that behavior as the current implementation does. There are a few reason I feel this way.

First, under the current implementation the behavior of simple_iptables resources is not consistent with general resource behavior in Chef. For example, if I change a file resource I don't expect it to delete the file the resource previously created. Also given the name of the rule resource action is :append I think the previous behavior was expected. So while I understand where you are coming from, I think the recent changes in simple_iptables resources actually make it harder for the general Chef user to use the cookbook. (While I may not be an advanced Chef user, I consider myself above average and these changes threw me for a loop.)

Second, in the previous implementation you could declaratively set iptables rules using attributes. I know the cookbook warns against it, but this was a convenient option if you only needed to set a few rules and didn't want to write a cookbook. For example, I often set the appropriate simple_iptables attributes via roles or in an attributes JSON file (for use with chef_solo in Vagrant) and then just include the simple_iptables::default recipe. This is often much simpler that writing a dedicate cookbook or adding a recipe to an existing cookbook to use the resources.

Finally, under the previous implementation notifications did work correctly. This may not be important for many people, but I see no reason not to support it.

Given the above considerations I think having a :reset action as I propose brings the simple_iptables cookbook in line with Chef conventions, but still allows for the behavior you desire. Those who want to reset iptables and don't care about notifications from these resources can include the reset recipe or otherwise execute the :reset action. In that case I think it is much more clear what is happening and more inline with general Chef resource behavior. For those who need notifications or want to set rules via attributes, they can include the default recipe with out resetting all attributes.

Thanks for considering!

[rtkrruvinskiy]

Thanks for the detailed explanation, Nathan! I can definitely see where you're coming from.

I don't have a problem in principle with the action as you have proposed it. However, my one reservation is that I do not want to make persistence the default unless the behaviour of the rule provider is modified so that if a rule resource is changed, the rule is updated, rather than something unpredictable happening. The default (and only) action being named append aside, I don't think that behaviour is what anyone would expect. To use your example, if I change a file resource, I don't expect the new contents to be appended to the old ones.

So, if you would like to implement a reset action, test it, and change the semantics of the provider, I'd be glad to merge the changes. Thanks again!

[kungfujoe]

Hey guys, please allow me to jump in, if only momentarily.

I'm using this (fantastic) cookbook to set up IPTables-based port forwarding from port 80 -> 8080 (you know, Tomcat style). The trick is this: We only set up this port forwarding if we aren't using a load balancer. If we are, then we typically let the balancer handle this, as it is more than capable of doing so. For our usage, it is more than plausible that, for one run of the cookbook, we need to do the forwarding, but for the next, we want to "turn it off", and manually put a load balancer in front of the server; In that event, I would find IPTables reset functionality very useful.

Of course, I could get off my lazy duff and reset them manually, but what's the fun in that? I'm kind of looking at an if-else: either turn on IPTables forwarding, or drop all IPTables rules.

I got a bit lost in your conversation above, but it appears that the original point of this merge request may have been to reset the rules on every run, and then (presumably) reapply them, which is not quite my case, but it sounds like this code would work towards my purposes. Any chance that merge is gonna get done anytime soon?

In any case, Merry Christmas, Happy New Year, and have a good one gentlemen, KFJ

[rtkrruvinskiy]

@kungfujoe If I understand you correctly, the cookbook currently works the way you want: we recompute the rules every time from the simple_iptables_rule resources defined during the current run. This pull request was meant to make that behaviour optional (i.e., to have a rule persist even if its respective resource is deleted).

[rtkrruvinskiy]

@nmische Did you still have plans to make the modifications we talked about? I haven't heard back in about 6 months now.

[nmische]

@rtkrruvinskiy The cleanest way I can think of implementing this in this pull request. It would require those who don't want rules to persist between runs to use the :reset action.

Unfortunately, the behavior you describe above would require some sort of registry or database for rules with each rule having a unique identifier that associates the rule with a host, such that you can identify when a rule is created, dropped, or updated. I don't know how you would do that in Chef.

[rtkrruvinskiy]

@nmische Well, you could store the rules in a hash in the attributes, with the key being the resource name, and the value being the rule. If the rule changes, you update the attribute. Then you regenerate the iptables-rules at the end like now and load if it's changed.

However, if you don't feel like implementing that, that's totally fine and not a problem at all. In that case, we can close this PR. At any rate, given the amount of time that has passed and the fact that no one else has chimed in in support of persistent attributes, I'd be very reluctant to change the default behaviour. (However, I'd be very willing to introduce persistence as an option, subject to the caveats I mentioned before.) Thanks again for the time you've spent on this and for your thoughtful comments!

[rtkrruvinskiy]

Closing PR due to inactivity. @nmische, if you still want to do this, let me know.