From c4ba81f1a99b3ab548e4b30947fb36dffde0a710 Mon Sep 17 00:00:00 2001
From: Alexander Ivanes <vergil@drive.net>
Date: Fri, 5 Apr 2024 20:57:54 +0300
Subject: [PATCH] Port ranges as string with hyphen should work

---
 lib/puppet/provider/firewall/firewall.rb      | 42 +++++++++++--------
 .../firewall_attributes_exceptions_spec.rb    | 30 +++++++++++++
 .../provider/firewall/firewall_public_spec.rb |  4 +-
 3 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/lib/puppet/provider/firewall/firewall.rb b/lib/puppet/provider/firewall/firewall.rb
index dc41b3eb7..5381d511e 100644
--- a/lib/puppet/provider/firewall/firewall.rb
+++ b/lib/puppet/provider/firewall/firewall.rb
@@ -416,28 +416,34 @@ def insync?(context, _name, property_name, is_hash, should_hash)
     when :dport, :sport, :state, :ctstate, :ctstatus
       is = is_hash[property_name]
       should = should_hash[property_name]
+      ports = [:dport, :sport]
+
+      if is.is_a?(Array) && should.is_a?(Array)
+        # 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
+        is[0] = ['!', is[0]].join(' ') if is_negated
 
-      # Unique logic is only needed when both values are arrays
-      return nil unless is.is_a?(Array) && should.is_a?(Array)
-
-      # 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
-      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 `:`
+          should = should.map { |value| value.to_s.tr('-', ':') }.sort if ports.include?(property_name)
+        end
+        should[0] = ['!', should[0]].join(' ') if should_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
+        is == should
+      elsif is.is_a?(String) && should.is_a?(String)
         # 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
+        should = should.tr('-', ':') if ports.include?(property_name)
 
-      is == should
+        is == should
+      else
+        return nil
+      end
     when :string_hex
       # Compare the values with any whitespace removed
       is = is_hash[property_name].to_s.gsub(%r{\s+}, '')
diff --git a/spec/acceptance/firewall_attributes_exceptions_spec.rb b/spec/acceptance/firewall_attributes_exceptions_spec.rb
index 9ad48f50c..8a679bd22 100644
--- a/spec/acceptance/firewall_attributes_exceptions_spec.rb
+++ b/spec/acceptance/firewall_attributes_exceptions_spec.rb
@@ -68,6 +68,21 @@ class { '::firewall': }
         end
       end
     end
+
+    context 'when port range as a string' do
+      pp23 = <<-PUPPETCODE
+          class { '::firewall': }
+          firewall { '562 - test port range':
+            proto  => tcp,
+            dport  => '561-570',
+            jump   => accept,
+          }
+      PUPPETCODE
+      it 'applies' do
+        idempotent_apply(pp23)
+        apply_manifest(pp23, catch_changes: true)
+      end
+    end
   end
 
   describe 'ensure' do
@@ -652,6 +667,21 @@ class { '::firewall': }
         end
       end
     end
+
+    context 'when port range as a string' do
+      pp20 = <<-PUPPETCODE
+          class { '::firewall': }
+          firewall { '561 - test port range':
+            proto  => tcp,
+            sport  => '561-570',
+            jump   => accept,
+          }
+      PUPPETCODE
+      it 'applies' do
+        idempotent_apply(pp20)
+        apply_manifest(pp20, catch_changes: true)
+      end
+    end
   end
 
   describe 'source' do
diff --git a/spec/unit/puppet/provider/firewall/firewall_public_spec.rb b/spec/unit/puppet/provider/firewall/firewall_public_spec.rb
index eaaf60271..e9405f6cc 100644
--- a/spec/unit/puppet/provider/firewall/firewall_public_spec.rb
+++ b/spec/unit/puppet/provider/firewall/firewall_public_spec.rb
@@ -151,7 +151,7 @@
           { is_hash: { mac_source: '! 0A:1B:3C:4D:5E:6F' }, should_hash: { mac_source: '0A:1B:3C:4D:5E:6F' }, result: false },
         ] },
         { testing: 'state/ctstate/ctstatus', property_name: :state, comparisons: [
-          { is_hash: { state: 'NEW' }, should_hash: { state: 'NEW' }, result: nil },
+          { is_hash: { state: 'NEW' }, should_hash: { state: 'NEW' }, result: true },
           { is_hash: { state: ['NEW'] }, should_hash: { state: 'NEW' }, result: nil },
           { is_hash: { state: 'NEW' }, should_hash: { state: ['NEW', 'INVALID'] }, result: nil },
           { is_hash: { state: ['INVALID', 'NEW'] }, should_hash: { state: ['NEW', 'INVALID'] }, result: true },
@@ -201,7 +201,7 @@
           { 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: nil },
+          { is_hash: { dport: '50' }, should_hash: { dport: '50' }, result: true },
           { 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 },