chore: Remove obsolete firewall_lib offline code

This code is very problematic:

 * It has not worked for years, at least not since RHEL 9 (probably
   earlier). If firewalld isn't running, this code already crashes in
   the `FirewallClient()` instantiation.

 * There haven't been any integration tests that cover this. The unit
   tests don't cover this as they mock the entire firewalld API, i.e.
   they test an API that simply does not exist.

 * `firewall.core.fw` is not supported API, but the internal CLI
   implementation. That can change at any time (and apparently did,
   assuming that the old code worked at *some* point). There is no
   offline API [1].

 * Even in the current state the implementation was very incomplete.
   None of the service/ipset/zone/etc. settings are implemented, only some
   parts of zone handling (and not even that, e.g. setting the default zone is
   missing as well).

Trying to implement the missing bits with the internal API would be a major
piece of work and go in the wrong direction. Let's rather use the official
methods (`firewall-offline-cmd` or XML editing).

To spare the next person from falling into the "oh, offline is supported"
pitfall, delete all the offline code.

[1] https://issues.redhat.com/browse/RHEL-88425?focusedId=27062772&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-27062772

Author: martinpitt

library/firewall_lib.py

@@ -383,38 +383,16 @@ def create_ipset(module, fw, ipset, ipset_type):
 
 
 def handle_interface_permanent(
-    zone, item, fw_zone, fw_settings, fw, fw_offline, module
+    zone, item, fw_zone, fw_settings, fw, module
 ):
-    if fw_offline:
-        iface_zone_objs = []
-        for zone in fw.config.get_zones():
-            old_zone_obj = fw.config.get_zone(zone)
-            if item in old_zone_obj.interfaces:
-                old_zone_settings = FirewallClientZoneSettings(
-                    fw.config.get_zone_config(old_zone_obj)
-                )
-                old_zone_settings.removeInterface(item)
-                fw.config.set_zone_config(old_zone_obj, old_zone_settings.settings)
-                iface_zone_objs.append(old_zone_obj)
-
-        old_zone_obj = iface_zone_objs[0]
-        if old_zone_obj.name != zone:
-            old_zone_settings = FirewallClientZoneSettings(
-                fw.config.get_zone_config(old_zone_obj)
-            )
+    old_zone_name = fw.config().getZoneOfInterface(item)
+    if old_zone_name != zone:
+        if old_zone_name:
+            old_zone_obj = fw.config().getZoneByName(old_zone_name)
+            old_zone_settings = old_zone_obj.getSettings()
             old_zone_settings.removeInterface(item)
-            fw.config.set_zone_config(old_zone_obj, old_zone_settings.settings)
-            fw_settings.addInterface(item)
-            fw.config.set_zone_config(fw_zone, fw_settings.settings)
-    else:
-        old_zone_name = fw.config().getZoneOfInterface(item)
-        if old_zone_name != zone:
-            if old_zone_name:
-                old_zone_obj = fw.config().getZoneByName(old_zone_name)
-                old_zone_settings = old_zone_obj.getSettings()
-                old_zone_settings.removeInterface(item)
-                old_zone_obj.update(old_zone_settings)
-            fw_settings.addInterface(item)
+            old_zone_obj.update(old_zone_settings)
+        fw_settings.addInterface(item)
 
 
 pci_ids = None
@@ -993,75 +971,37 @@ def main():
 
     fw = FirewallClient()
 
-    fw_offline = False
-    if not fw.connected:
-        # Firewalld is not currently running, permanent-only operations
-        fw_offline = True
-        runtime = False
-        permanent = True
-
-        # Pre-run version checking
-        if lsr_parse_version(FW_VERSION) < lsr_parse_version("0.3.9"):
-            module.fail_json(
-                msg="Unsupported firewalld version %s" " requires >= 0.3.9" % FW_VERSION
-            )
-
-        try:
-            from firewall.core.fw_test import Firewall_test
-
-            fw = Firewall_test()
-
-        except ImportError:
-            # In firewalld version 0.7.0 this behavior changed
-            from firewall.core.fw import Firewall
-
-            fw = Firewall(offline=True)
-
-        fw.start()
-    else:
-        # Pre-run version checking
-        if lsr_parse_version(FW_VERSION) < lsr_parse_version("0.2.11"):
-            module.fail_json(
-                msg="Unsupported firewalld version %s, requires >= 0.2.11" % FW_VERSION
-            )
+    # Pre-run version checking
+    if lsr_parse_version(FW_VERSION) < lsr_parse_version("0.2.11"):
+        module.fail_json(
+            msg="Unsupported firewalld version %s, requires >= 0.2.11" % FW_VERSION
+        )
 
