fix(dns): report every host override so duplicates can be cleared

The inventory collapsed rows that shared a name, domain, address and type,
which was meant to hide the alias records searchhostoverride lists alongside
their parent. It hid genuine duplicates too — and since sync_dns_zone deletes
what the inventory tells it about, it could only ever remove one copy per name
before adding a fresh one. A pile of identical overrides could grow but never
shrink.

An office firewall reached fifteen identical A records for its own name, all
tagged [netork], while netOrk's database showed one.

OPNsense flags alias rows with isAlias, so filter on that and report every
remaining row under its own UUID. Releases that predate the flag give no way to
tell an alias from a copy, so the content collapse stays as a fallback there.
sync_dns_zone now also pushes one override per logical record, because two
netOrk rows for one host is a state its database can legitimately be in.

The auto-PTR flag is read from addptr, which is what OPNsense 26.1 returns;
ptrrecord was absent from every row, so the default made every A record claim
it managed a PTR — and netOrk derives reverse-zone entries from exactly that
flag. addptr now goes out on writes alongside the legacy name.

Refs christianmanivong/netork#103, christianmanivong/netork#107

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.
+                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": "",

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