From: Hunter Haugen Date: Thu, 6 Feb 2014 23:47:27 +0000 (-0800) Subject: Fix for #286 for pre-existing rules at the start of a chain X-Git-Tag: 0.5.0~1^2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=9bd5518bb095443113f7a64d46185b18c5090485;p=puppet-modules%2Fpuppetlabs-firewall.git 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 056c587..0350a10 100644 --- 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 6e245fc..7cc3f75 100644 --- 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 a7ed2d1..872c1c6 100644 --- 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