-        # Set exception handler
-        def exception_handler(exception_message):
-            module.fail_json(msg=exception_message)
+    # Set exception handler
+    def exception_handler(exception_message):
+        module.fail_json(msg=exception_message)
 
-        fw.setExceptionHandler(exception_handler)
+    fw.setExceptionHandler(exception_handler)
 
     # Get default zone, the permanent zone and settings
     fw_zone = None
     fw_settings = None
-    if fw_offline:
-        # if zone is None, we will use default zone which always exists
-        zone_exists = zone is None or zone in fw.zone.get_zones()
-        if not zone_exists and not zone_operation:
-            module.fail_json(msg="Permanent zone '%s' does not exist." % zone)
-        elif zone_exists:
-            zone = zone or fw.get_default_zone()
-            fw_zone = fw.config.get_zone(zone)
-            fw_settings = FirewallClientZoneSettings(
-                list(fw.config.get_zone_config(fw_zone))
-            )
-    else:
-        zone_exists = False
-        if runtime:
-            zone_exists = zone_exists or zone is None or zone in fw.getZones()
-            err_str = "Runtime"
-        if permanent:
-            zone_exists = (
-                zone_exists or zone is None or zone in fw.config().getZoneNames()
-            )
-            err_str = "Permanent"
+    zone_exists = False
+    if runtime:
+        zone_exists = zone_exists or zone is None or zone in fw.getZones()
+        err_str = "Runtime"
+    if permanent:
+        zone_exists = (
+            zone_exists or zone is None or zone in fw.config().getZoneNames()
+        )
+        err_str = "Permanent"
 
-        if not zone_exists and not zone_operation:
-            module.fail_json(msg="%s zone '%s' does not exist." % (err_str, zone))
-        elif zone_exists:
-            zone = zone or fw.getDefaultZone()
-            fw_zone = fw.config().getZoneByName(zone)
-            fw_settings = fw_zone.getSettings()
+    if not zone_exists and not zone_operation:
+        module.fail_json(msg="%s zone '%s' does not exist." % (err_str, zone))
+    elif zone_exists:
+        zone = zone or fw.getDefaultZone()
+        fw_zone = fw.config().getZoneByName(zone)
+        fw_settings = fw_zone.getSettings()
 
     # Firewall modification starts here
 
@@ -1493,7 +1433,7 @@ def exception_handler(exception_message):
                 elif not fw_settings.queryInterface(item):
                     if not module.check_mode:
                         handle_interface_permanent(
-                            zone, item, fw_zone, fw_settings, fw, fw_offline, module
+                            zone, item, fw_zone, fw_settings, fw, module
                         )
                     changed = True
         elif state == "disabled":
@@ -1572,10 +1512,7 @@ def exception_handler(exception_message):
     # apply permanent changes
     if changed and (zone_operation or permanent):
         if fw_zone and fw_settings:
-            if fw_offline:
-                fw.config.set_zone_config(fw_zone, fw_settings.settings)
-            else:
-                fw_zone.update(fw_settings)
+            fw_zone.update(fw_settings)
         if need_reload:
             fw.reload()
 

tests/unit/test_firewall_lib.py

@@ -280,60 +280,6 @@ def __call__(self, **kwargs):
 class FirewallInterfaceTests(unittest.TestCase):
     """class to test Firewall interface tests"""
 
