fix: Improve firewall diff logic (#193)

* Improve firewall diff/update logic

* Improve logic

* cleanup

plugins/module_utils/linode_helper.py

@@ -10,6 +10,7 @@
 RETRY_INTERVAL_SECONDS = 4
 RETRY_STATUSES = {408}
 
+
 def dict_select_spec(target: dict, spec: dict) -> dict:
     """Returns a new dictionary that only selects the keys from target that are specified in spec"""
 
@@ -97,7 +98,7 @@ def handle_updates(obj: linode_api4.Base, params: dict, mutable_fields: set, reg
             if key in mutable_fields:
                 put_request[key] = new_value
                 register_func('Updated {0}: "{1}" -> "{2}"'.
-                                     format(key, old_value, new_value))
+                              format(key, old_value, new_value))
 
                 continue
 
@@ -126,6 +127,7 @@ def parse_linode_types(value: any) -> any:
 
     return value
 
+
 def jsonify_node_pool(pool: LKENodePool) -> Dict[str, Any]:
     """Converts an LKENodePool into a JSON-compatible dict"""
 
@@ -188,3 +190,14 @@ def request_retry(request_func: Callable, retry_statuses=None,
         return response
 
     raise Exception('exceeded maximum number of retries: {0}'.format(max_retries))
+
+
+def filter_null_values_recursive(obj: Any) -> Any:
+    """Recursively removes null values and keys from a structure."""
+    if isinstance(obj, dict):
+        return {k: filter_null_values_recursive(v) for k, v in obj.items() if v is not None}
+
+    if isinstance(obj, (list, set, tuple)):
+        return [filter_null_values_recursive(v) for v in obj if v is not None]
+
+    return obj

plugins/modules/firewall.py

@@ -8,13 +8,12 @@
 import copy
 from typing import Optional, List, Any
 
+import ipaddress
 from ansible_collections.linode.cloud.plugins.module_utils.linode_common import LinodeModuleBase
 from ansible_collections.linode.cloud.plugins.module_utils.linode_helper import \
-    filter_null_values, mapping_to_dict, paginated_list_to_json
+    filter_null_values, mapping_to_dict, paginated_list_to_json, filter_null_values_recursive
 from ansible_collections.linode.cloud.plugins.module_utils.linode_docs import global_authors, \
     global_requirements
-
-
 import ansible_collections.linode.cloud.plugins.module_utils.doc_fragments.firewall as docs
 
 try:
@@ -175,15 +174,13 @@ def _get_firewall_by_label(self, label: str) -> Optional[Firewall]:
             return self.fail(msg='failed to get firewall {0}: {1}'.format(label, exception))
 
     def _create_firewall(self) -> dict:
-        params = copy.deepcopy(self.module.params)
-
-        label = params.pop('label')
-        params = {k: v for k, v in params.items() if k in {
-            'rules', 'tags'
-        }}
+        params = self.module.params
 
+        label = params.get('label')
+        rules = filter_null_values_recursive(params['rules'])
+        tags = params['tags']
         try:
-            result = self.client.networking.firewall_create(label, **params)
+            result = self.client.networking.firewall_create(label, rules=rules, tags=tags)
         except Exception as exception:
             self.fail(msg='failed to create firewall: {0}'.format(exception))
 
@@ -225,6 +222,44 @@ def _update_devices(self, spec_devices: list) -> None:
         for device in device_map.values():
             self._delete_device(device)
 
+    @staticmethod
+    def _normalize_ips(rules: list) -> list:
+        result = []
+        for rule in rules:
+            item = copy.deepcopy(rule)
+
+            addresses = rule['addresses']
+
+            if 'ipv6' in addresses:
+                item['addresses']['ipv6'] = [str(ipaddress.IPv6Network(v))
+                                             for v in addresses['ipv6']]
+
+            if 'ipv4' in addresses:
+                item['addresses']['ipv4'] = [str(ipaddress.IPv4Network(v))
+                                             for v in addresses['ipv4']]
+
+            result.append(item)
+
+        return result
+
+    def _rules_changed(self) -> bool:
+        """Returns whether the local and remote firewall rules match."""
+        local_rules = filter_null_values_recursive(self.module.params['rules'])
+        remote_rules = filter_null_values_recursive(mapping_to_dict(self._firewall.rules))
+
+        # Normalize IP addresses for all rules
+        for field in {'inbound', 'outbound'}:
+            if field in local_rules:
+                local_rules[field] = self._normalize_ips(local_rules[field])
+            else:
+                # We should normalize missing keys to [] for diffing purposes
+                local_rules[field] = []
+
+            if field in remote_rules:
+                remote_rules[field] = self._normalize_ips(remote_rules[field])
+
+        return local_rules != remote_rules
+
     def _update_firewall(self) -> None:
         """Handles all update functionality for the current Firewall"""
 
@@ -249,17 +284,16 @@ def _update_firewall(self) -> None:
         if should_update:
             self._firewall.save()
 
-        # Update rules
-        if mapping_to_dict(self._firewall.rules) != params.get('rules'):
-            self._firewall.update_rules(params.get('rules'))
+        if self._rules_changed():
+            self._firewall.update_rules(filter_null_values_recursive(params.get('rules')))
             self.register_action('Updated Firewall rules')
 
         # Update devices
         devices: Optional[List[Any]] = params.get('devices')
         if devices is not None:
             self._update_devices(devices)
 
-    def _handle_firewall(self) -> None:
+    def _handle_present(self) -> None:
         """Updates the Firewall"""
         label = self.module.params.get('label')
 
@@ -276,7 +310,7 @@ def _handle_firewall(self) -> None:
         self.results['firewall'] = self._firewall._raw_json
         self.results['devices'] = paginated_list_to_json(self._firewall.devices)
 
-    def _handle_firewall_absent(self) -> None:
+    def _handle_absent(self) -> None:
         """Destroys the Firewall"""
         label = self.module.params.get('label')
 
@@ -294,10 +328,10 @@ def exec_module(self, **kwargs: Any) -> Optional[dict]:
         state = kwargs.get('state')
 
         if state == 'absent':
-            self._handle_firewall_absent()
+            self._handle_absent()
             return self.results
 
-        self._handle_firewall()
+        self._handle_present()
         return self.results
 
 

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

@@ -0,0 +1,92 @@
+- name: firewall_icmp
+  block:
+    - set_fact:
+        r: "{{ 1000000000 | random }}"
+    - name: Create a Linode Instance
+      linode.cloud.instance:
+        api_token: '{{ api_token }}'
+        label: 'ansible-test-{{ r }}'
+        region: us-southeast
+        type: g6-standard-1
+        image: linode/alpine3.13
+        state: present
+      register: create_instance
+
+    - name: Create a Linode Firewall
+      linode.cloud.firewall:
+        api_token: '{{ api_token }}'
+        api_version: v4beta
+        label: 'ansible-test-{{ r }}'
+        devices: []
+        rules:
+          outbound_policy: ACCEPT
+          inbound_policy: DROP
+          inbound:
+            - label: icmp
+              addresses:
+                ipv4: "{{ ['0.0.0.0/0'] }}"
+                ipv6: "{{ ['::0/0'] }}"
+              description: "Allow all icmp traffic"
+              protocol: ICMP
+              action: ACCEPT
+        state: present
+      register: create
+
+    - name: Assert firewall created
+      assert:
+        that:
+          - create.changed
+
+    - name: Unchanged check
+      linode.cloud.firewall:
+        api_token: '{{ api_token }}'
+        api_version: v4beta
+        label: 'ansible-test-{{ r }}'
+        devices: []
+        rules:
+          outbound_policy: ACCEPT
+          inbound_policy: DROP
+          inbound:
+            - label: icmp
+              addresses:
+                ipv4: "{{ ['0.0.0.0/0'] }}"
+                ipv6: "{{ ['::0/0'] }}"
+              description: "Allow all icmp traffic"
+              protocol: ICMP
+              action: ACCEPT
+        state: present
+      register: unchanged
+
+    - assert:
+        that:
+          - unchanged.changed == False
+
+  always:
+    - ignore_errors: yes
+      block:
+        - name: Delete a Linode Firewall
+          linode.cloud.firewall:
+            api_token: '{{ api_token }}'
+            api_version: v4beta
+            label: '{{ create.firewall.label }}'
+            state: absent
+          register: delete
+
+        - name: Assert Firewall delete succeeded
+          assert:
+            that:
+              - delete.changed
+              - delete.firewall.id == create.firewall.id
+
+        - name: Delete a Linode instance
+          linode.cloud.instance:
+            api_token: '{{ api_token }}'
+            label: '{{ create_instance.instance.label }}'
+            state: absent
+          register: delete_instance
+
+        - name: Assert instance delete succeeded
+          assert:
+            that:
+              - delete_instance.changed
+              - delete_instance.instance.id == create_instance.instance.id