amadan

carlos / platform Public

steering: fix review findings — case-fold Names, dedupe collisions, doc fixes

Task 4 review fix round.

FINDING 1 (Important): fqdn() now lowercases (strings.ToLower) as well
as trailing-dot-normalizes — DNS/Route53 are case-insensitive but
registry.Host is a case-sensitive primary key, so two rows differing
only by case ("App.example.com" / "app.example.com") would otherwise
mint two "different" Route53 names for one host. Adds dedupeRecords,
which collapses (Name, Box) collisions this reopens, keeping the first
occurrence and logging every drop loudly; its doc cites the two upstream
invariants (registry.Host PRIMARY KEY, one edges file per box by
filename) that make a wider duplicate guard unnecessary today.

FINDING 2 (Minor): added TestDesiredStateBlankAppsDomainLogged and a
blank-apps-domain subtest in TestDesiredState (per-host records present,
synthetic edge.<apps-domain> absent, log line present); desiredState's
doc now says the skip logs on every call (no throttling — the function
is pure and stateless, matching the file's no-dedup-window convention).

FINDING 3 (Minor): desiredRecord's doc no longer claims a HealthCheckRef
field that doesn't exist on the struct — replaced with a note that
Task 5's converger binds records to health checks by EIP.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev a8bba8d9208d3a401805b7d1e0d46a6e37cc16ad parent 8623687
2 files changed, +142 −13
  • internal/steering/steering.go +79 −13
  • internal/steering/steering_test.go +63 −0
diff --git a/internal/steering/steering.go b/internal/steering/steering.go
index 04dcffc39..664ae535e 100644
--- a/internal/steering/steering.go
+++ b/internal/steering/steering.go
@@ -27,12 +27,17 @@ import (
// A desiredRecord is one latency A record: name -> eip, keyed by the
// edge that serves it. SetIdentifier is the box label; Region is the
-// edge's AWS region; HealthCheckRef ties it to the edge's check.
+// edge's AWS region. Task 5's converger binds each record to a health
+// check by EIP (one check per distinct EIP, per the plan's Task 5
+// interfaces) — that binding is looked up there, not carried as a field
+// on this type.
//
-// Name is ALWAYS a trailing-dot FQDN (Route53's own wire shape,
-// "app.example.com."). desiredState normalizes it exactly once, here —
-// Task 5's converger must never re-normalize a Name, and must never treat
-// a bare (dot-less) Name as anything but a bug upstream of this fold.
+// Name is ALWAYS a trailing-dot, LOWERCASE FQDN (Route53's own wire
+// shape, "app.example.com.", and its own case-insensitivity — DNS names
+// are case-insensitive; Route53 itself lowercases on write). desiredState
+// normalizes it exactly once, here — Task 5's converger must never
+// re-normalize or re-case a Name, and must never treat a bare (dot-less
+// or mixed-case) Name as anything but a bug upstream of this fold.
type desiredRecord struct {
Name string // fqdn, trailing-dot normalized
EIP string
@@ -154,12 +159,67 @@ func steeredHosts(routes []registry.Route) []registry.Route {
return out
}
-// fqdn normalizes host to a trailing-dot FQDN, Route53's own wire shape —
-// exactly once, here, so every desiredRecord.Name downstream consumer
-// (Task 5) never has to reason about whether a Name is already
-// normalized.
+// fqdn normalizes host to a trailing-dot, LOWERCASE FQDN, Route53's own
+// wire shape — exactly once, here, so every desiredRecord.Name downstream
+// consumer (Task 5) never has to reason about whether a Name is already
+// normalized OR already case-folded.
+//
+// Lowercasing matters for the same reason Steering/Pool/wildcard-shape
+// already get a defensive, logged check in this file: registry.Host is a
+// case-SENSITIVE primary key (internal/registry/registry.go), but DNS —
+// and Route53's own record names — are not. Two registry rows differing
+// only by case ("App.example.com" and "app.example.com") are two
+// distinct, legitimate primary-key rows right up until this fold would
+// 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.
func fqdn(host string) string {
- return strings.TrimSuffix(host, ".") + "."
+ 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.
+//
+// This is deliberately narrow, not a general duplicate-input guard, and
+// two upstream invariants are why a wider one is not needed:
+//
+// - registry.Route.Host is the routes table's PRIMARY KEY
+// (internal/registry/registry.go's `routes` schema) — two rows with
+// BYTE-IDENTICAL Host values cannot coexist, so steeredHosts can
+// never hand desiredState two already-identical hosts.
+// - one edges file per box, at control/edges/<box>.json
+// (internal/host/edgesfile.go's edgesFileKey) — a box's OWN filename
+// 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.
+func dedupeRecords(records []desiredRecord) []desiredRecord {
+ if len(records) == 0 {
+ return records
+ }
+ type key struct{ name, box string }
+ seen := make(map[key]desiredRecord, len(records))
+ out := make([]desiredRecord, 0, len(records))
+ for _, rec := range records {
+ k := key{rec.Name, rec.Box}
+ if kept, ok := seen[k]; ok {
+ log.Printf("steering: record %s (box %s) dropped: case-folds to the same Route53 name as an already-kept record (EIP %s, kept EIP %s) — two registry rows differ only by case",
+ rec.Name, rec.Box, rec.EIP, kept.EIP)
+ continue
+ }
+ seen[k] = rec
+ out = append(out, rec)
+ }
+ return out
}
// desiredState folds steered routes × armed edges. The steering
@@ -170,8 +230,14 @@ func fqdn(host string) string {
//
// appsDomain empty with at least one armed edge is treated the same as
// any other missing input: no synthetic edge.<apps-domain> records are
-// minted (there is no domain to build the hostname from), logged once —
-// steered per-host records are unaffected either way.
+// minted (there is no domain to build the hostname from) — steered
+// per-host records are unaffected either way. The skip is logged on
+// EVERY call, not throttled: this function has no state to remember a
+// prior call's log across ticks (it is pure), so a box misconfigured
+// with no apps domain logs this once per housekeeper pass — the same
+// convention every other loud skip in this file follows (no dedup
+// window), and a deliberate choice: a duty this infrequent (Task 6's
+// 5min+jitter cadence) does not need throttling to stay readable.
func desiredState(routes []registry.Route, edges []edgesFileView, appsDomain string) []desiredRecord {
armed := armedEdges(edges)
if len(armed) == 0 {
@@ -196,5 +262,5 @@ func desiredState(routes []registry.Route, edges []edgesFileView, appsDomain str
}
}
- return out
+ return dedupeRecords(out)
}
diff --git a/internal/steering/steering_test.go b/internal/steering/steering_test.go
index 0ad59e7f4..bf3742028 100644
--- a/internal/steering/steering_test.go
+++ b/internal/steering/steering_test.go
@@ -148,6 +148,25 @@ func TestDesiredState(t *testing.T) {
{Name: "app.example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
},
},
+ {
+ name: "mixed-case host folds to one lowercase Name",
+ routes: []registry.Route{steeredRoute("App.Example.COM")},
+ edges: []edgesFileView{dublin},
+ appsDomain: "example.com",
+ want: []desiredRecord{
+ {Name: "edge.example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
+ {Name: "app.example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
+ },
+ },
+ {
+ name: "blank apps domain skips only the synthetic edge hostname",
+ routes: []registry.Route{steeredRoute("app.example.com")},
+ edges: []edgesFileView{dublin},
+ appsDomain: "",
+ want: []desiredRecord{
+ {Name: "app.example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
+ },
+ },
}
for _, tt := range tests {
@@ -211,6 +230,50 @@ func TestDesiredStateLogsEachSkipReason(t *testing.T) {
}
}
+// TestDesiredStateCaseFoldDedupe pins the case-collision half of the
+// 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.
+func TestDesiredStateCaseFoldDedupe(t *testing.T) {
+ dublin := edge("dublin", "1.2.3.4", "eu-west-1")
+ buf := captureLog(t)
+
+ routes := []registry.Route{
+ steeredRoute("App.example.com"),
+ steeredRoute("app.example.com"),
+ }
+ got := desiredState(routes, []edgesFileView{dublin}, "example.com")
+
+ var matches int
+ for _, rec := range got {
+ if rec.Name == "app.example.com." {
+ matches++
+ }
+ }
+ if matches != 1 {
+ t.Fatalf("desiredState() produced %d records for the case-colliding name, want exactly 1 (got %#v)", matches, got)
+ }
+ // Total should be the edge.<apps-domain> record plus the ONE surviving
+ // per-host record — not two.
+ 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())
+ }
+}
+
+func TestDesiredStateBlankAppsDomainLogged(t *testing.T) {
+ dublin := edge("dublin", "1.2.3.4", "eu-west-1")
+ buf := captureLog(t)
+ desiredState([]registry.Route{steeredRoute("app.example.com")}, []edgesFileView{dublin}, "")
+ want := "steering: no apps domain configured"
+ if !strings.Contains(buf.String(), want) {
+ t.Fatalf("log output =\n%s\nwant substring %q", buf.String(), want)
+ }
+}
+
func TestParseEdgesFileView(t *testing.T) {
tests := []struct {
name string