Refactor iptables

[thusoy]

Adds some tests and support for --log-* arguments.

Also stops writing to error log for the checks (iptables -C), since it's not really a failure.

And most importantly, makes the module much more readable.

Some questions:

• The build_rule function returns strings on errors ("Error: Table needs to be specified"), is is better if this raises an exception instead?
• The table argument to build_rule defaults to None, wouldn't it be simpler for this to default to 'filter', and thus not require that to be specified everywhere? (That would also close iptables.set_pocity KeyError: 'table' ubuntu 14.04 #18745)
• The iptables state isn't able to track changes to a state, and will duplicate the entry if anything is changed (such as a comment, a source IP, etc). Is it possible to maybe add a comment or something to to track where each line comes from, to be able to update the entry in case of modifications? Like SALT_ID: sha1()[:10]? Sort of like how the cron state does it. The workaround I've come up with involves flushing the rules on every state run, but that leaves a small window of opporunity between rules being flushed and applied again, so it's not ideal.

[jfindlay]

@thusoy, it looks like there are some test failures.

[thusoy]

It does indeed, fixed that and the pylint error and rebased, let's see what jenkins has to say now.

[jfindlay]

@thusoy, sorry for all the trouble but there were some changes to iptables.py recently that were not included in your rebase. Will you fetch and rebase on develop again? Thanks.

[thusoy]

@jfindlay Sure thing, no probs. There's still the questions raised in the first post, any opinion on any of those?

[techhat]

@thusoy I can answer a couple of your initial questions.

Raising exceptions can be tricky here, because they might get silently caught before they reach the user. Error strings will always reach the user. That said, I'm open to any suggestions that @jfindlay may have here.

Defaulting table to filter seems reasonable.

Thanks for the cleanup. I haven't had a chance to peruse it as thoroughly as I'd like, but offhand it looks much nicer.

[jfindlay]

I am still a novice in this and many other areas of the salt codebase. Consistency in output and return codes for modules is something that is going to happen in the next release or two. In the mean time, getting an error string back to the point of action is going to be better than raising an exception, as @techhat has already mentioned.

Iptables is difficult to state by its very nature. Any good ideas you can bring to that are most welcome.

[thusoy]

Should I include the table='filter' default in this PR or should I add a new one for that?

[jfindlay]

@thusoy, it's up to you. I think it would be simpler and easier to do that here, but if you have a reason to submit a change for that later, that would be fine.

[thusoy]

@jfindlay Okay, I set the default to 'filter', this is ready to merge as far as I'm concerned.

As for the idea about tracking SALT_ID in the comment field, I think it would work well for platforms where comments are supported, but not all iptables supports this (notably RedHat/CentOS). It could perhaps be used where supported and let non-supported platforms live with todays behavior, hoping they'll eventually catch up.

[thusoy]

Missed a test case...