Fix for #286 for pre-existing rules at the start of a chain

In #286 we fixed rule offset detection for existing managed and
unmanaged rules, but in the case where the first rule in a chain was
unmanaged, managed rules were still being inserted under it.

This patch changes it so that if the first rule detected for offset is
unmanaged, then we should insert before that for more consistent
behavior.

diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb
index 056c587c3cbda5768c7b4d8c126f13e880bb48ec..0350a10b4925fc6b8aece02fec2a4c6b22f9e683 100644 (file)
--- a/lib/puppet/provider/firewall/iptables.rb
+++ b/lib/puppet/provider/firewall/iptables.rb
@@ -454,6 +454,7 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
     my_rule = resource[:name].to_s
     rules << my_rule
 
+    unmanaged_rule_regex = /^9[0-9]{3}\s[a-f0-9]{32}$/
     # Find if this is a new rule or an existing rule, then find how many
     # unmanaged rules preceed it.
     if rules.length == rules.uniq.length
@@ -478,9 +479,12 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
     unnamed_offset = rules[0..rules.index(offset_rule)].inject(0) do |sum,rule|
       # This regex matches the names given to unmanaged rules (a number
       # 9000-9999 followed by an MD5 hash).
-      sum + (rule.match(/^9[0-9]{3}\s[a-f0-9]{32}$/) ? 1 : 0)
+      sum + (rule.match(unmanaged_rule_regex) ? 1 : 0)
     end
 
+    # We want our rules to come before unmanaged rules
+    unnamed_offset -= 1 if offset_rule.match(unmanaged_rule_regex)
+
     # Insert our new or updated rule in the correct order of named rules, but
     # offset for unnamed rules.
     rules.sort.index(my_rule) + 1 + unnamed_offset

diff --git a/spec/acceptance/purge_spec.rb b/spec/acceptance/purge_spec.rb
index 6e245fcf793c4a8bb43325b24fdeb003019a1962..7cc3f75e968a17bdfe6e88bece7e73ae50a89a3c 100644 (file)
--- a/spec/acceptance/purge_spec.rb
+++ b/spec/acceptance/purge_spec.rb
@@ -33,7 +33,8 @@ describe "purge tests:" do
     before(:each) do
       iptables_flush_all_tables
 
-      shell('/sbin/iptables -A INPUT -s 1.2.1.1')
+      shell('/sbin/iptables -A INPUT -p tcp -s 1.2.1.1')
+      shell('/sbin/iptables -A INPUT -p udp -s 1.2.1.1')
       shell('/sbin/iptables -A OUTPUT -s 1.2.1.2 -m comment --comment "010 output-1.2.1.2"')
     end
 
@@ -83,5 +84,41 @@ describe "purge tests:" do
 
       apply_manifest(pp, :catch_changes => true)
     end
+
+    it 'adds managed rules with ignored rules' do
+      pp = <<-EOS
+        class { 'firewall': }
+        firewallchain { 'INPUT:filter:IPv4':
+          purge => true,
+          ignore => [
+            '-s 1\.2\.1\.1',
+          ],
+        }
+        firewall { '014 input-1.2.1.6':
+          chain  => 'INPUT',
+          proto  => 'all',
+          source => '1.2.1.6',
+        }
+        -> firewall { '013 input-1.2.1.5':
+          chain  => 'INPUT',
+          proto  => 'all',
+          source => '1.2.1.5',
+        }
+        -> firewall { '012 input-1.2.1.4':
+          chain  => 'INPUT',
+          proto  => 'all',
+          source => '1.2.1.4',
+        }
+        -> firewall { '011 input-1.2.1.3':
+          chain  => 'INPUT',
+          proto  => 'all',
+          source => '1.2.1.3',
+        }
+      EOS
+
+      apply_manifest(pp, :catch_failures => true)
+
+      expect(shell('/sbin/iptables-save').stdout).to match(/-A INPUT -s 1\.2\.1\.1\/32 -p tcp \n-A INPUT -s 1\.2\.1\.1\/32 -p udp/)
+    end
   end
 end

diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb
index a7ed2d1a6ecc278992dbf14ace1beb01936d6bb9..872c1c62e7ec96924f21c6c33007a36018adc650 100644 (file)
--- a/spec/unit/puppet/provider/iptables_spec.rb
+++ b/spec/unit/puppet/provider/iptables_spec.rb
@@ -122,13 +122,14 @@ describe 'iptables provider' do
       expect(resource.provider.insert_order).to eq(3)
     end
 
