Merge pull request #1160 from david22swan/GH-1158/main/attribute_fix

(GH-1158) Fix for `dport/sport/state/ctstate/ctstatus` comparisons

Author: Ramesh7

lib/puppet/provider/firewall/firewall.rb

@@ -384,21 +384,6 @@ def insync?(context, _name, property_name, is_hash, should_hash)
     when :mac_source, :jump
       # Value of mac_source/jump may be downcased or upcased when returned depending on the OS
       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])
@@ -428,27 +413,31 @@ def insync?(context, _name, property_name, is_hash, should_hash)
       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 :dport, :sport
+    when :dport, :sport, :state, :ctstate, :ctstatus
       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)
+      # Unique logic is only needed when both values are arrays
+      return nil unless is.is_a?(Array) && 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?}, '')
+      # Ensure values are sorted
+      # Ensure any negation includes only the first value
+      is_negated = true if %r{^!\s}.match?(is[0].to_s)
+      is.each_with_index do |_value, _index|
+        is = is.map { |value| value.to_s.tr('! ', '') }.sort
       end
-      if should[0].start_with?('!')
-        should.append('!')
-        should[0] = should[0].gsub(%r{^!\s?}, '')
+      is[0] = ['!', is[0]].join(' ') if is_negated
+
+      should_negated = true if %r{^!\s}.match?(should[0].to_s)
+      should.each_with_index do |_value, _index|
+        should = should.map { |value| value.to_s.tr('! ', '') }.sort
+        # Port range can be passed as `-` but will always be set/returned as `:`
+        ports = [:dport, :sport]
+        should = should.map { |value| value.to_s.tr('-', ':') }.sort if ports.include?(property_name)
       end
+      should[0] = ['!', should[0]].join(' ') if should_negated
 
-      # 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
+      is == should
     when :string_hex
       # Compare the values with any whitespace removed
       is = is_hash[property_name].to_s.gsub(%r{\s+}, '')

spec/unit/puppet/provider/firewall/firewall_public_spec.rb

@@ -156,6 +156,7 @@
           { is_hash: { state: 'NEW' }, should_hash: { state: ['NEW', 'INVALID'] }, result: nil },
           { is_hash: { state: ['INVALID', 'NEW'] }, should_hash: { state: ['NEW', 'INVALID'] }, result: true },
           { is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', 'INVALID'] }, result: true },
+          { is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', '! INVALID'] }, result: true },
           { is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', 'INVALID', 'UNTRACKED'] }, result: false },
         ] },
         { testing: 'icmp', property_name: :icmp, comparisons: [
@@ -200,12 +201,11 @@
           { is_hash: { jump: 'accept' }, should_hash: { jump: 'drop' }, result: false },
         ] },
         { testing: 'dport/sport', property_name: :dport, comparisons: [
-          { is_hash: { dport: '! 50' }, should_hash: { dport: '! 50' }, result: true },
-          { is_hash: { dport: '50:60' }, should_hash: { dport: '50-60' }, result: true },
-          { is_hash: { dport: ['50:60'] }, should_hash: { dport: '50-60' }, result: true },
+          { is_hash: { dport: '50' }, should_hash: { dport: '50' }, result: nil },
+          { is_hash: { dport: ['50:60'] }, should_hash: { dport: '50-60' }, result: nil },
           { is_hash: { dport: ['50:60'] }, should_hash: { dport: ['50-60'] }, result: true },
-          { is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 90', '50-60'] }, result: true },
-          { is_hash: { dport: '50' }, should_hash: { dport: '90' }, result: false },
+          { is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 90', '! 50-60'] }, result: true },
+          { is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 100', '! 60-70'] }, result: false },
         ] },
         { testing: 'string_hex', property_name: :string_hex, comparisons: [
           { is_hash: { string_hex: '! |f4 6d 04 25 b2 02 00 0a|' }, should_hash: { string_hex: '! |f46d0425b202000a|' }, result: true },