carlos / platform Public

steering: fold refuses an out-of-zone steered host instead of wedging the batch

steeredHosts checked Steering/wildcard/Pool but never that the host lay
under appsDomain. An honestly opted-in, out-of-zone host (ValidRecordHost
allows one — e.g. an addrs record on the shared pool) reached
applyChanges and Route53 rejects the whole batch ("not contained in
zone", or AccessDenied under a name-bound policy) — atomic batches meant
every steering record in that chunk failed every tick, forever.

steeredHosts now excludes a route whose host is not equal to, or a
subdomain of, appsDomain — loudly, naming the CNAME path that actually
reaches the shared pool. A blank appsDomain excludes every per-host
record the same way (no zone to prove a host under). Both share one log
family with the existing skip-and-log discipline in this file.

Documents at steeringHostname (route53.go) that this in-zone filter also
closes its own edge.*-prefix order-dependence, as a side effect of a
different fix.

Also swaps TrimSpace/TrimSuffix order on the apps-domain normalization:
the old order stripped a trailing dot before trailing whitespace, so
"example.com. " kept its dot. Unreachable via the one production caller
(which already whitespace-trims before calling in), fixed anyway.
Paul Campbell pushed by paul@keymail.dev 9ecf8be5d0d9c07e7e7be8b37fd074855f179641 parent e6a1633
3 files changed, +140 −12
  • internal/steering/route53.go +10 −0
  • internal/steering/steering.go +62 −11
  • internal/steering/steering_test.go +68 −1
diff --git a/internal/steering/route53.go b/internal/steering/route53.go
index ebff7d946..28fd8fcdf 100644
--- a/internal/steering/route53.go
+++ b/internal/steering/route53.go
@@ -931,6 +931,16 @@ func distinctEIPs(want []desiredRecord) []string {
// converger only ever sees one such name and needs no special case: whether
// it came from the synthetic mint or from a real route, it is the same record
// and the same SNI name.
+//
+// The prefix-only match here relies on steeredHosts' in-zone check
+// (steering.go): every per-host record in want already lies under
+// appsDomain, so an "edge."-prefixed Name reaching this function can only
+// ever be the one true edge.<apps-domain> hostname, never some other
+// zone's "edge.something-else.com" riding along by coincidence of prefix.
+// That in-zone filter is what makes this match order-independent; it was
+// added for a different reason (a steered host outside the zone wedges
+// Route53's whole batch) but closes this hostname's own would-be
+// ambiguity as a side effect.
func steeringHostname(want []desiredRecord) string {
for _, rec := range want {
if strings.HasPrefix(rec.Name, "edge.") {
diff --git a/internal/steering/steering.go b/internal/steering/steering.go
index b5d50aa46..2da352900 100644
--- a/internal/steering/steering.go
+++ b/internal/steering/steering.go
@@ -132,11 +132,29 @@ func armedEdges(edges []edgesFileView) []edgesFileView {
// refuse steering:"latency" on a pool-scoped route (Task 3); this
// fold refuses it AGAIN, because a hand-edited bucket record can
// still carry both — the door guards the write path, not the bucket.
+// - The host must lie under appsDomain — equal to it, or ending in
+// "."+appsDomain. ValidRecordHost (internal/provision) deliberately
+// allows an honestly opted-in, out-of-zone host through the door (an
+// addrs record, shared pool, e.g. a console's own hostname on a
+// domain the deployment does not hold the Route53 zone for) — that
+// check is about path-traversal safety, not about which zone a name
+// lives in, and it has no reason to know. A steered route on such a
+// host reaching Route53 is what jams the fold: ChangeResourceRecordSets
+// rejects a name "not contained in" the target zone (or the call fails
+// AccessDenied under a name-bound policy), and every record in that
+// batch — every OTHER steered host too — fails with it, EVERY tick,
+// forever, because desiredState hands converge one batch and Route53
+// applies a batch atomically. This check is the fold's own zone
+// boundary, independent of what the door already allows: a steered
+// host outside it is excluded, loudly, naming the CNAME path that
+// actually reaches the shared pool — custom domains steer via CNAME to
+// edge.<apps-domain>; a direct Route53 record only ever lives in the
+// steering zone itself.
//
// 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 {
+func steeredHosts(routes []registry.Route, appsDomain string) []registry.Route {
out := make([]registry.Route, 0, len(routes))
for _, r := range routes {
switch r.Steering {
@@ -156,12 +174,33 @@ func steeredHosts(routes []registry.Route) []registry.Route {
log.Printf("steering: route %s excluded: pool %q set (steering requires the shared pool)", r.Host, r.Pool)
continue
}
+ if appsDomain == "" {
+ log.Printf("steering: route %s excluded: no apps domain configured — only an in-zone host can receive a direct steering record; custom domains steer via CNAME to edge.<apps-domain>, and a direct Route53 record only ever lives in the steering zone itself", r.Host)
+ continue
+ }
+ if !inAppsZone(r.Host, appsDomain) {
+ log.Printf("steering: route %s excluded: host is not under the apps domain %q — custom domains steer via CNAME to edge.<apps-domain>; a direct Route53 record only ever lives in the steering zone itself (Route53 would reject the whole batch, wedging every other steered host too)", r.Host, appsDomain)
+ continue
+ }
out = append(out, r)
}
sort.Slice(out, func(i, j int) bool { return out[i].Host < out[j].Host })
return dedupeCaseCollisions(out)
}
+// inAppsZone reports whether host lies under appsDomain — equal to it, or a
+// subdomain of it — case-insensitively and trailing-dot-tolerant (registry
+// Host values and appsDomain both arrive un-normalized; fqdn's own
+// normalization runs after this check, not before, so this compares raw
+// input on both sides itself). appsDomain arrives already trimmed of
+// whitespace and a trailing dot (desiredState does that once, before any
+// call here).
+func inAppsZone(host, appsDomain string) bool {
+ h := strings.ToLower(strings.TrimSuffix(host, "."))
+ d := strings.ToLower(appsDomain)
+ return h == d || strings.HasSuffix(h, "."+d)
+}
+
// 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
@@ -277,14 +316,17 @@ func dedupeRecords(records []desiredRecord) []desiredRecord {
//
// 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) — 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.
+// minted (there is no domain to build the hostname from), AND every
+// per-host steered route is excluded too (steeredHosts' own appsDomain
+// check) — with no apps domain there is no zone to test a host against,
+// so no per-host name can be proven in-zone, and only an in-zone name may
+// ever be written (see steeredHosts). 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 {
@@ -292,8 +334,17 @@ func desiredState(routes []registry.Route, edges []edgesFileView, appsDomain str
}
var out []desiredRecord
+ // TrimSpace OUTSIDE TrimSuffix, deliberately: TrimSuffix only strips a
+ // dot that is the very last byte, so the other order (TrimSuffix then
+ // TrimSpace) would trim the dot before the space and leave
+ // "example.com. "'s dot behind after the space comes off. Unreachable
+ // today — the one production caller (internal/host/steering_loop.go)
+ // already does strings.TrimSpace(os.Getenv(...)) before handing
+ // appsDomain to ConvergeDeployment — but a wrong order here would
+ // misfire silently the day that stops being true.
+ domain := strings.TrimSuffix(strings.TrimSpace(appsDomain), ".")
- if domain := strings.TrimSpace(strings.TrimSuffix(appsDomain, ".")); domain == "" {
+ if domain == "" {
log.Printf("steering: no apps domain configured — the edge.<apps-domain> steering hostname is not minted this pass")
} else {
edgeHost := fqdn("edge." + domain)
@@ -302,7 +353,7 @@ func desiredState(routes []registry.Route, edges []edgesFileView, appsDomain str
}
}
- for _, r := range steeredHosts(routes) {
+ for _, r := range steeredHosts(routes, domain) {
name := fqdn(r.Host)
for _, e := range armed {
out = append(out, desiredRecord{Name: name, EIP: e.EIP, Box: e.Box, Region: e.Region})
diff --git a/internal/steering/steering_test.go b/internal/steering/steering_test.go
index d42b141d6..a1a1e4aeb 100644
--- a/internal/steering/steering_test.go
+++ b/internal/steering/steering_test.go
@@ -159,14 +159,44 @@ func TestDesiredState(t *testing.T) {
},
},
{
- name: "blank apps domain skips only the synthetic edge hostname",
+ name: "blank apps domain skips the synthetic edge hostname AND every per-host record",
routes: []registry.Route{steeredRoute("app.example.com")},
edges: []edgesFileView{dublin},
appsDomain: "",
+ want: nil,
+ },
+ {
+ name: "out-of-zone steered host excluded, in-zone host still lands",
+ routes: []registry.Route{
+ steeredRoute("app.example.com"),
+ steeredRoute("console.carloku.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: "steered host equal to the apps domain itself (apex) lands",
+ routes: []registry.Route{steeredRoute("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: "example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
+ },
+ },
+ {
+ name: "steered host merely sharing a suffix (not a subdomain) is excluded",
+ routes: []registry.Route{steeredRoute("notexample.com")},
+ edges: []edgesFileView{dublin},
+ appsDomain: "example.com",
+ want: []desiredRecord{
+ {Name: "edge.example.com.", EIP: "1.2.3.4", Box: "dublin", Region: "eu-west-1"},
+ },
+ },
}
for _, tt := range tests {
@@ -217,6 +247,12 @@ func TestDesiredStateLogsEachSkipReason(t *testing.T) {
routes: []registry.Route{{Host: "hand.example.com", Steering: "geo"}},
wantLog: `steering: route hand.example.com excluded: invalid steering value "geo"`,
},
+ {
+ name: "out-of-zone steered host is named, with the CNAME guidance",
+ edges: []edgesFileView{dublin},
+ routes: []registry.Route{steeredRoute("console.carloku.com")},
+ wantLog: `steering: route console.carloku.com excluded: host is not under the apps domain "example.com" — custom domains steer via CNAME to edge.<apps-domain>`,
+ },
}
for _, tt := range tests {
@@ -230,6 +266,37 @@ func TestDesiredStateLogsEachSkipReason(t *testing.T) {
}
}
+// TestDesiredStateBlankAppsDomainExcludesEveryPerHostRecord pins the second
+// half of the Important review finding: appsDomain == "" is not just "no
+// synthetic edge.<apps-domain> record" — with no domain there is no zone a
+// per-host route can be proven to lie under, so EVERY steered route is
+// excluded too (steeredHosts' own appsDomain == "" branch), loudly, one
+// line per route, same log family as the out-of-zone case.
+func TestDesiredStateBlankAppsDomainExcludesEveryPerHostRecord(t *testing.T) {
+ dublin := edge("dublin", "1.2.3.4", "eu-west-1")
+ buf := captureLog(t)
+
+ got := desiredState(
+ []registry.Route{steeredRoute("app.example.com"), steeredRoute("other.example.com")},
+ []edgesFileView{dublin},
+ "",
+ )
+ if got != nil {
+ t.Fatalf("desiredState() = %#v, want nil (no domain, no synthetic hostname, no per-host record)", got)
+ }
+
+ logged := buf.String()
+ for _, want := range []string{
+ `steering: route app.example.com excluded: no apps domain configured`,
+ `steering: route other.example.com excluded: no apps domain configured`,
+ "custom domains steer via CNAME to edge.<apps-domain>",
+ } {
+ if !strings.Contains(logged, want) {
+ t.Fatalf("log output =\n%s\nwant substring %q", logged, want)
+ }
+ }
+}
+
// 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