From f1fd64a567aea7aa9d5053f5922c1dddecd28291 Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Thu, 1 Oct 2026 08:49:04 +0200 Subject: [PATCH] DESIGN.md: the operator only ever creates servers, never adopts one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the premise Stage 1's bring-your-own credential design was built on. Confirmed with the user directly: this operator creates and owns every TerdutServer it manages; there is no hand-deployed or chart-deployed install it's expected to target or migrate. - §1: states this explicitly -- the root the rest of this commit hangs off. - §4.1: spec.credentialsSecretRef (bring-your-own input) removed entirely, not kept as unused flexibility. status.credentialsSecretRef stays as pure output. - §6: self-registration is now the *only* bootstrap path, not one of two -- and, since it's now load-bearing rather than a fallback with an easy escape hatch, closed the two real crash windows in it properly rather than leaving them as theoretical gaps: a checkpoint Secret for the raw admin key between /api/bootstrap and minting the service account, and adopt-on-409 (§5's general rule) if a prior interrupted attempt already got that far. A checkpoint lost after being used crosses into the same fail-closed territory §5's webhook-Secret-loss rule already established -- same recovery (delete and recreate), not a new, one-off workaround. - §10: dropped the migrate-an-existing-install narrative and the chart-Job-vs-operator bootstrap race question entirely -- both presupposed an install the operator might adopt or race against, which doesn't exist. Kept the installer-chart framing on its own. - §13: dropped the now-stale "Helm chart migration execution" deferred item. §8 (Postgres) needed no change -- it already described both the DSN and Zalando paths as co-equal, full-design detail, with no sequencing between them to remove. --- DESIGN.md | 188 ++++++++++++++++++++++++++---------------------------- 1 file changed, 91 insertions(+), 97 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index f723a02..6520e92 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -18,6 +18,15 @@ escalation policies, dead man's switches and alert-source integrations — be fully described as Kubernetes objects and managed through gitops, following controller-runtime / Kubebuilder conventions. +**The operator creates and owns every `TerdutServer` it manages. It never +adopts a pre-existing, independently-deployed terdut-server** — whether +deployed by hand or by `charts/terdut-server`. There is no migration path +from an existing chart-based install, and none is planned (§10): starting +with the operator means applying a fresh `TerdutServer` CR, not converting +one. This is the root a few things downstream hang off of — notably §6's +bootstrap flow, which only has to handle the operator bootstrapping a server +it just created, never a server something else already bootstrapped first. + **Non-goals (v1):** - Not a general-purpose Postgres operator. It *consumes* a database that either the Zalando `postgres-operator` or something else already provides. @@ -169,15 +178,6 @@ 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 @@ -192,23 +192,14 @@ status: conditions: [...] # Ready, DatabaseReady, Bootstrapped observedGeneration: 3 serviceName: terdut - credentialsSecretRef: {name: terdut.platform-oncall-instance-credentials, key: token} # see §6; lives in the OPERATOR's namespace (always, implicitly -- not stored here), not this TerdutServer's. No `namespace` field: unlike an earlier draft, it's never anything other than the operator's own, so there's nothing to record. `key` replaces it, since that *does* vary -- fixed ("token") when the controller generated this Secret itself, whatever the human chose when it was adopted from spec.credentialsSecretRef instead. + credentialsSecretRef: {name: terdut.platform-oncall-instance-credentials, key: token} # see §6; pure output -- generated by the controller's own self-registration flow, always in the OPERATOR's namespace (always, implicitly -- not stored here, since it's never anything else), under a fixed key ("token"). ``` -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. +Field-for-field this is the chart's `values.yaml` reshaped as a spec — not +for migrating an existing chart-based install (§1: there is no such path), +just because the shape is already familiar from the chart, and the operator +absorbs what the chart's Deployment/Service/bootstrap-job templates used to +do. `spec.database` fields are `+kubebuilder:validation:XValidation` guarded to be mutually exclusive (`dsn` xor `postgresClusterRef`); mirrors the chart's @@ -427,22 +418,21 @@ source: `internal/api/service_accounts.go`, migration 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. +**`GET /api/service-accounts?name=` is not an unauthenticated lookup — +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 ever runs.** This would matter a great +deal if the operator's `/api/bootstrap` call could ever lose a race to +something else bootstrapping the same server first — a credential-less +loser would have no authenticated way to recover. It doesn't matter here, +by construction (§1): **the operator only ever calls `/api/bootstrap` +against a `TerdutServer` it just created**, so there is nothing else in a +position to race it. An earlier draft of this section added a +`spec.credentialsSecretRef` bring-your-own input specifically to work around +that race, for a world where the operator might adopt a server something +else had already bootstrapped. That world doesn't exist (§1), so the field +was removed rather than kept as unused flexibility — self-registration +(point 1, below) is simply the only path, not one of two. **Every credential the operator holds — the one instance-scoped key per `TerdutServer`, and one team-scoped key per `TerdutTeam` — lives in a Secret @@ -464,36 +454,47 @@ 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. **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). 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`, +1. **First reconcile, 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 `403`s regardless of identity. There are two crash + windows between "get an admin key" and "have a lasting, usable + credential" — getting from `/api/bootstrap`'s key to a minted + service-account key, and getting from that key to a persisted Secret — + and both get a checkpoint rather than being left as a theoretical gap, + the same rigor §5's general idempotent-create rule already applies + elsewhere: + - `status.credentialsSecretRef` already set: done, nothing to do. + - Otherwise, check for an intermediate `-bootstrap-admin` Secret in + the operator's own namespace first. If it exists, its key is a still- + valid admin credential from an earlier, interrupted attempt — skip + `/api/bootstrap` entirely and reuse it. If not, call `/api/bootstrap` + once the Deployment this `TerdutServer` created has a ready replica; + on `201`, immediately checkpoint its response's raw admin key + (`{"user": ..., "api_key": {"key": "", ...}}`) into that Secret + before doing anything else with it. A `403` with neither + `status.credentialsSecretRef` nor this checkpoint Secret present is + the one genuinely pathological case left (the checkpoint deleted out + from under a reconcile already past this point) — handled the same + way the design already handles unrecoverable server-issued material + elsewhere (§5's webhook-Secret-loss rule): fail closed, + `Ready: False, reason: BootstrapStateLost`, with the same recovery as + that case, delete and recreate the `TerdutServer` (its finalizer tears + down the Deployment/database-backing and server-side rows; a fresh + create starts clean) — not a workaround peculiar to this one path. + - With an admin key in hand (fresh or checkpointed): `POST + /api/service-accounts {name: "terdut-operator", scope: "instance"}`. + A `409` here means a prior attempt got this far before being + interrupted — adopt rather than error, per §5's general rule: + `GET /api/service-accounts?name=terdut-operator` (authenticated with + the checkpointed admin key, not an unauthenticated lookup) to find its + id, then `POST /api/service-accounts/{id}/keys` to mint a fresh key — + an orphaned first key some interrupted attempt minted and never used + is inert, not a cleanup obligation. +2. The resulting instance-scoped key — not the checkpointed admin key, which + is deleted once this step succeeds — is written to a generated Secret in + the **operator's own namespace** (e.g. + `.-instance-credentials`, under a fixed data key, `token`), referenced back from `TerdutServer.status.credentialsSecretRef: {name, key}` (§4.1). No `OwnerReference` (those can't cross namespaces, and this Secret doesn't @@ -637,33 +638,27 @@ documented and tested operationally: ## 10. Relationship to `charts/terdut-server` -**Recommendation** (flagged explicitly as a decision to confirm before -implementation starts, not settled by this document alone): the chart's -Deployment/Service/bootstrap-job templates become redundant once +The chart's Deployment/Service/bootstrap-job templates are redundant once `TerdutServer` exists — running both would mean two controllers (Helm and this operator) reconciling the same Deployment, which is exactly the -conflict Kubernetes operators exist to avoid. Proposed path: -- The chart is repurposed into an **installer chart**: it installs the - operator + CRDs (and optionally one `TerdutServer` CR from `values.yaml`, - for users who want "helm install and get a server" without hand-writing a - CR) rather than templating the Deployment directly. -- Existing installs migrate by: `helm template` the current release's - `values.yaml` into an equivalent `TerdutServer` CR (mechanical, since §4.1 - is deliberately shaped to make that mapping 1:1), install the operator, - apply the CR, then let Helm's release be uninstalled or reduced to just - the CRD/operator subchart. -- This is a breaking change to the chart's contract and needs its own - migration guide and probably a major chart version bump — out of scope - for this design doc beyond flagging it; do not start that migration work - without separately confirming this recommendation. -- **This decision isn't only about the migration — it also decides who - owns bootstrap.** §6 assumed the operator could always get its own - bootstrap identity separately from the chart's; §6 point 1 shows that's - false, so whichever of {chart's Job, operator controller} is expected to - call `/api/bootstrap` first has to be settled explicitly (a one-paragraph - call, not the full migration plan) *before* writing any operator - bootstrap/credential code, not deferred alongside the rest of this - section. +conflict Kubernetes operators exist to avoid. The chart is repurposed into +an **installer chart**: it installs the operator + CRDs (and optionally one +`TerdutServer` CR from `values.yaml`, for users who want "helm install and +get a server" without hand-writing a CR) rather than templating the +Deployment directly. + +**No migration path from an existing chart-based install, and none is +planned (§1).** An earlier draft of this section spent most of its length on +one — `helm template` the current release's `values.yaml` into an equivalent +`TerdutServer` CR, uninstall or shrink the old release, and a whole +sub-question about who gets to call `/api/bootstrap` first, the chart's Job +or the operator — all of which presupposed the operator might end up +managing a server the chart had already deployed and bootstrapped. §1 rules +that out: the operator only ever manages servers it created itself, so +there's nothing to migrate and no bootstrap race to settle (§6 covers why +that race doesn't exist either). Adopting the operator means applying a +fresh `TerdutServer` CR; whatever the chart deployed before stays exactly +what it was, a separate install, until someone deletes it. ## 11. Testing strategy @@ -721,5 +716,4 @@ conflict Kubernetes operators exist to avoid. Proposed path: - Admission webhooks / CEL-only validation limits (e.g. verifying a `teamRef` exists at admission time rather than surfacing it as a status condition after the fact). -- OLM packaging, Helm chart migration execution (§10 is a recommendation, - not a plan to execute). +- OLM packaging.