(#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 a6efda85fb3a466021dfa8be94343d483af2faa3..50f366e34d1f46df2765cfc1cad8d506885c8a68 100644 (file)
--- 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 26b2977eaff462250b000b1de352a9cb5a69bcdb..522b3fd61959bb1f2f5def0dbb0f249a6f18ad72 100644 (file)
--- 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 fdc04bc9efb616fe311517d012ff724a8681b307..79fda18eef5fa7efdef2126a53176d45870e849c 100644 (file)
--- 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 aef682e7d9c2662cf83231ff97b7eb755aeb96a9..d27fc14dcd50b01c0ac60d41dcd5c89ac7f4bea1 100644 (file)
--- 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