From 58e994297e96dd1e9f7697a967ec692a768400f9 Mon Sep 17 00:00:00 2001 From: Lena Garber Date: Thu, 15 Sep 2022 15:07:30 -0400 Subject: [PATCH] fix: Improve `firewall` diff logic (#193) * Improve firewall diff/update logic * Improve logic * cleanup --- plugins/module_utils/linode_helper.py | 15 ++- plugins/modules/firewall.py | 68 ++++++++++---- .../targets/firewall_icmp/tasks/main.yaml | 92 +++++++++++++++++++ 3 files changed, 157 insertions(+), 18 deletions(-) create mode 100644 tests/integration/targets/firewall_icmp/tasks/main.yaml diff --git a/plugins/module_utils/linode_helper.py b/plugins/module_utils/linode_helper.py index e26f4c59..0079fe3d 100644 --- a/plugins/module_utils/linode_helper.py +++ b/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 diff --git a/plugins/modules/firewall.py b/plugins/modules/firewall.py index 345fe000..a1efe72d 100644 --- a/plugins/modules/firewall.py +++ b/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,9 +284,8 @@ 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 @@ -259,7 +293,7 @@ def _update_firewall(self) -> None: 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 diff --git a/tests/integration/targets/firewall_icmp/tasks/main.yaml b/tests/integration/targets/firewall_icmp/tasks/main.yaml new file mode 100644 index 00000000..a09f989b --- /dev/null +++ b/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 \ No newline at end of file