Skip to content

refactor(server): make route registration and declaration the same act - #7030

Open
otavio wants to merge 1 commit into
refactor/pure-handlersfrom
refactor/route-declarations
Open

refactor(server): make route registration and declaration the same act#7030
otavio wants to merge 1 commit into
refactor/pure-handlersfrom
refactor/route-declarations

Conversation

@otavio

@otavio otavio commented Sep 3, 2026

Copy link
Copy Markdown
Member

What

Registration and declaration become one act. A route is mounted through the gateway, so its
declaration is born knowing its method, its full path, the permission it requires and whether it
blocks API keys — and the route-table tests stop being hand-maintained ledgers and become
invariants over every route the router mounts.

Request handling is unchanged: the same guards run, in the same order, returning the same
responses.

Stacked on #6941. Base is refactor/pure-handlers, not master.

Closes #7029

Why

A route made five claims about itself across three places: the shape, the unbounded claim and the
anonymous claim went to the gateway wrapper; the permission and the API-key policy stayed behind as
an echo middleware tail; anonymity was stated a second time in the authenticator's allowlist.

Worse, a declaration never learnt its own address — the wrapper built it, something else mounted the
result, and nothing joined the two. Echo deliberately hides a route's handler, so that join cannot
be closed after the fact: the route-table tests recovered a handler's identity by reflecting on its
function pointer against a hand-written list of three routes. Those audits could not catch a route
mounted with no claim, a claim recorded for a route nobody mounted, or the same address mounted
twice — and the conversion of the remaining ~160 handlers was about to rely on them.

Mounting through the gateway is what closes the join, because the group-prefixed address is only
knowable at mount time, and it is the same string the router reports and the authenticator matches
on.

Changes

  • gateway: One, List, None and the legacy Handler adapter return a Route — the
    handler plus its pending declaration — instead of a bare echo.HandlerFunc. Route options moved
    from the shape call to the mount call, so one line per route states everything it claims.
  • gateway.MountOn(router, target): target is satisfied by both *echo.Echo and
    *echo.Group, so one helper covers the API group, the admin groups and the router root. The
    router is carried separately because a group does not name the router it was carved from, and it
    is the router a claim belongs to.
  • Guards move into the gateway: RequiresPermission and BlockAPIKey are installed by
    Requires(permission) and NoAPIKey(). Declaring and enforcing are the same act, so a declared
    permission is evidence rather than documentation. routesmiddleware keeps both names as thin
    wrappers so cloud's direct callers keep compiling. The route middleware package imports the
    gateway, so the guards had to move in this direction.
  • Only two guards became declarative. The tenant guard and the legacy authorize middleware stay
    an explicit tail, written with Guard(...). The latter looks close to vestigial, and deciding its
    fate is a behaviour question that must not ride inside a mechanical change.
  • The registry is per router, read back through Declarations(router). The process-global
    accessor is gone: once declarations carry an address, an edition-gated route registered by one
    test leaves a claim a later test would read as stale.
  • The whole route table moved at once, 94 registrations including the ones still on the legacy
    adapter. Moving only the converted routes would have kept the hand-maintained ledger alive through
    the entire conversion, which is the thing this change exists to remove.
  • Tests: convertedRoutes, wrapperExemptRoutes and the runtime.FuncForPC name matching are
    deleted. Six invariants replace them — every mounted route is declared or exempt with a reason,
    every declaration names a mounted route, no address is mounted twice, the anonymity claims and the
    allowlist agree over all routes, every exception states why, and the composed server's routes are
    named and are not this router's. Each predicate is a pure function with a companion test feeding
    it a known-bad input.

No handler bodies changed. No permission was added, removed or changed.

Testing

CI does not run here: the workflows are gated on branches: [master], so a PR based on a feature
branch gets zero checks. Verified locally, in-container:

  • go test ./api/... green. TestInstallScriptRendersAValidShellScript fails only in the
    shellhub-server-1 container, which does not mount the repo root the test reads install.sh
    from; it passes in a one-off container with the root mounted, on this branch and its parent.
  • golangci-lint run ./api/... — 0 issues. go mod tidy leaves the tree clean.
  • The three table invariants were confirmed to bite by breaking routes.go three ways: a route
    mounted outside the gateway, a dropped Anonymous claim, and a double mount. Each failed the
    expected subtest and only that one.
  • Registrations were diffed mechanically against the parent commit: 94 to 94, same handler at the
    same path, permission multiset identical, BlockAPIKey 24 to 24, Authorize/RequiresTenant
    12 to 12.

Guard ordering is the main risk. Options apply in written order and mount appends each guard as it
applies it, which is what preserves the old tail's order. TestGuardsRunInTheOrderTheyAreWritten
pins it directly, and TestGuardsRefuseWhatTheyRefusedBefore drives one route per guard through the
built router.

Worth a reviewer's attention: eleven routes gained an Anonymous(reason) claim. On a route still
using the legacy adapter this has no runtime effect — that adapter does not resolve an actor — so
these are the claims the allowlist is now held against, not a change in reachability.

cloud/ does not compile against this branch. The four shape constructors changed their return
type, so internal/billing/routes, internal/admin/routes and internal/cloud/routes fail to
build. Cloud adopts the same registration in its own stacked change on cloud#2509; until that lands,
a workspace resolving shellhub through replace ../shellhub will be red.

A route made five claims about itself across three places. The shape, the unbounded claim and the
anonymous claim went to the gateway wrapper; the permission and the API-key policy stayed behind as
an echo middleware tail; anonymity was stated a second time in the authenticator's allowlist. And a
declaration never learnt its own address, because the wrapper built it and something else mounted
the result, so the route-table tests recovered a handler's identity by reflecting on its function
pointer against a hand-written list of three.

Mounting through the gateway is what closes that join. Echo deliberately hides a route's handler,
so a declaration cannot be matched to its route after the fact; it can only be made where the route
is mounted, which is also the only place the group-prefixed address is knowable. The ledgers become
invariants over every route the router mounts.

Nothing about request handling changes. The same guards run in the same order, returning the same
responses.

Three things the code cannot carry:

Guards install in the order their options are written, because a RouteOption returns the middleware
it enforces and mount appends them as it applies them. That is what preserves the old tail's order,
where RequiresTenant ran before RequiresPermission and BlockAPIKey before both.

The shape's build takes the Declaration by value, so a serving handler closes over a copy. Mount
writes the address onto its own local after echo has already made the route servable, and a handler
that shared that memory would be read while it was being written.

The per-router table never evicts. Keying on router identity is what keeps an edition-gated route
registered by one test from reading as a stale claim on another's, and the cost is bounded by
routers created per process: one in production, a few dozen in a test binary.

Anonymous on a route still using the legacy adapter has no runtime effect, since that adapter does
not resolve an actor. It is there as the claim the allowlist is held against, and eleven routes
gained it to state what the allowlist already granted them.

The SSH, reverse-dial, web-terminal and pprof routes are mounted onto the same router by the server
that composes it, after NewRouter returns, so this seam cannot check them. They are named with
their reasons, and the one invariant available over them is asserted: none is this router's.

The cloud repo no longer compiles against this, because the four shape constructors changed their
return type. It adopts the same registration in its own stacked change.

Fixes: #7029
@otavio
otavio requested review from a team as code owners September 5, 2026 19:21
@otavio
otavio force-pushed the refactor/route-declarations branch from b93b0db to 4706390 Compare September 5, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(server): make route registration and declaration the same act

1 participant