From fc3ecdc9d80b1bf990abf47b02cbe2ff9bfc4aff Mon Sep 17 00:00:00 2001
From: Chris Boulton <chris.boulton@interspire.com>
Date: Mon, 24 Oct 2011 17:27:31 +1100
Subject: [PATCH] (#9082) Sort iptables --state option values internally to
 keep it consistent across runs

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
---
 lib/puppet/provider/firewall/iptables.rb  |  4 ++++
 lib/puppet/type/firewall.rb               |  6 ++++++
 spec/fixtures/iptables/conversion_hash.rb | 17 +++++++++++++++++
 spec/unit/puppet/type/firewall_spec.rb    |  5 +++++
 4 files changed, 32 insertions(+)

diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb
index adc3d0d..f2518e8 100644
--- 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 ee37b15..ab86a35 100644
--- 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 c4d290a..842f9d9 100644
--- 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 3b19327..7352a07 100644
--- 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
-- 
2.45.2