From: david22swan Date: Wed, 7 Jun 2023 16:47:59 +0000 (+0100) Subject: (CONT-242) Fix duplicate rule detection X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=8b17271d9f31cb4e9bd3277f0bcafe2a2fc045c7;p=puppet-modules%2Fpuppetlabs-firewall.git (CONT-242) Fix duplicate rule detection This was previously accomplished b retrieving the full list of rules each time a rule was set in order to check it was unique. This was to allow the user to choose the response to a duplicate rule being found. However this caused a massive slowdown within certain module runs and as such we have changed the the location of the check, so that it instead runs a check for any duplicates when retrieving the current rules prior to any updates being made. As an effect of this the user is now unable to choose the response to a duplicate rule being found, however wee feel that this is a fair tradeoff for the increased speed and that the response that we have chosen is the correct one. --- diff --git a/README.md b/README.md index f1985e3..3b0519d 100644 --- a/README.md +++ b/README.md @@ -394,17 +394,9 @@ firewall {'666 for NFLOG': It is possible for an unmanaged rule to exist on the target system that has the same comment as the rule specified in the manifest. This configuration is not supported by the firewall module. -In the event of a duplicate rule, the module will by default display a warning message notifying the user that it has found a duplicate but will continue to update the resource. - -This behaviour is configurable via the `onduplicaterulebehaviour` parameter. Users can choose from the following behaviours: - -* `ignore` - The duplicate rule is ignored and any updates to the resource will continue unaffected. -* `warn` - The duplicate rule is logged as a warning and any updates to the resource will continue unaffected. -* `error` - The duplicate rule is logged as an error and any updates to the resource will be skipped. - -With either the `ignore` or `warn` (default) behaviour, Puppet may create another duplicate rule. -To prevent this behavior and report the resource as failing during the Puppet run, specify the `error` behaviour. +In the event of a duplicate rule, the module will throw an error message notifying the user that it has found a duplicate and halt in it's update. +This behaviour was previously configurable via the `onduplicaterulebehaviour` parameter. However the implementation of this resulted in a massive slowdown of the module runs and so this has been removed infavour of a simple error being thrown whenever a duplicate is detected. ### Additional information Access the inline documentation: diff --git a/REFERENCE.md b/REFERENCE.md index d2f8d17..0046053 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -1382,7 +1382,6 @@ The following parameters are available in the `firewall` type. * [`line`](#-firewall--line) * [`name`](#-firewall--name) -* [`onduplicaterulebehaviour`](#-firewall--onduplicaterulebehaviour) * [`provider`](#-firewall--provider) ##### `line` @@ -1404,24 +1403,6 @@ so make sure you prefix the rule with a number: Depending on the provider, the name of the rule can be stored using the comment feature of the underlying firewall subsystem. -##### `onduplicaterulebehaviour` - -Valid values: `ignore`, `warn`, `error` - -In certain situations it is possible for an unmanaged rule to exist -on the target system that has the same comment as the rule -specified in the manifest. - -This setting determines what happens when such a duplicate is found. - -It offers three options: - - * ignore - The duplicate rule is ignored and any updates to the resource will continue unaffected. - * warn - The duplicate rule is logged as a warning and any updates to the resource will continue unaffected. - * error - The duplicate rule is logged as an error and any updates to the resource will be skipped. - -Default value: `warn` - ##### `provider` The specific backend to use for this `firewall` resource. You will seldom need to specify this --- Puppet will usually diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 9d26f03..263e7dd 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -410,29 +410,9 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa end def exists? - if duplicate_rule?(@property_hash[:name]) - core_message = "Duplicate rule found for #{@property_hash[:name]}." - case @resource.parameters[:onduplicaterulebehaviour].value - when :error - raise "#{core_message} Skipping update." - when :warn - warning "#{core_message}. This may add an additional rule to the system." - when :ignore - debug "#{core_message}. This may add an additional rule to the system." - end - end - properties[:ensure] != :absent end - def duplicate_rule?(rule) - rules = self.class.instances - system_rule_count = Hash.new(0) - rules.each { |r| system_rule_count[r.name] += 1 } - duplicate_rules = rules.select { |r| system_rule_count[r.name] > 1 } - duplicate_rules.select { |r| r.name == rule }.any? - end - # Flush the property hash once done. def flush debug('[flush]') @@ -448,6 +428,8 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa debug '[instances]' table = nil rules = [] + # Used for detecting duplicate rules + duplicate_rules = [] counter = 1 # String#lines would be nice, but we need to support Ruby 1.8.5 @@ -459,6 +441,8 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa else hash = rule_to_hash(line, table, counter) if hash + raise "Duplicate rule found for #{hash[:name]}. Skipping update." if duplicate_rules.include?(hash[:name]) + duplicate_rules << hash[:name] rules << new(hash) counter += 1 end diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 86af41b..4d0205c 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -236,24 +236,6 @@ Puppet::Type.newtype(:firewall) do newvalues(%r{^\d+[[:graph:][:space:]]+$}) end - newparam(:onduplicaterulebehaviour) do - desc <<-PUPPETCODE - In certain situations it is possible for an unmanaged rule to exist - on the target system that has the same comment as the rule - specified in the manifest. - - This setting determines what happens when such a duplicate is found. - - It offers three options: - - * ignore - The duplicate rule is ignored and any updates to the resource will continue unaffected. - * warn - The duplicate rule is logged as a warning and any updates to the resource will continue unaffected. - * error - The duplicate rule is logged as an error and any updates to the resource will be skipped. - PUPPETCODE - newvalues(:ignore, :warn, :error) - defaultto :warn - end - newproperty(:action) do desc <<-PUPPETCODE This is the action to perform on a match. Can be one of: diff --git a/spec/acceptance/firewall_duplicate_comment_spec.rb b/spec/acceptance/firewall_duplicate_comment_spec.rb index e019a45..93251d0 100644 --- a/spec/acceptance/firewall_duplicate_comment_spec.rb +++ b/spec/acceptance/firewall_duplicate_comment_spec.rb @@ -2,8 +2,19 @@ require 'spec_helper_acceptance' -def make_manifest(behaviour) - pp = <<-PUPPETCODE +describe 'firewall - duplicate comments' do + before(:all) do + if os[:family] == 'ubuntu' || os[:family] == 'debian' + update_profile_file + end + end + + after(:each) do + iptables_flush_all_tables + end + + context 'when a duplicate comment is found' do + pp = <<-PUPPETCODE class { 'firewall': } resources { 'firewall': purge => true, @@ -14,61 +25,15 @@ def make_manifest(behaviour) dport => '550', action => accept, destination => '192.168.2.0/24', - onduplicaterulebehaviour => #{behaviour} } PUPPETCODE - pp -end - -describe 'firewall - duplicate comments' do - before(:all) do - if os[:family] == 'ubuntu' || os[:family] == 'debian' - update_profile_file - end - end - - before(:each) do - run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - end - - after(:each) do - iptables_flush_all_tables - end - - context 'when onduplicateerrorhevent is set to error' do it 'raises an error' do run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - pp = make_manifest('error') - - apply_manifest(pp) do |r| - expect(r.stderr).to include('Error: /Stage[main]/Main/Firewall[550 destination]: Could not evaluate: Duplicate rule found for 550 destination. Skipping update.') - end - end - end - - context 'when onduplicateerrorhevent is set to warn' do - run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - - it 'warns and continues' do - run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - pp = make_manifest('warn') - - apply_manifest(pp) do |r| - expect(r.stderr).to include('Warning: Firewall[550 destination](provider=iptables): Duplicate rule found for 550 destination.. This may add an additional rule to the system.') - end - end - end - - context 'when onduplicateerrorhevent is set to ignore' do - run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - - it 'continues silently' do - run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"') - pp = make_manifest('ignore') + run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 552 -j ACCEPT -m comment --comment "550 destination"') apply_manifest(pp) do |r| - expect(r.stderr).to be_empty + expect(r.stderr).to include('Duplicate rule found for 550 destination. Skipping update.') end end end