Fixup ICMP tests

REFERENCE.md

@@ -1232,6 +1232,8 @@ Valid values: `true`, `false`
 
 Can be set to true or false, specifies whether to set icmp_block_inversion from the zone
 
+Default value: `false`
+
 ##### `icmp_blocks`
 
 Specify the icmp-blocks for the zone. Can be a single string specifying one icmp type,

lib/puppet/provider/firewalld_zone/firewall_cmd.rb

@@ -93,9 +93,9 @@ def masquerade
   def masquerade=(bool)
     case bool
     when :true
-      execute_firewall_cmd(['--add-masquerade'])
+      execute_firewall_cmd(['--add-masquerade'], @resource[:name])
     when :false
-      execute_firewall_cmd(['--remove-masquerade'])
+      execute_firewall_cmd(['--remove-masquerade'], @resource[:name])
     end
   end
 
@@ -113,7 +113,6 @@ def icmp_blocks=(new_icmp_blocks)
     when Array
       get_icmp_blocks.each do |remove_block|
         unless new_icmp_blocks.include?(remove_block)
-          debug("removing block #{remove_block} from zone #{@resource[:name]}")
           remove_blocks.push(remove_block)
         end
       end
@@ -122,7 +121,6 @@ def icmp_blocks=(new_icmp_blocks)
         raise Puppet::Error, 'parameter icmp_blocks must be a string or array of strings!' unless block.is_a?(String)
 
         if icmp_types.include?(block)
-          debug("adding block #{block} to zone #{@resource[:name]}")
           set_blocks.push(block)
         else
           valid_types = icmp_types.join(', ')
@@ -131,11 +129,9 @@ def icmp_blocks=(new_icmp_blocks)
       end
     when String
       get_icmp_blocks.reject { |x| x == new_icmp_blocks }.each do |remove_block|
-        debug("removing block #{remove_block} from zone #{@resource[:name]}")
         remove_blocks.push(remove_block)
       end
       if icmp_types.include?(new_icmp_blocks)
-        debug("adding block #{new_icmp_blocks} to zone #{@resource[:name]}")
         set_blocks.push(new_icmp_blocks)
       else
         valid_types = icmp_types.join(', ')
@@ -144,23 +140,28 @@ def icmp_blocks=(new_icmp_blocks)
     else
       raise Puppet::Error, 'parameter icmp_blocks must be a string or array of strings!'
     end
-    unless remove_blocks.empty? # should this list the zone explicitly
+
+    # rubocop:disable Style/GuardClause
+    unless remove_blocks.empty?
       remove_blocks.each do |block|
-        execute_firewall_cmd(['--remove-icmp-block', block])
+        debug("removing block #{remove_block} from zone #{@resource[:name]}")
+        execute_firewall_cmd(['--remove-icmp-block', block], @resource[:name])
       end
     end
-    unless set_blocks.empty? # rubocop:disable Style/GuardClause
+    unless set_blocks.empty?
       set_blocks.each do |block|
-        execute_firewall_cmd(['--add-icmp-block', block])
+        debug("adding block #{new_icmp_blocks} to zone #{@resource[:name]}")
+        execute_firewall_cmd(['--add-icmp-block', block], @resource[:name])
       end
     end
+  # rubocop:enable Style/GuardClause
   end
 
   def icmp_block_inversion
-    if execute_firewall_cmd(['--query-icmp-block-inversion'], @resource[:name]).chomp == 'no'
-      :false
-    else
+    if execute_firewall_cmd(['--query-icmp-block-inversion'], @resource[:name], true, false).chomp == 'yes'
       :true
+    else
+      :false
     end
   end
 
@@ -175,7 +176,7 @@ def icmp_block_inversion=(bool)
     end
   end
 
-  # rubocop:disable Style/AccessorMethodName
+  # rubocop:disable Naming/AccessorMethodName
   def get_rules
     perm = execute_firewall_cmd(['--list-rich-rules']).split(%r{\n})
     curr = execute_firewall_cmd(['--list-rich-rules'], @resource[:name], false).split(%r{\n})
@@ -200,13 +201,13 @@ def get_ports
   end
 
   def get_icmp_blocks
-    execute_firewall_cmd(['--list-icmp-blocks']).split.sort
+    execute_firewall_cmd(['--list-icmp-blocks'], @resource[:name]).split.sort
   end
 
   def get_icmp_types
     execute_firewall_cmd(['--get-icmptypes'], nil).split
   end
-  # rubocop:enable Style/AccessorMethodName
+  # rubocop:enable Naming/AccessorMethodName
 
   def description
     execute_firewall_cmd(['--get-description'], @resource[:name], true, false)

lib/puppet/type/firewalld_zone.rb

@@ -128,6 +128,7 @@ def insync?(is)
 
   newproperty(:icmp_block_inversion) do
     desc 'Can be set to true or false, specifies whether to set icmp_block_inversion from the zone'
+    defaultto :false
     newvalue(:true)
     newvalue(:false)
   end

spec/unit/puppet/type/firewalld_zone_spec.rb

@@ -30,6 +30,61 @@
   ## Provider tests for the firewalld_zone type
   #
   describe 'provider' do
