feat(core): make controller startup importable and its auth replaceable - #2658
Open
jjamroga wants to merge 4 commits into
Open
feat(core): make controller startup importable and its auth replaceable#2658jjamroga wants to merge 4 commits into
jjamroga wants to merge 4 commits into
Conversation
The controller can only be started by this repository's own main package. Every wiring decision lives in func main() in package main, which nothing can import, and the authenticator and authorizer are constructed inline and cannot be substituted. Move the startup sequence into go/core/pkg/app behind Run(ctx, Options), and reduce cmd/controller-v2 to a signal handler and a call. Each Options field is optional and its zero value selects exactly what the controller does today, so this adds a seam without changing the shipped binary: Authenticator, Authorizer replace the unsecure and noop defaults SetupWithManager register controllers and scheme types HTTPMiddleware wrap the handler ahead of core's own auth Routes add endpoints alongside core's ExtraMigrations apply further tracks after the built-ins Three orderings are load-bearing and are commented where they are relied on. SetupWithManager runs after the manager exists and before it starts, so a scheme added there precedes any cache; it also runs before Routes, so a handler built in the callback can still be registered. ExtraMigrations are appended rather than merged, because the built-in tracks must reach their final version before tables that may reference them. The third is why SetupLogger is exported rather than kept inline. A caller builds its Options before Run, so anything it logs happens before Run installs the logger -- and controller-runtime discards everything written through log.Log until the first SetLogger call. A startup error in that window is lost entirely and the process appears to die in silence, which is how this surfaced. Calling SetupLogger first fixes it, and calling it twice is harmless: SetLogger fulfils a promise that can only be fulfilled once, so the first caller wins and Run's own call does nothing. The A2A handler is deliberately not among these. Core builds the gateway itself, with the instance workflow that lets it suspend an instance at the end of a turn; a caller supplying its own would get a less complete one, and the service that reaches agent runtimes should not accept whatever handler a library consumer assembles. Most of the diff is a move. 262 of the added lines are the former main() body, unchanged except for four edits: signal handling stays in main, log.Fatal becomes a returned error, the hardcoded auth pair becomes opts.resolve(), and group.Wait() is returned rather than fatal-logged. Reviewers can confirm that by diffing the two function bodies directly. One behaviour does change as a result. log.Fatal exits the process, so db.Close() and actors.Close() never ran on a startup failure. Returning the error runs them. Signed-off-by: Jonathan Jamroga <jjamroga@gmail.com>
github-actions
Bot
added
enhancement
EItanya
reviewed
Sep 1, 2026
Comment on lines
+83
to
+91
| // HTTPMiddleware wraps the HTTP handler, outermost first. It runs before | ||
| // routing, and therefore before the authentication core applies to its own | ||
| // endpoints -- which is the only order in which middleware can mark a | ||
| // request that the authenticator then reads. | ||
| HTTPMiddleware []func(http.Handler) http.Handler | ||
| // Routes registers additional HTTP endpoints. It runs after core has | ||
| // registered its own, so a more specific pattern wins over core's "/" | ||
| // without core having to know the library consumer's paths. | ||
| Routes func(*http.ServeMux) |
Contributor
There was a problem hiding this comment.
Is this still relevant given that the majority of our endpoints are gRPC now, do we still use this middleware or do we need more?
EItanya
reviewed
Sep 2, 2026
Comment on lines
+10
to
+19
| // Aliases, so a library consumer can name these without importing an internal | ||
| // package and every existing reference here keeps working unchanged. | ||
| type AccessMode = auth.AccessMode | ||
|
|
||
| const ( | ||
| AccessPublic AccessMode = "public" | ||
| AccessRead AccessMode = "read" | ||
| AccessCreate AccessMode = "create" | ||
| AccessUpdate AccessMode = "update" | ||
| AccessDelete AccessMode = "delete" | ||
| AccessPublic = auth.AccessPublic | ||
| AccessRead = auth.AccessRead | ||
| AccessCreate = auth.AccessCreate | ||
| AccessUpdate = auth.AccessUpdate | ||
| AccessDelete = auth.AccessDelete |
Contributor
There was a problem hiding this comment.
Why do we need to mirror these consts, can't we just use them directly?
…olicies A consumer that wants an endpoint the browser can reach before it has a token has only one option today: an HTTP route plus middleware that marks the request, paired with an authenticator wrapper that reads the mark. Neither half works alone, and the pair sits outside the gRPC server that already solves this -- grpcserver refuses any method it has no policy for, and AccessPublic skips authentication outright. Expose that. GRPCServices registers services on core's gRPC server, so a consumer's API shares core's transport, authenticator and interceptors rather than standing up a second server on another port. MethodPolicies declares their access. Run refuses a policy that names one of core's own methods. Merging permissively would let a config field make an authenticated core method public, which is a privilege escalation wearing a map literal. AccessMode moves to pkg/auth, since a consumer has to name these and grpcserver is internal. The old names stay as aliases, so nothing else changes. It stays a separate type from auth.Verb: Verb is what the authorizer is asked to allow, and "public" is not a verb. Signed-off-by: Jonathan Jamroga <jjamroga@gmail.com>
jjamroga
force-pushed
the
feat/implement-controller-v2-extensions
branch
from
September 2, 2026 13:00
1e061cd to
afa782e
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
2 participants
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
Controller startup lives in
func main()inpackage main— nothing can import it — and the authenticator and authorizer are hardcoded atcmd/controller-v2/main.go:161-162.This moves startup into
go/core/pkg/appbehindRun(ctx, Options)and makes those two replaceable.cmd/controller-v2becomes a signal handler and one call.API
AuthenticatorUnsecureAuthenticatorAuthorizerNoopAuthorizerSetupWithManagerHTTPMiddlewareRoutesExtraMigrationsEvery field is optional and its zero value is today's behaviour, so the shipped binary is unchanged.
SetupLoggeris also exported: callers buildOptionsbeforeRun, and controller-runtime discards everything logged before the firstSetLogger— so a startup error in that window vanishes. Calling it twice is a no-op; the first caller wins.Not included: the A2A handler. Core builds the gateway itself, including the instance workflow that suspends an instance at end of turn. A caller-supplied one would be less complete.
Reviewing this
262 of the 532 added lines are a verbatim move of the old
main()body:Only four edits should appear: signal handling stays in
main,log.Fatalbecomes a returned error, the auth pair becomesopts.resolve(), andgroup.Wait()is returned. The rest isOptions,resolve(),chain(),SetupLoggerand tests.Behaviour change:
log.Fatalexited the process, sodb.Close()andactors.Close()never ran on a startup failure. Returning the error runs them.Testing
6 unit tests in
go/core/pkg/app;go build ./...,go vet,make -C go lint(0 issues),go test -skip 'TestE2E.*' ./core/....Validated end to end on a kind cluster.