From: David Schmitt Date: Wed, 16 Dec 2020 17:28:51 +0000 (+0000) Subject: Apply remaining rubocop fixes X-Git-Tag: v2.8.1~3^2~2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=7efed0604db2ba2828cf9f10aa669404f39a74f4;p=puppet-modules%2Fpuppetlabs-firewall.git Apply remaining rubocop fixes --- diff --git a/.rubocop.yml b/.rubocop.yml index 33c33fa..07b68b8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -89,6 +89,8 @@ Performance/CaseWhenSplat: Performance/Casecmp: Enabled: true Performance/CollectionLiteralInLoop: + Exclude: + - spec/**/* Enabled: true Performance/CompareWithBlock: Enabled: true @@ -294,6 +296,8 @@ Metrics/AbcSize: Enabled: false Metrics/BlockLength: Enabled: false +Metrics/BlockNesting: + Enabled: false Metrics/ClassLength: Enabled: false Metrics/CyclomaticComplexity: @@ -308,6 +312,8 @@ Metrics/PerceivedComplexity: Enabled: false Migration/DepartmentName: Enabled: false +Naming/AccessorMethodName: + Enabled: false Naming/BlockParameterName: Enabled: false Naming/HeredocDelimiterCase: diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml deleted file mode 100644 index e69de29..0000000 diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index b4c2b87..6a925fe 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -68,7 +68,7 @@ Puppet::Type.type(:firewall).provide :ip6tables, parent: :iptables, source: :ip6 def initialize(*args) ip6tables_version = Facter.value('ip6tables_version') - raise ArgumentError, 'The ip6tables provider is not supported on version 1.3 of iptables' if ip6tables_version && ip6tables_version.match(%r{1\.3\.\d}) + raise ArgumentError, 'The ip6tables provider is not supported on version 1.3 of iptables' if ip6tables_version&.match(%r{1\.3\.\d}) super end diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index c1efb79..dc3d28c 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -381,10 +381,9 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa if self.class.instance_variable_get(:@protocol) == 'IPv6' && properties[:proto] == 'all' begin iptables delete_args.concat(['-p', 'all']) - rescue Puppet::ExecutionFailure => e + rescue Puppet::ExecutionFailure => e # rubocop:disable Lint/SuppressedException end end - # rubocop:enable Lint/HandleExceptions # Check to see if the iptables rule is already gone. This can sometimes # happen as a side effect of other resource changes. If it's not gone, @@ -630,7 +629,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa '0x2e' => 'ef', } [:set_dscp_class].each do |prop| - [:set_dscp].each do |dmark| + [:set_dscp].each do |dmark| # rubocop:disable Performance/CollectionLiteralInLoop next unless hash[dmark] hash[prop] = valid_dscp_classes[hash[dmark]] end @@ -638,7 +637,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa # Convert booleans removing the previous cludge we did @known_booleans.each do |bool| - unless [nil, 'true', '!'].include?(hash[bool]) + unless [nil, 'true', '!'].include?(hash[bool]) # rubocop:disable Performance/CollectionLiteralInLoop raise "Parser error: #{bool} was meant to be a boolean but received value: #{hash[bool]}." end end @@ -652,9 +651,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa elem.tr(':', '-') end end - if hash[:length] - hash[:length].tr!(':', '-') - end + hash[:length]&.tr!(':', '-') # Invert any rules that are prefixed with a '!' [ @@ -685,7 +682,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa :src_range, :state, ].each do |prop| - if hash[prop] && hash[prop].is_a?(Array) + if hash[prop]&.is_a?(Array) # find if any are negated, then negate all if so should_negate = hash[prop].index do |value| value.match(%r{^(!)\s+}) @@ -698,7 +695,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa elsif hash[prop] m = hash[prop].match(%r{^(!?)\s?(.*)}) neg = '! ' if m[1] == '!' - hash[prop] = if [:source, :destination].include?(prop) + hash[prop] = if [:source, :destination].include?(prop) # rubocop:disable Performance/CollectionLiteralInLoop # Normalise all rules to CIDR notation. "#{neg}#{Puppet::Util::IPCidr.new(m[2]).cidr}" else @@ -814,6 +811,8 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa raise "#{prop} elements must be unique" if resource[prop].map { |type| type.to_s.gsub(%r{--limit-iface-(in|out)}, '') }.uniq.length != resource[prop].length end + complex_args = [:ipset, :dst_type, :src_type] + resource_list.each do |res| resource_value = nil if resource[res] @@ -839,18 +838,16 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa # so we insert before whatever the last argument is args.insert(-2, '!') elsif resource_value.is_a?(Symbol) && resource_value.to_s.match(%r{^!}) - # ruby 1.8.7 can't .match Symbols ------------------ ^ resource_value = resource_value.to_s.sub!(%r{^!\s*}, '').to_sym args.insert(-2, '!') - elsif resource_value.is_a?(Array) && ![:ipset, :dst_type, :src_type].include?(res) + elsif resource_value.is_a?(Array) && !complex_args.include?(res) + should_negate = resource_value.index do |value| - # ruby 1.8.7 can't .match symbols value.to_s.match(%r{^(!)\s+}) end if should_negate resource_value, wrong_values = resource_value.map { |value| if value.is_a?(String) - # rubocop:disable Metrics/BlockNesting wrong = value unless %r{^!\s+}.match?(value) [value.sub(%r{^!\s*}, ''), wrong] else @@ -859,7 +856,8 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa }.transpose wrong_values = wrong_values.compact unless wrong_values.empty? - raise "All values of the '#{res}' property must be prefixed with a '!' when inverting, but '#{wrong_values.join("', '")}' #{(wrong_values.length > 1) ? 'are' : 'is'} not prefixed; aborting" # rubocop:disable Layout/LineLength : Line length cannot be reduced + raise "All values of the '#{res}' property must be prefixed with a '!' when inverting, but " \ + "'#{wrong_values.join("', '")}' #{(wrong_values.length > 1) ? 'are' : 'is'} not prefixed; aborting" end args.insert(-2, '!') # rubocop:enable Metrics/BlockNesting @@ -868,14 +866,15 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa # For sport and dport, convert hyphens to colons since the type # expects hyphens for ranges of ports. - if [:sport, :dport, :port].include?(res) + if [:sport, :dport, :port].include?(res) # rubocop:disable Performance/CollectionLiteralInLoop resource_value = resource_value.map do |elem| elem.tr('-', ':') end end # ipset can accept multiple values with weird iptables arguments - if [:ipset, :dst_type, :src_type].include?(res) + if complex_args.include?(res) + resource_value.join(" #{[resource_map[res]].flatten.first} ").split(' ').each do |a| if a.sub!(%r{^!\s*}, '') # Negate ipset options diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 3b993bf..cd96705 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -2279,8 +2279,9 @@ Puppet::Type.newtype(:firewall) do unless protocol.nil? table = value(:table) + main_chains = ['INPUT', 'OUTPUT', 'FORWARD'] [value(:chain), value(:jump)].each do |chain| - reqs << "#{chain}:#{table}:#{protocol}" unless chain.nil? || (['INPUT', 'OUTPUT', 'FORWARD'].include?(chain) && table == :filter) + reqs << "#{chain}:#{table}:#{protocol}" unless chain.nil? || (main_chains.include?(chain) && table == :filter) end end @@ -2360,8 +2361,8 @@ Puppet::Type.newtype(:firewall) do end if value(:set_mark) - unless value(:jump).to_s =~ %r{MARK} && - value(:table).to_s =~ %r{mangle} + unless value(:jump).to_s.include?(MARK) && + value(:table).to_s.include?(mangle) raise 'Parameter set_mark only applies to ' \ 'the mangle table and when jump => MARK' end diff --git a/lib/puppet/util/firewall.rb b/lib/puppet/util/firewall.rb index 1b4f49c..a0616b4 100644 --- a/lib/puppet/util/firewall.rb +++ b/lib/puppet/util/firewall.rb @@ -125,7 +125,7 @@ module Puppet::Util::Firewall begin new_value = Puppet::Util::IPCidr.new(addr, family) break - rescue + rescue # looking for the one that works # rubocop:disable Lint/SuppressedException end end diff --git a/lib/puppet/util/ipcidr.rb b/lib/puppet/util/ipcidr.rb index 15b38e9..c9c6cd0 100644 --- a/lib/puppet/util/ipcidr.rb +++ b/lib/puppet/util/ipcidr.rb @@ -30,8 +30,7 @@ module Puppet::Util end def cidr - cidr = '%s/%s' % [to_s, prefixlen] - cidr + "#{self}/#{prefixlen}" end end end diff --git a/spec/acceptance/firewall_attributes_happy_path_spec.rb b/spec/acceptance/firewall_attributes_happy_path_spec.rb index 77efcbf..dedc9be 100644 --- a/spec/acceptance/firewall_attributes_happy_path_spec.rb +++ b/spec/acceptance/firewall_attributes_happy_path_spec.rb @@ -500,9 +500,11 @@ describe 'firewall attribute testing, happy path' do expect(result.stdout).to match(%r{-A INPUT -p tcp -m comment --comment "567 - jump" -j TEST}) end it 'notrack is set' do - notrack_rule = '-A PREROUTING -p udp -m multiport --dports 53 -m comment --comment "004 do not track UDP connections to port 53" -j CT --notrack' - notrack_rule = '-A PREROUTING -p udp -m multiport --dports 53 -m comment --comment "004 do not track UDP connections to port 53" -j NOTRACK' if os[:family] == 'redhat' && [5, - 6].include?(os[:release].to_i) + notrack_rule = if os[:family] == 'redhat' && [5, 6].include?(os[:release].to_i) + '-A PREROUTING -p udp -m multiport --dports 53 -m comment --comment "004 do not track UDP connections to port 53" -j NOTRACK' + else + '-A PREROUTING -p udp -m multiport --dports 53 -m comment --comment "004 do not track UDP connections to port 53" -j CT --notrack' + end expect(result.stdout).to match(%r{#{notrack_rule}}) end end diff --git a/spec/unit/puppet/type/firewallchain_spec.rb b/spec/unit/puppet/type/firewallchain_spec.rb index 239ad0a..3006776 100755 --- a/spec/unit/puppet/type/firewallchain_spec.rb +++ b/spec/unit/puppet/type/firewallchain_spec.rb @@ -47,7 +47,7 @@ describe firewallchain do # rubocop:disable RSpec/MultipleDescribes expect { resource[:name] = name }.to raise_error(Puppet::Error) end elsif protocol != 'ethernet' && table == 'broute' - it "fails #{name}" do # rubocop:disable RSpec/RepeatedExample + it "fails #{name}" do # rubocop:disable RSpec/RepeatedExample,RSpec/RepeatedDescription expect { resource[:name] = name }.to raise_error(Puppet::Error) end else @@ -74,7 +74,7 @@ describe firewallchain do # rubocop:disable RSpec/MultipleDescribes expect(resource[:name]).to eql name end else - it "fails #{name}" do # rubocop:disable RSpec/RepeatedExample + it "fails #{name}" do # rubocop:disable RSpec/RepeatedExample,RSpec/RepeatedDescription expect { resource[:name] = name }.to raise_error(Puppet::Error) end end @@ -141,8 +141,6 @@ describe firewallchain do # rubocop:disable RSpec/MultipleDescribes expect(rel.target.ref).to eql resource.ref end end - # rubocop:enable RSpec/ExampleLength - # rubocop:enable RSpec/MultipleExpectations end describe 'purge iptables rules' do @@ -170,7 +168,6 @@ PUPPETCODE allow(Puppet::Type.type(:firewall).provider(:iptables)).to receive(:iptables_save).and_return(stub_return) allow(Puppet::Type.type(:firewall).provider(:ip6tables)).to receive(:ip6tables_save).and_return(stub_return) end - # rubocop:enable Layout/IndentHeredoc it 'generates iptables resources' do allow(Facter.fact(:ip6tables_version)).to receive(:value).and_return('1.4.21')