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

- Includes slight reorganization of providers
- Updates `dport`/`sport` section of `insync?` to allow for more accurate comparisons.

Author: david22swan

.rubocop_todo.yml

@@ -31,7 +31,7 @@ Metrics/BlockLength:
 # Offense count: 3
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 752
+  Max: 763
 
 # Offense count: 16
 # Configuration parameters: AllowedMethods, AllowedPatterns.

lib/puppet/provider/firewall/firewall.rb

@@ -4,6 +4,8 @@
 
 # Implementation for the iptables type using the Resource API.
 class Puppet::Provider::Firewall::Firewall
+  ###### GLOBAL VARIABLES ######
+
   # Command to list all chains and rules
   # $list_command = 'iptables-save'
   $list_command = {
@@ -253,6 +255,8 @@ class Puppet::Provider::Firewall::Firewall
     :bytecode, :ipvs, :helper, :zone, :cgroup, :rpfilter, :condition, :name, :notrack
   ]
 
+  ###### PUBLIC METHODS ######
+
   # Raw data is retrieved via `iptables -L` and then regexed to retrieve the different Chains and whether they have a set Policy
   def get(context)
     # Call the private method which returns the rules
@@ -319,6 +323,153 @@ def delete(context, name, is)
     PuppetX::Firewall::Utility.persist_iptables(context, name, is[:protocol])
   end
 
+  # Custom insync method
+  # Needed for uid and gid
+  def insync?(context, _name, property_name, is_hash, should_hash)
+    context.debug("Checking whether '#{property_name}' is out of sync")
+
+    # If either value is nil, no custom logic is required
+    return nil if is_hash[property_name].nil? || should_hash[property_name].nil?
+
+    case property_name
+    when :protocol
+      is = is_hash[property_name]
+      should = should_hash[property_name]
+
+      # Ensure the should value accurately matches the is
+      should = 'IPv4' if should == 'iptables'
+      should = 'IPv6' if should == 'ip6tables'
+
+      is == should
+    when :source, :destination
+      # Ensure source/destination has it's valid mask before you compare it
+      is_hash[property_name] == PuppetX::Firewall::Utility.host_to_mask(should_hash[property_name], should_hash[:protocol])
+    when :tcp_option, :ctproto, :hop_limit
+      # Ensure that the values are compared as strings
+      is_hash[property_name] == should_hash[property_name].to_s
+    when :tcp_flags
+      # Custom logic to account for `ALL` being returned as `FIN,SYN,RST,PSH,ACK,URG`
+      is = is_hash[property_name].split
+      should = should_hash[property_name].split
+
+      is = is.map { |x| (x == 'FIN,SYN,RST,PSH,ACK,URG') ? 'ALL' : x }
+      should = should.map { |x| (x == 'FIN,SYN,RST,PSH,ACK,URG') ? 'ALL' : x }
+
+      is.join(' ') == should.join(' ')
+    when :uid, :gid
+      require 'etc'
+      # The following code allow us to take into consideration unix mappings
+      # between string usernames and UIDs (integers). We also need to ignore
+      # spaces as they are irrelevant with respect to rule sync.
+
+      # Remove whitespace
+      is = is_hash[property_name].to_s.gsub(%r{\s+}, '')
+      should = should_hash[property_name].to_s.gsub(%r{\s+}, '')
+
+      # Keep track of negation, but remove the '!'
+      is_negate = ''
+      should_negate = ''
+      if is.start_with?('!')
+        is = is.gsub(%r{^!}, '')
+        is_negate = '!'
+      end
+      if should.start_with?('!')
+        should = should.gsub(%r{^!}, '')
+        should_negate = '!'
+      end
+
+      # If 'is' or 'should' contain anything other than digits or digit range,
+      # we assume that we have to do a lookup to convert to UID
+      is = Etc.getpwnam(is).uid unless is[%r{[0-9]+(-[0-9]+)?}] == is
+      should = Etc.getpwnam(should).uid unless should[%r{[0-9]+(-[0-9]+)?}] == should
+
+      "#{is_negate}#{is}" == "#{should_negate}#{should}"
+    when :mac_source
+      # Value of mac_source may be downcased when returned on certain OS, i.e. Debian 11 / Ubuntu 22.04 / SLES 15
+      is_hash[property_name].casecmp(should_hash[property_name]).zero?
+    when :state, :ctstate, :ctstatus
+      # Ensure that if both is and should are array values, they are correctly compared in order
+      is = is_hash[property_name]
+      should = should_hash[property_name]
+      return nil unless is.is_a?(Array) && should.is_a?(Array)
+
+      if is[0].start_with?('!')
+        is.append('!')
+        is[0] = is[0].gsub(%r{^!\s?}, '')
+      end
+      if should[0].start_with?('!')
+        should.append('!')
+        should[0] = should[0].gsub(%r{^!\s?}, '')
+      end
+      is.sort == should.sort
+    when :icmp
+      # Ensure that the values are compared to each other as icmp code numbers
+      is = PuppetX::Firewall::Utility.icmp_name_to_number(is_hash[property_name], is_hash[:protocol])
+      should = PuppetX::Firewall::Utility.icmp_name_to_number(should_hash[property_name], should_hash[:protocol])
+      is == should
+    when :log_level
+      # Ensure that the values are compared to each other as log level numbers
+      is = PuppetX::Firewall::Utility.log_level_name_to_number(is_hash[property_name])
+      should = PuppetX::Firewall::Utility.log_level_name_to_number(should_hash[property_name])
+      is == should
+    when :set_mark
+      # Ensure that the values are compared to eachother in hexidecimal format
+      is = PuppetX::Firewall::Utility.mark_mask_to_hex(is_hash[property_name])
+      should = PuppetX::Firewall::Utility.mark_mask_to_hex(should_hash[property_name])
+      is == should
+    when :match_mark, :connmark
+      # Ensure that the values are compared to eachother in hexidecimal format
+      is = PuppetX::Firewall::Utility.mark_to_hex(is_hash[property_name])
+      should = PuppetX::Firewall::Utility.mark_to_hex(should_hash[property_name])
+      is == should
+    when :time_start, :time_stop
+      # Ensure the values are compared in full `00:00:00` format
+      is = is_hash[property_name]
+      should = should_hash[property_name]
+
+      should = "0#{should}" if %r{^([0-9]):}.match?(should)
+      should = "#{should}:00" if %r{^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$}.match?(should)
+
+      is == should
+    when :jump
+      # Compare values as uppercase
+      is_hash[property_name].casecmp(should_hash[property_name]).zero?
+    when :dport, :sport
+      is = is_hash[property_name]
+      should = should_hash[property_name]
+
+      # Wrap compared values as arrays in order to simplify comparisons
+      is = [is] unless is.is_a?(Array)
+      should = [should] unless should.is_a?(Array)
+
+      # If first value includes a negation, retrieve it and set as it's own value
+      if is[0].start_with?('!')
+        is.append('!')
+        is[0] = is[0].gsub(%r{^!\s?}, '')
+      end
+      if should[0].start_with?('!')
+        should.append('!')
+        should[0] = should[0].gsub(%r{^!\s?}, '')
+      end
+
+      # Range can be passed as `-` but will always be set/returned as `:`
+      # Ensure values are sorted
+      is.sort == should.map { |port| port.to_s.tr('-', ':') }.sort
+    when :string_hex
+      # Compare the values with any whitespace removed
+      is = is_hash[property_name].to_s.gsub(%r{\s+}, '')
+      should = should_hash[property_name].to_s.gsub(%r{\s+}, '')
+
+      is == should
+    else
+      # Ensure that if both values are arrays, that they are sorted prior to comparison
+      return nil unless is_hash[property_name].is_a?(Array) && should_hash[property_name].is_a?(Array)
+
+      is_hash[property_name].sort == should_hash[property_name].sort
+    end
+  end
+
+  ###### PRIVATE METHODS ######
   ###### GET ######
 
   # Retrieve the rules
