amadan
console: make the path-route listing's tests able to fail, and its answer consistent
TestAPIRoutesListsPathRoutes named a tenancy rule it did not test. Both of its "other tenant" rows sat on hostnames that are not in routeFor at all, so the host filter excluded them whatever their stamps said — delete the account/app compare and the test stayed green. Two rows are added on connect.go.tito.io, a host that IS in the answer, stamped to another app and another account: only the tenancy compare keeps those out. routes_cli_test.go asserted that "no routes for titogo" was absent from a listing that prints two routes, which cannot fail. Replaced with a fixture where --match filters every route away while path rows exist, so the real property — a path row is a pointer, not a route, and must not inflate the count of what was shown — is the one under test. Two consistency fixes in apiroutes.go itself. `paths` was a nil slice and serialised as null while `routes` beside it is always [], and every deployment hits that branch until someone publishes a map; a client handling two spellings of "none" for two keys in one object will get one of them wrong. And the path filter compared p.Account raw while the routes filter twelve lines above uses release.RouteAccount and carries a comment warning that a second private notion of route ownership is how a listing ends up disagreeing with the reconciler. It failed closed, but "fails closed" is not the rule the file states. Fingerprint's doc-comment summary line still enumerated the tables it covers without route_paths.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+81
−8
internal/console/apiroutes.go+15 −2internal/console/apiroutes_test.go+31 −0internal/registry/registry.go+3 −2routes_cli_test.go+32 −4
diff --git a/internal/console/apiroutes.go b/internal/console/apiroutes.go| index 042c9b49b..4123bee93 100644 |
| --- a/internal/console/apiroutes.go |
| +++ b/internal/console/apiroutes.go |
| @@ -159,7 +159,11 @@ func (s *Server) handleAPIRoutes(w http.ResponseWriter, r *http.Request) { |
| // route_paths table cannot be read should still say what routes exist. |
| // Same tenant boundary as the rows above, read off the row's own stamped |
| // columns — ListPaths returns every path route on this box. |
| - var paths []apiRoutePath |
| + // make, not a nil slice: an empty answer must serialise as [] like |
| + // `routes` beside it does, never as null. A client that has to handle |
| + // both spellings of "none" for two keys in one object is a client that |
| + // will handle one of them wrong. |
| + paths := make([]apiRoutePath, 0) |
| if all, err := reg.ListPaths(); err != nil { |
| log.Printf("console: api routes: registry list paths: %v", err) |
| } else { |
| @@ -169,7 +173,16 @@ func (s *Server) handleAPIRoutes(w http.ResponseWriter, r *http.Request) { |
| // derives a row when the claim's host is a local route of the |
| // same app and account, so in practice every row passes — but a |
| // listing must not depend on another package's invariant. |
| - if p.App != app || p.Account != acct.Sqid { |
| + // release.RouteAccount, matching the routes filter above rather |
| + // than comparing the column raw. The warning there applies here |
| + // word for word: a second, private notion of "whose row is this" |
| + // is exactly how a listing ends up disagreeing with the |
| + // reconciler about ownership. A raw compare fails CLOSED today |
| + // (an unstamped account never equals a sqid, so the row is just |
| + // hidden), but "fails closed" is not the rule this file states, |
| + // and the two filters must resolve ownership identically or a |
| + // route can appear with its own prefixes missing. |
| + if p.App != app || release.RouteAccount(registry.Route{Account: p.Account}) != acct.Sqid { |
| continue |
| } |
| if _, shown := routeFor[p.Host]; !shown { |
diff --git a/internal/console/apiroutes_test.go b/internal/console/apiroutes_test.go| index 785ebab1f..e2a9a6a33 100644 |
| --- a/internal/console/apiroutes_test.go |
| +++ b/internal/console/apiroutes_test.go |
| @@ -9,6 +9,7 @@ package console |
| import ( |
| "net/http" |
| + "strings" |
| "testing" |
| "time" |
| @@ -208,6 +209,21 @@ func TestAPIRoutesAppTokenIsRefused(t *testing.T) { |
| } |
| } |
| +// TestAPIRoutesPathsIsAnEmptyArrayNotNull: no path rows must serialise as [], |
| +// the way `routes` beside it always does. A key that is sometimes null and |
| +// sometimes an array is a key a client eventually handles wrong, and every |
| +// deployment on the platform hits this branch until someone publishes a map. |
| +func TestAPIRoutesPathsIsAnEmptyArrayNotNull(t *testing.T) { |
| + fx, token := routesFixture(t, "code-routes-nopaths") |
| + status, body := getRaw(t, routesURL(fx, "titogo"), token) |
| + if status != http.StatusOK { |
| + t.Fatalf("GET routes = %d %s", status, body) |
| + } |
| + if !strings.Contains(body, `"paths":[]`) { |
| + t.Fatalf("routes body = %s, want an empty paths ARRAY, not null", body) |
| + } |
| +} |
| + |
| // TestAPIRoutesListsPathRoutes: a hostname whose traffic is split by leading |
| // path segment reports those prefixes, so an operator can read a migration's |
| // flipped set from the command they already use. Same tenant boundary as the |
| @@ -226,6 +242,15 @@ func TestAPIRoutesListsPathRoutes(t *testing.T) { |
| // carry neither, exactly as it carries neither of their routes. |
| {Host: "neighbour.example.com", Prefix: "paulca", Target: "connect.go.tito.io", Account: "bab", App: "kass"}, |
| {Host: "foreign.example.com", Prefix: "paulca", Target: "foreign.example.com", Account: "zzzz", App: "titogo"}, |
| + // The rows that actually exercise the TENANCY compare. The two above |
| + // sit on hostnames that are not in routeFor at all, so the host |
| + // filter excludes them whatever their stamps say — delete the |
| + // account/app compare and the test still passes. These two sit on |
| + // connect.go.tito.io, which IS in the answer, and are stamped to |
| + // another app and another account: only the tenancy compare keeps |
| + // them out. |
| + {Host: "connect.go.tito.io", Prefix: "otherapp", Target: "router-a.go.tito.io", Account: "bab", App: "kass"}, |
| + {Host: "connect.go.tito.io", Prefix: "otheracct", Target: "router-a.go.tito.io", Account: "zzzz", App: "titogo"}, |
| } { |
| if err := reg.UpsertPath(p); err != nil { |
| t.Fatal(err) |
| @@ -242,6 +267,12 @@ func TestAPIRoutesListsPathRoutes(t *testing.T) { |
| if len(rows) != 2 { |
| t.Fatalf("paths = %v, want exactly titogo's two rows in this account", out["paths"]) |
| } |
| + for _, r := range rows { |
| + m, _ := r.(map[string]any) |
| + if p := str(t, m, "prefix"); p == "otherapp" || p == "otheracct" { |
| + t.Fatalf("paths carries %q — a row on a hostname in the answer but stamped to another app or account is another tenant's routing table", p) |
| + } |
| + } |
| // ListPaths orders by host then prefix, and the CLI prints them in the |
| // order they arrive — so the order is part of the answer. |
| first, _ := rows[0].(map[string]any) |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index fb026b591..07baf89ce 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -1675,8 +1675,9 @@ func (r *Registry) DeletePath(host, prefix string) error { |
| return err |
| } |
| -// Fingerprint summarizes the routes, fleets, fleet_boxes, route_aliases and |
| -// wildcard_domains tables over a fresh connection, for the edge's cheap |
| +// Fingerprint summarizes the routes, fleets, fleet_boxes, route_aliases, |
| +// wildcard_domains and route_paths tables over a fresh connection, for the |
| +// edge's cheap |
| // change poll. It is content-accurate: it hashes the actual VALUES of the |
| // reload-relevant columns, not a length-weighted aggregate, so two |
| // DIFFERENT values of the SAME length |
diff --git a/routes_cli_test.go b/routes_cli_test.go| index fcd6504b2..c4010c829 100644 |
| --- a/routes_cli_test.go |
| +++ b/routes_cli_test.go |
| @@ -207,10 +207,38 @@ func TestRoutesShowsPathRoutesUnderTheirHost(t *testing.T) { |
| if !strings.Contains(lines[prefixAt["paulca"]], "\u2014") { |
| t.Fatalf("path row = %q, want em dashes in the columns a pointer has none of", lines[prefixAt["paulca"]]) |
| } |
| - // And they are not routes: a host with only path rows under it must not |
| - // make the "no routes" line disappear, nor inflate the count of what is. |
| - if strings.Contains(out.String(), "no routes for titogo") { |
| - t.Fatalf("routes output claims there are none:\n%s", out.String()) |
| +} |
| + |
| +// TestRoutesPathRowsAreNotCountedAsRoutes: a path row is a pointer at a |
| +// route, not a route. It is printed under its host, so when --match filters |
| +// every host away nothing is printed at all — and the "no routes" line must |
| +// still appear, because path rows must not inflate the count of what was |
| +// shown. The obvious spelling of this check (assert the line is ABSENT from |
| +// a listing that prints two routes) cannot fail, which is why it is written |
| +// the other way round. |
| +func TestRoutesPathRowsAreNotCountedAsRoutes(t *testing.T) { |
| + useTempCredentials(t) |
| + origin := routesConsolePaths(t, |
| + []map[string]any{ |
| + {"host": "connect.go.tito.io", "kind": "instance", "upstream": "127.0.0.1:9001", |
| + "channel": "stable", "status": "ready", "health": "Running"}, |
| + }, |
| + []map[string]any{ |
| + {"host": "connect.go.tito.io", "prefix": "paulca", "target": "router-a.go.tito.io"}, |
| + }) |
| + var out bytes.Buffer |
| + old := stdout |
| + stdout = &out |
| + t.Cleanup(func() { stdout = old }) |
| + |
| + runRoutes([]string{"-app", "titogo", "-account-id", "bae", "-console", origin, |
| + "-match", "nothing-matches-this"}) |
| + |
| + if !strings.Contains(out.String(), "no routes for titogo") { |
| + t.Fatalf("every route was filtered away, so the listing must say so:\n%s", out.String()) |
| + } |
| + if strings.Contains(out.String(), "/paulca") { |
| + t.Fatalf("a path row survived a filter its host did not:\n%s", out.String()) |
| } |
| } |