-    context 'with an unname rule' do
+    context 'with unname rules between' do
       let(:iptables_save_output) { [
         '-A INPUT -s 8.0.0.2/32 -p tcp -m multiport --ports 100 -m comment --comment "100 test" -j ACCEPT',
+        '-A INPUT -s 8.0.0.2/32 -p tcp -m multiport --ports 150 -m comment --comment "150 test" -j ACCEPT',
         '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 200 -j ACCEPT',
+        '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 250 -j ACCEPT',
         '-A INPUT -s 8.0.0.4/32 -p tcp -m multiport --ports 300 -m comment --comment "300 test" -j ACCEPT',
-        '-A INPUT -s 8.0.0.5/32 -p tcp -m multiport --ports 400 -j ACCEPT',
-        '-A INPUT -s 8.0.0.6/32 -p tcp -m multiport --ports 500 -m comment --comment "500 test" -j ACCEPT'
+        '-A INPUT -s 8.0.0.4/32 -p tcp -m multiport --ports 350 -m comment --comment "350 test" -j ACCEPT',
       ] }
       it 'understands offsets for adding rules before unnamed rules' do
         resource = Puppet::Type.type(:firewall).new({ :name => '001 test', })
@@ -140,26 +141,61 @@ describe 'iptables provider' do
         allow(resource.provider.class).to receive(:instances).and_return(providers)
         expect(resource.provider.insert_order).to eq(1)
       end
-      it 'understands offsets for adding rules between unnamed rules' do
-        resource = Puppet::Type.type(:firewall).new({ :name => '301 test', })
+      it 'understands offsets for adding rules between managed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '120 test', })
         allow(resource.provider.class).to receive(:instances).and_return(providers)
-        expect(resource.provider.insert_order).to eq(4)
+        expect(resource.provider.insert_order).to eq(2)
       end
-      it 'understands offsets for editing rules between unnamed rules' do
-        resource = Puppet::Type.type(:firewall).new({ :name => '300 test', })
+      it 'understands offsets for adding rules between unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '151 test', })
         allow(resource.provider.class).to receive(:instances).and_return(providers)
         expect(resource.provider.insert_order).to eq(3)
       end
       it 'understands offsets for adding rules after unnamed rules' do
-        resource = Puppet::Type.type(:firewall).new({ :name => '501 test', })
+        resource = Puppet::Type.type(:firewall).new({ :name => '351 test', })
         allow(resource.provider.class).to receive(:instances).and_return(providers)
-        expect(resource.provider.insert_order).to eq(6)
+        expect(resource.provider.insert_order).to eq(7)
       end
-      it 'understands offsets for editing rules after unnamed rules' do
-        resource = Puppet::Type.type(:firewall).new({ :name => '500 test', })
+    end
+
+    context 'with unname rules before and after' do
+      let(:iptables_save_output) { [
+        '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 050 -j ACCEPT',
+        '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 090 -j ACCEPT',
+        '-A INPUT -s 8.0.0.2/32 -p tcp -m multiport --ports 100 -m comment --comment "100 test" -j ACCEPT',
+        '-A INPUT -s 8.0.0.2/32 -p tcp -m multiport --ports 150 -m comment --comment "150 test" -j ACCEPT',
+        '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 200 -j ACCEPT',
+        '-A INPUT -s 8.0.0.3/32 -p tcp -m multiport --ports 250 -j ACCEPT',
+        '-A INPUT -s 8.0.0.4/32 -p tcp -m multiport --ports 300 -m comment --comment "300 test" -j ACCEPT',
+        '-A INPUT -s 8.0.0.4/32 -p tcp -m multiport --ports 350 -m comment --comment "350 test" -j ACCEPT',
+        '-A INPUT -s 8.0.0.5/32 -p tcp -m multiport --ports 400 -j ACCEPT',
+        '-A INPUT -s 8.0.0.5/32 -p tcp -m multiport --ports 450 -j ACCEPT',
+      ] }
+      it 'understands offsets for adding rules before unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '001 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(1)
+      end
+      it 'understands offsets for editing rules before unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '100 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(3)
+      end
+      it 'understands offsets for adding rules between managed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '120 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(4)
+      end
+      it 'understands offsets for adding rules between unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '151 test', })
         allow(resource.provider.class).to receive(:instances).and_return(providers)
         expect(resource.provider.insert_order).to eq(5)
       end
+      it 'understands offsets for adding rules after unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '351 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(9)
+      end
     end
   end