(CAT-376) Update treatment of array values

- Allow certain array values to be negated either by negating their first value, or all their values.

Author: david22swan

README.md

@@ -272,10 +272,13 @@ class profile::apache {
 
 ### Rule inversion
 
-Firewall rules may be inverted by prefixing the value of a parameter by "! ". If the value is an array, then the first value of the array must be prefixed in order to invert them all.
+Firewall rules may be inverted by prefixing the value of a parameter by "! ".
 
 Parameters that understand inversion are: connmark, ctstate, destination, dport, dst\_range, dst\_type, iniface, outiface, port, proto, source, sport, src\_range and src\_type.
 
+If the value is an array, then either the first value of the array, or all of its values must be prefixed in order to invert them all.
+For most array attributes it is not possible to invert only one passed value.
+
 Examples:
 
 ```puppet
@@ -295,7 +298,7 @@ firewall { '002 drop NEW external website packets with FIN/RST/ACK set and SYN u
 }
 ```
 
-There are exceptions to this however, with attributes such as src\_type, dst\_type and ipset allowing the user to negate any passed values seperately.
+There are exceptions to this however, with attributes such as src\_type, dst\_type and ipset allowing the user to negate each passed values seperately.
 
 Examples:
 

lib/puppet/provider/firewall/firewall.rb

@@ -227,9 +227,8 @@ class Puppet::Provider::Firewall::Firewall
     cgroup: [:cgroup]
   }
 
