From: Stefan Pijnappels <stefan@puppet.com>
Date: Tue, 23 May 2017 12:35:44 +0000 (+0100)
Subject: (MODULES-1141) Fail on sending array of ICMP types
X-Git-Tag: 1.10.0~31^2~1
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=caee3ef646d42c4283d21dc0100aeb1ac917879e;p=puppet-modules%2Fpuppetlabs-firewall.git

(MODULES-1141) Fail on sending array of ICMP types
---

diff --git a/README.markdown b/README.markdown
index b4ba88c..8eea1b0 100644
--- a/README.markdown
+++ b/README.markdown
@@ -589,7 +589,7 @@ If Puppet is managing the iptables or iptables-persistent packages, and the prov
 
 * `hop_limit`: Hop limiting value for matched packets. Values must match '/^\d+$/'. Requires the `hop_limiting` feature.
 
-* `icmp`: When matching ICMP packets, this indicates the type of ICMP packet to match. A value of 'any' is not supported. To match any type of ICMP packet, the parameter should be omitted or undefined. Requires the `icmp_match` feature.
+* `icmp`: When matching ICMP packets, this indicates the type of ICMP packet to match. A value of 'any' is not supported. To match any type of ICMP packet, the parameter should be omitted or undefined. Passing in an array of values is not supported. You can either create separate rules for each ICMP type, or alternatively look at the firewall_multi module (https://forge.puppetlabs.com/alexharvey/firewall_multi). Requires the `icmp_match` feature.
 
 * `iniface`: Input interface to filter on. Values must match '/^!?\s?[a-zA-Z0-9\-\._\+\:]+$/'.  Requires the `interface_match` feature.  Supports interface alias (eth0:0) and negation.
 
@@ -692,7 +692,7 @@ firewall { '999 this runs last':
 
 * `provider`: The specific backend to use for this firewall resource. You will seldom need to specify this --- Puppet will usually discover the appropriate provider for your platform. Available providers are ip6tables and iptables. See the [Providers](#providers) section above for details about these providers.
 
-* `queue_bypass`: When using a `jump` value of 'NFQUEUE' this boolean will allow packets to bypass `queue_num`. This is useful when the process in userspace may not be listening on `queue_num` all the time. 
+* `queue_bypass`: When using a `jump` value of 'NFQUEUE' this boolean will allow packets to bypass `queue_num`. This is useful when the process in userspace may not be listening on `queue_num` all the time.
 
 * `queue_num`: When using a `jump` value of 'NFQUEUE' this parameter specifies the queue number to send packets to.
 
diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb
index c480beb..a1a5ff1 100644
--- a/lib/puppet/type/firewall.rb
+++ b/lib/puppet/type/firewall.rb
@@ -691,6 +691,8 @@ Puppet::Type.newtype(:firewall) do
 
       A value of "any" is not supported. To achieve this behaviour the
       parameter should simply be omitted or undefined.
+      An array of values is also not supported. To match against multiple ICMP
+      types, please use separate rules for each ICMP type.
     EOS
 
     validate do |value|
@@ -699,6 +701,11 @@ Puppet::Type.newtype(:firewall) do
           "Value 'any' is not valid. This behaviour should be achieved " \
           "by omitting or undefining the ICMP parameter."
       end
+      if value.kind_of?(Array)
+        raise ArgumentError,
+          "Argument must not be an array of values. To match multiple " \
+          "ICMP types, please use separate rules for each ICMP type."
+      end
     end
 
     munge do |value|
@@ -722,6 +729,7 @@ Puppet::Type.newtype(:firewall) do
         self.fail("cannot work out icmp type")
       end
       value
+      
     end
   end
 
diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb
index 793c0fa..8ed8921 100755
--- a/spec/unit/puppet/type/firewall_spec.rb
+++ b/spec/unit/puppet/type/firewall_spec.rb
@@ -345,6 +345,9 @@ describe firewall do
     it 'should fail if icmp type is "any"' do
       expect(lambda { @resource[:icmp] = 'any' }).to raise_error(Puppet::Error)
     end
+    it 'should fail if icmp type is an array' do
+      expect(lambda { @resource[:icmp] = ['0', '8'] }).to raise_error(Puppet::Error)
+    end
 
     it 'should fail if icmp type cannot be mapped to a numeric' do
       expect(lambda { @resource[:icmp] = 'foo' }).to raise_error(Puppet::Error)