From: Jonathan Boyett Date: Wed, 28 Sep 2011 20:55:02 +0000 (-0700) Subject: (#9439) fix parsing and deleting existing rules X-Git-Tag: v0.0.1~3^2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=960f4300479c835100a84ac53c5265075816032a;p=puppet-modules%2Fpuppetlabs-firewall.git (#9439) fix parsing and deleting existing rules Previously we hadn't been able to parse existing rules that were missing a comment field. This patch fixes that by using an MD5 hash of the iptables raw line as the name of the property. We have also cleaned up the way we delete arguments by adding a new delete_args function to return valid delete_args. Instead of having to work out the rule order we now just delete the rule based on the specification. --- diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index a6efda8..50f366e 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -1,4 +1,5 @@ require 'puppet/provider/firewall' +require 'digest/md5' Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Firewall do include Puppet::Util::Firewall @@ -60,7 +61,7 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir def delete debug 'Deleting rule %s' % resource[:name] - iptables "-D", properties[:chain], insert_order + iptables delete_args end def exists? @@ -119,7 +120,15 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir [:dport, :sport, :state].each do |prop| hash[prop] = hash[prop].split(',') if ! hash[prop].nil? end - + + # 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 + if ! hash[:name] + hash[:name] = "9999 #{Digest::MD5.hexdigest(line)}" + end + + hash[:line] = line hash[:provider] = self.name.to_s hash[:table] = table hash[:ensure] = :present @@ -127,7 +136,6 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir # Munge some vars here ... # proto should equal 'all' if undefined hash[:proto] = "all" if !hash.include?(:proto) - hash end @@ -145,6 +153,31 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir args end + def delete_args + count = [] + line = properties[:line].gsub(/\-A/, '-D').split + + # Grab all comment indices + line.each do |v| + if v =~ /"/ + count << line.index(v) + end + end + + if ! count.empty? + # Remove quotes and set first comment index to full string + line[count.first] = line[count.first..count.last].join(' ').gsub(/"/, '') + + # Make all remaining comment indices nil + ((count.first + 1)..count.last).each do |i| + line[i] = nil + end + end + + # Return array without nils + line.compact + end + def general_args debug "Current resource: %s" % resource.class args = [] diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 26b2977..522b3fd 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -195,6 +195,10 @@ Puppet::Type.newtype(:firewall) do desc "Rate limiting burst value (per second)." newvalue(/^\d+$/) end + + newparam(:line) do + desc 'Read-only property for caching the rule line' + end validate do debug("[validate]") diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fdc04bc..79fda18 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,8 +1,6 @@ dir = File.expand_path(File.dirname(__FILE__)) $LOAD_PATH.unshift File.join(dir, 'lib') -p dir - # Don't want puppet getting the command line arguments for rake or autotest ARGV.clear diff --git a/spec/unit/provider/iptables_prov_spec.rb b/spec/unit/provider/iptables_prov_spec.rb index aef682e..d27fc14 100644 --- a/spec/unit/provider/iptables_prov_spec.rb +++ b/spec/unit/provider/iptables_prov_spec.rb @@ -38,7 +38,6 @@ describe 'iptables provider detection' do :name => '000 test foo' }) }.should raise_error(Puppet::DevError, "Could not find a default provider for firewall") end - end describe 'iptables provider' do @@ -81,14 +80,57 @@ describe 'iptables provider' do end end - describe 'when modifying resources' do + describe 'when converting rules without comments to resources' do + before :each do + @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' + @resource = @provider.rule_to_hash(@rule, 'filter', 0) + @instance = @provider.new(@resource) + end + + it 'rule name contains a MD5 sum of the line' do + @resource[:name].should == "9999 #{Digest::MD5.hexdigest(@resource[:line])}" + end + end + + describe 'when creating resources' do before :each do @instance = @provider.new(@resource) @provider.expects(:execute).with(['/sbin/iptables-save']).returns("") end - it 'should do something' do + it 'insert_args should be an array' do @instance.insert_args.class.should == Array end end + + describe 'when modifying resources' do + before :each do + @instance = @provider.new(@resource) + @provider.expects(:execute).with(['/sbin/iptables-save']).returns "" + end + + it 'update_args should be an array' do + @instance.update_args.class.should == Array + end + end + + describe 'when deleting resources' do + before :each do + @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' + @resource = @provider.rule_to_hash(@rule, 'filter', 0) + @instance = @provider.new(@resource) + end + + it 'resource[:line] looks like the original rule' do + @resource[:line] == @rule + end + + it 'delete_args is an array' do + @instance.delete_args.class.should == Array + end + + it 'delete_args is the same as the rule string when joined' do + @instance.delete_args.join(' ').should == @rule.gsub(/\-A/, '-D') + end + end end