Merge pull request #402 from gnfzdz/bugfix_offline_add_interface

Fix adding interface to zone when firewalld is offline

SUMMARY

Fixes issue #357
The existing implementation had several issues which have been resolved by this PR:

incorrectly assumed some zone always exists that contains the interface
incorrectly included the logic to add the interface to the target zone inside of the condition checking if the interface is already assigned to a different zone (and needs to be removed)
passed an invalid argument to the constructor for FirewallClientZoneSettings

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ansible.posix.firewalld
ADDITIONAL INFORMATION


  - name: Add lo interface to trusted zone
    ansible.posix.firewalld:
      interface: lo
      zone: trusted
      permanent: Yes
      state: enabled


Before
TASK [firewalld : Add lo interface to trusted zone] ****************************
task path: /root/ansible_collections/ansible/posix/tests/output/.tmp/integration/firewalld-gpgqwc7n-ÅÑŚÌβŁÈ/tests/integration/targets/firewalld/tasks/interface_test_cases.yml:7
Using module file /root/ansible_collections/ansible/posix/plugins/modules/firewalld.py
Pipelining is enabled.
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c '/usr/bin/python3.10 && sleep 0'
The full traceback is:
  File "/tmp/ansible_ansible.posix.firewalld_payload_2vetziz9/ansible_ansible.posix.firewalld_payload.zip/ansible_collections/ansible/posix/plugins/module_utils/firewalld.py", line 112, in action_handler
    return action_func(*action_func_args)
  File "/tmp/ansible_ansible.posix.firewalld_payload_2vetziz9/ansible_ansible.posix.firewalld_payload.zip/ansible_collections/ansible/posix/plugins/modules/firewalld.py", line 481, in set_enabled_permanent
fatal: [testhost]: FAILED! => {
    "changed": false,
    "invocation": {
        "module_args": {
            "icmp_block": null,
            "icmp_block_inversion": null,
            "immediate": false,
            "interface": "lo",
            "masquerade": null,
            "offline": null,
            "permanent": true,
            "port": null,
            "port_forward": null,
            "rich_rule": null,
            "service": null,
            "source": null,
            "state": "enabled",
            "target": null,
            "timeout": 0,
            "zone": "trusted"
        }
    },
    "msg": "ERROR: Exception caught: list index out of range Permanent operation"
}

After
TASK [firewalld : Add lo interface to trusted zone] ****************************
task path: /root/ansible_collections/ansible/posix/tests/output/.tmp/integration/firewalld-tr92i6e1-ÅÑŚÌβŁÈ/tests/integration/targets/firewalld/tasks/interface_test_cases.yml:7
Using module file /root/ansible_collections/ansible/posix/plugins/modules/firewalld.py
Pipelining is enabled.
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c '/usr/bin/python3.10 && sleep 0'
changed: [testhost] => {
    "changed": true,
    "invocation": {
        "module_args": {
            "icmp_block": null,
            "icmp_block_inversion": null,
            "immediate": false,
            "interface": "lo",
            "masquerade": null,
            "offline": null,
            "permanent": true,
            "port": null,
            "port_forward": null,
            "rich_rule": null,
            "service": null,
            "source": null,
            "state": "enabled",
            "target": null,
            "timeout": 0,
            "zone": "trusted"
        }
    },
    "msg": "Permanent operation, Changed lo to zone trusted, (offline operation: only on-disk configs were altered)"
}

Reviewed-by: Adam Miller <admiller@redhat.com>

changelogs/fragments/402_firewall_fix_offline_interface_add.yml

@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - firewall - Fix issue where adding an interface to a zone would fail when the daemon is offline

plugins/modules/firewalld.py

@@ -520,6 +520,7 @@ class InterfaceTransaction(FirewallTransaction):
                 old_zone_obj = self.fw.config.get_zone(zone)
                 if interface in old_zone_obj.interfaces:
                     iface_zone_objs.append(old_zone_obj)
