feat(openbao): run three raft replicas so a node drain is not an outage #646

Merged
k.renner merged 1 commits from openbao-ha-3-replicas into master 2026-08-26 20:51:03 +00:00
Member

OpenBao ran a single replica, so every node drain took the whole secret store down with it. Today the k8s-1 Talos upgrade sealed it, the ClusterSecretStore went InvalidProviderConfig, and it stayed that way for two hours because nothing was watching.

Three, not two. Raft needs a quorum and the quorum of two is two — a two-replica cluster tolerates no failures and would stop the moment one pod is sealed or rescheduled, which is worse than the single replica it replaces. Three tolerates one.

What this buys: a drain seals the replica on that node, and raft elects a new leader from the remaining standbys within seconds. The service does not go down.

What it does not buy, stated plainly because it is easy to assume otherwise: a sealed server can neither serve requests nor act as a standby, so that replica stays out until someone unseals it. Scaling removes the outage, not the manual unseal.

For rolling node maintenance this matters — a Talos upgrade drains all three nodes in turn, so unseal each replica as it comes back, or the third drain takes the last unsealed one and quorum is lost anyway.

retry_join is what makes a scaled replica join the existing cluster rather than come up isolated and uninitialized; without it each new pod would need a manual bao operator raft join. All three peers are listed on every node — the stanza is a list of possible leaders, a node ignores its own entry, and listing all three keeps the config identical across replicas regardless of which pod currently leads. Addressed through the headless service so each entry resolves to one specific pod.

Snapshot job moves from the plain service to openbao-active. With one replica the plain service could only hit the one pod; with three it selects any of them, and a raft snapshot has to be taken against the leader.

No affinity added — the chart already applies a required podAntiAffinity on kubernetes.io/hostname, verified against the running StatefulSet, so replicas spread across the three nodes on their own.

Still missing and not in this PR: there is no OpenBao alerting at all — no rule, no ServiceMonitor. That is why today went unnoticed for two hours, and it matters more once a sealed replica is a quiet degradation rather than a visible outage.

OpenBao ran a single replica, so every node drain took the whole secret store down with it. Today the k8s-1 Talos upgrade sealed it, the ClusterSecretStore went `InvalidProviderConfig`, and it stayed that way for two hours because nothing was watching. **Three, not two.** Raft needs a quorum and the quorum of two is two — a two-replica cluster tolerates no failures and would stop the moment one pod is sealed or rescheduled, which is worse than the single replica it replaces. Three tolerates one. **What this buys:** a drain seals the replica on that node, and raft elects a new leader from the remaining standbys within seconds. The service does not go down. **What it does not buy,** stated plainly because it is easy to assume otherwise: a sealed server can neither serve requests nor act as a standby, so that replica stays out until someone unseals it. Scaling removes the outage, not the manual unseal. For rolling node maintenance this matters — a Talos upgrade drains all three nodes in turn, so unseal each replica as it comes back, or the third drain takes the last unsealed one and quorum is lost anyway. **retry_join** is what makes a scaled replica join the existing cluster rather than come up isolated and uninitialized; without it each new pod would need a manual `bao operator raft join`. All three peers are listed on every node — the stanza is a list of possible leaders, a node ignores its own entry, and listing all three keeps the config identical across replicas regardless of which pod currently leads. Addressed through the headless service so each entry resolves to one specific pod. **Snapshot job** moves from the plain service to `openbao-active`. With one replica the plain service could only hit the one pod; with three it selects any of them, and a raft snapshot has to be taken against the leader. **No affinity added** — the chart already applies a required `podAntiAffinity` on `kubernetes.io/hostname`, verified against the running StatefulSet, so replicas spread across the three nodes on their own. Still missing and not in this PR: there is no OpenBao alerting at all — no rule, no ServiceMonitor. That is why today went unnoticed for two hours, and it matters more once a sealed replica is a quiet degradation rather than a visible outage.
claude-bot added 1 commit 2026-08-26 20:50:03 +00:00
feat(openbao): run three raft replicas so a node drain is not an outage
ci/woodpecker/pr/ci-scripts-test Pipeline was successful
ci/woodpecker/pr/ci-manifests-test Pipeline was successful
ci/woodpecker/pr/pr-review Pipeline was successful
0809823511
OpenBao ran a single replica. Every node drain took the whole secret store
down with it: today the k8s-1 Talos upgrade sealed it, the ClusterSecretStore
went InvalidProviderConfig, and it stayed that way for two hours because
nothing was watching.

Three, not two. Raft needs a quorum and the quorum of two is two, so a
two-replica cluster tolerates no failures — it would stop the moment one pod
is sealed or rescheduled, which is worse than the single replica it replaced.
Three tolerates one.

What this buys: a drain seals the replica on that node, and raft elects a new
leader from the remaining standbys within seconds. The service does not go
down.

