diff --git a/napalm_opnsense/opnsense.py b/napalm_opnsense/opnsense.py index 1a7c6a6..bafcd7f 100644 --- a/napalm_opnsense/opnsense.py +++ b/napalm_opnsense/opnsense.py @@ -2662,19 +2662,32 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): "/api/unbound/settings/searchhostoverride", {"current": 1, "rowCount": -1, "searchPhrase": ""}, ) - for row in data.get("rows", []): + rows = data.get("rows", []) or [] + # searchhostoverride lists alias records alongside the host override + # they hang off, each under its own UUID. OPNsense flags them, so + # skip them by that flag and report every remaining row as it is — + # including two rows that happen to hold the same name and address. + # Those are duplicates on the device, and sync_dns_zone can only + # clear the copies it is told about. + flagged = any("isAlias" in row for row in rows) + for row in rows: + if flagged and row.get("isAlias"): + continue host = row.get("hostname", "") or "" domain = row.get("domain", "") or "" fqdn = f"{host}.{domain}" if host and domain else host or domain ip = row.get("server", "") or "" rr = row.get("rr", "A") or "A" - # OPNsense searchhostoverride includes alias records alongside parent - # records; aliases often have identical content but separate UUIDs. - # Deduplicate by logical key to avoid inflating the zone with copies. - key = (host.lower(), domain.lower(), ip, rr) - if key in seen: - continue - seen.add(key) + if not flagged: + # Releases without the flag give nothing to tell an alias + # apart from a copy, so collapsing by content stays the + # safer read there. + key = (host.lower(), domain.lower(), ip, rr) + if key in seen: + continue + seen.add(key) + # The auto-PTR flag is "addptr" since 25.x, "ptrrecord" before. + ptr = row.get("addptr", row.get("ptrrecord", "1")) result.append({ "uuid": row.get("uuid", ""), "hostname": host, @@ -2684,7 +2697,7 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): "record_type": rr, "description": row.get("description", "") or "", "enabled": str(row.get("enabled", "1")) == "1", - "ptrrecord": str(row.get("ptrrecord", "1")) == "1", + "ptrrecord": str(ptr) == "1", "service": "unbound", }) except Exception as exc: @@ -2790,14 +2803,21 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): logger.info("Removed %s host override %s.%s (uuid %s)", service, entry["hostname"], zone_clean, uid) - # Re-add all enabled A/AAAA records + # Re-add all enabled A/AAAA records, one override per logical record. + # Two rows for the same name is a state netOrk's own database can + # legitimately be in; the device must still end up with one. added = 0 + pushed: set[tuple[str, str, str]] = set() for rec in records: if not rec.get("enabled", True): continue rtype = rec.get("record_type", "A") if rtype not in ("A", "AAAA"): continue + key = (str(rec.get("hostname", "")).lower(), rtype, str(rec.get("ip", ""))) + if key in pushed: + continue + pushed.add(key) if service == "unbound": self._post("/api/unbound/settings/addhostoverride", { "host": { @@ -2807,6 +2827,7 @@ class OPNsenseDriver(OPNsensePingMixin, FirewallDriver): "rr": rtype, "server": rec.get("ip", ""), "description": self._NETORK_TAG, + "addptr": "1", "ptrrecord": "1", "mxprio": "", "mx": "", diff --git a/tests/unit/test_driver.py b/tests/unit/test_driver.py index def9e88..3098c6c 100644 --- a/tests/unit/test_driver.py +++ b/tests/unit/test_driver.py @@ -2419,3 +2419,128 @@ class TestKeaSubnetRecordKey: def test_unknown_shape_yields_an_empty_record(self, driver): assert driver._kea_subnet_record({"something_else": {}}) == {} + + +# --------------------------------------------------------------------------- +# DNS host overrides +# --------------------------------------------------------------------------- + +def _override_row(uuid, host="opnsense-office", domain="office.local", + ip="10.10.0.1", description="[netork]", **extra): + row = { + "uuid": uuid, + "hostname": host, + "domain": domain, + "server": ip, + "rr": "A", + "description": description, + "enabled": "1", + "addptr": "1", + "isAlias": False, + } + row.update(extra) + return row + + +class TestUnboundHostOverrideInventory: + """The inventory is what `sync_dns_zone` deletes from, so it has to report + the device as it is. Collapsing rows that merely look alike hides genuine + duplicates and — worse — puts them out of reach of the cleanup pass.""" + + def test_reports_every_duplicate_under_its_own_uuid(self, driver): + driver._detect_active_dns = lambda: "unbound" + driver._post = lambda path, data=None: { + "rows": [_override_row("uuid-1"), _override_row("uuid-2"), _override_row("uuid-3")] + } + + entries = driver._get_unbound_host_overrides() + + assert [e["uuid"] for e in entries] == ["uuid-1", "uuid-2", "uuid-3"] + + def test_drops_alias_rows(self, driver): + driver._post = lambda path, data=None: { + "rows": [ + _override_row("parent"), + _override_row("alias", host="www", isAlias=True), + ] + } + + entries = driver._get_unbound_host_overrides() + + assert [e["uuid"] for e in entries] == ["parent"] + + def test_falls_back_to_logical_dedup_when_the_flag_is_absent(self, driver): + """Releases that predate `isAlias` give no way to tell an alias from a + copy, so the old collapse stays the safer read there.""" + rows = [_override_row("uuid-1"), _override_row("uuid-2")] + for row in rows: + del row["isAlias"] + driver._post = lambda path, data=None: {"rows": rows} + + entries = driver._get_unbound_host_overrides() + + assert [e["uuid"] for e in entries] == ["uuid-1"] + + def test_reads_the_auto_ptr_flag_from_addptr(self, driver): + driver._post = lambda path, data=None: {"rows": [_override_row("u", addptr="0")]} + + assert driver._get_unbound_host_overrides()[0]["ptrrecord"] is False + + def test_falls_back_to_the_legacy_ptrrecord_field(self, driver): + row = _override_row("u", ptrrecord="0") + del row["addptr"] + driver._post = lambda path, data=None: {"rows": [row]} + + assert driver._get_unbound_host_overrides()[0]["ptrrecord"] is False + + +class TestSyncDnsZone: + def _record_calls(self, driver, rows): + calls = [] + + def _post(path, data=None): + calls.append((path, data)) + if path.endswith("searchhostoverride"): + return {"rows": rows} + return {"result": "saved"} + + driver._detect_active_dns = lambda: "unbound" + driver._post = _post + return calls + + def test_deletes_every_managed_copy_not_just_one(self, driver): + rows = [_override_row("uuid-1"), _override_row("uuid-2"), _override_row("uuid-3")] + calls = self._record_calls(driver, rows) + + driver.sync_dns_zone("office.local", [ + {"hostname": "opnsense-office", "ip": "10.10.0.1", "record_type": "A", "enabled": True} + ]) + + deleted = [p for p, _ in calls if "delhostoverride" in p] + assert sorted(p.rsplit("/", 1)[-1] for p in deleted) == ["uuid-1", "uuid-2", "uuid-3"] + assert len([p for p, _ in calls if p.endswith("addhostoverride")]) == 1 + + def test_leaves_records_it_does_not_manage_alone(self, driver): + rows = [ + _override_row("managed"), + _override_row("by-hand", host="nas", description="added by hand"), + ] + calls = self._record_calls(driver, rows) + + driver.sync_dns_zone("office.local", []) + + deleted = [p.rsplit("/", 1)[-1] for p, _ in calls if "delhostoverride" in p] + assert deleted == ["managed"] + + def test_pushes_one_row_per_logical_record(self, driver): + """Two netOrk rows for one host is a state the DB can legitimately be + in; the device still ends up with a single override.""" + calls = self._record_calls(driver, []) + + driver.sync_dns_zone("office.local", [ + {"hostname": "opnsense-office", "ip": "10.10.0.1", "record_type": "A", "enabled": True}, + {"hostname": "opnsense-office", "ip": "10.10.0.1", "record_type": "A", "enabled": True}, + ]) + + added = [d for p, d in calls if p.endswith("addhostoverride")] + assert len(added) == 1