niklas/terdut-operator
1
0
Fork 0
Code Issues 3 Pull Requests Actions Packages Projects Releases Wiki Activity

Housekeeping: gitignore, and finish the webhook-Secret-loss/RBAC fix

Browse Source 
- Add .gitignore (build artifacts, editor swapfiles, envtest testbin).
- Remove the stray .DESIGN.md.swp that was sitting untracked in the repo.
- Carries the DESIGN.md §5/§9/§13 edits from the secret-loss discussion:
  fail-closed (not self-healed) TerdutAlertSource webhook Secret loss, and
  the corrected RBAC section (the webhook Secret lives in the CR's tenant
  namespace, not the operator's own namespace as an earlier draft claimed).
This commit is contained in:
Niklas Ye
2026-09-30 19:16:37 +02:00
parent ee39b8e668
commit 1feffd791a
2 changed files with 186 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files
DESIGN.md
+172 -131
View File
@@ -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.<serverSideID>` 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
(`<name>-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 `<serverRef.name>.<serverRef.namespace>-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. `<serverRef.namespace>.<serverRef.name>-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: <status.teamID>`. The resulting key is written to
its own generated Secret, again in the **operator's own namespace**
(e.g. `<teamNamespace>.<teamName>-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