(CAT-1260) Unit test coverage for firewallchain Provider

Author: david22swan

lib/puppet/provider/firewallchain/firewallchain.rb

@@ -89,7 +89,7 @@ def set(context, changes)
   end
 
   def create(context, name, should)
-    context.notice("Creating '#{name}' with #{should.inspect}")
+    context.notice("Creating Chain '#{name}' with #{should.inspect}")
     Puppet::Provider.execute([$base_command[should[:protocol]], should[:table], $chain_create_command, should[:chain]].join(' '))
     PuppetX::Firewall::Utility.persist_iptables(context, name, should[:protocol])
   end
@@ -99,7 +99,7 @@ def update(context, name, should, is)
     return if !$built_in_regex.match(should[:chain]) ||
               ($built_in_regex.match(should[:chain]) && is[:policy] == should[:policy])
 
-    context.notice("Updating '#{name}' with #{should.inspect}")
+    context.notice("Updating Chain '#{name}' with #{should.inspect}")
     Puppet::Provider.execute([$base_command[should[:protocol]], should[:table], $chain_policy_command, should[:chain], should[:policy].upcase].join(' '))
     PuppetX::Firewall::Utility.persist_iptables(context, name, should[:protocol])
   end
@@ -129,10 +129,11 @@ def self.process_input(is, should)
     should[:name] = should[:title] if should[:name].nil?
     should[:chain], should[:table], should[:protocol] = should[:name].split(':')
 
-    # If an in-built chain, always treat it as being present
+    # If an in-built chain, always treat it as being present and ensure it is assigned a policy
     # The retrieval of in-built chains may get confused by `iptables-save` tendency to not return table information
     # for tables that have not yet been interacted with.
     is[:ensure] = 'present' if $built_in_regex.match(is[:chain])
+    is[:policy] = 'accept' if $built_in_regex.match(is[:chain]) && is[:policy].nil?
     # For the same reason assign it the default policy as an intended state if it does not have one
     should[:policy] = 'accept' if $built_in_regex.match(should[:chain]) && should[:policy].nil?
 
@@ -145,23 +146,23 @@ def self.verify(_is, should)
     # Verify that no incorrect chain names are passed
     case should[:table]
     when 'filter'
-      raise ArgumentError, "INPUT, OUTPUT and FORWARD are the only inbuilt chains that can be used in table 'filter'" if %r{^(PREROUTING|POSTROUTING|BROUTING)$}.match?(should[:chain])
+      raise ArgumentError, 'INPUT, OUTPUT and FORWARD are the only inbuilt chains that can be used in table \'filter\'' if %r{^(PREROUTING|POSTROUTING|BROUTING)$}.match?(should[:chain])
     when 'mangle'
-      raise ArgumentError, "PREROUTING, POSTROUTING, INPUT, FORWARD and OUTPUT are the only inbuilt chains that can be used in table 'mangle'" if %r{^(BROUTING)$}.match?(should[:chain])
+      raise ArgumentError, 'PREROUTING, POSTROUTING, INPUT, FORWARD and OUTPUT are the only inbuilt chains that can be used in table \'mangle\'' if %r{^(BROUTING)$}.match?(should[:chain])
     when 'nat'
-      raise ArgumentError, "PREROUTING, POSTROUTING, INPUT, and OUTPUT are the only inbuilt chains that can be used in table 'nat'" if %r{^(BROUTING|FORWARD)$}.match?(should[:chain])
-      raise ArgumentError, "table nat isn't valid in IPv6. You must specify ':IPv4' as the name suffix" if %r{^(IP(v6)?)?$}.match?(should[:protocol])
+      raise ArgumentError, 'PREROUTING, POSTROUTING, INPUT, and OUTPUT are the only inbuilt chains that can be used in table \'nat\'' if %r{^(BROUTING|FORWARD)$}.match?(should[:chain])
+      raise ArgumentError, 'table nat isn\'t valid in IPv6. You must specify \':IPv4\' as the name suffix' if %r{^(IP(v6)?)?$}.match?(should[:protocol])
     when 'raw'
       raise ArgumentError, 'PREROUTING and OUTPUT are the only inbuilt chains in the table \'raw\'' if %r{^(POSTROUTING|BROUTING|INPUT|FORWARD)$}.match?(should[:chain])
     when 'broute'
       raise ArgumentError, 'BROUTE is only valid with protocol \'ethernet\'' if should[:protocol] != 'ethernet'
       raise ArgumentError, 'BROUTING is the only inbuilt chain allowed on on table \'broute\'' if %r{^PREROUTING|POSTROUTING|INPUT|FORWARD|OUTPUT$}.match?(should[:chain])
     when 'security'
-      raise ArgumentError, "INPUT, OUTPUT and FORWARD are the only inbuilt chains that can be used in table 'security'" if %r{^(PREROUTING|POSTROUTING|BROUTING)$}.match?(should[:chain])
+      raise ArgumentError, 'INPUT, OUTPUT and FORWARD are the only inbuilt chains that can be used in table \'security\'' if %r{^(PREROUTING|POSTROUTING|BROUTING)$}.match?(should[:chain])
     end
 
     # Verify that Policy is only passed for the inbuilt chains
-    raise "`policy` can only be set on Internal Chains. Setting for `#{should[:name]}` is invalid" if !$built_in_regex.match(should[:chain]) && should.key?(:policy)
+    raise ArgumentError, "'policy' can only be set on Internal Chains. Setting for '#{should[:name]}' is invalid" if !$built_in_regex.match(should[:chain]) && should.key?(:policy)
 
     # Warn that inbuilt chains will be flushed, not deleted
     warn "Warning: Inbuilt Chains may not be deleted. Chain `#{should[:name]}` will be flushed and have it's policy reverted to default." if $built_in_regex.match(should[:chain]) &&
@@ -186,6 +187,8 @@ def generate(_context, title, _is, should)
     end
 
     # Remove rules which match our ignore filter
+    # Ensure ignore value is wrapped as an array to simplify the code
+    should[:ignore] = [should[:ignore]] if should[:ignore].is_a?(String)
     rules_resources.delete_if { |resource| should[:ignore].find_index { |ignore| resource.rsapi_current_state[:name].match(ignore) } } if should[:ignore]
 
     # Remove rules that were (presumably) not put in by puppet
@@ -203,7 +206,7 @@ def generate(_context, title, _is, should)
 
   # Custom insync method
   def insync?(context, _name, property_name, _is_hash, _should_hash)
-    context.debug("Checking whether #{property_name} is out of sync")
+    context.debug("Checking whether '#{property_name}' is out of sync")
 
     case property_name
     when :purge, :ignore, :ignore_foreign