Merge pull request #1212 from 2fa/port-ranges-with-hyphens

Port ranges as string with hyphen as range indicator should work

Author: shubhamshinde360

lib/puppet/provider/firewall/firewall.rb

@@ -418,28 +418,32 @@ 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
+      end
     when :string_hex
       # Compare the values with any whitespace removed
       is = is_hash[property_name].to_s.gsub(%r{\s+}, '')

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

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 },