fix: reap DNS record when a hostname is removed#284
Conversation
A hostname removed from a Gateway could leave its DNSRecordSet orphaned in PowerDNS. ensureHostnameVerification granted hostnames a grace period by reading them back from the downstream gateway's pre-reconcile listeners, so a hostname would survive if its Domain was deleted after verification. That check never confirmed the hostname still existed on the upstream gateway at all, so a hostname the user removed outright stayed "claimed" for one extra reconcile pass. Nothing forces that follow-up pass to happen, so the record could orphan indefinitely. Key changes: - Only apply the downstream grace period to hostnames still present in the upstream gateway's current listeners - Add a regression test driving two reconcile passes end-to-end (ensureHostnamesClaimed -> getDesiredDownstreamGateway -> ensureDNSRecordSets) that fails on the old code and passes on the fix Fixes #283
|
Rebased the #283 test suite (#285) onto this branch and reran — reporting the before/after. The suite is three tests covering the reap machinery: hostname-removal round-trip (apex ALIAS + non-apex CNAME), Gateway-deletion cleanup, and the HTTPProxy→child-Gateway owner-ref wiring the GC cascade relies on. They pass on That green-both-ways is the useful signal: these exercise the reap mechanisms, which were already individually correct — so they confirm the real #283 defect was upstream of them, in the hostname-claim grace period this PR fixes. Matches your commit message exactly. So the coverage splits cleanly: your regression test pins the specific bug (red on old, green here); this suite guards the surrounding plumbing (gateway-delete, proxy-delete cascade, apex/non-apex) against future regressions. Complementary, not redundant. #285 is stacked on this branch as a draft so it rides along into the fix rather than landing on its own — merge it in or lift the file into this PR, whichever you prefer. Still open, not covered by either: a real end-to-end/chainsaw test for the K8s GC cascade (my scenario 3 proves the pieces, not the live cascade). Worth a follow-up. |
garbageCollectDNSRecordSets and cleanupDNSRecordSets were only ever unit-tested in isolation; nothing drove the reconcile-level round trip that actually reaps a stale hostname's DNS record in production. Add three tests pinning the #283 behaviors: Key changes: - TestEnsureDNSRecordSets_HostnameRemovalReapsRecord: two ensureDNSRecordSets passes (hostname claimed, then removed) must GC the stale record while a still-claimed sibling survives; covers both CNAME (non-apex) and ALIAS (apex) record types - TestCleanupDNSRecordSets_OnGatewayDeletion: Gateway finalization must reap all of its own DNSRecordSets without touching another gateway's records - TestHTTPProxyDeletionCascadesDNSRecordCleanup: drives the real HTTPProxy reconcile to prove the Gateway owner-reference wiring native GC depends on, then simulates the GC cascade (the fake client runs no GC controller) and asserts cleanupDNSRecordSets reaps the child Gateway's records All three pass on origin/main today, which narrows the #283 root cause: the reap mechanisms themselves are correct, so the bug is upstream in how claimedHostnames is computed/retained across reconciles, not in garbage collection. This is a test-only commit for a draft PR; it merges into the #283 fix branch and is not meant to land standalone.
scotwells
left a comment
scotwells
left a comment
There was a problem hiding this comment.
Can we get end to end tests in place before we close the issue?
|
@scotwells yeah @ecv is working on them. |
3 participants
Summary
Fixes the reap gap described in #283. Removing a custom hostname from a Gateway could leave its DNSRecordSet orphaned in PowerDNS, because
ensureHostnameVerificationgranted a grace period to hostnames still present on the downstream gateway's pre-reconcile listeners, a mechanism meant to let a hostname survive its Domain being deleted after verification. That check never confirmed the hostname still existed on the upstream gateway at all, so a hostname removed outright stayed "claimed" for one extra reconcile pass. Nothing forces that follow-up pass to happen, so the stale record could persist indefinitely, matching the liveab.dk/www.ab.dkincident in datum-cloud/infra#3407.The fix restricts the downstream grace period to hostnames that are still present in the upstream gateway's current listeners, closing the gap while preserving the original deleted-Domain behavior.
Root cause
Confirmed with a fast in-process regression test rather than a full dev-cluster repro. Chaining
ensureHostnamesClaimed,getDesiredDownstreamGateway, andensureDNSRecordSetsacross two reconcile passes (hostname present, then removed) reproduces the orphan against the old code and passes against the fix.Changes
ensureHostnameVerificationnow only treats a downstream-listener hostname as still-verified when it's also present in the upstream gateway's currentspec.Listeners. AddedTestHostnameRemoval_ReapsRecordInSameReconcileingateway_dns_reap_regression_test.go, which fails against the old code and passes against the fix.go test ./...,go vet, andgofmtare all clean.Not in this PR
Per the plan in #283, gateway-delete and proxy-delete DNS cleanup currently have no test coverage, and there's no chainsaw e2e test touching DNS records. Left for a follow-up, or happy to add here if preferred.