From 59796b3fa1f2bb49e32d11106a2954a9cf92968d Mon Sep 17 00:00:00 2001 From: Christian Manivong Date: Wed, 21 Oct 2026 01:01:52 +0200 Subject: [PATCH] fix: leave an unchanged Kea reservation and Kea alone create_dhcp_reservation rewrote an existing reservation and called service/reconfigure even when it already held exactly what was asked, e.g. on a retried provisioning job. reconfigure stops and starts Kea (the Kea service controller keeps reconfigureForceRestart() = true and configd has no reload action), which once left a firewall without DHCP. A reservation whose subnet, MAC (normalised), IP, hostname and description already match is now left alone. Refs NetOrk/netork#1187 --- napalm_opnsense/opnsense.py | 40 ++++++++++++++++++- tests/unit/test_driver.py | 77 +++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/napalm_opnsense/opnsense.py b/napalm_opnsense/opnsense.py index d38b5b6..2a7720b 100644 --- a/napalm_opnsense/opnsense.py +++ b/napalm_opnsense/opnsense.py @@ -55,6 +55,7 @@ import requests from requests.exceptions import RequestException from napalm_device_types import ConfigSyncMixin, FingerprintRule, FirewallDriver, SnmpAgentMixin +from napalm_device_types.dhcp import normalize_mac from napalm_device_types.models import ( FixStepOutcomeDict, SnmpAgentSettingsDict, @@ -1393,6 +1394,11 @@ class OPNsenseDriver( plugin as "reservations unsupported" (``RuntimeError``), not fall back to plain DHCP silently. + A reservation that already reads exactly as requested is left alone: + no ``setReservation`` and no ``service/reconfigure``. The reconfigure + stops and starts Kea rather than reloading it, so restarting the + DHCP server for a write that changes nothing is pure risk. + :param mac: NIC MAC address (any common formatting; sent as-is to Kea's ``hw_address`` field). :param ip: IP address to reserve; must fall inside a subnet Kea @@ -1409,10 +1415,12 @@ class OPNsenseDriver( ip_obj = ip_address(ip) subnet_uuid = None + subnet_cidr = "" for row in subnets: try: if ip_obj in ip_network(row["subnet"], strict=False): subnet_uuid = row["uuid"] + subnet_cidr = str(row["subnet"]).strip() break except ValueError: continue @@ -1422,7 +1430,7 @@ class OPNsenseDriver( # Idempotency: reuse an existing reservation for this IP if one # already exists (e.g. a retried provisioning job), rather than # creating a duplicate Kea rejects anyway. - existing_uuid = None + existing_row: dict[str, Any] | None = None try: existing = self._post( "/api/kea/dhcpv4/searchReservation", @@ -1430,7 +1438,7 @@ class OPNsenseDriver( ) for row in existing.get("rows") or []: if row.get("ip_address") == ip: - existing_uuid = row.get("uuid") + existing_row = row break except Exception as exc: logger.debug("Kea reservation search failed, proceeding to add: %s", exc) @@ -1444,6 +1452,13 @@ class OPNsenseDriver( "description": self._NETORK_TAG, } } + if existing_row is not None and self._kea_reservation_unchanged( + existing_row, payload["reservation"], subnet_cidr + ): + logger.debug("Kea reservation for %s is already as requested, not touched", ip) + return + + existing_uuid = existing_row.get("uuid") if existing_row is not None else None path = ( f"/api/kea/dhcpv4/setReservation/{existing_uuid}" if existing_uuid @@ -1455,6 +1470,27 @@ class OPNsenseDriver( self._post("/api/kea/service/reconfigure") + @staticmethod + def _kea_reservation_unchanged( + row: dict[str, Any], wanted: dict[str, Any], subnet_cidr: str + ) -> bool: + """Whether a ``searchReservation`` row already holds every field we would write. + + The MAC is compared normalised: Kea keeps it as it was entered, in + either case and with ``:`` or ``-``. The subnet is a model relation that + comes back as the subnet's CIDR or its UUID depending on the OPNsense + version, so either counts. A field the row does not carry counts as a + difference -- writing once too often is the safe side. + """ + if str(row.get("subnet") or "").strip() not in {wanted["subnet"], subnet_cidr}: + return False + if normalize_mac(row.get("hw_address")) != normalize_mac(wanted["hw_address"]): + return False + return all( + str(row.get(field) or "") == str(wanted[field] or "") + for field in ("ip_address", "hostname", "description") + ) + def delete_dhcp_reservation_and_lease(self, mac: str, ip: str) -> dict[str, Any]: """Remove a Kea DHCPv4 static reservation and any active lease for (mac, ip). diff --git a/tests/unit/test_driver.py b/tests/unit/test_driver.py index 0fdb74c..e6b6c00 100644 --- a/tests/unit/test_driver.py +++ b/tests/unit/test_driver.py @@ -1787,6 +1787,83 @@ class TestCreateDhcpReservation: == "https://opnsense.example.com/api/kea/dhcpv4/setReservation/existing-uuid-456" ) + # A reconfigure stops and starts Kea (ApiMutableServiceControllerBase, + # reconfigureForceRestart() is true for Kea). Writing a reservation that + # is already there, and restarting the daemon for it, once left a + # firewall without DHCP (#1187) -- so an unchanged one is not touched. + + @pytest.mark.parametrize( + "subnet", + ["172.22.8.0/24", "6854cab3-ebb5-4031-b987-0edcc6723546"], + ids=["subnet-as-cidr", "subnet-as-uuid"], + ) + def test_unchanged_reservation_is_neither_written_nor_reconfigured(self, driver, subnet): + driver.session.get.return_value = _make_json_response(KEA_SUBNETS_RESPONSE) + driver.session.post.side_effect = [ + _make_json_response( + { + "rows": [ + { + "uuid": "existing-uuid-456", + "subnet": subnet, + "ip_address": "172.22.8.253", + # Kea keeps the MAC as it was entered -- case and + # separator do not make it a different reservation. + "hw_address": "02-AA-BB-CC-DD-EE", + "hostname": "new-vm", + "description": "[netork]", + } + ] + } + ), + ] + + driver.create_dhcp_reservation( + mac="02:aa:bb:cc:dd:ee", ip="172.22.8.253", hostname="new-vm" + ) + + # searchReservation only -- no setReservation, no service/reconfigure. + assert driver.session.post.call_count == 1 + + @pytest.mark.parametrize( + "field, live_value", + [ + ("hw_address", "02:aa:bb:cc:dd:ff"), + ("hostname", "old-vm"), + ("description", "made by hand"), + ("subnet", "172.22.0.0/24"), + ], + ) + def test_reservation_differing_in_one_field_is_written_and_reconfigured( + self, driver, field, live_value + ): + row = { + "uuid": "existing-uuid-456", + "subnet": "172.22.8.0/24", + "ip_address": "172.22.8.253", + "hw_address": "02:aa:bb:cc:dd:ee", + "hostname": "new-vm", + "description": "[netork]", + } + row[field] = live_value + driver.session.get.return_value = _make_json_response(KEA_SUBNETS_RESPONSE) + driver.session.post.side_effect = [ + _make_json_response({"rows": [row]}), + _make_json_response({"result": "saved", "uuid": "existing-uuid-456"}), + _make_json_response({"status": "ok"}), + ] + + driver.create_dhcp_reservation( + mac="02:aa:bb:cc:dd:ee", ip="172.22.8.253", hostname="new-vm" + ) + + paths = [c.args[0] for c in driver.session.post.call_args_list] + assert paths == [ + "https://opnsense.example.com/api/kea/dhcpv4/searchReservation", + "https://opnsense.example.com/api/kea/dhcpv4/setReservation/existing-uuid-456", + "https://opnsense.example.com/api/kea/service/reconfigure", + ] + def test_raises_when_ip_not_in_any_kea_subnet(self, driver): driver.session.get.return_value = _make_json_response(KEA_SUBNETS_RESPONSE)