refactor(server): make route registration and declaration the same act - #7030
Open
otavio wants to merge 1 commit into
Open
refactor(server): make route registration and declaration the same act#7030otavio wants to merge 1 commit into
otavio wants to merge 1 commit into
Conversation
otavio
force-pushed
the
refactor/route-declarations
branch
from
September 3, 2026 19:40
82155be to
ed644df
Compare
This was referenced Sep 3, 2026
otavio
force-pushed
the
refactor/route-declarations
branch
from
September 3, 2026 21:02
ed644df to
b93b0db
Compare
This was referenced Sep 3, 2026
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
force-pushed
the
refactor/route-declarations
branch
from
September 5, 2026 19:21
b93b0db to
4706390
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, notmaster.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,Noneand the legacyHandleradapter return aRoute— thehandler plus its pending declaration — instead of a bare
echo.HandlerFunc. Route options movedfrom the shape call to the mount call, so one line per route states everything it claims.
gateway.MountOn(router, target):targetis satisfied by both*echo.Echoand*echo.Group, so one helper covers the API group, the admin groups and the router root. Therouter is carried separately because a group does not name the router it was carved from, and it
is the router a claim belongs to.
RequiresPermissionandBlockAPIKeyare installed byRequires(permission)andNoAPIKey(). Declaring and enforcing are the same act, so a declaredpermission is evidence rather than documentation.
routesmiddlewarekeeps both names as thinwrappers so cloud's direct callers keep compiling. The route middleware package imports the
gateway, so the guards had to move in this direction.
an explicit tail, written with
Guard(...). The latter looks close to vestigial, and deciding itsfate is a behaviour question that must not ride inside a mechanical change.
Declarations(router). The process-globalaccessor 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.
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.
convertedRoutes,wrapperExemptRoutesand theruntime.FuncForPCname matching aredeleted. 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 featurebranch gets zero checks. Verified locally, in-container:
go test ./api/...green.TestInstallScriptRendersAValidShellScriptfails only in theshellhub-server-1container, which does not mount the repo root the test readsinstall.shfrom; 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 tidyleaves the tree clean.routes.gothree ways: a routemounted outside the gateway, a dropped
Anonymousclaim, and a double mount. Each failed theexpected subtest and only that one.
same path, permission multiset identical,
BlockAPIKey24 to 24,Authorize/RequiresTenant12 to 12.
Guard ordering is the main risk. Options apply in written order and
mountappends each guard as itapplies it, which is what preserves the old tail's order.
TestGuardsRunInTheOrderTheyAreWrittenpins it directly, and
TestGuardsRefuseWhatTheyRefusedBeforedrives one route per guard through thebuilt router.
Worth a reviewer's attention: eleven routes gained an
Anonymous(reason)claim. On a route stillusing 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 returntype, so
internal/billing/routes,internal/admin/routesandinternal/cloud/routesfail tobuild. Cloud adopts the same registration in its own stacked change on cloud#2509; until that lands,
a workspace resolving shellhub through
replace ../shellhubwill be red.