From: Hunter Haugen <hunter@puppetlabs.com>
Date: Tue, 28 Jan 2014 01:31:22 +0000 (-0800)
Subject: (MODULES-439) Work around existing rules
X-Git-Tag: 0.5.0~13^2
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=2ec07247a2c16f749daddde4a00730e983b039f1;p=puppet-modules%2Fpuppetlabs-firewall.git

(MODULES-439) Work around existing rules

The firewall resource is not intended to be used with rules that are not
also managed by puppet; the behavior when doing so was undefined. This
is an attempt to make it more defined.

The behavior is that any rule added by puppet will be inserted in its
given order in relation to the other rules managed by puppet, but ahead
of any rules not managed by puppet.
---

diff --git a/Gemfile b/Gemfile
index 2909ace..690b772 100644
--- a/Gemfile
+++ b/Gemfile
@@ -6,6 +6,7 @@ group :development, :test do
   gem 'serverspec',             :require => false
   gem 'beaker-rspec',           :require => false
   gem 'puppet-lint',            :require => false
+  gem 'pry',                    :require => false
 end
 
 if puppetversion = ENV['PUPPET_GEM_VERSION']
diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb
index 8c07a62..905ab54 100644
--- a/lib/puppet/provider/firewall/iptables.rb
+++ b/lib/puppet/provider/firewall/iptables.rb
@@ -408,8 +408,39 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
     # No rules at all? Just bail now.
     return 1 if rules.empty?
 
+    # Add our rule to the end of the array of known rules
     my_rule = resource[:name].to_s
     rules << my_rule
-    rules.sort.index(my_rule) + 1
+
+    # 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
+      # This is a new rule so find its ordered location.
+      new_rule_location = rules.sort.uniq.index(my_rule)
+      if new_rule_location == 0
+        # The rule will be the first rule in the chain because nothing came
+        # before it.
+        offset_rule = rules[0]
+      else
+        # This rule will come after other managed rules, so find the rule
+        # immediately preceeding it.
+        offset_rule = rules.sort.uniq[new_rule_location - 1]
+      end
+    else
+      # This is a pre-existing rule, so find the offset from the original
+      # ordering.
+      offset_rule = my_rule
+    end
+    # Count how many unmanaged rules are ahead of the target rule so we know
+    # how much to add to the insert order
+    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)
+    end
+
+    # 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
   end
 end
diff --git a/spec/acceptance/change_source_spec.rb b/spec/acceptance/change_source_spec.rb
new file mode 100644
index 0000000..02760b0
--- /dev/null
+++ b/spec/acceptance/change_source_spec.rb
@@ -0,0 +1,77 @@
+require 'spec_helper_acceptance'
+
+describe 'firewall type' do
+  describe 'reset' do
+    it 'deletes all rules' do
+      shell('iptables --flush; iptables -t nat --flush; iptables -t mangle --flush')
+    end
+  end
+
+  describe 'when unmanaged rules exist' do
+    it 'applies with 8.0.0.1 first' do
+      pp = <<-EOS
+          class { '::firewall': }
+          firewall { '101 test source changes':
+            proto  => tcp,
+            port   => '101',
+            action => accept,
+            source => '8.0.0.1',
+          }
+          firewall { '100 test source static':
+            proto  => tcp,
+            port   => '100',
+            action => accept,
+            source => '8.0.0.2',
+          }
+      EOS
+
+      apply_manifest(pp, :catch_failures => true)
+    end
+
+    it 'contains the changable 8.0.0.1 rule' do
+      shell('iptables -S') do |r|
+        expect(r.stdout).to match(/-A INPUT -s 8\.0\.0\.1\/32 -p tcp -m multiport --ports 101 -m comment --comment "101 test source changes" -j ACCEPT/)
+      end
+    end
+    it 'contains the static 8.0.0.2 rule' do
+      shell('iptables -S') do |r|
+        expect(r.stdout).to match(/-A INPUT -s 8\.0\.0\.2\/32 -p tcp -m multiport --ports 100 -m comment --comment "100 test source static" -j ACCEPT/)
+      end
+    end
+
+    it 'adds a unmanaged rule without a comment' do
+      shell('/sbin/iptables -R INPUT 1 -t filter -s 8.0.0.3/32 -p tcp -m multiport --ports 100 -j ACCEPT')
+      expect(shell('iptables -S').stdout).to match(/-A INPUT -s 8\.0\.0\.3\/32 -p tcp -m multiport --ports 100 -j ACCEPT/)
+    end
+
+    it 'changes to 8.0.0.4 second' do
+      pp = <<-EOS
+          class { '::firewall': }
+          firewall { '101 test source changes':
+            proto  => tcp,
+            port   => '101',
+            action => accept,
+            source => '8.0.0.4',
+          }
+      EOS
+
+      expect(apply_manifest(pp, :catch_failures => true).stdout).to match(/Notice: \/Stage\[main\]\/Main\/Firewall\[101 test source changes\]\/source: source changed '8\.0\.0\.1\/32' to '8\.0\.0\.4\/32'/)
+    end
+
+    it 'does not contain the old changing 8.0.0.1 rule' do
+      shell('iptables -S') do |r|
+        expect(r.stdout).to_not match(/8\.0\.0\.1/)
+      end
+    end
+    it 'contains the staic 8.0.0.2 rule' do
+      shell('iptables -S') do |r|
+        expect(r.stdout).to match(/-A INPUT -s 8\.0\.0\.2\/32 -p tcp -m multiport --ports 100 -m comment --comment "100 test source static" -j ACCEPT/)
+      end
+    end
+    it 'contains the changing new 8.0.0.4 rule' do
+      shell('iptables -S') do |r|
+        expect(r.stdout).to match(/-A INPUT -s 8\.0\.0\.4\/32 -p tcp -m multiport --ports 101 -m comment --comment "101 test source changes" -j ACCEPT/)
+      end
+    end
+  end
+end
diff --git a/spec/acceptance/nodesets/centos-59-x64.yml b/spec/acceptance/nodesets/centos-59-x64.yml
index b0a4ba8..b41a947 100644
--- a/spec/acceptance/nodesets/centos-59-x64.yml
+++ b/spec/acceptance/nodesets/centos-59-x64.yml
@@ -2,7 +2,9 @@ HOSTS:
   centos-59-x64:
     roles:
       - master
