fix: el7 interface functionality requires NetworkManager

richm · richm · commit 608ccc9ed836 · 2026-02-18T13:36:56.000-05:00
Cause: NetworkManager is not installed on some EL7 systems by default.

Consequence: The interface_pci_id could not be found.

Fix: Ensure NetworkManager is installed on EL7.

Result: The interface_pci_id can be used on EL7.

Also - `module.warn` is deprecated - use `warnings` instead if `module.warn` is
not supported.

Fix the interface pci test to ensure NetworkManager is running before the test.

add warnings parameter for tests

Signed-off-by: Rich Megginson &lt;rmeggins@redhat.com&gt;
diff --git a/library/firewall_lib.py b/library/firewall_lib.py
@@ -1664,22 +1664,26 @@ def parse_pci_id(module, item):
     if not module.params["online"]:
         module.fail_json(msg="interface_pci_id is not supported in offline mode.")
 
+    warning = ""
     if PCI_REGEX.search(item):
         global pci_ids
         if not pci_ids:
             pci_ids = get_interface_pci()
 
-        interface_name = pci_ids.get(item)
-        if interface_name:
-            return interface_name
+        interface_names = pci_ids.get(item)
+        if interface_names:
+            return interface_names, warning
 
-        module.warn(msg="No network interfaces found with PCI device ID %s" % item)
+        warning = "No network interfaces found with PCI device ID %s" % item
+        if callable(getattr(module, "warn", None)):
+            module.warn(warning)
+            warning = ""
     else:
         module.fail_json(
             msg="PCI id %s does not match format: XXXX:XXXX (X = hexadecimal number)"
             % item
         )
-    return []
+    return [], warning
 
 
 def parse_port(module, item):
@@ -1938,6 +1942,7 @@ def main():
     if not HAS_FIREWALLD:
         module.fail_json(msg="No firewall backend could be imported.")
 
+    warnings = []
     # Argument parse
     firewalld_conf = module.params["firewalld_conf"]
     if firewalld_conf:
@@ -1946,9 +1951,14 @@ def main():
             FW_VERSION
         ) >= lsr_parse_version("1.0.0")
         if allow_zone_drifting_deprecated and firewalld_conf.get("allow_zone_drifting"):
-            module.warn(
-                "AllowZoneDrifting is deprecated in this version of firewalld and no longer supported"
-            )
+            if callable(getattr(module, "warn", None)):
+                module.warn(
+                    "AllowZoneDrifting is deprecated in this version of firewalld and no longer supported"
+                )
+            else:
+                warnings.append(
+                    "AllowZoneDrifting is deprecated in this version of firewalld and no longer supported"
+                )
     else:
         # CodeQL will produce an error without this line
         allow_zone_drifting_deprecated = None
@@ -1991,9 +2001,12 @@ def main():
             module.fail_json(msg="cannot have more than one destination ipv6")
     interface = module.params["interface"]
     for _interface in module.params["interface_pci_id"]:
-        for interface_name in parse_pci_id(module, _interface):
+        interface_names, warning = parse_pci_id(module, _interface)
+        for interface_name in interface_names:
             if interface_name not in interface:
                 interface.append(interface_name)
+        if warning:
+            warnings.append(warning)
     icmp_block = module.params["icmp_block"]
     icmp_block_inversion = module.params["icmp_block_inversion"]
     timeout = module.params["timeout"]
@@ -2282,7 +2295,7 @@ def main():
     backend.finalize()
 
     changed = backend.changed and module.params["__report_changed"]
-    module.exit_json(changed=changed, __firewall_changed=changed)
+    module.exit_json(changed=changed, __firewall_changed=changed, warnings=warnings)
 
 
 #################################################
diff --git a/tests/tests_interface_pci.yml b/tests/tests_interface_pci.yml
@@ -2,22 +2,20 @@
 # Tests interface_pci field, test must be used in VM
 - name: Test interfaces with PCI ids
   hosts: all
+  become: true
   tags:
     # container builds have no PCI devices, so ethernet interface detection
     # won't find anything; so save ourselves the extra initial role invocation
     - tests::booted
-  become: true
-  roles:
-    - linux-system-roles.firewall
-
+  gather_facts: true
+  vars_files:
+    - vars/rh_distros_vars.yml
   tasks:
     - name: Set expected backend
       set_fact:
-        nftables_backend:
-          "{{ true
-            if ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
-            and ansible_facts['distribution_major_version'] is version('8', '>=')
-            else false }}"
+        nftables_backend: "{{
+          __firewall_is_rh_distro_fedora and
+          ansible_facts['distribution_major_version'] is version('8', '>=') }}"
 
     - name: Find ethernet interface
       shell: |
@@ -57,6 +55,7 @@
             set -euo pipefail
             nft list ruleset > {{ temp_dir.path }}/nft_before.txt || :
           changed_when: false