+    context 'with minimal parameters' do
+      let(:resource) do
+        described_class.new(
+          name: 'restricted',
+          target: '%%REJECT%%',
+          interfaces: ['eth0']
+        )
+      end
+      let(:provider) do
+        resource.provider
+      end
+
+      it 'checks if it exists' do
+        provider.expects(:execute_firewall_cmd).with(['--get-zones'], nil).returns('public restricted')
+        expect(provider).to be_exists
+      end
+
+      it 'evalulates target' do
+        provider.expects(:execute_firewall_cmd).with(['--get-target']).returns('%%REJECT%%')
+        expect(provider.target).to eq('%%REJECT%%')
+      end
+
+      it 'gets masquerading state as false when not set' do
+        provider.expects(:execute_firewall_cmd).with(['--query-masquerade'], 'restricted', true, false).returns("no\n")
+        expect(provider.masquerade).to eq(:false)
+      end
+
+      it 'sets target' do
+        provider.expects(:execute_firewall_cmd).with(['--set-target', '%%REJECT%%'])
+        provider.target = '%%REJECT%%'
+      end
+
+      it 'gets interfaces' do
+        provider.expects(:execute_firewall_cmd).with(['--list-interfaces']).returns('')
+        provider.interfaces
+      end
+
+      it 'sets interfaces' do
+        provider.expects(:interfaces).returns(['eth1'])
+        provider.expects(:execute_firewall_cmd).with(['--add-interface', 'eth0'])
+        provider.expects(:execute_firewall_cmd).with(['--remove-interface', 'eth1'])
+        provider.interfaces = ['eth0']
+      end
+
+      it 'creates' do
+        provider.expects(:execute_firewall_cmd).with(['--new-zone', 'restricted'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--set-target', '%%REJECT%%'])
+        provider.expects(:execute_firewall_cmd).with(['--remove-icmp-block-inversion'], 'restricted')
+
+        provider.expects(:interfaces).returns([])
+        provider.expects(:execute_firewall_cmd).with(['--add-interface', 'eth0'])
+        provider.create
+      end
+    end
+
     context 'with standard parameters' do
       let(:resource) do
         described_class.new(
@@ -65,6 +120,11 @@
         expect(provider.target).to eq('%%REJECT%%')
       end
 
+      it 'gets masquerading state as false when not set' do
+        provider.expects(:execute_firewall_cmd).with(['--query-masquerade'], 'restricted', true, false).returns("no\n")
+        expect(provider.masquerade).to eq(:false)
+      end
+
       it 'creates' do
         provider.expects(:execute_firewall_cmd).with(['--new-zone', 'restricted'], nil)
         provider.expects(:execute_firewall_cmd).with(['--set-target', '%%REJECT%%'])
@@ -120,25 +180,25 @@
         provider.sources = ['valy']
       end
 
-      it 'sets icmp_block_inversion' do
-        provider.expects(:execute_firewall_cmd).with(['--query-icmp-block-inversion'], 'restricted').returns('no')
-        provider.expects(:execute_firewall_cmd).with(['--add-icmp-block-inversion'], 'restricted')
+      it 'gets icmp_block_inversion' do
+        provider.expects(:execute_firewall_cmd).with(['--query-icmp-block-inversion'], 'restricted', true, false).returns("no\n")
+        expect(provider.icmp_block_inversion).to eq(:false)
+      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])
       end
 
       it 'gets icmp_blocks' do
-        # shouldn't this list the zone?
-        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks']).returns('val')
+        provider.expects(:execute_firewall_cmd).with(['--list-icmp-blocks'], 'restricted').returns('val')
         expect(provider.icmp_blocks).to eq(['val'])
       end
 
       it 'sets icmp_blocks' do
-        # shouldn't this list the zone?
-        provider.expects(:execute_firewall_cmd).with(['--add-icmp-blocks', 'redirect,router-advertisment'])
-      end
-
-      it 'lists icmp types' do
-        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])
+        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')
       end
     end
 
@@ -155,12 +215,12 @@
       end
 
       it 'sets masquerading' do
-        provider.expects(:execute_firewall_cmd).with(['--add-masquerade'])
+        provider.expects(:execute_firewall_cmd).with(['--add-masquerade'], 'public')
         provider.masquerade = :true
       end
 
       it 'disables masquerading' do
-        provider.expects(:execute_firewall_cmd).with(['--remove-masquerade'])
+        provider.expects(:execute_firewall_cmd).with(['--remove-masquerade'], 'public')
         provider.masquerade = :false
       end
 
@@ -174,6 +234,68 @@
         expect(provider.masquerade).to eq(:true)
       end
     end
+
+    context 'when specifiying a single icmp block' do
+      let(:resource) do
+        described_class.new(
+          name: 'public',
+          ensure: :present,
+          sources: '192.168.2.2',
+          icmp_blocks: 'echo-request'
+        )
+      end
+      let(:provider) do
+        resource.provider
+      end
+
+      it 'sets icmp_block_inversion' do
+        provider.expects(:execute_firewall_cmd).with(['--add-icmp-block-inversion'], 'public')
+        provider.icmp_block_inversion = :true
+      end
+
+      it 'disables icmp_block_inversion' do
+        provider.expects(:execute_firewall_cmd).with(['--remove-icmp-block-inversion'], 'public')
+        provider.icmp_block_inversion = :false
+      end
+
+      it 'gets icmp_block_inversion state as false when not set' do
+        provider.expects(:execute_firewall_cmd).with(['--query-icmp-block-inversion'], 'public', true, false).returns("no\n")
+        expect(provider.icmp_block_inversion).to eq(:false)
+      end
+
+      it 'gets icmp_block_inversion state as true when set' do
+        provider.expects(:execute_firewall_cmd).with(['--query-icmp-block-inversion'], 'public', true, false).returns("yes\n")
+        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')
+        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
+      end
+    end
+
+    context 'when specifiying a bad icmp block' do
+      let(:resource) do
+        described_class.new(
+          name: 'public',
+          ensure: :present,
+          sources: '192.168.2.2',
+          icmp_blocks: 'invalid-request'
+        )
+      end
+      let(:provider) do
+        resource.provider
+      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
+      end
+    end
   end
 
   context 'autorequires' do