-  # This is the order of resources as they appear in iptables-save output,
-  # we need it to properly parse and apply rules, if the order of resource
-  # changes between puppet runs, the changed rules will be re-applied again.
+  # This is the order of resources as they appear in ip(6)tables-save output,
+  # it is used in order to ensure that the rules are applied in the correct order.
   # This order can be determined by going through iptables source code or just tweaking and trying manually
   $resource_list = [
     :source, :destination, :iniface, :outiface,
@@ -257,7 +256,6 @@ class Puppet::Provider::Firewall::Firewall
 
   ###### PUBLIC METHODS ######
 
-  # Raw data is retrieved via `iptables -L` and then regexed to retrieve the different Chains and whether they have a set Policy
   def get(context)
     # Call the private method which returns the rules
     # The method is seperated out in this way as it is re-used later in the code
@@ -287,7 +285,6 @@ def set(context, changes)
         end
       elsif is[:ensure].to_s == 'present' && should[:ensure].to_s == 'absent'
         context.deleting(name) do
-          # Delete command requires additional context
           delete(context, name, is)
         end
       elsif is[:ensure].to_s == 'present'
@@ -762,13 +759,15 @@ def self.validate_input(_is, should)
     # `stat_mode` must be set to `random` for `stat_probability` to be set
     raise ArgumentError, '`stat_mode` must be set to `random` for `stat_probability` to be set.' if should[:stat_probability] && should[:stat_mode] != 'random'
 
-    # Verify that if dport/sport/state/ctstate/ctstatus is passed as an array, that only the first value is negated
+    # Verify that if dport/sport/state/ctstate/ctstatus is passed as an array, that any negation includes either the first value or al values
     [:dport, :sport, :state, :ctstate, :ctstatus].each do |key|
       next unless should[key].is_a?(Array)
-
-      should[key].each_with_index do |value, index|
-        raise ArgumentError, "Only the first value in a `#{key}` array must be negated in order to negate the combined values." if index >= 1 && value.to_s.match(%r{^!})
+      negated_values = 0
+      should[key].each do |value|
+        negated_values += 1 if value.to_s.match(%r{^!\s})
       end
+      raise ArgumentError, "When negating a `#{key}` array, you must negate either the first given value only or all the given values." if (negated_values == 1 && !should[key][0].to_s.match(%r{^!\s})) || 
+                                                                                                                                           (negated_values >= 2 && negated_values != should[key].length)
     end
     raise ArgumentError, 'Value `any` is not valid. This behaviour should be achieved by omitting or undefining the ICMP parameter.' if should[:icmp] && should[:icmp] == 'any'
     raise ArgumentError, '`burst` cannot be set without `limit`.' if should[:burst] && !(should[:limit])
@@ -860,6 +859,17 @@ def self.validate_input(_is, should)
   # Certain attributes need processed in ways that can vary between IPv4 and IPv6
   # @api.private
   def self.process_input(should)
+    # `dport`, `sport` `state` `ctstate` and `ctstatus` arrays should only have the first value negated.
+    [:dport, :sport, :state, :ctstate, :ctstatus].each do |key|
+      next unless should[key].is_a?(Array)
+
+      negated = true if should[key][0].to_s.match(%r{^!\s})
+      should[key].each_with_index do |value, index|
+        should[key] = should[key].map { |value| value.to_s.tr('! ', '') }
+      end
+      should[key][0] = ['!', should[key][0]].join(' ') if negated
+    end
+
     # `jump` values should always be uppercase
     should[:jump] = should[:jump].upcase if should[:jump]
 

lib/puppet/type/firewall.rb

@@ -437,7 +437,9 @@
 
           sport => ['! 54','23']
 
-      Note: this will negate all passed ports, it is not possible to negate a single one of the array.
+      Note:
+        This will negate all passed ports, it is not possible to negate a single one of the array.
+        In order to maintain compatibility it is also possible to negate all values given in the array to achieve the same behaviour.
       DESC
     },
     dport: {
@@ -461,7 +463,9 @@
 
           dport => ['! 54','23']
 
-      Note: this will negate all passed ports, it is not possible to negate a single one of the array.
+      Note:
+        This will negate all passed ports, it is not possible to negate a single one of the array.
+        In order to maintain compatibility it is also possible to negate all values given in the array to achieve the same behaviour.
       DESC
     },
     src_type: {
@@ -584,7 +588,9 @@
 
           state => ['! INVALID', 'ESTABLISHED']
 
-      Note: this will negate all passed states, it is not possible to negate a single one of the array.
+      Note:
+        This will negate all passed states, it is not possible to negate a single one of the array.
+        In order to maintain compatibility it is also possible to negate all values given in the array to achieve the same behaviour.
       DESC
     },
     ctstate: {
@@ -612,7 +618,9 @@
 
       ctstate => ['! INVALID', 'ESTABLISHED']
 
-      Note: this will negate all passed states, it is not possible to negate a single one of the array.
+      Note:
+        This will negate all passed states, it is not possible to negate a single one of the array.
+        In order to maintain compatibility it is also possible to negate all values given in the array to achieve the same behaviour.
       DESC
     },
     ctproto: {
@@ -768,7 +776,9 @@
 
         ctstatus => ['! EXPECTED', 'CONFIRMED']
 
-      Note: this will negate all passed states, it is not possible to negate a single one of the array.
+      Note: 
+        This will negate all passed states, it is not possible to negate a single one of the array.
+        In order to maintain compatibility it is also possible to negate all values given in the array to achieve the same behaviour.
       DESC
     },
     ctexpire: {

spec/unit/puppet/provider/firewall/firewall_private_set_spec.rb

@@ -42,9 +42,15 @@
         },
         {
           # Covers `dport`, `sport`, `state`, `ctstate` and `ctstatus`
-          valid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', dport: ['! 54', '64'] } },
-          invalid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', dport: ['! 54', '! 64'] } },
-          error: 'Only the first value in a `dport` array must be negated in order to negate the combined values.'
+          valid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', dport: ['! 54', '64', '74'] } },
+          invalid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', dport: ['54', '! 64', '74'] } },
+          error: 'When negating a `dport` array, you must negate either the first given value only or all the given values.'
+        },
+        {
+          # Covers `dport`, `sport`, `state`, `ctstate` and `ctstatus`
+          valid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', sport: ['! 54', '! 64', '! 74'] } },
+          invalid: { is: {}, should: { ensure: 'present', name: '001 Test Rule', sport: ['! 54', '! 64', '74'] } },
+          error: 'When negating a `sport` array, you must negate either the first given value only or all the given values.'
         },
         {
           valid: { is: {}, should: { ensure: 'present', name: '001 Test Rule' } },
@@ -250,6 +256,11 @@
 
     describe 'self.process_input(should)' do
       [
+        {
+          process: '`dport`, `sport` `state` `ctstate` and `ctstatus` arrays should only have the first value negated',
+          should: { dport: ['! 54', '! 64', '! 74'] },
+          result: { dport: ['! 54', '64', '74'] }
+        },
         {
           process: '`jump` values should always be uppercase',
           should: { jump: 'accept' },