From 7f439605c45639e757762358b6e2a92b7446e841 Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Wed, 30 Sep 2026 22:20:31 +0200 Subject: [PATCH] =?UTF-8?q?DESIGN.md=20=C2=A74.1/=C2=A76:=20fix=20the=20bo?= =?UTF-8?q?otstrap=20self-registration=20deadlock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Traced the actual flow against terdut-server's real source before writing any Stage 1 controller code, rather than trusting this section's own prior description of it: - internal/api/middleware.go's AuthMiddleware hard-rejects with 401 any request carrying neither a Bearer token nor a session cookie, before handleListServiceAccounts' own (more permissive) internal check ever runs. So "on 403, self-lookup via GET /api/service-accounts?name=" -- this section's described fallback -- cannot work unauthenticated; an earlier draft of this section assumed otherwise. - That only actually matters in the rare case where this TerdutServer's own controller loses the /api/bootstrap race... except Stage 1's own setup (ROADMAP.md) guarantees it loses every time: terdut-server is deployed via its existing chart, which runs its own bootstrap Job, before the TerdutServer CR or its controller exist at all. The self-registration flow was never going to complete for the one scenario Stage 1 actually exercises. Fix: spec.credentialsSecretRef (§4.1), bring-your-own -- a human mints an instance-scoped service account once, manually, with their own admin session, and hands the controller that Secret directly. This is now the primary, expected path; self-registration on a genuinely fresh install (where this controller might actually win the race) stays as the fallback it was always meant to be, not the only path. Also corrected: this section's opening paragraph still said "v1-blocking, not v1-shippable" pending SERVICE-ACCOUNTS.md landing -- confirmed shipped (internal/api/service_accounts.go, migration 014) since Stage 0's work on this repo; stale framing removed. --- DESIGN.md | 86 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 17 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index b0544f3..16634b0 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -169,6 +169,15 @@ spec: adminGroup: "" sessionMaxAge: 12h passwordLogin: true + # Bring-your-own instance credential: a human mints this once, manually, with + # their own admin session (`POST /api/service-accounts {name: ..., scope: + # instance}`) and creates this Secret themselves, in the OPERATOR's own + # namespace (same namespace status.credentialsSecretRef below would otherwise + # point into). When set and the Secret exists, the controller adopts it + # directly and skips bootstrap entirely -- see §6 for why this is required, + # not optional polish, whenever terdut-server was already bootstrapped by its + # chart or a human before this CR existed (the normal case, not an edge one). + credentialsSecretRef: {name: "", key: token} # Consent for TerdutTeams in OTHER namespaces to set serverRef at this # TerdutServer. Same-namespace TerdutTeams never need this. Modeled on # Gateway API's Gateway.spec.allowedListeners.namespaces (the ListenerSet @@ -190,6 +199,17 @@ Field-for-field this is the chart's `values.yaml` reshaped as a spec — the operator absorbs the chart's Deployment/Service/bootstrap-job templates, so existing installs have a direct mapping when migrating (see §10). +`spec.credentialsSecretRef` is an *input* (bring-your-own), distinct from +`status.credentialsSecretRef`'s *output* (generated-by-the-controller) — +when the input is set, the controller treats it as the credential outright +and echoes its name back into `status.credentialsSecretRef` rather than +generating a second Secret alongside it. Left unset, the controller falls +back to the self-registration flow (§6) — which only actually completes if +this `TerdutServer`'s own first reconcile is the one that wins the +`/api/bootstrap` race against a genuinely empty install; see §6 for why +that fallback is the exception, not the common case, and why this field +exists at all rather than being deferred as nice-to-have. + `spec.database` fields are `+kubebuilder:validation:XValidation` guarded to be mutually exclusive (`dsn` xor `postgresClusterRef`); mirrors the chart's "chart provisions no database" stance — this operator provisions no database @@ -399,11 +419,30 @@ General rules for every controller: ## 6. Bootstrap & authentication to terdut-server's API -terdut-server has no first-class "service account" token type today — API -keys belong to a real user row (`api_keys.user_id`). This section depends on -a server-side feature request (`terdut-server`'s `SERVICE-ACCOUNTS.md`) that -adds one; treat this whole section as v1-blocking, not v1-shippable, until -that lands (§10, §13). +terdut-server's scoped service-account credential type +(`terdut-server`'s `SERVICE-ACCOUNTS.md`) has shipped — confirmed against +source: `internal/api/service_accounts.go`, migration +`014_service_accounts.sql`, and `internal/api/router.go` wiring it in under +`AuthMiddleware`. This section is no longer blocked on it; the "v1-blocking" +framing here was accurate when this section was first written and is stale +now. + +**`GET /api/service-accounts?name=` is not an unauthenticated lookup, unlike +an earlier draft of this section assumed — confirmed against +`internal/api/middleware.go`'s `AuthMiddleware`, which hard-rejects any +request carrying neither a Bearer token nor a session cookie with `401` +before any handler (including this one's own internal, more permissive +name-filter check) ever runs.** This matters beyond a technicality: it means +the self-registration flow below (point 1) only ever completes for the +`TerdutServer` whose own controller happens to win the `/api/bootstrap` race +on a genuinely empty install. Every other case — including Stage 1's own +setup (`ROADMAP.md`): terdut-server deployed by its existing chart, which +runs its own bootstrap Job, *before* the `TerdutServer` CR or its controller +ever exist — leaves the controller with no credential and no authenticated +way to get one. `spec.credentialsSecretRef` (§4.1) exists to make that the +normal path, not an unhandled edge case: a human mints an instance-scoped +service account once, manually, with their own admin session, and hands the +controller that Secret directly. **Every credential the operator holds — the one instance-scoped key per `TerdutServer`, and one team-scoped key per `TerdutTeam` — lives in a Secret @@ -425,22 +464,35 @@ when `allowedTeams` narrows, no "OwnerReferences can't cross namespaces so track it in status instead" workaround — none of that machinery is needed when nothing ever crosses into a tenant namespace in the first place. -1. **Bootstrap, confirmed against source** (`internal/api/users.go`'s +1. **First reconcile checks `spec.credentialsSecretRef` before anything + else.** Set and the Secret exists: adopt it as-is, set + `status.credentialsSecretRef` to the same reference, `Bootstrapped: True`, + done — no API call made at all. This is the path every Stage 1 install + actually takes (`ROADMAP.md`): terdut-server deployed by its existing + chart, bootstrapped by that chart's own Job, before this CR exists. + Unset (or the named Secret doesn't exist yet): fall through to + self-registration, confirmed against source (`internal/api/users.go`'s `handleBootstrap`): `/api/bootstrap` is single-shot *per install*, gated on `SELECT COUNT(*) FROM users` — once non-zero, every call 403s regardless of identity, exactly as `charts/terdut-server`'s own `bootstrap-job.yaml` already assumes (403 → "already bootstrapped, - nothing to do", exit 0). So the `TerdutServer` controller's first-reconcile - flow is: call `/api/bootstrap` **only if nothing has bootstrapped yet** - (a genuinely empty install, e.g. chart's bootstrap Job disabled in favor - of the operator owning this — see §10); on 403, look itself up instead — - `GET /api/service-accounts?name=terdut-operator` — and self-register via - `POST /api/service-accounts` (instance scope) if it isn't already - registered. This also resolves the race if both the chart's Job and this - controller call `/api/bootstrap` against the same fresh install: whichever - loses treats 403 as "go get my own credential the other way," not an - error. -2. The resulting instance-scoped key is written to a generated Secret in the + nothing to do", exit 0). Call `/api/bootstrap`; on `201`, its response + (`{"user": ..., "api_key": {"key": "", ...}}`) hands back a real, + usable admin key directly — use it for exactly one further call, + `POST /api/service-accounts {name: "terdut-operator", scope: "instance"}`, + and keep *that* key, not the raw admin one, as the lasting credential. + **On `403`, there is no further fallback**: per this section's opening + note, `GET /api/service-accounts?name=` needs a credential this + controller does not have, so self-lookup cannot run unauthenticated. Set + `Ready: False, reason: WaitingForCredential` with an event telling the + human to mint an instance-scoped service account with their own admin + session and set `spec.credentialsSecretRef`, and requeue with backoff — + this is the expected, steady-state outcome whenever the operator loses + (or never entered) the bootstrap race, not a transient error to retry + past. +2. On the self-registration path only (point 1's bring-your-own path + generates nothing — it adopts the human-provided Secret directly): the + resulting instance-scoped key is written to a generated Secret in the **operator's own namespace** (e.g. `.-instance-credentials`), referenced back from `TerdutServer.status.credentialsSecretRef: {name, namespace}` (§4.1) — `namespace` is part of the reference now, since it's