-    platform: centos-59-x64
+    platform: el-5-x86_64
     box : centos-59-x64-vbox4210-nocm
     box_url : http://puppet-vagrant-boxes.puppetlabs.com/centos-59-x64-vbox4210-nocm.box
     hypervisor : vagrant
+CONFIG:
+  type: foss
diff --git a/spec/acceptance/nodesets/centos-64-x64-fusion.yml b/spec/acceptance/nodesets/centos-64-x64-fusion.yml
new file mode 100644
index 0000000..d516673
--- /dev/null
+++ b/spec/acceptance/nodesets/centos-64-x64-fusion.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  centos-64-x64:
+    roles:
+      - master
+    platform: el-6-x86_64
+    box : centos-64-x64-fusion503-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/centos-64-x64-fusion503-nocm.box
+    hypervisor : fusion
+CONFIG:
+  type: foss
diff --git a/spec/acceptance/nodesets/centos-64-x64-pe.yml b/spec/acceptance/nodesets/centos-64-x64-pe.yml
new file mode 100644
index 0000000..7d9242f
--- /dev/null
+++ b/spec/acceptance/nodesets/centos-64-x64-pe.yml
@@ -0,0 +1,12 @@
+HOSTS:
+  centos-64-x64:
+    roles:
+      - master
+      - database
+      - dashboard
+    platform: el-6-x86_64
+    box : centos-64-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/centos-64-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: pe
diff --git a/spec/acceptance/nodesets/centos-64-x64.yml b/spec/acceptance/nodesets/centos-64-x64.yml
index 8f57028..05540ed 100644
--- a/spec/acceptance/nodesets/centos-64-x64.yml
+++ b/spec/acceptance/nodesets/centos-64-x64.yml
@@ -2,7 +2,9 @@ HOSTS:
   centos-64-x64:
     roles:
       - master
-    platform: el-6-i386
+    platform: el-6-x86_64
     box : centos-64-x64-vbox4210-nocm
     box_url : http://puppet-vagrant-boxes.puppetlabs.com/centos-64-x64-vbox4210-nocm.box
     hypervisor : vagrant
