Merge pull request #356 from jcpunk/tests

Try to fixup failing ICMP tests

lib/puppet/provider/firewalld_zone/firewall_cmd.rb

@@ -104,55 +104,26 @@ def icmp_blocks
   end
 
   def icmp_blocks=(new_icmp_blocks)
-    set_blocks = []
-    remove_blocks = []
+    new_icmp_blocks = new_icmp_blocks.split(%r{\s+}) if new_icmp_blocks.is_a?(String)
+    raise Puppet::Error, 'parameter icmp_blocks must be a string or array of strings!' unless new_icmp_blocks.is_a?(Array)
 
     icmp_types = get_icmp_types
+    invalid_blocks = new_icmp_blocks - icmp_types
+    raise Puppet::Error, "Invalid ICMP types: '#{invalid_blocks.join(', ')}'! Valid types are: '#{icmp_types.join(', ')}'" unless invalid_blocks.empty?
 
-    case new_icmp_blocks
-    when Array
-      get_icmp_blocks.each do |remove_block|
-        remove_blocks.push(remove_block) unless new_icmp_blocks.include?(remove_block)
-      end
-
-      new_icmp_blocks.each do |block|
-        raise Puppet::Error, 'parameter icmp_blocks must be a string or array of strings!' unless block.is_a?(String)
-
-        if icmp_types.include?(block)
-          set_blocks.push(block)
-        else
-          valid_types = icmp_types.join(', ')
-          raise Puppet::Error, "#{block} is not a valid icmp type on this system! Valid types are: #{valid_types}"
-        end
-      end
-    when String
-      get_icmp_blocks.reject { |x| x == new_icmp_blocks }.each do |remove_block|
-        remove_blocks.push(remove_block)
-      end
-      if icmp_types.include?(new_icmp_blocks)
-        set_blocks.push(new_icmp_blocks)
-      else
-        valid_types = icmp_types.join(', ')
-        raise Puppet::Error, "#{new_icmp_blocks} is not a valid icmp type on this system! Valid types are: #{valid_types}"
-      end
-    else
-      raise Puppet::Error, 'parameter icmp_blocks must be a string or array of strings!'
-    end
+    icmp_blocks = get_icmp_blocks
+
+    set_blocks = new_icmp_blocks - icmp_blocks
+    remove_blocks = icmp_blocks - new_icmp_blocks
 
-    # rubocop:disable Style/GuardClause
-    unless remove_blocks.empty?
-      remove_blocks.each do |block|
-        debug("removing block #{remove_block} from zone #{@resource[:name]}")
-        execute_firewall_cmd(['--remove-icmp-block', block], @resource[:name])
-      end
+    Array(remove_blocks).each do |block|
+      debug("removing block #{block} from zone #{@resource[:name]}")
+      execute_firewall_cmd(['--remove-icmp-block', block], @resource[:name])
     end
-    unless set_blocks.empty?
-      set_blocks.each do |block|
-        debug("adding block #{new_icmp_blocks} to zone #{@resource[:name]}")
-        execute_firewall_cmd(['--add-icmp-block', block], @resource[:name])
-      end
+    Array(set_blocks).each do |block|
+      debug("adding block #{new_icmp_blocks} to zone #{@resource[:name]}")
+      execute_firewall_cmd(['--add-icmp-block', block], @resource[:name])
     end
-    # rubocop:enable Style/GuardClause
   end
 
   def icmp_block_inversion

spec/unit/puppet/type/firewalld_zone_spec.rb

@@ -7,6 +7,56 @@
     Puppet::Provider::Firewalld.any_instance.stubs(:state).returns(:true) # rubocop:disable RSpec/AnyInstance
   end
 
+  let(:icmptypes) do
+    %w[
+      address-unreachable
+      bad-header
+      beyond-scope
+      communication-prohibited
+      destination-unreachable
+      echo-reply
+      echo-request
+      failed-policy
+      fragmentation-needed
+      host-precedence-violation
+      host-prohibited
+      host-redirect
+      host-unknown
+      host-unreachable
+      ip-header-bad
+      neighbour-advertisement
+      neighbour-solicitation
+      network-prohibited
+      network-redirect
+      network-unknown
+      network-unreachable
+      no-route
+      packet-too-big
+      parameter-problem
+      port-unreachable
+      precedence-cutoff
+      protocol-unreachable
+      redirect
+      reject-route
+      required-option-missing
+      router-advertisement
+      router-solicitation
+      source-quench
+      source-route-failed
+      time-exceeded
+      timestamp-reply
+      timestamp-request
+      tos-host-redirect
+      tos-host-unreachable
+      tos-network-redirect
+      tos-network-unreachable
+      ttl-zero-during-reassembly
+      ttl-zero-during-transit
+      unknown-header-type
+      unknown-option
+    ]
+  end
+
   describe 'type' do
     context 'with no params' do
       describe 'when validating attributes' do
@@ -186,19 +236,21 @@
       end
 
       it 'lists icmp types' do
-        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns('echo-reply echo-request val')
-        expect(provider.get_icmp_types).to eq(%w[echo-reply echo-request val])
+        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns('echo-reply echo-request')
+        expect(provider.get_icmp_types).to eq(%w[echo-reply echo-request])
       end
 
       it 'gets icmp_blocks' do
-        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'restricted').returns('val')
-        expect(provider.icmp_blocks).to eq(['val'])
+        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'restricted').returns('redirect router-advertisement')
+        expect(provider.icmp_blocks).to eq(%w[redirect router-advertisement])
       end
 
       it 'sets icmp_blocks' do
-        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'restricted').returns('val')
-        provider.expects(:execute_firewall_cmd).with(['--add-icmp-blocks', 'redirect,router-advertisment'], 'restricted')
-        provider.expects(:execute_firewall_cmd).with(['--remove-icmp-block', 'val'], 'public')
+        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'restricted').returns('redirect router-advertisement')
+        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns(icmptypes.join(' '))
+        provider.expects(:execute_firewall_cmd).with(['--add-icmp-block', 'bad-header'], 'restricted')
+        provider.expects(:execute_firewall_cmd).with(['--remove-icmp-block', 'router-advertisement'], 'restricted')
+        provider.icmp_blocks = %w[redirect bad-header]
       end
     end
 
@@ -268,11 +320,11 @@
         expect(provider.icmp_block_inversion).to eq(:true)
       end
 
-      it 'sets icmp blocks' do
-        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'public').returns('val')
+      it 'sets icmp_blocks' do
+        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'public').returns('')
+        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns(icmptypes.join(' '))
         provider.expects(:execute_firewall_cmd).with(['--add-icmp-block', 'echo-request'], 'public')
-        provider.expects(:execute_firewall_cmd).with(['--remove-icmp-block', 'val'], 'public')
-        provider.create
+        provider.icmp_blocks = %w[echo-request]
       end
     end
 
@@ -290,10 +342,8 @@
       end
 
       it 'errors out' do
-        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns('echo-reply echo-request val')
-        expect(provider.get_icmp_types).to eq(%w[echo-reply echo-request val])
-        expect(provider).to raise_error(Puppet::Error, %r{is not a valid icmp type})
-        provider.create
+        provider.expects(:execute_firewall_cmd).with(['--get-icmptypes'], nil).returns(icmptypes.join(' '))
+        expect { provider.icmp_blocks = 'banana' }.to raise_error(Puppet::Error, %r{Invalid ICMP types})
       end
     end
   end