Scout stale-cluster ARecord cleanup scoped by DNS zone - #497
Conversation
Signed-off-by: Prabhjot Singh Bawa <prabhjot.bawa@rbccm.com> Signed-off-by: Prabhjot Singh Bawa <prabhjotbawa@gmail.com>
51fde36 to
eb66a52
Compare
Signed-off-by: Prabhjot Singh Bawa <prabhjotbawa@gmail.com>
eb66a52 to
fe214e4
Compare
|
Code QL error fixed, Security Scan Passed as well |
ebourgeois
left a comment
There was a problem hiding this comment.
Review: stale-cluster ARecord cleanup scoped by DNS zone
Thanks for this — the mechanical change is sound and consistently threaded. All four selector builders and all four delete_stale_cluster_*_arecords functions take current_zone, and all 12 call sites (4 create, 4 delete, 4 opt-out) resolve the zone from the right object's own annotations via resolve_zone. LABEL_ZONE is written by every build_*_arecord and predates the stale-cleanup feature, so there is no migration gap with unlabeled legacy records. CI is green.
Two things I'd like addressed before merge, plus one lower-severity item — all inline.
- The fix doesn't cover the same-zone case, but the new rustdoc and user docs claim it does. The zone clause only disambiguates clusters publishing into different zones. Several clusters publishing into one shared zone — the common multi-cluster-DNS topology — still mutually clobber. The docs assert this limits cleanup "to true renames of the same physical Scout instance," which isn't true there.
- Stale cleanup is now silently skipped on the delete and opt-out paths when no zone resolves, leaving permanently unreachable orphans. The
Nonearm returnsOk(()), the finalizer is removed, and the object is gone — so no future reconcile can re-drive the cleanup. Pre-PR these paths deleted the records. - Low: a user-controlled annotation now feeds a delete-path label selector unvalidated, which can turn a previously-infallible cleanup into a 5-minute deletion stall.
Notes, not blocking
delete_stale_cluster_arecords_deletes_only_returned_recordsmounts its GET mock without alabelSelectormatcher, so it asserts nothing about zone scoping. That matches its stated purpose, but it isn't a second line of defense for the new clause.reconcile_servicehas no stale-cluster cleanup at all (onlyservice_arecord_label_selector), so LoadBalancer Services are untouched by both the bug and the fix. Pre-existing and out of scope here — but relevant if #474 is to be closed as fully fixed.
Signed-off-by: Prabhjot Singh Bawa <prabhjot.bawa@rbccm.com>
|
For completeness, review comments incorporated as below: Fixed
|
Added a current_zone parameter to all four stale-selector builders (stale_arecord_label_selector, stale_httproute_arecord_label_selector, stale_tlsroute_arecord_label_selector,
stale_tcproute_arecord_label_selector) and their corresponding delete_stale_cluster_*_arecords functions, appending a zone=<current_zone> clause to the selector.
Zone is resolved per-call via resolve_zone() from the resource's annotations (falling back to ctx.default_zone), matching normal create-path zone resolution — including on the delete/opt-out cleanup paths, where
the zone is taken from the object being deleted.
Fixes: #474