From 960aaefa1325eecfe8d7e395ed8d0a1dd49eb4f7 Mon Sep 17 00:00:00 2001 From: Christian Manivong Date: Thu, 20 Aug 2026 11:49:16 +0700 Subject: [PATCH] fix(dhcp): read the Kea subnet record from subnet4, not subnet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getSubnet wraps its record under `subnet4`. The driver read `subnet`, got nothing, and carried on: - get_dhcp_subnets() reported every subnet with no pools, no options and no description. Only the CIDR survived, and only because it falls back to the searchSubnet row. Confirmed against a live OPNsense serving six subnets: all six came back with empty pools while the device had "10.10.0.100-10.10.0.250" and routers/DNS/NTP set on each. - apply_dhcp_subnet() read the same key to merge the options it was not asked to change. An empty record means nothing to preserve, so updating a subnet with only domain_search set would have written back only that one option and blanked the routers Kea autocollected — stranding every client on that VLAN without a gateway. That is precisely the failure the merge exists to prevent. The unit fixtures encoded the wrong shape, which is why the safety test test_unnamed_options_are_preserved_on_update passed while the real thing was broken. They now carry the response captured from OPNsense 25.x, and correcting them turns that test red against the old parse. Both call sites go through _kea_subnet_record(), which prefers `subnet4` and falls back to `subnet` for older builds. --- napalm_opnsense/opnsense.py | 18 ++++++++++++++++-- tests/unit/test_driver.py | 29 ++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/napalm_opnsense/opnsense.py b/napalm_opnsense/opnsense.py index c8fdc2a..1a7c6a6 100644 --- a/napalm_opnsense/opnsense.py +++ b/napalm_opnsense/opnsense.py @@ -1618,6 +1618,20 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): return selected[0] if selected else "" return str(raw or "").strip() + @staticmethod + def _kea_subnet_record(detail: dict[str, Any]) -> dict[str, Any]: + """The subnet record out of a ``getSubnet`` response. + + OPNsense wraps it under ``subnet4``; older builds used ``subnet``, and + keeping the fallback costs nothing. Reading the wrong key costs a + great deal: the record comes back empty, so a subnet reports no pools + and no options at all, and on update the partial-option merge has + nothing to preserve and blanks every option Kea autocollected -- + stranding a whole VLAN without a gateway, which is the exact failure + that merge exists to prevent. + """ + return detail.get("subnet4") or detail.get("subnet") or {} + def get_dhcp_subnets(self) -> list[dict[str, Any]]: """Return every Kea DHCPv4 subnet with its pools and options. @@ -1650,7 +1664,7 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): logger.warning("Kea subnet %s detail fetch failed, skipping: %s", uuid, exc) continue - record = detail.get("subnet") or {} + record = self._kea_subnet_record(detail) raw_options = record.get("option_data") or {} option_data: dict[str, Any] = {} @@ -1706,7 +1720,7 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): if uuid and desired_options: try: current = self._get(f"/api/kea/dhcpv4/getSubnet/{uuid}") - raw = (current.get("subnet") or {}).get("option_data") or {} + raw = self._kea_subnet_record(current).get("option_data") or {} except Exception as exc: raise RuntimeError( f"Kea subnet {uuid} could not be read before update: {exc}" diff --git a/tests/unit/test_driver.py b/tests/unit/test_driver.py index 21b7970..def9e88 100644 --- a/tests/unit/test_driver.py +++ b/tests/unit/test_driver.py @@ -2177,10 +2177,11 @@ KEA_SUBNET_SEARCH_RESPONSE = { ] } -# getSubnet wraps the record and, unlike searchSubnet, carries the options. -# OPNsense renders AsList fields as comma-separated strings. +# getSubnet wraps the record under `subnet4` -- captured from OPNsense 25.x, +# not guessed. Unlike searchSubnet it carries the options, and OPNsense +# renders AsList fields as comma-separated strings. KEA_SUBNET_DETAIL_RESPONSE = { - "subnet": { + "subnet4": { "subnet": "10.10.20.0/24", "description": "Home", "pools": "10.10.20.100-10.10.20.200", @@ -2243,7 +2244,7 @@ class TestGetDhcpSubnets: def test_multiline_pools_become_a_list(self, driver): detail = { - "subnet": { + "subnet4": { "subnet": "10.10.20.0/24", "description": "", "pools": "10.10.20.100-10.10.20.150\n10.10.20.180-10.10.20.200", @@ -2259,7 +2260,7 @@ class TestGetDhcpSubnets: def test_selection_map_shape_is_accepted(self, driver): """Some OPNsense versions render list fields as a selection map.""" detail = { - "subnet": { + "subnet4": { "subnet": "10.10.20.0/24", "description": "", "pools": "", @@ -2400,3 +2401,21 @@ class TestCommitDhcpSubnets: assert driver.commit_dhcp_subnets() == {"success": True} assert calls == ["/api/kea/service/reconfigure"] + + +class TestKeaSubnetRecordKey: + """`getSubnet` wraps its record under `subnet4`. Reading the wrong key is + not a cosmetic miss: the record comes back empty, so every subnet reports + no pools and no options, and the partial-option merge on update has + nothing to preserve.""" + + def test_reads_the_subnet4_wrapper(self, driver): + record = driver._kea_subnet_record({"subnet4": {"subnet": "10.0.0.0/24"}}) + assert record == {"subnet": "10.0.0.0/24"} + + def test_falls_back_to_the_legacy_subnet_wrapper(self, driver): + record = driver._kea_subnet_record({"subnet": {"subnet": "10.0.0.0/24"}}) + assert record == {"subnet": "10.0.0.0/24"} + + def test_unknown_shape_yields_an_empty_record(self, driver): + assert driver._kea_subnet_record({"something_else": {}}) == {}