Fix handling of weight attribute in simple_iptables_rule resource

Typically, the order of the rules in an iptables chain is determined by the
order of simple_iptables_rules resources in a recipe. However, if a non-standard
`weight` attribute value was used in *any* simple_iptables_rule, the relative
order of the rules that had the same weight became arbitrary. This is because
the code repeatedly calls `sort!` on the array of rules after adding every rule,
using the weights as the sort key, which potentially causes rules with the same
weight to be sorted in a different order with respect to each other on each
invocation of the `sort!` method, since they're viewed as equivalent.

Store rules in a hash where keys are weights and values are arrays of rules
having that weight. As new rules are added, they're appended to the relevant list.
When the final iptables rule file is generated, we output all the rules in
decreasing weight order.

Also take this opportunity to store rule in default-precedence attributes rather
than "normal" (persistent) attributes. As we reset all the attributes at the
beginning of the Chef run, there is no point to persisting the attributes.
This also resolves deprecation warnings for node.set in newer versions of Chef.

Resolves #81

attributes/default.rb

@@ -1,7 +1,7 @@
-default["simple_iptables"]["ipv4"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
+default["simple_iptables"]["ipv4"]["rules"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
 default["simple_iptables"]["ipv4"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
 default["simple_iptables"]["ipv4"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
-default["simple_iptables"]["ipv6"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
+default["simple_iptables"]["ipv6"]["rules"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
 default["simple_iptables"]["ipv6"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
 default["simple_iptables"]["ipv6"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
 

providers/policy.rb

@@ -11,6 +11,6 @@
 
 def handle_policy(new_resource, ip_version)
   Chef::Log.debug("[#{ip_version}] setting policy for #{new_resource.chain} to #{new_resource.policy}")
-  node.set["simple_iptables"][ip_version]["policy"][new_resource.table][new_resource.chain] = new_resource.policy
+  node.default["simple_iptables"][ip_version]["policy"][new_resource.table][new_resource.chain] = new_resource.policy
   return true
 end

providers/rule.rb

@@ -25,36 +25,40 @@ def handle_rule(new_resource, ip_version)
   else
     rules = ['']
   end
-  if not node["simple_iptables"][ip_version]["chains"][new_resource.table].include?(new_resource.chain)
-    node.set["simple_iptables"][ip_version]["chains"][new_resource.table] = node["simple_iptables"][ip_version]["chains"][new_resource.table].dup << new_resource.chain unless ["PREROUTING", "INPUT", "FORWARD", "OUTPUT", "POSTROUTING"].include?(new_resource.chain)
+
+  unless node["simple_iptables"][ip_version]["rules"][new_resource.table].include?(new_resource.weight)
+    node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight] = []
+  end
+  unless node["simple_iptables"][ip_version]["chains"][new_resource.table].include?(new_resource.chain)
+    unless ["PREROUTING", "INPUT", "FORWARD", "OUTPUT", "POSTROUTING"].include?(new_resource.chain)
+      node.default["simple_iptables"][ip_version]["chains"][new_resource.table] << new_resource.chain
+    end
     unless new_resource.chain == new_resource.direction || new_resource.direction == :none
-      node.set["simple_iptables"][ip_version]["rules"][new_resource.table] << {:rule => "-A #{new_resource.direction} #{new_resource.chain_condition} --jump #{new_resource.chain}", :weight => new_resource.weight}
+      node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight] << "-A #{new_resource.direction} #{new_resource.chain_condition} --jump #{new_resource.chain}"
     end
   end
 
   # Then apply the rules to the node
   updated = false
   rules.each do |rule|
-    new_rule_string = rule_string(new_resource, rule, false)
-    new_rule = {:rule => new_rule_string, :weight => new_resource.weight}
-    table_rules = node.set["simple_iptables"][ip_version]["rules"][new_resource.table]
+    new_rule = rule_string(new_resource, rule, false)
+    table_rules = node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight]
 
     unless table_rules.include?(new_rule)
       table_rules << new_rule
-      table_rules.sort! {|a,b| a[:weight] <=> b[:weight]}
       updated = true
-      Chef::Log.debug("[#{ip_version}] added rule '#{new_rule_string}'")
+      Chef::Log.debug("[#{ip_version}] added rule '#{new_rule}'")
     else
-      Chef::Log.debug("[#{ip_version}] ignoring duplicate simple_iptables_rule '#{new_rule_string}'")
+      Chef::Log.debug("[#{ip_version}] ignoring duplicate simple_iptables_rule '#{new_rule}'")
     end
   end
-  return updated
+  updated
 end
 
 def rule_string(new_resource, rule, include_table)
   jump = new_resource.jump ? "--jump #{new_resource.jump} " : ""
   table = include_table ? "--table #{new_resource.table} " : ""
-  comment = %Q{ -m comment --comment "#{new_resource.comment || new_resource.name}" }
+  comment = %Q{ -m comment --comment "#{new_resource.comment || new_resource.name}"}
   rule = "#{table}-A #{new_resource.chain} #{jump}#{rule}#{comment}"
   rule
 end

recipes/default.rb

@@ -33,16 +33,9 @@
 # better way to do this, please let me know!
 ruby_block "run-iptables-resources-early" do
   block do
-    # Before executing the simple_iptables_* resources, reset the
-    # node attributes to their defaults. This gives "action :delete"
-    # semantics for free by removing a resource from a recipe.
-    node.set["simple_iptables"]["ipv4"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
-    node.set["simple_iptables"]["ipv4"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
-    node.set["simple_iptables"]["ipv4"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
-
-    node.set["simple_iptables"]["ipv6"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
-    node.set["simple_iptables"]["ipv6"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []}
-    node.set["simple_iptables"]["ipv6"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}}
+    # Clear old normal-level attributes set by previous versions of the cookbook
+    node.rm_normal("simple_iptables", "ipv4")
+    node.rm_normal("simple_iptables", "ipv6")
     # Then run all the simple_iptables_* resources
     run_context.resource_collection.each do |resource|
       if resource.kind_of?(Chef::Resource::SimpleIptablesRule)
@@ -54,7 +47,7 @@
       end
     end
 
-    Chef::Log.debug("After run-iptables-resources-early data is: #{node['simple_iptables'].inspect}")
+    Chef::Log.debug("After run-iptables-resources-early data is: #{node['simple_iptables']}")
   end
 end
 
@@ -137,4 +130,3 @@
     end
   end
 end
-

spec/spec_helper.rb

@@ -0,0 +1,6 @@
+require "chefspec"
+
+RSpec.configure do |config|
+  config.cookbook_path = ["..",
+                          "spec/support/cookbooks"]
+end

spec/support/cookbooks/test_simple_iptables/metadata.rb

@@ -0,0 +1,13 @@
+maintainer       "Arctic Wolf Networks"
+maintainer_email "[email protected]"
+license          "BSD"
+description      "Support cookbook for ChefSpec tests for simple_iptables"
+version          "1.0.0"
+name             "test_simple_iptables"
+
+supports "debian", ">= 6.0"
+supports "centos", ">= 5.8"
+supports "redhat", ">= 5.8"
+supports "ubuntu", ">= 10.04"
+
+depends "simple_iptables"

spec/support/cookbooks/test_simple_iptables/recipes/weight.rb

@@ -0,0 +1,39 @@
+include_recipe "simple_iptables"
+
+simple_iptables_rule "rule1" do
+  direction "INPUT"
+  rule "rule1 content"
+  jump "ACCEPT"
+end
+
+simple_iptables_rule "rule2" do
+  direction "INPUT"
+  rule "rule2 content"
+  jump "ACCEPT"
+end
+
+simple_iptables_rule "rule3" do
+  direction "INPUT"
+  rule "rule3 content"
+  jump "REJECT"
+  weight 95
+end
+
+simple_iptables_rule "rule4" do
+  direction "INPUT"
+  rule ["rule4.1 content", "rule4.2 content"]
+  jump "ACCEPT"
+end
+
+simple_iptables_rule "rule5" do
+  direction "INPUT"
+  rule "rule5 content"
+  jump "ACCEPT"
+end
+
+simple_iptables_rule "rule6" do
+  direction "INPUT"
+  rule "rule6 content"
+  jump "REJECT"
+  weight 95
+end

spec/weight_spec.rb

@@ -0,0 +1,69 @@
+require "spec_helper"
+
+describe "test_simple_iptables::weight" do
+  let(:chef_run) do
+    ChefSpec::SoloRunner.new(platform: "ubuntu", version: "14.04",
+                             step_into: ["ruby_block", "simple_iptables_rule"])
+  end
+
+  it "generates rules in resource appearance order for every weight" do
+    expected_rules =
+%{# This file generated by Chef. Changes will be overwritten.
+*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+COMMIT
+# Completed
+# This file generated by Chef. Changes will be overwritten.
+*mangle
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+COMMIT
+# Completed
+# This file generated by Chef. Changes will be overwritten.
+*filter
+:INPUT ACCEPT [0:0]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:rule1 - [0:0]
+:rule2 - [0:0]
+:rule3 - [0:0]
+:rule4 - [0:0]
+:rule5 - [0:0]
+:rule6 - [0:0]
+-A INPUT  --jump rule1
+-A rule1 --jump ACCEPT rule1 content -m comment --comment "rule1"
+-A INPUT  --jump rule2
+-A rule2 --jump ACCEPT rule2 content -m comment --comment "rule2"
+-A INPUT  --jump rule4
+-A rule4 --jump ACCEPT rule4.1 content -m comment --comment "rule4"
+-A rule4 --jump ACCEPT rule4.2 content -m comment --comment "rule4"
+-A INPUT  --jump rule5
+-A rule5 --jump ACCEPT rule5 content -m comment --comment "rule5"
+-A INPUT  --jump rule3
+-A rule3 --jump REJECT rule3 content -m comment --comment "rule3"
+-A INPUT  --jump rule6
+-A rule6 --jump REJECT rule6 content -m comment --comment "rule6"
+COMMIT
+# Completed
+# This file generated by Chef. Changes will be overwritten.
+*raw
+:PREROUTING ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+COMMIT
+# Completed
+}
+    chef_run.converge(described_recipe)
+    t = chef_run.template("/etc/iptables-rules")
+    actual_content = ChefSpec::Renderer.new(chef_run, t).content
+    File.open("/tmp/foo", "w") { |file| file.write(actual_content) }
+    File.open("/tmp/foo1", "w") { |file| file.write(expected_rules) }
+    expect(chef_run).to render_file("/etc/iptables-rules")
+                        .with_content(expected_rules)
+  end
+end

templates/default/ip6tables-rules.erb

@@ -9,8 +9,10 @@
 <% node["simple_iptables"]["ipv6"]["chains"]["nat"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv6"]["rules"]["nat"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv6"]["rules"]["nat"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -26,8 +28,10 @@ COMMIT
 <% node["simple_iptables"]["ipv6"]["chains"]["mangle"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv6"]["rules"]["mangle"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv6"]["rules"]["mangle"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -41,8 +45,10 @@ COMMIT
 <% node["simple_iptables"]["ipv6"]["chains"]["filter"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv6"]["rules"]["filter"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv6"]["rules"]["filter"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -55,8 +61,10 @@ COMMIT
 <% node["simple_iptables"]["ipv6"]["chains"]["raw"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv6"]["rules"]["raw"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv6"]["rules"]["raw"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed

templates/default/iptables-rules.erb

@@ -8,8 +8,10 @@
 <% node["simple_iptables"]["ipv4"]["chains"]["nat"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv4"]["rules"]["nat"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv4"]["rules"]["nat"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -25,8 +27,10 @@ COMMIT
 <% node["simple_iptables"]["ipv4"]["chains"]["mangle"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv4"]["rules"]["mangle"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv4"]["rules"]["mangle"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -40,8 +44,10 @@ COMMIT
 <% node["simple_iptables"]["ipv4"]["chains"]["filter"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv4"]["rules"]["filter"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv4"]["rules"]["filter"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed
@@ -54,8 +60,10 @@ COMMIT
 <% node["simple_iptables"]["ipv4"]["chains"]["raw"].each do |chain| -%>
 :<%= chain %> - [0:0]
 <% end -%>
-<% node["simple_iptables"]["ipv4"]["rules"]["raw"].each do |rule| -%>
-<%= rule[:rule] %>
+<% node["simple_iptables"]["ipv4"]["rules"]["raw"].sort.each do |weight, rules| -%>
+<% rules.each do |rule| -%>
+<%= rule %>
+<% end -%>
 <% end -%>
 COMMIT
 # Completed

test/fixtures/smoke/recipes/ipv6_default.rb

@@ -1,4 +1,4 @@
-node.set["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"]
+node.default["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"]
 
 include_recipe "simple_iptables::default"
 

test/fixtures/smoke/recipes/ipv6_tables.rb

@@ -1,6 +1,6 @@
-node.set["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"]
-node.set["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle)
-node.set["simple_iptables"]["ipv6"]["tables"] = %w(filter mangle)
+node.default["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"]
+node.default["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle)
+node.default["simple_iptables"]["ipv6"]["tables"] = %w(filter mangle)
 
 include_recipe "simple_iptables::default"
 

test/fixtures/smoke/recipes/tables.rb

@@ -1,4 +1,4 @@
-node.set["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle)
+node.default["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle)
 
 include_recipe "simple_iptables::default"