TPT-3529: Handle non-existing rules for firewall updates

plugins/modules/firewall.py

@@ -7,6 +7,7 @@
 
 import copy
 import ipaddress
+import time
 from typing import Any, List, Optional
 
 import ansible_collections.linode.cloud.plugins.module_utils.doc_fragments.firewall as docs
@@ -219,14 +220,28 @@ def __init__(self) -> None:
         super().__init__(module_arg_spec=self.module_arg_spec)
 
     def _get_firewall_by_label(self, label: str) -> Optional[Firewall]:
-        try:
-            return self.client.networking.firewalls(Firewall.label == label)[0]
-        except IndexError:
-            return None
-        except Exception as exception:
-            return self.fail(
-                msg="failed to get firewall {0}: {1}".format(label, exception)
-            )
+        attempt = 0
+        retries = 5
+        retry_delay = 3
+
+        while attempt < retries:
+            try:
+                return self.client.networking.firewalls(
+                    Firewall.label == label
+                )[0]
+            except IndexError:
+                attempt += 1
+                if attempt < retries:
+                    time.sleep(retry_delay)
+                else:
+                    return None
+            except Exception as exception:
+                return self.fail(
+                    msg="failed to get firewall {0}: {1}".format(
+                        label, exception
+                    )
+                )
+        return None
 
     def _create_firewall(self) -> Optional[Firewall]:
         params = self.module.params
@@ -348,20 +363,21 @@ def _update_rules(self, remote_rules: dict, local_rules: dict) -> dict:
         if self._state != "update":
             return local_rules
 
-        # Add new local rules to remote rules if they don't exist
+        # Amend existing rules and append new local rules
         for direction in ["inbound", "outbound"]:
