(#9082) Sort iptables --state option values internally to keep it consistent across...

Previously we were getting multiple re-runs due to the fact that iptables
returns a different order with iptables-save then what was used when creating
the rule.

This patch fixes that by sorting states with should=.

Added unit tests to ensure states are correctly sorted. Also added comments in
code to ensure people understand why

diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb
index adc3d0d49b584e50154bacb7ec8ba915405db479..f2518e82ffef7f9277738c5e9699afff46db7091 100644 (file)
--- a/lib/puppet/provider/firewall/iptables.rb
+++ b/lib/puppet/provider/firewall/iptables.rb
@@ -131,6 +131,10 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
       end
     end
 
+    # States should always be sorted. This ensures that the output from
+    # iptables-save and user supplied resources is consistent.
+    hash[:state] = hash[:state].sort unless hash[:state].nil?
+
     # This forces all existing, commentless rules to be moved to the bottom of the stack.
     # Puppet-firewall requires that all rules have comments (resource names) and will fail if
     # a rule in iptables does not have a comment. We get around this by appending a high level

diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb
index ee37b1595d1cea3033e0ff72b47e7bf769bc0e5e..ab86a359bd2c38a3634b920b677dcbd8e4693486 100644 (file)
--- a/lib/puppet/type/firewall.rb
+++ b/lib/puppet/type/firewall.rb
@@ -332,6 +332,12 @@ Puppet::Type.newtype(:firewall) do
 
     newvalues(:INVALID,:ESTABLISHED,:NEW,:RELATED)
 
+    # States should always be sorted. This normalizes the resource states to
+    # keep it consistent with the sorted result from iptables-save.
+    def should=(values)
+      @should = super(values).sort
+    end
+
     def should_to_s(value)
       value = [value] unless value.is_a?(Array)
       value.join(',')

diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb
index c4d290a86bcd35f2e52bc91cce2521a9fbc5ef99..842f9d9c868fa4f296f8ba51efc759806d50a084 100644 (file)
--- a/spec/fixtures/iptables/conversion_hash.rb
+++ b/spec/fixtures/iptables/conversion_hash.rb
@@ -85,6 +85,14 @@ ARGS_TO_HASH = {
       :sport => ["15","512-1024"],
     },
   },
+  'state_returns_sorted_values' => {
+    :line => '-A INPUT -m state --state INVALID,RELATED,ESTABLISHED',
+    :table => 'filter',
+    :params => {
+      :state => ['ESTABLISHED', 'INVALID', 'RELATED'],
+      :action => nil,
+    },
+  },
 }
 
 # This hash is for testing converting a hash to an argument line.
@@ -159,4 +167,13 @@ HASH_TO_ARGS = {
     },  
     :args => ["-t", :filter, "-p", :tcp, "-m", "multiport", "--dports", "15,512:1024", "-m", "comment", "--comment", "100 sport range"],
   },
+  'states_set_from_array' => {
+    :params => {
+      :name => "100 states_set_from_array",
+      :table => "filter",
+      :state => ['ESTABLISHED', 'INVALID']
+    },
+    :args => ["-t", :filter, "-p", :tcp, "-m", "comment", "--comment", "100 states_set_from_array",
+      "-m", "state", "--state", "ESTABLISHED,INVALID"],
+  },
 }

diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb
index 3b19327f1a979e12d3fcfce925040758014f3247..7352a0769e9878da530dad29149d11da7675db16 100644 (file)
--- a/spec/unit/puppet/type/firewall_spec.rb
+++ b/spec/unit/puppet/type/firewall_spec.rb
@@ -218,6 +218,11 @@ describe firewall do
       @resource[:state] = [:INVALID, :NEW]
       @resource[:state].should == [:INVALID, :NEW]
     end
+
+    it 'should sort values alphabetically' do
+      @resource[:state] = [:NEW, :ESTABLISHED]
+      @resource[:state].should == [:ESTABLISHED, :NEW]
+    end
   end
 
   describe ':burst' do