What it does not buy, stated plainly because it is easy to assume otherwise: a
sealed server can neither serve requests nor act as a standby, so that replica
stays out of the cluster until someone unseals it. Scaling removes the outage,
not the manual unseal. For rolling node maintenance the consequence matters —
a Talos upgrade drains all three nodes in turn, so unseal each replica as it
comes back, or the third drain takes the last unsealed one and quorum is lost
anyway.

retry_join is what makes a scaled replica join the existing cluster rather
than come up isolated and uninitialized; without it each new pod would need a
manual `bao operator raft join`. All three peers are listed on every node —
the stanza is a list of possible leaders, a node ignores its own entry, and
listing all three keeps the config identical across replicas regardless of
which pod currently leads. Addressed through the headless service so each
entry resolves to one specific pod.

The snapshot job moves from the plain service to openbao-active. With one
replica the plain service could only ever hit the one pod; with three it
selects any of them, and a raft snapshot has to be taken against the leader.

No affinity was added: the chart already applies a required podAntiAffinity on
kubernetes.io/hostname, so the replicas spread across the three nodes on their
own — verified against the running StatefulSet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
k.renner merged commit 71d6bd3724 into master 2026-08-26 20:51:03 +00:00
review-bot left a comment
Member

🤖 Automated review — model CLI default (complexity: complex), commit 08098235119b3927a17493db99d2c57456685586.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.


Summary

