(CAT-1260) Addition of tests for firewall provider private get methods

- Slight reorganization of providers
- Corrections made to certain regex
- Clarification of returned errors

Author: david22swan

lib/puppet/provider/firewall/firewall.rb

@@ -473,7 +473,6 @@ def insync?(context, _name, property_name, is_hash, should_hash)
   ###### GET ######
 
   # Retrieve the rules
-  # Placed in utility as the code is reused when determining order
   # Optional values lets you return a simplified set of data, used for determining order when adding/updating/deleting rules,
   #   while also allowing for the protocols used to retrieve the rules to be limited.
   # @api.private
@@ -594,7 +593,7 @@ def self.rule_to_hash(_context, rule, table_name, protocol)
         split_regex = value.split(%r{ })
         if rule[0].match(Regexp.new("#{split_regex[1]}\\s(?:(!)\\s)?#{split_regex[2]}\\s"))
           # The exact information retrieved changes dependeing on the key
-          value_regex = Regexp.new("#{split_regex[1]}\\s(?:(!)\\s)?#{split_regex[2]}\\s(\\S+)\\s(--limit-iface-(?:in|out))?") if [:src_type, :dst_type].include?(key)
+          value_regex = Regexp.new("#{split_regex[1]}\\s(?:(!)\\s)?#{split_regex[2]}\\s(\\S+)\\s?(--limit-iface-(?:in|out))?") if [:src_type, :dst_type].include?(key)
           value_regex = Regexp.new("#{split_regex[1]}\\s(?:(!)\\s)?#{split_regex[2]}\\s(\\S+\\s\\S+)") if [:ipset].include?(key)
           value_regex = Regexp.new("#{split_regex[1]}\\s(?:(!)\\s)?#{split_regex[2]}\\s(\\S+)") if [:match_mark, :mss, :connmark].include?(key)
           # Since multiple values can be recovered, we must loop through each instance
@@ -635,11 +634,11 @@ def self.rule_to_hash(_context, rule, table_name, protocol)
           rule_hash[key] = key_value[0]
         end
       when :recent
-        if rule[0].match(Regexp.new("#{value}\\s--"))
+        if rule[0].match(Regexp.new("#{value}\\s"))
           value_regex = Regexp.new("#{value}\\s(!\\s)?--(\\S+)")
           key_value = rule[0].scan(value_regex)[0]
           # If it has, combine the retrieved '!' with the actual value to make one string
-          key_value[1] = [key_value[0], key_value[1]].join(' ') unless key_value[0].nil?
+          key_value[1] = [key_value[0], key_value[1]].join('') unless key_value[0].nil?
           rule_hash[key] = key_value[1] if key_value
         end
       when :rpfilter
@@ -709,7 +708,7 @@ def self.validate_get(_context, rules)
     rules.each do |rule|
       names << rule[:name]
     end
-    raise 'Duplicate names have been found within your Firewalls. This will prevent the module from working correctly and must be manually resolved.' if names.length != names.uniq.length
+    raise ArgumentError, 'Duplicate names have been found within your Firewalls. This prevents the module from working correctly and must be manually resolved.' if names.length != names.uniq.length
     # Verify that the current order of the retrieved puppet rules is correct
   end
 
@@ -718,7 +717,7 @@ def self.validate_get(_context, rules)
   def self.process_get(_context, rule_hash, rule, counter)
     # Puppet-firewall requires that all rules have structured comments (resource names) and will fail if a
     # rule in iptables does not have a matching comment.
-    if !rule_hash[:name]
+    if !rule_hash.key?(:name)
       num = 9000 + counter
       rule_hash[:name] = "#{num} #{Digest::SHA256.hexdigest(rule[0])}"
     elsif !rule_hash[:name].match(%r{(^\d+(?:[ \t-]\S+)+$)})

lib/puppet/provider/firewallchain/firewallchain.rb

@@ -4,6 +4,8 @@
 
 # Implementation for the firewallchain type using the Resource API.
 class Puppet::Provider::Firewallchain::Firewallchain
+  ###### GLOBAL VARIABLES ######
+
   # Command to list all chains and rules
   $list_command = {
     'IPv4' => 'iptables-save',
@@ -31,6 +33,8 @@ class Puppet::Provider::Firewallchain::Firewallchain
   # Check if the given chain name references a built in one
   $built_in_regex = %r{^(?:INPUT|OUTPUT|FORWARD|PREROUTING|POSTROUTING)$}
 
+  ###### PUBLIC METHODS ######
+
   # Raw data is retrieved via `iptables-save` and then regexed to retrieve the different Chains and whether they have a set Policy
   def get(_context)
     # Create empty return array
@@ -120,6 +124,23 @@ def delete(context, name, is)
     PuppetX::Firewall::Utility.persist_iptables(context, name, is[:protocol])
   end
 
+  # Custom insync method
+  def insync?(context, _name, property_name, _is_hash, _should_hash)
+    context.debug("Checking whether '#{property_name}' is out of sync")
+
+    case property_name
+    when :purge, :ignore, :ignore_foreign
+      # Suppres any update notifications for the purge/ignore(_foreign) values
+      # They are used in the generate method which is ran prior to this point and have no
+      # bearing on it's actual state.
+      true
+    else
+      nil
+    end
+  end
+
+  ###### PRIVATE METHODS ######
+
   # Process the information so that it can be correctly applied
   # @api.private
   def self.process_input(is, should)
@@ -203,19 +224,4 @@ def generate(_context, title, _is, should)
 
     rules_resources
   end
-
-  # Custom insync method
-  def insync?(context, _name, property_name, _is_hash, _should_hash)
-    context.debug("Checking whether '#{property_name}' is out of sync")
-
-    case property_name
-    when :purge, :ignore, :ignore_foreign
-      # Suppres any update notifications for the purge/ignore(_foreign) values
-      # They are used in the generate method which is ran prior to this point and have no
-      # bearing on it's actual state.
-      true
-    else
-      nil
-    end
-  end
 end

lib/puppet/type/firewall.rb

@@ -942,9 +942,9 @@
       desc: <<-DESC
       Matches against the specified ipset list.
       Requires ipset kernel module. Will accept a single element or an array.
-      The value is the name of the blacklist, followed by a space, and then
+      The value is the name of the denylist, followed by a space, and then
       'src' and/or 'dst' separated by a comma.
-      For example: 'blacklist src,dst'
+      For example: 'denylist src,dst'
       To negate simply place a space seperated `!` at the beginning of a value.
       Values can de negated independently.
       DESC

spec/acceptance/firewall_duplicate_comment_spec.rb

@@ -31,7 +31,7 @@ class { 'firewall': }
       run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 552 -j ACCEPT -m comment --comment "550 destination"')
 
       apply_manifest(pp) do |r|
-        expect(r.stderr).to include('Duplicate names have been found within your Firewalls. This will prevent the module from working correctly and must be manually resolved.')
+        expect(r.stderr).to include('Duplicate names have been found within your Firewalls. This prevents the module from working correctly and must be manually resolved.')
       end
     end
   end