@@ -597,7 +748,7 @@ def self.create_absent(namevar, title)
     result
   end
 
-  ###### SET/UPDATE ######
+  ###### SET ######
 
   # Verify that the information being set is correct
   # @api.private
@@ -918,134 +1069,4 @@ def self.insert_order(context, name, chain, table, protocol)
     sorted_rules = rules.reject { |r| r.match($unmanaged_rule_regex) }.sort
     sorted_rules.index(name) + 1 + unnamed_offset
   end
-
-  # Custom insync method
-  # Needed for uid and gid
-  def insync?(context, _name, property_name, is_hash, should_hash)
-    context.debug("Checking whether #{property_name} is out of sync")
-
-    # If either value is nil, no custom logic is required
-    return nil if is_hash[property_name].nil? || should_hash[property_name].nil?
-
-    case property_name
-    when :protocol
-      is = is_hash[property_name]
-      should = should_hash[property_name]
-
-      # Ensure the should value accurately matches the is
-      should = 'IPv4' if should == 'iptables'
-      should = 'IPv6' if should == 'ip6tables'
-
-      is == should
-    when :source, :destination
-      # Ensure source/destination has it's valid mask before you compare it
-      is_hash[property_name] == PuppetX::Firewall::Utility.host_to_mask(should_hash[property_name], should_hash[:protocol])
-    when :tcp_option, :ctproto, :hop_limit
-      # Ensure that the values are compared as strings
-      is_hash[property_name] == should_hash[property_name].to_s
-    when :tcp_flags
-      # Custom logic to account for `ALL` being returned as `FIN,SYN,RST,PSH,ACK,URG`
-      is = is_hash[property_name].split
-      should = should_hash[property_name].split
-
-      is = is.map { |x| (x == 'FIN,SYN,RST,PSH,ACK,URG') ? 'ALL' : x }
-      should = should.map { |x| (x == 'FIN,SYN,RST,PSH,ACK,URG') ? 'ALL' : x }
-
-      is.join(' ') == should.join(' ')
-    when :uid, :gid
-      require 'etc'
-      # The following code allow us to take into consideration unix mappings
-      # between string usernames and UIDs (integers). We also need to ignore
-      # spaces as they are irrelevant with respect to rule sync.
-
-      # Remove whitespace
-      is = is_hash[property_name].to_s.gsub(%r{\s+}, '')
-      should = should_hash[property_name].to_s.gsub(%r{\s+}, '')
-
-      # Keep track of negation, but remove the '!'
-      is_negate = ''
-      should_negate = ''
-      if is.start_with?('!')
-        is = is.gsub(%r{^!}, '')
-        is_negate = '!'
-      end
-      if should.start_with?('!')
-        should = should.gsub(%r{^!}, '')
-        should_negate = '!'
-      end
-
-      # If 'is' or 'should' contain anything other than digits or digit range,
-      # we assume that we have to do a lookup to convert to UID
-      is = Etc.getpwnam(is).uid unless is[%r{[0-9]+(-[0-9]+)?}] == is
-      should = Etc.getpwnam(should).uid unless should[%r{[0-9]+(-[0-9]+)?}] == should
-
-      "#{is_negate}#{is}" == "#{should_negate}#{should}"
-    when :mac_source
-      # Value of mac_source may be downcased when returned on certain OS, i.e. Debian 11 / Ubuntu 22.04 / SLES 15
-      is_hash[property_name].casecmp(should_hash[property_name]).zero?
-    when :state, :ctstate, :ctstatus
-      # Ensure that if both is and should are array values, they are correctly compared in order
-      is = is_hash[property_name]
-      should = should_hash[property_name]
-      return nil unless is.is_a?(Array) && should.is_a?(Array)
-
-      if is[0].start_with?('!')
-        is.append('!')
-        is[0] = is[0].gsub(%r{^!\s?}, '')
-      end
-      if should[0].start_with?('!')
-        should.append('!')
-        should[0] = should[0].gsub(%r{^!\s?}, '')
-      end
-      is.sort == should.sort
-    when :icmp
-      # Ensure that the values are compared to each other as icmp code numbers
-      is = PuppetX::Firewall::Utility.icmp_name_to_number(is_hash[property_name], is_hash[:protocol])
-      should = PuppetX::Firewall::Utility.icmp_name_to_number(should_hash[property_name], should_hash[:protocol])
-      is == should
-    when :log_level
-      # Ensure that the values are compared to each other as log level numbers
-      is = PuppetX::Firewall::Utility.log_level_name_to_number(is_hash[property_name])
-      should = PuppetX::Firewall::Utility.log_level_name_to_number(should_hash[property_name])
-      is == should
-    when :set_mark
-      # Ensure that the values are compared to eachother in hexidecimal format
-      is = PuppetX::Firewall::Utility.mark_mask_to_hex(is_hash[property_name])
-      should = PuppetX::Firewall::Utility.mark_mask_to_hex(should_hash[property_name])
-      is == should
-    when :match_mark, :connmark
-      # Ensure that the values are compared to eachother in hexidecimal format
-      is = PuppetX::Firewall::Utility.mark_to_hex(is_hash[property_name])
-      should = PuppetX::Firewall::Utility.mark_to_hex(should_hash[property_name])
-      is == should
-    when :time_start, :time_stop
-      # Ensure the values are compared in full `00:00:00` format
-      is = is_hash[property_name]
-      should = should_hash[property_name]
-
-      should = "0#{should}" if %r{^([0-9]):}.match?(should)
-      should = "#{should}:00" if %r{^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$}.match?(should)
-
-      is == should
-    when :jump
-      # Compare values as uppercase
-      is_hash[property_name].casecmp(should_hash[property_name]).zero?
-    when :dport, :sport
-      # Ensure values are compared as string
-      # Range can be passed as `-` but will always be set/returned as `:`
-      is_hash[property_name] == should_hash[property_name].to_s.tr('-', ':') unless should_hash[property_name].is_a?(Array)
-      is_hash[property_name] == should_hash[property_name].map { |port| port.to_s.tr('-', ':') } if should_hash[property_name].is_a?(Array)
-    when :string_hex
-      # Compare the values with any whitespace removed
-      is = is_hash[property_name].to_s.gsub(%r{\s+}, '')
-      should = should_hash[property_name].to_s.gsub(%r{\s+}, '')
-
-      is == should
-    else
-      # Ensure that if both values are arrays, that they are sorted prior to comparison
-      return nil unless is_hash[property_name].is_a?(Array) && should_hash[property_name].is_a?(Array)
-
-      is_hash[property_name].sort == should_hash[property_name].sort
-    end
-  end
 end