+          when: nftables_backend | bool
 
         - name: Determine interface vendor/product ID
           shell: |
@@ -67,6 +66,21 @@
           register: pci_id
           changed_when: false
 
+        - name: Set up for el7
+          when:
+            - __firewall_is_rh_distro
+            - ansible_facts['distribution_major_version'] is version('8', '<')
+          block:
+            - name: Install NetworkManager
+              package:
+                name: NetworkManager
+                state: present
+
+            - name: Start NetworkManager
+              service:
+                name: NetworkManager
+                state: started
+
         - name: Add pci device ethernet controller
           include_role:
             name: linux-system-roles.firewall
@@ -94,6 +108,7 @@
             diff -u {{ temp_dir.path }}/nft_before.txt {{ temp_dir.path }}/nft_after.txt || :
             rm -rf {{ temp_dir.path }}
           changed_when: false
+          when: nftables_backend | bool
 
         - name: Get nftable ruleset
           command: nft list ruleset
diff --git a/tests/unit/test_firewall_lib.py b/tests/unit/test_firewall_lib.py
@@ -948,10 +948,16 @@ def test_allow_zone_drifting_deprecated(self, firewall_class, am_class):
             "permanent": True,
         }
         firewall_lib.main()
-        am.warn.assert_called_with(
-            "AllowZoneDrifting is deprecated in this version of firewalld and no longer supported"
-        )
-        am.exit_json.assert_called_with(changed=False, __firewall_changed=False)
+        warning_msg = "AllowZoneDrifting is deprecated in this version of firewalld and no longer supported"
+        if getattr(am, "warn", None):
+            am.warn.assert_called_with(warning_msg)
+            am.exit_json.assert_called_with(
+                changed=False, __firewall_changed=False, warnings=[]
+            )
+        else:
+            am.exit_json.assert_called_with(
+                changed=False, __firewall_changed=False, warnings=[warning_msg]
+            )
 
     @patch("firewall_lib.HAS_FIREWALLD", True)
     @patch("firewall_lib.FW_VERSION", "0.9.0", create=True)
@@ -1003,7 +1009,9 @@ def test_nm_integration_interfaces(
                 try_set_zone_of_interface.return_value = rv
                 fw_settings.queryService.return_value = state == "disabled"
                 firewall_lib.main()
-                am.exit_json.assert_called_with(changed=rv[1], __firewall_changed=rv[1])
+                am.exit_json.assert_called_with(
+                    changed=rv[1], __firewall_changed=rv[1], warnings=[]
+                )
 
         for state in ["enabled", "disabled"]:
             am.params["state"] = state
@@ -1012,7 +1020,9 @@ def test_nm_integration_interfaces(
                 try_set_zone_of_interface.return_value = rv
                 fw_settings.queryService.return_value = state == "disabled"
                 firewall_lib.main()
-                am.exit_json.assert_called_with(changed=True, __firewall_changed=True)
+                am.exit_json.assert_called_with(
+                    changed=True, __firewall_changed=True, warnings=[]
+                )
 
     @patch("firewall_lib.HAS_FIREWALLD", True)
     @patch("firewall_lib.FW_VERSION", "0.9.0", create=True)
@@ -1534,7 +1544,9 @@ def test_module_parameters(method, state, input, expected, _offline_cmd):
         if permanent:
             called_mock = getattr(fw_settings, called_mock_name)
             assert expected["permanent"] == called_mock.call_args_list
-        am.exit_json.assert_called_once_with(changed=True, __firewall_changed=True)
+        am.exit_json.assert_called_once_with(
+            changed=True, __firewall_changed=True, warnings=[]
+        )
     finally:
         am_class_patcher.stop()
         fw_client_patcher.stop()
@@ -1614,7 +1626,9 @@ def mock_run_command(args, check_rc=False):
         else:
             firewall_lib.main()
             assert called_cmds == expected["offline"]
-            am.exit_json.assert_called_once_with(changed=True, __firewall_changed=True)
+            am.exit_json.assert_called_once_with(
+                changed=True, __firewall_changed=True, warnings=[]
+            )
     finally:
         am_class_patcher.stop()
         has_fw_patcher.stop()
diff --git a/vars/main.yml b/vars/main.yml
@@ -22,7 +22,10 @@ __firewall_required_facts_subsets: "{{ ['!all', '!min'] +
 
 __firewall_packages_base: [firewalld]
 
-__firewall_packages_extra: "{{ ['python-ipaddress']
+# NetworkManager is required for the interface_pci_id functionality
+# python-ipaddress is required for the ipaddress module
+# These are built-in (?) on EL8 and later
+__firewall_packages_extra: "{{ ['NetworkManager', 'python-ipaddress']
   if __firewall_is_rh_distro and ansible_facts['distribution_major_version'] | int < 8
   else [] }}"
 