-    @patch("firewall_lib.FirewallClientZoneSettings", create=True)
-    @patch("firewall_lib.FirewallClient", create=True)
-    @patch("firewall_lib.HAS_FIREWALLD", True)
-    @patch("firewall_lib.FW_VERSION", "0.3.8", create=True)
-    def test_handle_interface_offline_true(self, zone_settings, firewall_class):
-        module = Mock()
-        zone = "dmz"
-        item = "eth2"
-        fw = firewall_class.return_value
-        fw_config = Mock()
-        fw.config.return_value = fw_config
-        fw_zone = Mock()
-        fw.config.getZoneByName.return_value = fw_zone
-        fw_settings = Mock()
-        fw.config.getZoneByName.getSettings.return_value = fw_settings
-        fw_offline = True
-        fw.config.get_zones.return_value = ["dmz"]
-        fw_zone_two = Mock()
-        fw.config.get_zone.return_value = fw_zone_two
-        fw_zone_two.interfaces = ["eth2"]
-
-        firewall_lib.handle_interface_permanent(
-            zone, item, fw_zone, fw_settings, fw, fw_offline, module
-        )
-        called_mock = getattr(fw_settings, "addInterface")
-        assert [call("eth2")] == called_mock.call_args_list
-
-    @patch("firewall_lib.FirewallClientZoneSettings", create=True)
-    @patch("firewall_lib.FirewallClient", create=True)
-    @patch("firewall_lib.HAS_FIREWALLD", True)
-    @patch("firewall_lib.FW_VERSION", "0.3.8", create=True)
-    def test_handle_interface_offline_false(self, zone_settings, firewall_class):
-        module = Mock()
-        zone = "dmz"
-        item = "eth2"
-        fw = firewall_class.return_value
-        fw_config = Mock()
-        fw.config.return_value = fw_config
-        fw_zone = Mock()
-        fw.config.getZoneByName.return_value = fw_zone
-        fw_settings = Mock()
-        fw.config.getZoneByName.getSettings.return_value = fw_settings
-        fw_offline = False
-        fw.config.get_zones.return_value = ["dmz"]
-        fw_zone_two = Mock()
-        fw.config.get_zone.return_value = fw_zone_two
-        fw_zone_two.interfaces = ["eth2"]
-
-        firewall_lib.handle_interface_permanent(
-            zone, item, fw_zone, fw_settings, fw, fw_offline, module
-        )
-        called_mock = getattr(fw_settings, "addInterface")
-        assert [call("eth2")] == called_mock.call_args_list
-
     @patch("firewall_lib.nm_get_connection_of_interface", create=True)
     def test_try_get_connection_of_interface(self, nm_get_connection_of_interface):
         nm_get_connection_of_interface.return_value = Mock()
@@ -593,20 +539,6 @@ def test_main_error_add_service_permanent_false(self, am_class):
             msg="permanent must be enabled for service configuration. Additionally, service runtime configuration is not possible"
         )
 
-    @patch("firewall_lib.HAS_FIREWALLD", True)
-    def test_permanent_runtime_offline(self, am_class):
-        am = am_class.return_value
-        am.params = {
-            "icmp_block_inversion": True,
-            "permanent": False,
-            "runtime": False,
-        }
-        with self.assertRaises(MockException):
-            firewall_lib.main()
-        am.fail_json.assert_called_with(
-            msg="One of permanent, runtime needs to be enabled"
-        )
-
     @patch("firewall_lib.HAS_FIREWALLD", True)
     def test_timeout_with_disabled_state(self, am_class):
         am = am_class.return_value
@@ -663,23 +595,6 @@ def test_main_error_timeout_target(self, am_class):
             firewall_lib.main()
         am.fail_json.assert_called_with(msg="timeout can not be used with target only")
 
-    @patch("firewall_lib.FirewallClient", create=True)
-    @patch("firewall_lib.HAS_FIREWALLD", True)
-    @patch("firewall_lib.FW_VERSION", "0.3.8", create=True)
-    def test_firewalld_offline_version_disconnected(self, firewall_class, am_class):
-        am = am_class.return_value
-        am.params = {
-            "icmp_block_inversion": True,
-            "permanent": True,
-        }
-        fw = firewall_class.return_value
-        fw.connected = False
-        with self.assertRaises(MockException):
-            firewall_lib.main()
-        am.fail_json.assert_called_with(
-            msg="Unsupported firewalld version 0.3.8 requires >= 0.3.9"
-        )
-
     @patch("firewall_lib.FirewallClient", create=True)
     @patch("firewall_lib.HAS_FIREWALLD", True)
     @patch("firewall_lib.FW_VERSION", "0.2.8", create=True)