amadan

carlos / platform Public

steering: fix round 2 — name both original hosts at the case-collision site

Round-2 review: F1's log-naming half wasn't addressed by round 1's
dedupeRecords, which only sees the already-folded Name plus Box/EIP —
identical on both sides for a same-edge collision, so the log couldn't
say which two registry rows to go fix.

Moves the case-collision check to where the pre-fold Host strings still
exist: steeredHosts now runs its sorted output through
dedupeCaseCollisions, which drops the second of any two routes whose
Host values differ only by case, keeping the first (sorted order,
deterministic) and logging both original Host values plus the folded
DNS name verbatim ("case-collision: %q and %q fold to the same DNS name
%q — keeping the first, fix the registry rows").

dedupeRecords stays as a structural backstop at the (Name, Box) level;
its doc now says plainly that dedupeCaseCollisions makes its own log
line unreachable for the case-fold path today, and why it is kept
anyway (fail loud, not silent, if some future producer bypasses
steeredHosts).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev ac2e728ee4b75c7dba14cb9a487db901d93040b7 parent a8bba8d
2 files changed, +87 −18
  • internal/steering/steering.go +61 −15
  • internal/steering/steering_test.go +26 −3
diff --git a/internal/steering/steering.go b/internal/steering/steering.go
index 664ae535e..7d09bf781 100644
--- a/internal/steering/steering.go
+++ b/internal/steering/steering.go
@@ -132,7 +132,9 @@ func armedEdges(edges []edgesFileView) []edgesFileView {
// fold refuses it AGAIN, because a hand-edited bucket record can
// still carry both — the door guards the write path, not the bucket.
//
-// The result is sorted by Host for the same determinism armedEdges keeps.
+// The result is sorted by Host, then run through dedupeCaseCollisions —
+// see that function's doc for why the case-fold collision check belongs
+// HERE and not later, once records exist.
func steeredHosts(routes []registry.Route) []registry.Route {
out := make([]registry.Route, 0, len(routes))
for _, r := range routes {
@@ -156,6 +158,41 @@ func steeredHosts(routes []registry.Route) []registry.Route {
out = append(out, r)
}
sort.Slice(out, func(i, j int) bool { return out[i].Host < out[j].Host })
+ return dedupeCaseCollisions(out)
+}
+
+// dedupeCaseCollisions drops the second of any two routes whose Host
+// values differ only by case — "App.example.com" and "app.example.com"
+// both fold to the same DNS name via fqdn (the lowercasing FINDING 1
+// added). This check runs HERE, inside steeredHosts, rather than later
+// once desiredRecords exist, because ONLY here do both original Host
+// strings — the two registry primary keys an operator actually has to go
+// fix — still exist to name in the log. By the time records are built and
+// crossed with edges, a same-edge collision has Box/EIP identical for
+// both rows too, so a log at that layer can only repeat the one already-
+// folded Name and a redundant EIP, forcing the operator to go query the
+// registry to find which two rows collided. Naming the collision at its
+// source is the whole fix; dedupeRecords below stays as a structural
+// backstop that this check makes unreachable for the case-fold path.
+//
+// The kept row is the first in Host's sort order (deterministic, matching
+// every other exclusion in this file) — which one wins is arbitrary; that
+// a decision is made, logged, and reproducible is what matters.
+func dedupeCaseCollisions(routes []registry.Route) []registry.Route {
+ if len(routes) == 0 {
+ return routes
+ }
+ seen := make(map[string]string, len(routes)) // lowercase host -> first Host kept, original case
+ out := make([]registry.Route, 0, len(routes))
+ for _, r := range routes {
+ lower := strings.ToLower(r.Host)
+ if kept, ok := seen[lower]; ok {
+ log.Printf("steering: case-collision: %q and %q fold to the same DNS name %q — keeping the first, fix the registry rows", kept, r.Host, fqdn(r.Host))
+ continue
+ }
+ seen[lower] = r.Host
+ out = append(out, r)
+ }
return out
}
@@ -173,20 +210,23 @@ func steeredHosts(routes []registry.Route) []registry.Route {
// otherwise hand Route53 two "different" names for what every resolver
// on earth treats as the same host. Folding here, not refusing, is the
// right answer — case is an accident of how a hostname was typed, not a
-// meaningful distinction — but it does reopen a collision dedupeRecords
-// then has to close.
+// meaningful distinction — but it does reopen a collision:
+// dedupeCaseCollisions (steeredHosts) closes it while both original Host
+// strings are still around to name in the log; dedupeRecords is the
+// backstop below that.
func fqdn(host string) string {
return strings.ToLower(strings.TrimSuffix(host, ".")) + "."
}
-// dedupeRecords collapses records whose (Name, Box) pair collides after
-// fqdn's case-fold, keeping the first occurrence (deterministic: both
-// steeredHosts and armedEdges hand this function pre-sorted input) and
-// logging every collapse loudly — the same discipline every other
-// exclusion in this file follows: never guess, never go silent.
+// dedupeRecords collapses records whose (Name, Box) pair collides,
+// keeping the first occurrence (deterministic: both steeredHosts and
+// armedEdges hand desiredState pre-sorted input) and logging every
+// collapse — the same discipline every other exclusion in this file
+// follows: never guess, never go silent.
//
-// This is deliberately narrow, not a general duplicate-input guard, and
-// two upstream invariants are why a wider one is not needed:
+// This is a STRUCTURAL BACKSTOP, not the primary defense, and is
+// deliberately narrow rather than a general duplicate-input guard. Three
+// invariants are why:
//
// - registry.Route.Host is the routes table's PRIMARY KEY
// (internal/registry/registry.go's `routes` schema) — two rows with
@@ -197,11 +237,17 @@ func fqdn(host string) string {
// is its identity in the listing this reads, so two edgesFileView
// entries with byte-identical Box values would mean the same stored
// object parsed twice, not two boxes racing to claim one label.
-//
-// Only a CASE-DIFFERING pair — "App.example.com" and "app.example.com",
-// two distinct primary-key rows fqdn now folds onto the same Route53
-// name — can reach this path, which is exactly the collision fqdn's own
-// doc comment says it reopens.
+// - the CASE-DIFFERING pair that fqdn's lowercasing (FINDING 1) could
+// otherwise fold together — "App.example.com" and
+// "app.example.com" — is now caught EARLIER, by
+// dedupeCaseCollisions inside steeredHosts, while both original Host
+// strings still exist to name in the log. This function's own log
+// line is therefore UNREACHABLE for that path today (it fires only
+// if some future caller builds desiredRecords by a route other than
+// steeredHosts — e.g. a second producer added without running
+// through the same dedupe); it stays as the backstop that fails loud
+// rather than silent if that ever happens, not as live code any
+// current test exercises via a case collision.
func dedupeRecords(records []desiredRecord) []desiredRecord {
if len(records) == 0 {
return records
diff --git a/internal/steering/steering_test.go b/internal/steering/steering_test.go
index bf3742028..d42b141d6 100644
--- a/internal/steering/steering_test.go
+++ b/internal/steering/steering_test.go
@@ -234,7 +234,14 @@ func TestDesiredStateLogsEachSkipReason(t *testing.T) {
// review's Important finding: two registry rows differing only by case
// are two legitimate primary-key rows right up until fqdn folds them onto
// one Route53 name, at which point desiredState must produce exactly ONE
-// record per armed edge for that name, not two, and must say so loudly.
+// record per armed edge for that name, not two — and the log must name
+// BOTH original (still mixed-case) Host values, not the already-folded
+// Name, because that is the only information an operator can act on:
+// which two registry rows to go fix. That naming has to happen where the
+// originals still exist — steeredHosts, via dedupeCaseCollisions — not
+// later at the desiredRecord level, where a same-edge collision has
+// identical Box/EIP on both sides and the log would just repeat the one
+// folded Name.
func TestDesiredStateCaseFoldDedupe(t *testing.T) {
dublin := edge("dublin", "1.2.3.4", "eu-west-1")
buf := captureLog(t)
@@ -259,8 +266,24 @@ func TestDesiredStateCaseFoldDedupe(t *testing.T) {
if len(got) != 2 {
t.Fatalf("desiredState() = %#v, want 2 records (1 synthetic + 1 deduped)", got)
}
- if !strings.Contains(buf.String(), "steering: record app.example.com. (box dublin) dropped") {
- t.Fatalf("log output =\n%s\nwant a dropped-record line naming the collision", buf.String())
+
+ logged := buf.String()
+ // Sorted-Host order (ASCII, uppercase < lowercase) keeps
+ // "App.example.com" and drops "app.example.com" — both original
+ // strings, plus the folded DNS name, must appear verbatim.
+ for _, want := range []string{`"App.example.com"`, `"app.example.com"`, `"app.example.com."`} {
+ if !strings.Contains(logged, want) {
+ t.Fatalf("log output =\n%s\nwant substring %s naming the original colliding host/DNS name", logged, want)
+ }
+ }
+ if !strings.Contains(logged, "steering: case-collision:") {
+ t.Fatalf("log output =\n%s\nwant a case-collision line", logged)
+ }
+ // The record-level backstop (dedupeRecords) must NOT be the one that
+ // fired — the collision was already resolved at the route level, one
+ // layer earlier, where the log could still name both original hosts.
+ if strings.Contains(logged, "steering: record ") {
+ t.Fatalf("log output =\n%s\nwant no dedupeRecords line — the collision should be caught earlier, in steeredHosts", logged)
}
}