Skip to content

Commit 674c5d6

Browse files
authored
Merge pull request #1160 from david22swan/GH-1158/main/attribute_fix
(GH-1158) Fix for `dport/sport/state/ctstate/ctstatus` comparisons
2 parents a301fff + eedccb3 commit 674c5d6

File tree

2 files changed

+23
-34
lines changed

2 files changed

+23
-34
lines changed

lib/puppet/provider/firewall/firewall.rb

Lines changed: 18 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -384,21 +384,6 @@ def insync?(context, _name, property_name, is_hash, should_hash)
384384
when :mac_source, :jump
385385
# Value of mac_source/jump may be downcased or upcased when returned depending on the OS
386386
is_hash[property_name].casecmp(should_hash[property_name]).zero?
387-
when :state, :ctstate, :ctstatus
388-
# Ensure that if both is and should are array values, they are correctly compared in order
389-
is = is_hash[property_name]
390-
should = should_hash[property_name]
391-
return nil unless is.is_a?(Array) && should.is_a?(Array)
392-
393-
if is[0].start_with?('!')
394-
is.append('!')
395-
is[0] = is[0].gsub(%r{^!\s?}, '')
396-
end
397-
if should[0].start_with?('!')
398-
should.append('!')
399-
should[0] = should[0].gsub(%r{^!\s?}, '')
400-
end
401-
is.sort == should.sort
402387
when :icmp
403388
# Ensure that the values are compared to each other as icmp code numbers
404389
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)
428413
should = "#{should}:00" if %r{^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$}.match?(should)
429414

430415
is == should
431-
when :dport, :sport
416+
when :dport, :sport, :state, :ctstate, :ctstatus
432417
is = is_hash[property_name]
433418
should = should_hash[property_name]
434419

435-
# Wrap compared values as arrays in order to simplify comparisons
436-
is = [is] unless is.is_a?(Array)
437-
should = [should] unless should.is_a?(Array)
420+
# Unique logic is only needed when both values are arrays
421+
return nil unless is.is_a?(Array) && should.is_a?(Array)
438422

439-
# If first value includes a negation, retrieve it and set as it's own value
440-
if is[0].start_with?('!')
441-
is.append('!')
442-
is[0] = is[0].gsub(%r{^!\s?}, '')
423+
# Ensure values are sorted
424+
# Ensure any negation includes only the first value
425+
is_negated = true if %r{^!\s}.match?(is[0].to_s)
426+
is.each_with_index do |_value, _index|
427+
is = is.map { |value| value.to_s.tr('! ', '') }.sort
443428
end
444-
if should[0].start_with?('!')
445-
should.append('!')
446-
should[0] = should[0].gsub(%r{^!\s?}, '')
429+
is[0] = ['!', is[0]].join(' ') if is_negated
430+
431+
should_negated = true if %r{^!\s}.match?(should[0].to_s)
432+
should.each_with_index do |_value, _index|
433+
should = should.map { |value| value.to_s.tr('! ', '') }.sort
434+
# Port range can be passed as `-` but will always be set/returned as `:`
435+
ports = [:dport, :sport]
436+
should = should.map { |value| value.to_s.tr('-', ':') }.sort if ports.include?(property_name)
447437
end
438+
should[0] = ['!', should[0]].join(' ') if should_negated
448439

449-
# Range can be passed as `-` but will always be set/returned as `:`
450-
# Ensure values are sorted
451-
is.sort == should.map { |port| port.to_s.tr('-', ':') }.sort
440+
is == should
452441
when :string_hex
453442
# Compare the values with any whitespace removed
454443
is = is_hash[property_name].to_s.gsub(%r{\s+}, '')

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@
156156
{ is_hash: { state: 'NEW' }, should_hash: { state: ['NEW', 'INVALID'] }, result: nil },
157157
{ is_hash: { state: ['INVALID', 'NEW'] }, should_hash: { state: ['NEW', 'INVALID'] }, result: true },
158158
{ is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', 'INVALID'] }, result: true },
159+
{ is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', '! INVALID'] }, result: true },
159160
{ is_hash: { state: ['! INVALID', 'NEW'] }, should_hash: { state: ['! NEW', 'INVALID', 'UNTRACKED'] }, result: false },
160161
] },
161162
{ testing: 'icmp', property_name: :icmp, comparisons: [
@@ -200,12 +201,11 @@
200201
{ is_hash: { jump: 'accept' }, should_hash: { jump: 'drop' }, result: false },
201202
] },
202203
{ testing: 'dport/sport', property_name: :dport, comparisons: [
203-
{ is_hash: { dport: '! 50' }, should_hash: { dport: '! 50' }, result: true },
204-
{ is_hash: { dport: '50:60' }, should_hash: { dport: '50-60' }, result: true },
205-
{ is_hash: { dport: ['50:60'] }, should_hash: { dport: '50-60' }, result: true },
204+
{ is_hash: { dport: '50' }, should_hash: { dport: '50' }, result: nil },
205+
{ is_hash: { dport: ['50:60'] }, should_hash: { dport: '50-60' }, result: nil },
206206
{ is_hash: { dport: ['50:60'] }, should_hash: { dport: ['50-60'] }, result: true },
207-
{ is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 90', '50-60'] }, result: true },
208-
{ is_hash: { dport: '50' }, should_hash: { dport: '90' }, result: false },
207+
{ is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 90', '! 50-60'] }, result: true },
208+
{ is_hash: { dport: ['! 50:60', '90'] }, should_hash: { dport: ['! 100', '! 60-70'] }, result: false },
209209
] },
210210
{ testing: 'string_hex', property_name: :string_hex, comparisons: [
211211
{ is_hash: { string_hex: '! |f4 6d 04 25 b2 02 00 0a|' }, should_hash: { string_hex: '! |f46d0425b202000a|' }, result: true },

0 commit comments

Comments
 (0)