Ticket/10162 firewallchain support for merge

[kbarber]

This patch set provides the firewallchain resource from Daniel Black.

This is a patch on top of PR #59 to fix a number of problems I found during testing.

[grooverdan]

nice. Needs fix to include small documentation bug that thomasvs found: thomasvs@1a4fcfc

[kbarber]

@grooverdan done.

[chrisboulton]

@kbarber, looks like when the name order was changed in 9af5947, that you forgot to update the parse order in allvalidchains (iptables_chain.rb), as it's still looking for the old order:

@resource[:name].match(Nameformat)
table = $1
chain = $2
protocol = $3

[chrisboulton]

Ah, one more in self.instances. The name = line should be:

name =  $1 + ':' + (table == 'filter' ? 'filter' : table) + ':' + p.to_s

[kbarber]

@chrisboulton: does that look better Chris?

[chrisboulton]

Should do. I'll test it out when I'm in the office tomorrow.

[chrisboulton]

Looks good - have the module in use now with no problems.

[kbarber]

@chrisboulton good to hear - thanks for the independent tests mate your efforts are very much appreciated. @dcarley, @saysjonathan if you can spend some time looking at this that would be sweet - if you are happy feel free to merge this in.

[saysjonathan]

Will do. I'll finish my review this evening.

[saysjonathan]

I'm happy. Any objections for @dcarley?

[dcarley]

Looks good! Tested on EL5 and EL6.

Small fix to the README. One of the namevars is the wrong way round and there's a missing require param:

diff --git a/README.markdown b/README.markdown
index b80d292..bbef110 100644
--- a/README.markdown
+++ b/README.markdown
@@ -105,7 +105,7 @@ Creating a new rule that forwards to a chain, then adding a rule to this chain:
     firewall { '100 forward to MY_CHAIN':
       chain   => 'INPUT',
       jump    => 'MY_CHAIN',
-      require => Firewallchain["filter:MY_CHAIN:IPv4"],
+      require => Firewallchain["MY_CHAIN:filter:IPv4"],
     }
     # The namevar here is in the format chain_name:table:protocol
     firewallchain { 'MY_CHAIN:filter:IPv4':
@@ -116,6 +116,7 @@ Creating a new rule that forwards to a chain, then adding a rule to this chain:
       action  => 'accept',
       proto   => 'tcp',
       dport   => 5000,
+      require => Firewallchain["MY_CHAIN:filter:IPv4"],
     }

 You can make firewall rules persistent with the following iptables example:

On that topic, my only suggestion would be for any Firewall[] resources with jump/chain params to autorequire the appropriate Firewallchain[] resource. That'd save a lot of leg work with require parameters and safeguard people from the slightly misleading error that iptables raises when you jump to a non-existent chain.

err: /Firewall[100 forward to MY_CHAIN]/ensure: change from absent to present failed: Execution of '/sbin/iptables -I INPUT 1 -t filter -p tcp -m comment --comment 100 forward to MY_CHAIN -j MY_CHAIN' returned 2: iptables v1.4.7: Couldn't load target `MY_CHAIN':/lib64/xtables/libipt_MY_CHAIN.so: cannot open shared object file: No such file or directory

Try `iptables -h' or 'iptables --help' for more information.

I thought this might be easy to add, but of course a jump/chain string of MY_CHAIN doesn't match the composite name of Firewallchain["MY_CHAIN:filter:IPv4"]. Is it possible or sane to search the catalog within an autorequire block? Would it help to use title_patterns to split out the values within the firewallchain type?

[kbarber]

@dcarley I've fixed the README now. I think the autorequire should be done in another commit at this point. You shouldn't need to do a search, as the table & protocol should be known to the firewall resource (table is either set or defaulting to filter - protocol is based on the provider).

[dcarley]

Good point 👍

[kbarber]

Can someone hit merge? :-)