+
             if len(iface_zone_objs) > 1:
                 # Even it shouldn't happen, it's actually possible that
                 # the same interface is in several zone XML files
@@ -529,18 +530,17 @@ class InterfaceTransaction(FirewallTransaction):
                         len(iface_zone_objs)
                     )
                 )
-            old_zone_obj = iface_zone_objs[0]
-            if old_zone_obj.name != self.zone:
-                old_zone_settings = FirewallClientZoneSettings(
-                    self.fw.config.get_zone_config(old_zone_obj)
-                )
+            elif len(iface_zone_objs) == 1 and iface_zone_objs[0].name != self.zone:
+                old_zone_obj = iface_zone_objs[0]
+                old_zone_config = self.fw.config.get_zone_config(old_zone_obj)
+                old_zone_settings = FirewallClientZoneSettings(list(old_zone_config))
                 old_zone_settings.removeInterface(interface)    # remove from old
                 self.fw.config.set_zone_config(
                     old_zone_obj,
                     old_zone_settings.settings
                 )
-                fw_settings.addInterface(interface)             # add to new
-                self.fw.config.set_zone_config(fw_zone, fw_settings.settings)
+            fw_settings.addInterface(interface)             # add to new
+            self.fw.config.set_zone_config(fw_zone, fw_settings.settings)
         else:
             old_zone_name = self.fw.config().getZoneOfInterface(interface)
             if old_zone_name != self.zone:

tests/integration/targets/firewalld/tasks/interface_test_cases.yml

@@ -0,0 +1,87 @@
+# Test playbook for the firewalld module - interface operations
+# (c) 2022, Gregory Furlong <gnfzdz@fzdz.io>
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+- name: Validate adding interface
+  block:
+  - name: Add lo interface to trusted zone
+    ansible.posix.firewalld:
+      interface: lo
+      zone: trusted
+      permanent: Yes
+      state: enabled
+    register: result
+
+  - name: assert lo was added to trusted zone
+    assert:
+      that:
+      - result is changed
+
+  - name: Add lo interface to trusted zone (verify not changed)
+    ansible.posix.firewalld:
+      interface: lo
+      zone: trusted
+      permanent: Yes
+      state: enabled
+    register: result
+
+  - name: assert lo was added to trusted zone (verify not changed)
+    assert:
+      that:
+      - result is not changed
+
+- name: Validate moving interfaces
+  block:
+  - name: Move lo interface from trusted zone to internal zone
+    ansible.posix.firewalld:
+      interface: lo
+      zone: internal
+      permanent: Yes
+      state: enabled
+    register: result
+
+  - name: Assert lo was moved from trusted zone to internal zone
+    assert:
+      that:
+      - result is changed
+
+  - name: Move lo interface from trusted zone to internal zone (verify not changed)
+    ansible.posix.firewalld:
+      interface: lo
+      zone: internal
+      permanent: Yes
+      state: enabled
+    register: result
+
+  - name: assert lo was moved from trusted zone to internal zone (verify not changed)
+    assert:
+      that:
+      - result is not changed
+
+- name: Validate removing interface
+  block:
+  - name: Remove lo interface from internal zone
+    ansible.posix.firewalld:
+      interface: lo
+      zone: internal
+      permanent: Yes
+      state: disabled
+    register: result
+
+  - name: Assert lo interface was removed from internal zone
+    assert:
+      that:
+      - result is changed
+
+  - name: Remove lo interface from internal zone (verify not changed)
+    ansible.posix.firewalld:
+      interface: lo
+      zone: internal
+      permanent: Yes
+      state: disabled
+    register: result
+
+  - name: Assert lo interface was removed from internal zone (verify not changed)
+    assert:
+      that:
+      - result is not changed

tests/integration/targets/firewalld/tasks/run_all_tests.yml

@@ -27,3 +27,6 @@
 
 # firewalld port forwarding operation test cases
 - include_tasks: port_forward_test_cases.yml
+
+# firewalld interface operation test cases
+- include_tasks: interface_test_cases.yml