refactor(gateway): dynamic gRPC-reflection routing, Postgres migration, deployment modes, and critical bug fixes - #29
Merged
Conversation
…C-shaped and so have nothing for grpcbridge to discover via reflection: health, list-clusters, get-cluster. Everything else that used to live on ProwController (workload/node CRUD, cluster metrics, forgery passthrough) is now served dynamically — see internal/router/bindings.go — since it's a straight AgentControl or ForgeryControl RPC with no logic of its own beyond what the dynamic bridge already does.
…e into a set of gin routes with zero generated-stub boilerplate: no per-RPC wrapper method, no per-RPC controller handler, no per-RPC route line.
…c RPC surface is declared. Adding a method for persysctl or the SDK to call is very likely a one-line addition here — a MethodAlias, or nothing at all if the generic /rpc/<Service>/<Method> path is fine.
… it mounts behind.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Summary
Large refactor of persys-gateway's routing, auth, and persistence layers,
plus fixes for a stale-naming problem, a hardcoded secret, and a couple
of bugs the refactor itself introduced along the way (documented below,
not swept under the rug).
Dynamic, reflection-based routing
internal/grpcbridge: discovers RPC methods onAgentControl(scheduler) and
ForgeryControl(CI/CD) via gRPC server reflection,and dispatches generically via
dynamicpb/protojson— no morehand-written controller + route + service-wrapper triplet per RPC.
descriptor (
controlv1.File_control_proto/forgeryv1.File_forgery_proto)if a backend doesn't implement reflection yet. Works against existing
scheduler/forgery deployments unmodified; upgrades to live discovery
automatically once they add
reflection.Register.GET /clusters/:cluster_id/rpc/_metaand.../forgery/rpc/_metalist every callable method, aliased or not.internal/router/bindings.go.(
/clusters/:cluster_id/workloads/...) and once flat(
/workloads/...,/nodes/...,/cluster/metrics,/forgery/...) — so existingpersysctlinstalls keep workingagainst the gateway's default cluster with zero changes required.
Renames (dead "Prow" naming removed)
ProwService→ClusterControlService,ProwController→ClusterMetaController(trimmed to the 4 handlers that were neverRPC-shaped),
ProwConfig→LegacySchedulerConfig. "Prow" nevercorresponded to any real Persys service. Forgery was also split out of
ClusterControlServiceinto its ownForgeryService— it's a singlefixed address with no pool/failover, and sharing the scheduler pool's
shape was never an honest description of what it is.
Security fixes
"unicornsAreAwesome", committed in 3 places)→
app.jwt_secret, env-sourced (PERSYS_GATEWAY_JWT_SECRET),auto-generated with a startup warning in self-hosted mode, required
in managed mode (fails fast at startup otherwise).
github.routes.go:authControllerwas never actually injected —Auth()ran on a zero-value struct and only appeared to work via anaccidental dependency on package-level globals set by a different
controller. Fixed.
login attempt (race under concurrent logins). Now a real row per
attempt, consumed exactly once atomically.
auth.impl.service.go: fixed a bug whereSignInUserran anunconditional insert regardless of whether the user already existed,
relying on a unique index that was never actually created — silently
producing duplicate user rows on every login.
tests/auth_test.go. Removed; test now requiresPERSYS_TEST_POSTGRES_DSNand skips otherwise. Rotate thatcredential regardless of this PR — it was public.
Deployment modes
New
deployment.mode:self-hosted(default) ormanaged./auth/*and/github/*aren't mounted, mTLS is the only trust boundary, no database
required.
database required (fails fast if unset).
GET /healthreports bothdeployment_modeanddatabase_enabled.Database: MongoDB → Postgres, and made optional
internal/store(pgx), three tables (users,oauth_sessions,webhook_events) replacing Mongo entirely. Schema applies asidempotent
CREATE TABLE IF NOT EXISTSon startup, no separatemigration step.
written anywhere (
repos,cluster_state), plus two dead*mongo.Collectionconstructor params that were never used.database.dsnis optional in self-hosted mode — the only thingsthat touch it are gated to managed mode already, or (webhook audit
persistence) already degrade to in-memory-only behavior on a nil store.
Other fixes found and fixed along the way
grpcbridgeitself: generic dispatch only decoded theJSON body, never URL path params — so
GET /workloads/:idetc. sentan empty ID to the backend.
MethodAliasgainedPathParamstomap path segments onto proto fields; verified against the actual
generated
.pb.gofield names rather than assumed.TaintNodeRequest.Taintis a nested message, unlike its siblings —fixed the request shape accordingly.
Config / docs
config.yaml,README.mdupdated to match all of the above(
legacy_scheduler:renamed section,deployment:/database:blocks,dynamic-routing docs). New
PERSYS_GATEWAY_DEPLOYMENT_MODEenv overrideadded (didn't previously exist — needed for container/compose
deployments).
Breaking changes
database.mongo_uri/database.name→database.dsn;prow:→legacy_scheduler:(with renamed fields).persysctlclient update (companion PR/patch) forTaintNode— its request body shape was wrong before this fix in away the old hand-written controller had masked.
Follow-ups (not in this PR)
go.mod/go.sum: rungo mod tidyonce after merging — addedpgx/v5, removedmongo-driver.cluster_owners(multi-tenant ownership) designed but not wired in;needs its own migration once managed-mode ownership checks land.
GET /workloads/:idend-to-end — the path-param bug above would have been caught by one.