DESIGN.md: the operator only ever creates servers, never adopts one
CI / test (push) Has been cancelled
CI / test (push) Has been cancelled
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.
This commit is contained in:
@@ -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
|
fully described as Kubernetes objects and managed through gitops, following
|
||||||
controller-runtime / Kubebuilder conventions.
|
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):**
|
**Non-goals (v1):**
|
||||||
- Not a general-purpose Postgres operator. It *consumes* a database that
|
- Not a general-purpose Postgres operator. It *consumes* a database that
|
||||||
either the Zalando `postgres-operator` or something else already provides.
|
either the Zalando `postgres-operator` or something else already provides.
|
||||||
@@ -169,15 +178,6 @@ spec:
|
|||||||
adminGroup: ""
|
adminGroup: ""
|
||||||
sessionMaxAge: 12h
|
sessionMaxAge: 12h
|
||||||
passwordLogin: true
|
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
|
# Consent for TerdutTeams in OTHER namespaces to set serverRef at this
|
||||||
# TerdutServer. Same-namespace TerdutTeams never need this. Modeled on
|
# TerdutServer. Same-namespace TerdutTeams never need this. Modeled on
|
||||||
# Gateway API's Gateway.spec.allowedListeners.namespaces (the ListenerSet
|
# Gateway API's Gateway.spec.allowedListeners.namespaces (the ListenerSet
|
||||||
@@ -192,23 +192,14 @@ status:
|
|||||||
conditions: [...] # Ready, DatabaseReady, Bootstrapped
|
conditions: [...] # Ready, DatabaseReady, Bootstrapped
|
||||||
observedGeneration: 3
|
observedGeneration: 3
|
||||||
serviceName: terdut
|
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
|
Field-for-field this is the chart's `values.yaml` reshaped as a spec — not
|
||||||
operator absorbs the chart's Deployment/Service/bootstrap-job templates, so
|
for migrating an existing chart-based install (§1: there is no such path),
|
||||||
existing installs have a direct mapping when migrating (see §10).
|
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
|
||||||
`spec.credentialsSecretRef` is an *input* (bring-your-own), distinct from
|
do.
|
||||||
`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
|
`spec.database` fields are `+kubebuilder:validation:XValidation` guarded to
|
||||||
be mutually exclusive (`dsn` xor `postgresClusterRef`); mirrors the chart's
|
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
|
framing here was accurate when this section was first written and is stale
|
||||||
now.
|
now.
|
||||||
|
|
||||||
**`GET /api/service-accounts?name=` is not an unauthenticated lookup, unlike
|
**`GET /api/service-accounts?name=` is not an unauthenticated lookup —
|
||||||
an earlier draft of this section assumed — confirmed against
|
confirmed against `internal/api/middleware.go`'s `AuthMiddleware`, which
|
||||||
`internal/api/middleware.go`'s `AuthMiddleware`, which hard-rejects any
|
hard-rejects any request carrying neither a Bearer token nor a session
|
||||||
request carrying neither a Bearer token nor a session cookie with `401`
|
cookie with `401` before any handler ever runs.** This would matter a great
|
||||||
before any handler (including this one's own internal, more permissive
|
deal if the operator's `/api/bootstrap` call could ever lose a race to
|
||||||
name-filter check) ever runs.** This matters beyond a technicality: it means
|
something else bootstrapping the same server first — a credential-less
|
||||||
the self-registration flow below (point 1) only ever completes for the
|
loser would have no authenticated way to recover. It doesn't matter here,
|
||||||
`TerdutServer` whose own controller happens to win the `/api/bootstrap` race
|
by construction (§1): **the operator only ever calls `/api/bootstrap`
|
||||||
on a genuinely empty install. Every other case — including Stage 1's own
|
against a `TerdutServer` it just created**, so there is nothing else in a
|
||||||
setup (`ROADMAP.md`): terdut-server deployed by its existing chart, which
|
position to race it. An earlier draft of this section added a
|
||||||
runs its own bootstrap Job, *before* the `TerdutServer` CR or its controller
|
`spec.credentialsSecretRef` bring-your-own input specifically to work around
|
||||||
ever exist — leaves the controller with no credential and no authenticated
|
that race, for a world where the operator might adopt a server something
|
||||||
way to get one. `spec.credentialsSecretRef` (§4.1) exists to make that the
|
else had already bootstrapped. That world doesn't exist (§1), so the field
|
||||||
normal path, not an unhandled edge case: a human mints an instance-scoped
|
was removed rather than kept as unused flexibility — self-registration
|
||||||
service account once, manually, with their own admin session, and hands the
|
(point 1, below) is simply the only path, not one of two.
|
||||||
controller that Secret directly.
|
|
||||||
|
|
||||||
**Every credential the operator holds — the one instance-scoped key per
|
**Every credential the operator holds — the one instance-scoped key per
|
||||||
`TerdutServer`, and one team-scoped key per `TerdutTeam` — lives in a Secret
|
`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
|
track it in status instead" workaround — none of that machinery is needed
|
||||||
when nothing ever crosses into a tenant namespace in the first place.
|
when nothing ever crosses into a tenant namespace in the first place.
|
||||||
|
|
||||||
1. **First reconcile checks `spec.credentialsSecretRef` before anything
|
1. **First reconcile, confirmed against source**
|
||||||
else.** Set and the Secret exists: adopt it as-is, set
|
(`internal/api/users.go`'s `handleBootstrap`): `/api/bootstrap` is
|
||||||
`status.credentialsSecretRef` to the same reference, `Bootstrapped: True`,
|
single-shot *per install*, gated on `SELECT COUNT(*) FROM users` — once
|
||||||
done — no API call made at all. This is the path every Stage 1 install
|
non-zero, every call `403`s regardless of identity. There are two crash
|
||||||
actually takes (`ROADMAP.md`): terdut-server deployed by its existing
|
windows between "get an admin key" and "have a lasting, usable
|
||||||
chart, bootstrapped by that chart's own Job, before this CR exists.
|
credential" — getting from `/api/bootstrap`'s key to a minted
|
||||||
Unset (or the named Secret doesn't exist yet): fall through to
|
service-account key, and getting from that key to a persisted Secret —
|
||||||
self-registration, confirmed against source (`internal/api/users.go`'s
|
and both get a checkpoint rather than being left as a theoretical gap,
|
||||||
`handleBootstrap`): `/api/bootstrap` is single-shot *per install*, gated
|
the same rigor §5's general idempotent-create rule already applies
|
||||||
on `SELECT COUNT(*) FROM users` — once non-zero, every call 403s
|
elsewhere:
|
||||||
regardless of identity, exactly as `charts/terdut-server`'s own
|
- `status.credentialsSecretRef` already set: done, nothing to do.
|
||||||
`bootstrap-job.yaml` already assumes (403 → "already bootstrapped,
|
- Otherwise, check for an intermediate `<name>-bootstrap-admin` Secret in
|
||||||
nothing to do", exit 0). Call `/api/bootstrap`; on `201`, its response
|
the operator's own namespace first. If it exists, its key is a still-
|
||||||
(`{"user": ..., "api_key": {"key": "<raw>", ...}}`) hands back a real,
|
valid admin credential from an earlier, interrupted attempt — skip
|
||||||
usable admin key directly — use it for exactly one further call,
|
`/api/bootstrap` entirely and reuse it. If not, call `/api/bootstrap`
|
||||||
`POST /api/service-accounts {name: "terdut-operator", scope: "instance"}`,
|
once the Deployment this `TerdutServer` created has a ready replica;
|
||||||
and keep *that* key, not the raw admin one, as the lasting credential.
|
on `201`, immediately checkpoint its response's raw admin key
|
||||||
**On `403`, there is no further fallback**: per this section's opening
|
(`{"user": ..., "api_key": {"key": "<raw>", ...}}`) into that Secret
|
||||||
note, `GET /api/service-accounts?name=` needs a credential this
|
before doing anything else with it. A `403` with neither
|
||||||
controller does not have, so self-lookup cannot run unauthenticated. Set
|
`status.credentialsSecretRef` nor this checkpoint Secret present is
|
||||||
`Ready: False, reason: WaitingForCredential` with an event telling the
|
the one genuinely pathological case left (the checkpoint deleted out
|
||||||
human to mint an instance-scoped service account with their own admin
|
from under a reconcile already past this point) — handled the same
|
||||||
session and set `spec.credentialsSecretRef`, and requeue with backoff —
|
way the design already handles unrecoverable server-issued material
|
||||||
this is the expected, steady-state outcome whenever the operator loses
|
elsewhere (§5's webhook-Secret-loss rule): fail closed,
|
||||||
(or never entered) the bootstrap race, not a transient error to retry
|
`Ready: False, reason: BootstrapStateLost`, with the same recovery as
|
||||||
past.
|
that case, delete and recreate the `TerdutServer` (its finalizer tears
|
||||||
2. On the self-registration path only (point 1's bring-your-own path
|
down the Deployment/database-backing and server-side rows; a fresh
|
||||||
generates nothing — it adopts the human-provided Secret directly): the
|
create starts clean) — not a workaround peculiar to this one path.
|
||||||
resulting instance-scoped key is written to a generated Secret in the
|
- With an admin key in hand (fresh or checkpointed): `POST
|
||||||
**operator's own namespace** (e.g. `<serverRef.namespace>.<serverRef.name>-instance-credentials`,
|
/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.
|
||||||
|
`<serverRef.namespace>.<serverRef.name>-instance-credentials`,
|
||||||
under a fixed data key, `token`), referenced back from
|
under a fixed data key, `token`), referenced back from
|
||||||
`TerdutServer.status.credentialsSecretRef: {name, key}` (§4.1). No
|
`TerdutServer.status.credentialsSecretRef: {name, key}` (§4.1). No
|
||||||
`OwnerReference` (those can't cross namespaces, and this Secret doesn't
|
`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`
|
## 10. Relationship to `charts/terdut-server`
|
||||||
|
|
||||||
**Recommendation** (flagged explicitly as a decision to confirm before
|
The chart's Deployment/Service/bootstrap-job templates are redundant once
|
||||||
implementation starts, not settled by this document alone): the chart's
|
|
||||||
Deployment/Service/bootstrap-job templates become redundant once
|
|
||||||
`TerdutServer` exists — running both would mean two controllers (Helm and
|
`TerdutServer` exists — running both would mean two controllers (Helm and
|
||||||
this operator) reconciling the same Deployment, which is exactly the
|
this operator) reconciling the same Deployment, which is exactly the
|
||||||
conflict Kubernetes operators exist to avoid. Proposed path:
|
conflict Kubernetes operators exist to avoid. The chart is repurposed into
|
||||||
- The chart is repurposed into an **installer chart**: it installs the
|
an **installer chart**: it installs the operator + CRDs (and optionally one
|
||||||
operator + CRDs (and optionally one `TerdutServer` CR from `values.yaml`,
|
`TerdutServer` CR from `values.yaml`, for users who want "helm install and
|
||||||
for users who want "helm install and get a server" without hand-writing a
|
get a server" without hand-writing a CR) rather than templating the
|
||||||
CR) rather than templating the Deployment directly.
|
Deployment directly.
|
||||||
- Existing installs migrate by: `helm template` the current release's
|
|
||||||
`values.yaml` into an equivalent `TerdutServer` CR (mechanical, since §4.1
|
**No migration path from an existing chart-based install, and none is
|
||||||
is deliberately shaped to make that mapping 1:1), install the operator,
|
planned (§1).** An earlier draft of this section spent most of its length on
|
||||||
apply the CR, then let Helm's release be uninstalled or reduced to just
|
one — `helm template` the current release's `values.yaml` into an equivalent
|
||||||
the CRD/operator subchart.
|
`TerdutServer` CR, uninstall or shrink the old release, and a whole
|
||||||
- This is a breaking change to the chart's contract and needs its own
|
sub-question about who gets to call `/api/bootstrap` first, the chart's Job
|
||||||
migration guide and probably a major chart version bump — out of scope
|
or the operator — all of which presupposed the operator might end up
|
||||||
for this design doc beyond flagging it; do not start that migration work
|
managing a server the chart had already deployed and bootstrapped. §1 rules
|
||||||
without separately confirming this recommendation.
|
that out: the operator only ever manages servers it created itself, so
|
||||||
- **This decision isn't only about the migration — it also decides who
|
there's nothing to migrate and no bootstrap race to settle (§6 covers why
|
||||||
owns bootstrap.** §6 assumed the operator could always get its own
|
that race doesn't exist either). Adopting the operator means applying a
|
||||||
bootstrap identity separately from the chart's; §6 point 1 shows that's
|
fresh `TerdutServer` CR; whatever the chart deployed before stays exactly
|
||||||
false, so whichever of {chart's Job, operator controller} is expected to
|
what it was, a separate install, until someone deletes it.
|
||||||
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.
|
|
||||||
|
|
||||||
## 11. Testing strategy
|
## 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
|
- Admission webhooks / CEL-only validation limits (e.g. verifying a
|
||||||
`teamRef` exists at admission time rather than surfacing it as a status
|
`teamRef` exists at admission time rather than surfacing it as a status
|
||||||
condition after the fact).
|
condition after the fact).
|
||||||
- OLM packaging, Helm chart migration execution (§10 is a recommendation,
|
- OLM packaging.
|
||||||
not a plan to execute).
|
|
||||||
|
|||||||
Reference in New Issue
Block a user