+CONFIG:
+  type: foss
diff --git a/spec/acceptance/nodesets/debian-607-x64.yml b/spec/acceptance/nodesets/debian-607-x64.yml
new file mode 100644
index 0000000..4c8be42
--- /dev/null
+++ b/spec/acceptance/nodesets/debian-607-x64.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  debian-607-x64:
+    roles:
+      - master
+    platform: debian-6-amd64
+    box : debian-607-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/debian-607-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: git
diff --git a/spec/acceptance/nodesets/debian-70rc1-x64.yml b/spec/acceptance/nodesets/debian-70rc1-x64.yml
new file mode 100644
index 0000000..19181c1
--- /dev/null
+++ b/spec/acceptance/nodesets/debian-70rc1-x64.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  debian-70rc1-x64:
+    roles:
+      - master
+    platform: debian-7-amd64
+    box : debian-70rc1-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/debian-70rc1-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: git
diff --git a/spec/acceptance/nodesets/fedora-18-x64.yml b/spec/acceptance/nodesets/fedora-18-x64.yml
new file mode 100644
index 0000000..624b537
--- /dev/null
+++ b/spec/acceptance/nodesets/fedora-18-x64.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  fedora-18-x64:
+    roles:
+      - master
+    platform: fedora-18-x86_64
+    box : fedora-18-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/fedora-18-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: git
diff --git a/spec/acceptance/nodesets/sles-11sp1-x64.yml b/spec/acceptance/nodesets/sles-11sp1-x64.yml
new file mode 100644
index 0000000..554c37a
--- /dev/null
+++ b/spec/acceptance/nodesets/sles-11sp1-x64.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  sles-11sp1-x64:
+    roles:
+      - master
+    platform: sles-11-x86_64
+    box : sles-11sp1-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/sles-11sp1-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: git
diff --git a/spec/acceptance/nodesets/ubuntu-server-10044-x64.yml b/spec/acceptance/nodesets/ubuntu-server-10044-x64.yml
new file mode 100644
index 0000000..5047017
--- /dev/null
+++ b/spec/acceptance/nodesets/ubuntu-server-10044-x64.yml
@@ -0,0 +1,10 @@
+HOSTS:
+  ubuntu-server-10044-x64:
+    roles:
+      - master
+    platform: ubuntu-10.04-amd64
+    box : ubuntu-server-10044-x64-vbox4210-nocm
+    box_url : http://puppet-vagrant-boxes.puppetlabs.com/ubuntu-server-10044-x64-vbox4210-nocm.box
+    hypervisor : vagrant
+CONFIG:
+  type: git
diff --git a/spec/acceptance/nodesets/ubuntu-server-12042-x64.yml b/spec/acceptance/nodesets/ubuntu-server-12042-x64.yml
index 2b8fe4a..d065b30 100644
--- a/spec/acceptance/nodesets/ubuntu-server-12042-x64.yml
+++ b/spec/acceptance/nodesets/ubuntu-server-12042-x64.yml
@@ -2,7 +2,9 @@ HOSTS:
   ubuntu-server-12042-x64:
     roles:
       - master
-    platform: ubuntu-server-12.04-amd64
+    platform: ubuntu-12.04-amd64
     box : ubuntu-server-12042-x64-vbox4210-nocm
     box_url : http://puppet-vagrant-boxes.puppetlabs.com/ubuntu-server-12042-x64-vbox4210-nocm.box
     hypervisor : vagrant
+CONFIG:
+  type: foss
diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb
index d4dcea5..d34679f 100644
--- a/spec/unit/puppet/provider/iptables_spec.rb
+++ b/spec/unit/puppet/provider/iptables_spec.rb
@@ -79,6 +79,90 @@ describe 'iptables provider' do
     expect(provider.instances.length).to be_zero
   end
 
+  describe '#insert_order' 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.3/32 -p tcp -m multiport --ports 200 -m comment --comment "200 test" -j ACCEPT',
+      '-A INPUT -s 8.0.0.4/32 -p tcp -m multiport --ports 300 -m comment --comment "300 test" -j ACCEPT'
+    ] }
+    let(:resources) do
+      iptables_save_output.each_with_index.collect { |l,index| provider.rule_to_hash(l, 'filter', index) }
+    end
+    let(:providers) do
+      resources.collect { |r| provider.new(r) }
+    end
+    it 'understands offsets for adding rules to the beginning' 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) # 1-indexed
+    end
+    it 'understands offsets for editing rules at the beginning' 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(1)
+    end
+    it 'understands offsets for adding rules to the middle' do
+      resource = Puppet::Type.type(:firewall).new({ :name => '101 test', })
+      allow(resource.provider.class).to receive(:instances).and_return(providers)
+      expect(resource.provider.insert_order).to eq(2)
+    end
+    it 'understands offsets for editing rules at the middle' do
+      resource = Puppet::Type.type(:firewall).new({ :name => '200 test', })
+      allow(resource.provider.class).to receive(:instances).and_return(providers)
+      expect(resource.provider.insert_order).to eq(2)
+    end
+    it 'understands offsets for adding rules to the end' do
+      resource = Puppet::Type.type(:firewall).new({ :name => '301 test', })
+      allow(resource.provider.class).to receive(:instances).and_return(providers)
+      expect(resource.provider.insert_order).to eq(4)
+    end
+    it 'understands offsets for editing rules at the end' do
+      resource = Puppet::Type.type(:firewall).new({ :name => '300 test', })
+      allow(resource.provider.class).to receive(:instances).and_return(providers)
+      expect(resource.provider.insert_order).to eq(3)
+    end
+
+    context 'with an unname rule' 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.3/32 -p tcp -m multiport --ports 200 -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'
+      ] }
+      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(1)
+      end
+      it 'understands offsets for adding rules between unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '301 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(4)
+      end
+      it 'understands offsets for editing rules between unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '300 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', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(6)
+      end
+      it 'understands offsets for editing rules after unnamed rules' do
+        resource = Puppet::Type.type(:firewall).new({ :name => '500 test', })
+        allow(resource.provider.class).to receive(:instances).and_return(providers)
+        expect(resource.provider.insert_order).to eq(5)
+      end
+    end
+  end
+
   # Load in ruby hash for test fixtures.
   load 'spec/fixtures/iptables/conversion_hash.rb'