diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..f505a81 --- /dev/null +++ b/.gitignore @@ -0,0 +1,14 @@ +# Go build artifacts +bin/ +*.exe + +# editor swapfiles +*.swp +*.swo +*~ + +# envtest binaries downloaded by setup-envtest +/testbin/ + +# local kubeconfig/secrets some workflows write here +*.kubeconfig diff --git a/DESIGN.md b/DESIGN.md index eced8ab..b0544f3 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -57,9 +57,12 @@ unchanged. This cross-namespace edge needs the target namespace's explicit consent — otherwise any namespace in the cluster could point a `TerdutTeam` at -someone else's `TerdutServer` and have the operator provision a team + -credentials against it, which is a namespace-boundary violation, not a -gitops convenience. Kubernetes has two established patterns for this kind +someone else's `TerdutServer` and have the operator provision a team on +its behalf, which is a namespace-boundary violation, not a gitops +convenience. (This consent gate is about which `TerdutTeam`s the operator +will act on, not about credential exposure — no terdut-server credential +is ever placed in a `TerdutTeam`'s own namespace regardless of this +setting; see §6.) Kubernetes has two established patterns for this kind of consent, and Gateway API itself uses both, for two different relationships: @@ -180,7 +183,7 @@ status: conditions: [...] # Ready, DatabaseReady, Bootstrapped observedGeneration: 3 serviceName: terdut - operatorCredentialsSecretRef: {name: terdut-operator-credentials} # see §6 + credentialsSecretRef: {name: terdut.platform-oncall-instance-credentials, namespace: terdut-operator-system} # see §6; lives in the OPERATOR's namespace, not this TerdutServer's ``` Field-for-field this is the chart's `values.yaml` reshaped as a spec — the @@ -216,6 +219,7 @@ spec: status: conditions: [...] teamID: 42 # the server-side ID; needed by every child object's controller + credentialsSecretRef: {name: platform-oncall.platform-team-credentials, namespace: terdut-operator-system} # see §6; this team's own scoped key, in the OPERATOR's namespace observedGeneration: 1 ``` @@ -342,7 +346,7 @@ resource a full update verb, so reconciliation strategy is per-resource: | Team | POST create, PUT rename, DELETE, PUT oidc-groups | Real update-in-place: diff spec vs. last-applied, PUT the changed pieces. | | Escalation policy | GET/PUT whole-policy | Update-in-place: PUT the full desired policy every reconcile that finds drift; cheap because whole-policy is small and already loaded whole server-side. | | Dead man's switch | POST create, DELETE — **no PUT** | Delete-and-recreate on any spec diff other than `name`. The controller diffs against `status` (which mirrors what was last successfully applied) rather than re-reading the server every reconcile, to avoid a spurious recreate from field reordering. | -| Integration (alert source) | POST create, PATCH rename, DELETE | Rename via PATCH; any other spec change (kind) is delete-and-recreate, which **rotates the webhook key** — called out loudly in the CRD's field docs and in a `Warning` event, since it breaks whatever sends to the old URL/key until the new Secret is picked up. | +| Integration (alert source) | POST create, PATCH rename, DELETE | Rename via PATCH; any other spec change (kind) is delete-and-recreate, which **rotates the webhook key** — called out loudly in the CRD's field docs and in a `Warning` event, since it breaks whatever sends to the old URL/key until the new Secret is picked up. (See the general rule below for what happens if the Secret is lost with *no* spec change.) | General rules for every controller: - **Idempotent create**: before POSTing, check `status.` is @@ -360,13 +364,32 @@ General rules for every controller: Failure to delete server-side (e.g. server unreachable) blocks finalizer removal and surfaces as a `Degraded` condition + event, rather than silently orphaning a row. +- **Generated Secrets holding unrecoverable server-issued material are + watched, and their loss is fail-closed, not self-healed.** Currently this + is just `TerdutAlertSource`'s webhook Secret (§4.5): the controller adds + it to its `Owns()` watches, not just the CR. If it disappears while + `status.integrationID` is still set, the controller does **not** attempt + to recreate it — the key is genuinely gone (§4.5: never stored anywhere + but that one Secret), so silently minting a replacement would rotate a + live production webhook URL with no corresponding spec change to explain + why. Instead it flips `Ready: False, reason: WebhookSecretLost` and fires + a `Warning` event telling the operator to delete and recreate the + `TerdutAlertSource`. No new mechanism is needed for recovery: deleting the + CR runs the existing finalizer (DELETE the still-live integration + server-side, above), and recreating it runs the existing idempotent-create + path (this same section) — a fresh POST, a new key, a new Secret. This is + deliberately the same recovery motion as the kind-change rotation above, + just human-triggered instead of spec-triggered. - **Owner chain for status resolution, not API calls**: `TerdutTeam`'s controller does not call any other controller; every child CRD's - controller independently resolves its own `teamRef` → `TerdutTeam.status.teamID` - and `serverRef` chain down to `TerdutServer.status.operatorCredentialsSecretRef`, - the way any two independent controller-runtime reconcilers would. If the - referenced parent isn't `Ready` yet, the child requeues with backoff and - reports `Ready: False, reason: WaitingForTeam` — no cross-controller RPC. + controller independently resolves its own `teamRef` → `TerdutTeam.status` + for both the `teamID` and the `credentialsSecretRef` it needs to call the + API (§6) — it never needs to chain further up to `TerdutServer` at all, + since the team's own scoped credential is everything a child resource's + controller requires. If the referenced `TerdutTeam` isn't `Ready` yet + (which includes not having a `credentialsSecretRef` set), the child + requeues with backoff and reports `Ready: False, reason: WaitingForTeam` + — no cross-controller RPC. - **Cross-namespace `serverRef` is re-checked every reconcile, not just at creation**: `TerdutTeam`'s controller reads the target `TerdutServer`'s `spec.allowedTeams` (and, under `Selector`, a `Get` on its own `Namespace` @@ -376,100 +399,106 @@ General rules for every controller: ## 6. Bootstrap & authentication to terdut-server's API -terdut-server has no first-class "service account" token type — API keys -belong to a real user row (`api_keys.user_id`). The chart's current answer is -a one-shot Job that calls `POST /api/bootstrap`, gets a one-time admin key -back, and stores it in a Secret (`bootstrap-job.yaml`). The operator absorbs -this rather than shelling out to curl: +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). -1. **Confirmed against source** (`internal/api/users.go`'s `handleBootstrap`): - `/api/bootstrap` is single-shot *per install*, not per identity — it - gates on `SELECT COUNT(*) FROM users`, so any call once one user exists - 403s regardless of who's asking, exactly as `charts/terdut-server`'s own +**Every credential the operator holds — the one instance-scoped key per +`TerdutServer`, and one team-scoped key per `TerdutTeam` — lives in a Secret +in the *operator's own* namespace, never in the namespace of the CR it +authenticates for.** Reconciliation happens entirely inside the operator's +controller loop, which is a single Deployment/ServiceAccount already +watching every namespace it's granted (§9); nothing about calling +terdut-server's API on a CR's behalf requires the credential to be +physically located near that CR, and no CR owner (human or otherwise) ever +needs to see, hold, or have RBAC to read a terdut-server credential. This is +a straight simplification of an earlier draft of this section, which mirrored +a shared credential into each consenting namespace instead — that version +conflated "the CR's owner never needs to see this" (true, and preserved +here) with "so the credential must live in the CR's namespace" (a +non-sequitur once you don't need to grant *anyone else* namespace-local +read access). Dropping that assumption also removes an entire class of +complexity: no on-demand mirroring, no garbage-collecting an orphaned copy +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 + `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). This rules out the two-identity plan this - section originally described: there is no way for the operator to get - its *own* bootstrap identity once the chart (or a human) has already - bootstrapped the server. The operator's real first-reconcile flow has to - be: call `/api/bootstrap` **only if nothing has bootstrapped yet** - (an empty-DB fresh install with no chart bootstrap job enabled), and - otherwise obtain its credential through a dedicated, repeatable - service-account endpoint — see the note at the end of this section. - This also surfaces an unhandled race worth designing around explicitly - once §10 is settled: if both the chart's bootstrap Job and this - controller call `/api/bootstrap` against the same fresh install, - exactly one gets the 201 and the other must treat 403 as "someone else - already bootstrapped, go get my own credential the other way" rather - than as an error. -2. The returned API key is written to a generated Secret - (`-operator-credentials`), owner-referenced to the `TerdutServer`, - referenced back from `status.operatorCredentialsSecretRef`. -3. Every other **same-namespace** controller (EscalationRule, DeadmanSwitch, - AlertSource, and any same-namespace `TerdutTeam`) reads that Secret - directly to call the API — never its own credentials. -4. **Cross-namespace `TerdutTeam` never reads the source Secret directly.** - Granting arbitrary consuming namespaces `get`/`list`/`watch` on a Secret - in the server's namespace would mean *any* future workload in that - namespace with Secret-read RBAC could be pointed at it too — `allowedTeams` - (§4.1, §4.6) only authorizes the `TerdutTeam` *kind* to resolve a - reference, not "read this Secret". Instead, the `TerdutServer` controller - (which already holds the real credentials, and whose ServiceAccount is - the only thing with legitimate cross-namespace write access — see §9) - watches `TerdutTeam` objects across the cluster for ones that both name - it and pass its `allowedTeams` check, and mirrors a copy of the - credentials Secret into each such namespace on demand — not proactively - into every namespace a `Selector`/`All` policy *could* admit, only into - ones an actual permitted `TerdutTeam` currently references. The mirror is - named `.-terdut-credentials`, owned - not by an `OwnerReference` (those can't cross namespaces) but tracked in - the `TerdutServer`'s status and cleaned up once no permitted `TerdutTeam` - in that namespace references it any more (revoked `allowedTeams`, or the - last referencing `TerdutTeam` deleted). The remote `TerdutTeam`'s - controller reads only this local mirror, never the original. - - This is one shared mirrored Secret per (server, consuming namespace) - pair, not one per `TerdutTeam` — terdut-server's own API key isn't - team-scoped (§13), so there is nothing finer to hand out; "scoped" here - means scoped by *namespace boundary*, not by team permission. **This is - the design's real weak point, not the mirroring mechanism itself:** the - RBAC argument above (mirror rather than grant broad cross-namespace - Secret-read) is sound on its own terms, but every mirrored copy is - still server-admin-equivalent regardless of which team's namespace it - lands in — `allowedTeams` gates whether a namespace may attach a - `TerdutTeam` at all, it does nothing to bound what that namespace's - copy of the credential can then do to every *other* team on the same - server. This goes away, mirroring included, once team-scoped service- - account tokens exist (see the note below): mint one key per - `TerdutTeam`, directly into its own namespace, owner-referenced to the - CR. No mirror, no shared-per-namespace blast radius — a leaked Secret - compromises exactly one team. -5. **Rotation**: the key is a bearer credential with no expiry modeled - server-side today. This section's original plan — delete the Secret + - the `api_keys` row, let the controller re-bootstrap — **does not work**: - deleting an `api_keys` row doesn't reduce `users` to zero, so the next - `/api/bootstrap` call still 403s (see point 1 above). Until terdut-server - grows a real credential-issuance endpoint, there is no working rotation - story here at all; do not implement this as written. -6. **This entire section is a stand-in for a real scoped service-account - token type, and more than a nice-to-have**: it's the dependency that - makes points 1 and 5 above actually resolvable. Recommended shape (raised - as a terdut-server feature request, tracked in §13): a - `POST /api/service-accounts` (instance-scoped, admin-only, safely - callable repeatedly — unlike `/api/bootstrap`) to create the operator's - own identity and mint its first key, `POST /api/service-accounts/{id}/keys` - to rotate without recreating the account, and `GET /api/service-accounts?name=` - so a 403 from a stale lookup resolves to "fetch my existing account" instead - of an unhandled error. Team-scoped accounts (rather than the one - instance-scoped operator identity) are what let point 4 above mint a key - per `TerdutTeam` instead of mirroring. Until this lands server-side, - treat this section's bootstrap flow as v1-blocking, not v1-shippable — - see §10's note on sequencing. + 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 + **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 + no longer the same namespace as the `TerdutServer` itself. No + `OwnerReference` (those can't cross namespaces); the `TerdutServer`'s + finalizer deletes this Secret directly as part of its own teardown, + the same way it already has to clean up the server-side resources it + created (§5's general finalizer rule extends naturally to this Secret). +3. When a `TerdutTeam` first becomes `Ready` (its `serverRef` resolved, + `allowedTeams` satisfied if cross-namespace), its controller uses the + `TerdutServer`'s instance-scoped credential (read from the operator's own + namespace, resolved via the owner chain in §5) to mint a **team-scoped** + service account for itself: `POST /api/service-accounts` with + `scope: team, teamID: `. The resulting key is written to + its own generated Secret, again in the **operator's own namespace** + (e.g. `.-team-credentials`), referenced from + `TerdutTeam.status.credentialsSecretRef` (§4.2). Same finalizer pattern as + point 2: the `TerdutTeam`'s finalizer deletes this Secret as part of its + own teardown. +4. Every child controller (`TerdutEscalationRule`, `TerdutDeadmanSwitch`, + `TerdutAlertSource`) reads its team's `credentialsSecretRef` — resolved + through its `teamRef` → `TerdutTeam.status` (§5) — and never touches the + instance-scoped credential at all. Since child CRDs stay same-namespace- + as-their-`TerdutTeam` in v1 (§1), and the credential itself lives in the + operator's namespace regardless of where the `TerdutTeam` or its children + are, this works identically whether the `TerdutTeam` is same-namespace or + cross-namespace relative to its `TerdutServer` — there is no separate + cross-namespace case to handle here at all, unlike the mirroring design + this replaced. +5. **Blast radius**: a team-scoped key can only touch its own `team_id`'s + escalation policy, dead-man switches, integrations, schedule and OIDC + group bindings server-side (enforced by terdut-server itself, per + `SERVICE-ACCOUNTS.md`) — compromising one such Secret (e.g. a bug that + leaks operator-namespace Secrets, or an overly broad RBAC grant on that + one namespace) exposes exactly one team, never the whole server. This is + the real fix for what an earlier draft of this section called out as its + weak point (every mirrored copy being server-admin-equivalent); it falls + out of team-scoped credentials existing at all, independent of where + they're stored — the operator-private storage described above closes the + RBAC-footprint half of the problem, team scoping closes the credential- + privilege half. +6. **Rotation**: `POST /api/service-accounts/{id}/keys` mints a new key on + the existing account without recreating it; the old key is revoked via + `DELETE /api/service-accounts/{id}/keys/{keyID}`; the operator's local + Secret is updated in place. No DB-level workaround, no re-triggering a + single-shot endpoint that can't fire twice (which is what made rotation + unworkable under the old `/api/bootstrap`-only design). ## 7. Ownership, status, garbage collection -- Every generated object (Deployment, Service, credentials Secret, webhook - Secret) carries a `metav1.OwnerReference` to the CR that caused it, in the - same namespace — standard GC, no finalizer needed for these (only for the - server-side REST resources, per §5). +- Every generated object that lives in the *same* namespace as the CR that + caused it (Deployment, Service, webhook Secret) carries a standard + `metav1.OwnerReference` — GC handles these, no finalizer needed. The two + credential Secrets from §6 are the one exception: they live in the + operator's own namespace regardless of where their owning CR lives, so + `OwnerReference` doesn't apply (cross-namespace) and cleanup instead runs + through that CR's finalizer directly, alongside the server-side DELETE + it already has to issue (§5). - Status conditions follow the standard `metav1.Condition` shape with at least `Ready` on every kind, plus kind-specific ones (`TerdutServer`: `DatabaseReady`, `Bootstrapped`; children: `Synced`). @@ -511,40 +540,48 @@ documented and tested operationally: ## 9. RBAC - The operator's own ServiceAccount needs, per namespace it's granted: - `get/list/watch/create/update/patch/delete` on `Deployments`, `Services`, - `Secrets` it owns, and `get/list/watch` on `postgresql.acid.zalan.do` - (optional, degrade gracefully if absent per §8), plus cluster-wide - `get/list` on `Namespace` (labels only, for `allowedTeams: {from: Selector}` - evaluation — §4.1, §4.6). + `get/list/watch/create/update/patch/delete` on `Deployments`, `Services` + it owns, and `get/list/watch` on `postgresql.acid.zalan.do` (optional, + degrade gracefully if absent per §8), plus cluster-wide `get/list` on + `Namespace` (labels only, for `allowedTeams: {from: Selector}` evaluation + — §4.1, §4.6). +- **Two different `Secret` scopes, not one — corrected from an earlier draft + of this section.** That earlier draft said `Secret` access was "scoped to + the operator's own namespace only... nowhere else," reasoning that with + every credential held privately in the operator's own namespace (§6) + there was no legitimate reason to touch a `Secret` anywhere else. That was + wrong once §4.5 existed: + - The §6 credential Secrets (one instance-scoped key per `TerdutServer`, + one team-scoped key per `TerdutTeam`) do live in, and are only ever + touched from, the operator's own namespace — + `get/list/watch/create/update/patch/delete` there, nowhere else. The + rest of the original reasoning stands for *these* Secrets specifically: + no human or team's own RBAC is ever granted access to a terdut-server + credential by this design, and the operator itself never needs + cross-namespace access to reach them. + - The §4.5 webhook Secret is different: it's owned by and lives beside + its `TerdutAlertSource`, in that CR's own tenant namespace, not the + operator's. The per-namespace `Role` already granted for + `Deployments`/`Services` in each watched namespace (below) must carry + the same `Secret` verbs there too, or the controller cannot create, + watch, or even detect the loss of (§5) that Secret at all. + - This necessarily widens the operator's footprint in each watched + tenant namespace to "any `Secret` in that namespace," not just the ones + it created — Kubernetes RBAC has no owner-scoped grant finer than the + namespace itself, and the design already accepts this same granularity + for Deployments/Services there. Flagged as an accepted trade-off, not a + silent gap (§13). - No cluster-scoped resources are created by this operator (namespaced CRDs - only, per §1) — a `Role` + `RoleBinding` per watched namespace is - sufficient; a `ClusterRole` is only needed for watching CRDs across all - namespaces, which is the normal Kubebuilder multi-tenant-operator default - and doesn't imply cluster-scoped *managed* resources. -- **The operator's ServiceAccount is the only thing that ever holds - cross-namespace `Secret` write.** It is a single Deployment/binary already - watching every namespace it's granted (the normal Kubebuilder shape), so - mirroring a credentials Secret into a consenting namespace (§6) is not a - new Kubernetes RBAC *boundary* — it's the same ServiceAccount that already - reconciles objects there — but it is new *scope* (`create`/`update` on - `Secrets` cluster-wide rather than only within each object's own - namespace), and should be called out explicitly in the operator's - ClusterRole/RBAC review, not left implicit. No human or team's own - RBAC is ever granted cross-namespace Secret access by this design — - `TerdutServer.spec.allowedTeams` only ever authorizes the operator to act - on a `TerdutTeam`'s behalf, never a person or a workload directly. - **This is a statement about Kubernetes RBAC only, though — it says - nothing about what the mirrored terdut-server credential itself can do - once it's there.** Today that credential is the server-admin-equivalent - bot key (§6), so a compromised or over-read namespace can reach every - team on the server, not just its own; `allowedTeams` bounds who may - *attach*, not what an attached namespace's copy of the credential can - then *do*. That's a real privilege-boundary gap, and it closes once §6's - team-scoped service-account keys exist and mirroring is dropped in favor - of a key minted directly per `TerdutTeam`. + only, per §1) — a `Role` + `RoleBinding` per watched (tenant) namespace is + sufficient for Deployments/Services/the webhook `Secret`/the optional + Zalando CRD, plus a separate `Role` + `RoleBinding` in the operator's own + namespace for the §6 credential Secrets; a `ClusterRole` is only needed + for watching CRDs across all namespaces (the normal Kubebuilder + multi-tenant-operator default) — none of the `Secret` access above needs + to be cluster-scoped. - terdut-server's own RBAC is unaffected — the operator talks to it purely - over HTTP with the bot user's API key, never via the Kubernetes API for - app-level state. + over HTTP with service-account API keys (§6), never via the Kubernetes + API for app-level state. ## 10. Relationship to `charts/terdut-server` @@ -623,6 +660,10 @@ conflict Kubernetes operators exist to avoid. Proposed path: `TerdutDeadmanSwitch`, `TerdutAlertSource`) — only `TerdutTeam.serverRef` crosses namespaces in v1 (§2, §4.2, §4.6); these stay same-namespace as their `TerdutTeam` until a real need for splitting them out shows up. +- Narrower-than-namespace RBAC for the §4.5 webhook Secret (Kubernetes RBAC + has no owner-scoped grant below the namespace itself, per §9) — revisit + if the widened per-tenant-namespace `Secret` access proves too broad in + practice. - Gitops-managed team *membership* (see §4.2). - Automatic Deployment restart on upstream Postgres credential rotation. - Admission webhooks / CEL-only validation limits (e.g. verifying a