Scales OpenBao from 1 to 3 raft replicas (helmrelease.yaml) and repoints the nightly snapshot CronJob from the plain openbao service to openbao-active (snapshot-cronjob.yaml). The HCL/YAML changes match the well-established Vault/OpenBao raft-HA pattern (retry_join list, headless service for peer addressing, an -active service for leader-only traffic) and are internally consistent with the rest of the file (tls_disable=1 matches the plain-HTTP retry_join addresses; service_registration "kubernetes" {} — already present — is exactly what makes the -active service's leader label work). I found no blocking defects in the changed manifests themselves. The main gap is operational: two runbooks that describe exactly the scenarios this PR is about (cold rebuild, pod restart/unseal) still describe a single-node cluster.

🟡 Suggestions

  • Two runbooks are now out of date and both are load-bearing for this exact change.

    • kubernetes/MIGRATION-sops-openbao.md:32 still says "single-node raft," and the init/unseal walkthrough at lines 39–54 only ever execs into openbao-0. It never mentions that openbao-1/openbao-2 also need bao operator unseal after they retry_join — under key-shares=1, key-threshold=1 the same key unseals every node, but each node still starts sealed and needs the command run against it individually.
    • kubernetes/RUNBOOK-cold-rebuild.md:144-173, Step 4, is more concerning: it waits for openbao-0, restores the snapshot into openbao-0, and unseals only openbao-0, then verifies success via ClusterSecretStore going Ready. That verification will pass with only 1/3 pods unsealed — the plain openbao service (what the ClusterSecretStore targets, per this same doc's phase-2 example) only routes to Ready/unsealed pods, so ESO recovers as soon as the leader is up. A rebuild operator following this runbook literally will get a functioning-looking cluster that is silently back to the single-point-of-failure this PR set out to fix, with openbao-1/openbao-2 sitting sealed and out of quorum indefinitely — the same class of "nothing was watching" gap called out in the PR description, just relocated to the rebuild path instead of the drain path.
    • Recommend updating both docs (in this PR or as an immediate fast-follow, given the repo's own convention of MIGRATION-sops-openbao.md/RUNBOOK-cold-rebuild.md being the authoritative operational references).
  • Service-name assumptions (openbao-internal, openbao-active) are not verified against a rendered chart. They match the standard vault-helm/openbao-helm convention (headless <fullname>-internal for per-pod DNS, <fullname>-active selecting the raft leader via the label service_registration "kubernetes" maintains), and the existing service_registration "kubernetes" {} stanza (unchanged by this diff) is the piece that makes the -active selector work — so this is plausible, and the PR body says it was checked against the running StatefulSet. I don't have helm/kubectl or a copy of the chart in this environment to render it and confirm the exact service names myself. Given that a wrong name here fails silently (the CronJob would just get "no such host" once a month), it's worth a one-time confirmation via kubectl get svc -n openbao or helm template before/after this merges.

🟢 Nits

  • kubernetes/infrastructure/network-policies/openbao-ns.yaml:9-10's comment says the snapshot CronJob talks to "the openbao Service" — after this PR it's openbao-active. Not touched by this diff and not a functional problem (the policy allows all intra-namespace traffic regardless of target service), just a comment now slightly stale.

Tests

No automated tests exist or are expected for Helm values/HCL config in this GitOps repo. .woodpecker/ci-manifests-test.yml's kustomize-build step will validate that the openbao kustomization still assembles, but it does not render the Helm chart (the HelmRelease is applied as-is; rendering happens at reconcile time via helm-controller), so it cannot catch a wrong retry_join/service-name value or malformed HCL inside the config: block — that class of error would only surface post-merge, in-cluster. This is a pre-existing gap in CI coverage, not something this PR introduces, but it's the reason the service-name assumption above is worth a manual check rather than treating CI green as confirmation.

Recommendation

Looks reasonable — no blocking issues; comments above (especially the two stale runbooks) are worth addressing before or shortly after merge.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `08098235119b3927a17493db99d2c57456685586`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Scales OpenBao from 1 to 3 raft replicas (`helmrelease.yaml`) and repoints the nightly snapshot CronJob from the plain `openbao` service to `openbao-active` (`snapshot-cronjob.yaml`). The HCL/YAML changes match the well-established Vault/OpenBao raft-HA pattern (retry_join list, headless service for peer addressing, an `-active` service for leader-only traffic) and are internally consistent with the rest of the file (`tls_disable=1` matches the plain-HTTP `retry_join` addresses; `service_registration "kubernetes" {}` — already present — is exactly what makes the `-active` service's leader label work). I found no blocking defects in the changed manifests themselves. The main gap is operational: two runbooks that describe exactly the scenarios this PR is about (cold rebuild, pod restart/unseal) still describe a single-node cluster. ## 🟡 Suggestions - **Two runbooks are now out of date and both are load-bearing for this exact change.** - `kubernetes/MIGRATION-sops-openbao.md:32` still says "single-node raft," and the init/unseal walkthrough at lines 39–54 only ever execs into `openbao-0`. It never mentions that `openbao-1`/`openbao-2` also need `bao operator unseal` after they `retry_join` — under `key-shares=1, key-threshold=1` the same key unseals every node, but each node still starts sealed and needs the command run against it individually. - `kubernetes/RUNBOOK-cold-rebuild.md:144-173`, Step 4, is more concerning: it waits for `openbao-0`, restores the snapshot into `openbao-0`, and unseals only `openbao-0`, then verifies success via `ClusterSecretStore` going Ready. That verification will pass with only 1/3 pods unsealed — the plain `openbao` service (what the `ClusterSecretStore` targets, per this same doc's phase-2 example) only routes to Ready/unsealed pods, so ESO recovers as soon as the leader is up. A rebuild operator following this runbook literally will get a functioning-looking cluster that is silently back to the single-point-of-failure this PR set out to fix, with `openbao-1`/`openbao-2` sitting sealed and out of quorum indefinitely — the same class of "nothing was watching" gap called out in the PR description, just relocated to the rebuild path instead of the drain path. - Recommend updating both docs (in this PR or as an immediate fast-follow, given the repo's own convention of `MIGRATION-sops-openbao.md`/`RUNBOOK-cold-rebuild.md` being the authoritative operational references). - **Service-name assumptions (`openbao-internal`, `openbao-active`) are not verified against a rendered chart.** They match the standard vault-helm/openbao-helm convention (headless `<fullname>-internal` for per-pod DNS, `<fullname>-active` selecting the raft leader via the label `service_registration "kubernetes"` maintains), and the existing `service_registration "kubernetes" {}` stanza (unchanged by this diff) is the piece that makes the `-active` selector work — so this is plausible, and the PR body says it was checked against the running StatefulSet. I don't have `helm`/`kubectl` or a copy of the chart in this environment to render it and confirm the exact service names myself. Given that a wrong name here fails silently (the CronJob would just get "no such host" once a month), it's worth a one-time confirmation via `kubectl get svc -n openbao` or `helm template` before/after this merges. ## 🟢 Nits - `kubernetes/infrastructure/network-policies/openbao-ns.yaml:9-10`'s comment says the snapshot CronJob talks to "the openbao Service" — after this PR it's `openbao-active`. Not touched by this diff and not a functional problem (the policy allows all intra-namespace traffic regardless of target service), just a comment now slightly stale. ## Tests No automated tests exist or are expected for Helm values/HCL config in this GitOps repo. `.woodpecker/ci-manifests-test.yml`'s `kustomize-build` step will validate that the `openbao` kustomization still assembles, but it does not render the Helm chart (the `HelmRelease` is applied as-is; rendering happens at reconcile time via helm-controller), so it cannot catch a wrong `retry_join`/service-name value or malformed HCL inside the `config:` block — that class of error would only surface post-merge, in-cluster. This is a pre-existing gap in CI coverage, not something this PR introduces, but it's the reason the service-name assumption above is worth a manual check rather than treating CI green as confirmation. ## Recommendation Looks reasonable — no blocking issues; comments above (especially the two stale runbooks) are worth addressing before or shortly after merge. <!-- pr-review:sha=08098235119b3927a17493db99d2c57456685586 -->
Sign in to join this conversation.