Fix ordering bugs for firewall rules (#450)

* Fix ordering bugs for firewall rules

* Fix whitespace

* Fix idempotency with src or dest with any

* Fix whitespace

* Fix whitespace between methods

changelogs/fragments/302-firewallorder.yaml

@@ -0,0 +1,2 @@
+minor_changes:
+- meraki_mx_site_to_site_firewall - Fix updating VPN rules per issue 302.

plugins/modules/meraki_mx_site_to_site_firewall.py

@@ -199,11 +199,15 @@ def assemble_payload(meraki):
                   'syslog_enabled': 'syslogEnabled',
                   'comment': 'comment',
                   }
+    normalize_keys = ['dest_port', 'dest_cidr', 'src_port', 'src_cidr']
     rules = []
     for rule in meraki.params['rules']:
         proposed_rule = dict()
         for k, v in rule.items():
             proposed_rule[params_map[k]] = v
+            if k in normalize_keys:
+                if v.lower() == "any":
+                    proposed_rule[params_map[k]] = "Any"
         rules.append(proposed_rule)
     payload = {'rules': rules}
     return payload
@@ -216,6 +220,18 @@ def get_rules(meraki, org_id):
         return response
 
 
+def compare_rule_count(original, payload):
+    if len(original['rules']) - 1 != len(payload['rules']):  # Quick and simple check to avoid more processing
+        return True
+    return False
+
+
+def compare_default_rule(original, default_rule):
+    if original['rules'][len(original['rules']) - 1]['syslogEnabled'] != default_rule:
+        return True
+    return False
+
+
 def main():
     # define the available arguments/parameters that a user can pass to
     # the module
@@ -281,24 +297,21 @@ def main():
             payload['syslogDefaultRule'] = meraki.params['syslog_default_rule']
         try:
             if meraki.params['rules'] is not None:
-                if len(rules['rules']) - 1 != len(payload['rules']):  # Quick and simple check to avoid more processing
-                    update = True
-            if meraki.params['syslog_default_rule'] is not None:
-                if rules['rules'][len(rules['rules']) - 1]['syslogEnabled'] != meraki.params['syslog_default_rule']:
-                    update = True
+                update = compare_rule_count(rules, payload)
+            if update is False and meraki.params['syslog_default_rule'] is not None:
+                update = compare_default_rule(rules, meraki.params['syslog_default_rule'])
             if update is False:
-                default_rule = rules['rules'][len(rules['rules']) - 1].copy()
-                # meraki.fail_json(msg=update)
+                default_rule = rules['rules'][len(rules['rules']) - 1].copy()  # Create copy of default rule
                 del rules['rules'][len(rules['rules']) - 1]  # Remove default rule for comparison
                 if len(rules['rules']) - 1 == 0:
                     if meraki.is_update_required(rules['rules'][0], payload['rules'][0]) is True:
                         update = True
                 else:
-                    for r in range(len(rules) - 1):
+                    for r in range(len(rules)):
                         if meraki.is_update_required(rules['rules'][r], payload['rules'][r]) is True:
                             update = True
                 rules['rules'].append(default_rule)
-        except KeyError:
+        except IndexError:
             pass
         if update is True:
             if meraki.check_mode is True:

tests/integration/targets/meraki_mx_site_to_site_firewall/tasks/main.yml

@@ -18,6 +18,127 @@
       type: appliance
     delegate_to: localhost
 
+  - name: Test issue 302
+    meraki_mx_site_to_site_firewall:
+      auth_key: '{{ auth_key }}'
+      org_name: '{{test_org_name}}'
+      state: present
+      syslog_default_rule: true
+      rules:
+        - comment: Block All telnet Traffic Rename
+          src_cidr: 192.0.2.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32
+          dest_port: 23, 22
+          protocol: tcp
+          policy: deny
+          syslog_enabled: true
+
+        - comment: Allow Middle Rule
+          src_cidr: 192.0.1.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32
+          dest_port: 9443, 8080
+          protocol: tcp
+          policy: allow
+          syslog_enabled: true
+
+        - comment: Allow Web traffic
+          src_cidr: 192.0.1.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32
+          dest_port: 80, 443, 8443
+          protocol: tcp
+          policy: allow
+          syslog_enabled: true
+    delegate_to: localhost
+    register: threeohtwo_original
+
+  - debug:
+      var: threeohtwo_original
+
+  - assert:
+      that:
+        - threeohtwo_original.data is defined
+        - threeohtwo_original.data.rules[0].comment == "Block All telnet Traffic Rename"
+        - threeohtwo_original.data.rules[1].comment == "Allow Middle Rule"
+        - threeohtwo_original.data.rules[2].comment == "Allow Web traffic"
+        - threeohtwo_original.data.rules[0].dest_cidr == "192.0.2.2/32"
+
+  - name: Update rules to test 302 
+    meraki_mx_site_to_site_firewall:
+      auth_key: '{{ auth_key }}'
+      org_name: '{{test_org_name}}'
+      state: present
+      syslog_default_rule: true
+      rules:
+        - comment: Block All telnet Traffic Rename
+          src_cidr: 192.0.2.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32, 192.0.3.0/24
+          dest_port: 23, 22
+          protocol: tcp
+          policy: deny
+          syslog_enabled: true
+
+        - comment: Allow Web traffic
+          src_cidr: 192.0.1.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32
+          dest_port: 80, 443, 8443
+          protocol: tcp
+          policy: allow
+          syslog_enabled: true
+
+        - comment: Allow Middle Rule
+          src_cidr: 192.0.1.0/24
+          src_port: any
+          dest_cidr: 192.0.2.2/32
+          dest_port: 9443, 8080
+          protocol: tcp
+          policy: allow
+          syslog_enabled: true
+    delegate_to: localhost
+    register: threeohtwo_update
+
+  - debug:
+      var: threeohtwo_update
+
+  - assert:
+      that:
+        - threeohtwo_update.data is defined
+        - threeohtwo_update.data.rules[0].comment == "Block All telnet Traffic Rename"
+        - threeohtwo_update.data.rules[1].comment == "Allow Web traffic"
+        - threeohtwo_update.data.rules[2].comment == "Allow Middle Rule"
+        - threeohtwo_update.data.rules[0].dest_cidr == "192.0.2.2/32,192.0.3.0/24"
+
+  - name: Test capitalization for protocol
+    meraki_mx_site_to_site_firewall:
+      auth_key: '{{ auth_key }}'
+      org_name: '{{test_org_name}}'
+      state: present
+      syslog_default_rule: true
+      rules:
+        - comment: Block All telnet Traffic Rename
+          src_cidr: 192.0.2.0/24
+          src_port: Any
+          dest_cidr: 192.0.2.2/32, 192.0.3.0/24
+          dest_port: 23, 22
+          protocol: any 
+          policy: deny
+          syslog_enabled: true
+    delegate_to: localhost
+    register: protocol_any
+
+  - debug:
+      var: protocol_any
+
+  - assert:
+      that:
+        - protocol_any is success
+        - protocol_any is changed
+        - protocol_any.data.rules[0].protocol == "any"
+
   - name: Query firewall rules
     meraki_mx_site_to_site_firewall:
       auth_key: '{{ auth_key }}'
@@ -29,10 +150,6 @@
   - debug:
       var: query
 
-  # - assert:
-  #     that:
-  #       - query.data|length == 1
-
   - name: Set one firewall rule with check mode
     meraki_mx_site_to_site_firewall:
       auth_key: '{{ auth_key }}'