-            rlr = {
-                remote_rule["label"]
-                for remote_rule in remote_rules.get(direction, {})
+            remote_labeled = {
+                r["label"] for r in remote_rules.get(direction, [])
             }
-            for local_rule in local_rules.get(direction, {}):
-                if local_rule["label"] not in rlr:
-                    remote_rules[direction].append(local_rule)
-
-        for direction in ["inbound", "outbound"]:
-            local_rules[direction] = self._amend_rules(
-                remote_rules[direction], local_rules[direction]
+            # Start with amended existing remote rules
+            amended = self._amend_rules(
+                remote_rules.get(direction, []),
+                local_rules.get(direction, []),
             )
+            # Append any new local rules not present in remote
+            for local_rule in local_rules.get(direction, []):
+                if local_rule["label"] not in remote_labeled:
+                    amended.append(local_rule)
+            local_rules[direction] = amended
         return local_rules
 
     def _change_rules(self) -> Optional[dict]:
@@ -438,6 +454,12 @@ def _handle_present(self) -> None:
         self._firewall = self._get_firewall_by_label(label)
 
         if self._firewall is None:
+            if self._state == "update":
+                self.fail(
+                    msg="Firewall {0} does not exist and cannot be updated.".format(
+                        label
+                    )
+                )
             self._firewall = self._create_firewall()
             self.register_action("Created Firewall {0}".format(label))
 

tests/integration/targets/firewall_update/tasks/main.yaml

@@ -79,7 +79,7 @@
           - updated_firewall.firewall.rules.inbound | length == 1
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv4'] is defined)
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv6'] is not defined)
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -158,7 +158,7 @@
           - updated_firewall.firewall.rules.inbound | length == 1
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv4'] is not defined)
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv6'] is defined)
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -238,7 +238,7 @@
           - updated_firewall.firewall.rules.inbound | length == 1
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv4'] is defined) and (updated_firewall.firewall.rules.inbound[0].addresses['ipv4'] == ['0.0.0.0/0'])
           - (updated_firewall.firewall.rules.inbound[0].addresses['ipv6'] is defined) and (updated_firewall.firewall.rules.inbound[0].addresses['ipv6'] == ['ff00::/8'])
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -286,7 +286,7 @@
         that:
           - updated_firewall.changed
           - updated_firewall.firewall.status == 'disabled'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -326,7 +326,7 @@
         that:
           - updated_firewall.changed
           - updated_firewall.firewall.status == 'enabled'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -370,7 +370,7 @@
           - updated_firewall.changed
           - updated_firewall.firewall.rules.inbound_policy == 'ACCEPT'
           - updated_firewall.firewall.rules.outbound_policy == 'ACCEPT'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -447,7 +447,7 @@
           - updated_firewall.firewall.rules.outbound_policy == 'DROP'
           - updated_firewall.firewall.rules.outbound[0].addresses['ipv4'] == ['0.0.0.0/0']
           - updated_firewall.firewall.rules.outbound[0].addresses['ipv6'] == ['ff00::/8']
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -507,7 +507,7 @@
           - updated_firewall.firewall.rules.outbound[0].ports == '80,443'
           - updated_firewall.firewall.rules.outbound[0].protocol == 'TCP'
           - updated_firewall.firewall.rules.outbound[0].action == 'ACCEPT'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -570,7 +570,7 @@
           - updated_firewall.firewall.rules.outbound[0].protocol == 'TCP'
           - updated_firewall.firewall.rules.outbound[0].action == 'ACCEPT'
           - updated_firewall.firewall.rules.outbound_policy == 'DROP'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -635,7 +635,7 @@
           - updated_firewall.firewall.rules.outbound[0].action == 'ACCEPT'
           - updated_firewall.firewall.rules.outbound[0].addresses['ipv4'] == ['0.0.0.0/0','8.8.8.8/32' ]
           - updated_firewall.firewall.rules.outbound_policy == 'DROP'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -700,7 +700,7 @@
           - updated_firewall.firewall.rules.outbound[0].action == 'ACCEPT'
           - updated_firewall.firewall.rules.outbound[0].addresses['ipv4'] == ['0.0.0.0/0','8.8.8.8/32' ]
           - updated_firewall.firewall.rules.outbound_policy == 'DROP'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -749,7 +749,7 @@
           - updated_firewall.changed
           - updated_firewall.firewall.rules.inbound[0].description == 'Amazing firewall rule.'
           - updated_firewall.firewall.rules.outbound[0].description == 'Amazing firewall rule.'
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -798,7 +798,7 @@
       assert:
         that:
           - firewall_info.devices|length == 0
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -847,7 +847,7 @@
       assert:
         that:
           - not updated_firewall.changed
-          
+
           - firewall_info.firewall.id == create.firewall.id
           - firewall_info.devices|length == updated_firewall.devices|length
           - firewall_info.firewall.status == updated_firewall.firewall.status
@@ -867,6 +867,70 @@
           - (firewall_info.firewall.rules.outbound | length == 0 and updated_firewall.firewall.rules.outbound | length == 0) or ((firewall_info.firewall.rules.outbound[0].ports is not defined and updated_firewall.firewall.rules.outbound[0].ports is not defined) or (firewall_info.firewall.rules.outbound[0].ports == updated_firewall.firewall.rules.outbound[0].ports))
           - (firewall_info.firewall.rules.outbound | length == 0 and updated_firewall.firewall.rules.outbound | length == 0) or ((firewall_info.firewall.rules.outbound[0].protocol is not defined and updated_firewall.firewall.rules.outbound[0].protocol is not defined) or (firewall_info.firewall.rules.outbound[0].protocol == updated_firewall.firewall.rules.outbound[0].protocol))
           - (firewall_info.firewall.rules.outbound | length == 0 and updated_firewall.firewall.rules.outbound | length == 0) or ((firewall_info.firewall.rules.outbound[0].action is not defined and updated_firewall.firewall.rules.outbound[0].action is not defined) or (firewall_info.firewall.rules.outbound[0].action == updated_firewall.firewall.rules.outbound[0].action))
+
+    - name: Append a new inbound rule via update
+      linode.cloud.firewall:
+        state: update
+        api_version: v4beta
+        label: '{{ create.firewall.label }}'
+        rules:
+          inbound:
+            - label: new_rule
+              action: ACCEPT
+              protocol: TCP
+              addresses:
+                ipv4: ['2.2.2.2/32']
+              description: 'appended rule'
+              ports: '8080'
+      register: append_rule
+
+    - name: Assert new rule was appended
+      assert:
+        that:
+          - append_rule.changed
+          - append_rule.firewall.rules.inbound | length >= 2
+          - append_rule.firewall.rules.inbound | selectattr('label', 'equalto', 'new_rule') | list | length == 1
+
+    - name: Get firewall info after appending a new inbound rule
+      linode.cloud.firewall_info:
+        api_version: v4beta
+        label: '{{ create.firewall.label }}'
+      register: append_rule_info
+
+    - name: Assert appended rule exists in remote firewall state
+      assert:
+        that:
+          - append_rule_info.firewall.rules.inbound | length >= 2
+          - append_rule_info.firewall.rules.inbound | selectattr('label', 'equalto', 'new_rule') | list | length == 1
+
+    - name: Re-run append update to verify idempotency
+      linode.cloud.firewall:
+        state: update
+        api_version: v4beta
+        label: '{{ create.firewall.label }}'
+        rules:
+          inbound:
+            - label: new_rule
+              action: ACCEPT
+              protocol: TCP
+              addresses:
+                ipv4: ['2.2.2.2/32']
+              description: 'appended rule'
+              ports: '8080'
+      register: append_rule_again
+
+    - name: Get firewall info after idempotency check
+      linode.cloud.firewall_info:
+        api_version: v4beta
+        label: '{{ create.firewall.label }}'
+      register: append_rule_info_again
+
+    - name: Assert append update is idempotent
+      assert:
+        that:
+          - not append_rule_again.changed
+          - append_rule_info_again.firewall.rules.inbound | selectattr('label', 'equalto', 'new_rule') | list | length == 1
+
   always:
     - ignore_errors: yes
       block: