diff --git a/lib/puppet/provider/firewall/firewall.rb b/lib/puppet/provider/firewall/firewall.rb index dc41b3eb..5381d511 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 9ad48f50..8a679bd2 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 eaaf6027..e9405f6c 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 },