feat(dhcp): add DhcpServerMixin for static DHCP reservations

Adds the generic half of DHCP reservation management: diff_dhcp_reservations matches desired against live reservations by normalised MAC, and apply_dhcp_reservationset walks the diff and commits once at the end. Both are concrete here because neither is vendor-specific — only get_dhcp_reservations/apply_dhcp_reservation/commit_dhcp_reservations touch the device (Kea REST on OPNsense, dnsmasq/odhcpd UCI on OpenWrt). Two deliberate choices: - The MAC is the matching key, not a description as with firewall rules. A reservation has a natural identity and this is it. That also means a host moving to another VLAN is an update of the existing entry rather than a second one for the same MAC. - An empty diff skips the commit. Committing reloads the DHCP daemon and drops in-flight requests, which is too high a price for a no-op run. This differs from apply_firewall_ruleset, which always commits. Live reservations with no desired counterpart are never reported for deletion — a DHCP server routinely carries hand-created entries the caller's desired set was never meant to describe. Mixed into FirewallDriver and ResidentialGatewayDriver: both device types commonly run the DHCP server for their networks.
2026-08-19 07:24:39 +07:00
parent a211629875
commit b8977cdaa5
7 changed files with 463 additions and 2 deletions
@@ -0,0 +1,195 @@
+"""Tests for DhcpServerMixin's generic reservation diff/apply mechanism.
+
+diff_dhcp_reservations/apply_dhcp_reservationset are concrete methods on the
+mixin (not overridden by concrete drivers) — they only depend on the three
+abstract methods (get_dhcp_reservations/apply_dhcp_reservation/
+commit_dhcp_reservations), so a fake in-memory driver is enough to exercise
+them fully; no real device or vendor driver needed. See README.md "Design
+principle: generic vs. device-specific logic" for why this logic lives here
+and not in a vendor driver.
+"""
+
+from typing import Any, Dict, List, Optional
+
+import pytest
+from napalm_device_types import DhcpServerMixin, FirewallDriver, ResidentialGatewayDriver
+from napalm_device_types.dhcp import normalize_mac
+from napalm_device_types.models import DhcpReservationDict
+
+
+def _reservation(**overrides: Any) -> DhcpReservationDict:
+    base: DhcpReservationDict = {
+        "uuid": "",
+        "mac": "aa:bb:cc:dd:ee:01",
+        "ip": "10.10.20.50",
+        "hostname": "nas",
+        "description": "Home NAS",
+        "subnet": "10.10.20.0/24",
+    }
+    base.update(overrides)  # type: ignore[typeddict-item]
+    return base
+
+
+class _FakeDhcpServer(DhcpServerMixin):
+    """In-memory fake — no network, no Kea/UCI specifics."""
+
+    def __init__(self, live: Optional[List[DhcpReservationDict]] = None) -> None:
+        self.live = live or []
+        self.applied: List[Dict[str, Any]] = []
+        self.commits = 0
+
+    def get_dhcp_reservations(self) -> List[DhcpReservationDict]:
+        return self.live
+
+    def apply_dhcp_reservation(
+        self, reservation: DhcpReservationDict, *, uuid: Optional[str] = None
+    ) -> Dict[str, Any]:
+        self.applied.append({"reservation": reservation, "uuid": uuid})
+        return {"success": True}
+
+    def commit_dhcp_reservations(self) -> Dict[str, Any]:
+        self.commits += 1
+        return {"success": True}
+
+
+class TestNormalizeMac:
+    def test_lowercases_and_colon_separates(self):
+        assert normalize_mac("AA-BB-CC-DD-EE-01") == "aa:bb:cc:dd:ee:01"
+
+    def test_accepts_bare_hex(self):
+        assert normalize_mac("aabbccddee01") == "aa:bb:cc:dd:ee:01"
+
+    def test_accepts_cisco_dotted(self):
+        assert normalize_mac("aabb.ccdd.ee01") == "aa:bb:cc:dd:ee:01"
+
+    def test_passes_through_unparseable_value_lowercased(self):
+        # Not 12 hex digits — return something stable rather than raising, so a
+        # malformed device response degrades to "never matches" instead of
+        # aborting the whole diff.
+        assert normalize_mac("not-a-mac") == "not-a-mac"
+
+    def test_handles_none(self):
+        assert normalize_mac(None) == ""
+
+
+class TestAbstractContract:
+    def test_get_dhcp_reservations_raises_not_implemented_by_default(self):
+        with pytest.raises(NotImplementedError):
+            DhcpServerMixin().get_dhcp_reservations()
+
+    def test_apply_dhcp_reservation_raises_not_implemented_by_default(self):
+        with pytest.raises(NotImplementedError):
+            DhcpServerMixin().apply_dhcp_reservation(_reservation())
+
+    def test_commit_dhcp_reservations_raises_not_implemented_by_default(self):
+        with pytest.raises(NotImplementedError):
+            DhcpServerMixin().commit_dhcp_reservations()
+
+
+class TestMixedIntoDriverTypes:
+    """Both firewalls and residential gateways run DHCP servers, so the mixin
+    must be reachable from either base class without re-declaring it."""
+
+    def test_firewall_driver_has_dhcp_reservation_methods(self):
+        assert issubclass(FirewallDriver, DhcpServerMixin)
+
+    def test_residential_gateway_driver_has_dhcp_reservation_methods(self):
+        assert issubclass(ResidentialGatewayDriver, DhcpServerMixin)
+
+
+class TestDiff:
+    def test_empty_device_yields_all_adds(self):
+        driver = _FakeDhcpServer([])
+        diff = driver.diff_dhcp_reservations(
+            [_reservation(), _reservation(mac="aa:bb:cc:dd:ee:02")]
+        )
+
+        assert len(diff["add"]) == 2
+        assert diff["update"] == []
+
+    def test_identical_reservation_produces_no_change(self):
+        live = _reservation(uuid="u1")
+        driver = _FakeDhcpServer([live])
+
+        diff = driver.diff_dhcp_reservations([_reservation()])
+
+        assert diff == {"add": [], "update": []}
+
+    def test_changed_ip_produces_update_with_changed_fields(self):
+        driver = _FakeDhcpServer([_reservation(uuid="u1")])
+
+        diff = driver.diff_dhcp_reservations([_reservation(ip="10.10.20.51")])
+
+        assert diff["add"] == []
+        assert len(diff["update"]) == 1
+        assert diff["update"][0]["uuid"] == "u1"
+        assert diff["update"][0]["changed_fields"] == ["ip"]
+
+    def test_changed_subnet_produces_update_not_add(self):
+        # A host moved to another VLAN keeps its MAC — that is an update of the
+        # existing reservation, not a second reservation for the same MAC.
+        driver = _FakeDhcpServer([_reservation(uuid="u1")])
+
+        diff = driver.diff_dhcp_reservations(
+            [_reservation(ip="10.30.20.50", subnet="10.30.20.0/24")]
+        )
+
+        assert diff["add"] == []
+        assert diff["update"][0]["changed_fields"] == ["ip", "subnet"]
+
+    def test_mac_formatting_differences_still_match(self):
+        driver = _FakeDhcpServer([_reservation(uuid="u1", mac="AA-BB-CC-DD-EE-01")])
+
+        diff = driver.diff_dhcp_reservations([_reservation(mac="aabb.ccdd.ee01")])
+
+        assert diff == {"add": [], "update": []}
+
+    def test_unmanaged_live_reservation_is_never_deleted(self):
+        # Hand-created reservations must survive — the desired set was never
+        # meant to describe them.
+        driver = _FakeDhcpServer([_reservation(uuid="u1", mac="aa:bb:cc:dd:ee:99")])
+
+        diff = driver.diff_dhcp_reservations([_reservation()])
+
+        assert len(diff["add"]) == 1
+        assert diff["update"] == []
+        assert "delete" not in diff
+
+
+class TestApplyReservationSet:
+    def test_applies_adds_and_updates_then_commits(self):
+        driver = _FakeDhcpServer([_reservation(uuid="u1", ip="10.10.20.9")])
+
+        lines = list(
+            driver.apply_dhcp_reservationset(
+                [_reservation(), _reservation(mac="aa:bb:cc:dd:ee:02", hostname="printer")]
+            )
+        )
+
+        assert driver.commits == 1
+        assert len(driver.applied) == 2
+        # The update targets the live uuid; the add does not.
+        by_uuid = {entry["uuid"] for entry in driver.applied}
+        assert by_uuid == {"u1", None}
+        assert any(line.startswith("[add]") for line in lines)
+        assert any(line.startswith("[update]") for line in lines)
+        assert lines[-1].startswith("[commit]")
+
+    def test_progress_lines_name_the_reservation(self):
+        driver = _FakeDhcpServer([])
+
+        lines = list(driver.apply_dhcp_reservationset([_reservation()]))
+
+        assert "aa:bb:cc:dd:ee:01" in lines[0]
+        assert "10.10.20.50" in lines[0]
+
+    def test_no_changes_skips_the_commit(self):
+        # Committing means reloading the DHCP daemon (Kea `service/reconfigure`),
+        # which drops in-flight requests. A no-op diff must not cause that.
+        driver = _FakeDhcpServer([_reservation(uuid="u1")])
+
+        lines = list(driver.apply_dhcp_reservationset([_reservation()]))
+
+        assert driver.commits == 0
+        assert driver.applied == []
+        assert lines == ["[commit] no changes"]