feat(tls): support handing TLS over from BYO Secrets to operator-managed certs - #349
feat(tls): support handing TLS over from BYO Secrets to operator-managed certs#349Timofei Larkin (lllamnyp) wants to merge 4 commits into
Conversation
…ged certs
A cluster installed with user-provided TLS Secrets had no route onto
operator-managed cert-manager issuance: spec.tls was CEL-immutable
post-create, so the only way to change the source of the material was to
delete and recreate the cluster — i.e. to destroy its data.
Permit exactly one post-create change to spec.tls: client.serverSecretRef →
client.certManager, and peer.secretRef → peer.certManager. Everything else
about the subtree stays frozen. The handover is one-way, may not flip whether
client mTLS is in effect, and cannot turn a plane on or off.
The operator drives the move itself:
1. It emits the new Certificates, refusing to adopt any that another
controller already owns — the realistic case, since a chart installing a
cluster named `etcd` emits `etcd-peer` and `etcd-server` under exactly
the names the operator wants. Adopting would mean two controllers
reconciling one object forever, and would repoint a live cluster at
material the operator never issued. It reports TLSHandover=False/Blocked
and names the object instead.
2. It waits until every Secret the new spec names carries the keys etcd
will be started against. Repointing before that strands the cluster on a
Secret that does not exist, with the old material already gone from its
spec.
3. It repoints every member in one pass and lets the member controller
rebuild their Pods together.
Step 3 is a simultaneous roll, not a rolling restart, and that is the whole
design. The new material is signed by a different CA, so an old member and a
new one cannot authenticate to each other at all: every intermediate state of
a staggered roll is a split cluster. Rolling one at a time would stretch the
outage across N pod startups instead of one. Brief downtime is the accepted
cost.
Progress is reported on a new TLSHandover condition rather than as a reason on
Available/Degraded/Progressing, which updateStatus rewrites from quorum health
on every pass. A conflict is deliberately not fatal to the reconcile either:
the cluster keeps serving on the material it already holds, and freezing the
loop would leave its readyMembers and health conditions stale — a cluster that
silently stops reporting is worse than one that reports it cannot converge.
TLS is also now the one mirrored member field that is re-templated after
creation. Storage, Resources and Version are frozen per member on purpose;
holding the wrong TLS material does not degrade a member, it isolates it.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
api-change
controllers
documentation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds a one-way BYO Secret to cert-manager TLS handover, granular CEL validation, ownership-conflict handling, handover status conditions, member and Pod reconciliation, tests, and operational documentation. ChangesTLS handover contract and admission validation
TLS material ownership and reconcile wiring
TLS handover readiness and member repointing
Member Pod TLS refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EtcdCluster
participant EtcdClusterReconciler
participant CertManagerSecrets
participant EtcdMembers
participant MemberPods
EtcdCluster->>EtcdClusterReconciler: update TLS references
EtcdClusterReconciler->>CertManagerSecrets: check certificate material
CertManagerSecrets-->>EtcdClusterReconciler: return readiness or missing keys
EtcdClusterReconciler->>EtcdMembers: repoint stale TLS references
EtcdMembers->>MemberPods: expose updated Secret references
EtcdClusterReconciler->>EtcdCluster: update TLSHandover condition
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A conflict short-circuited on the stale-member check, so a cluster whose members were already aligned reported TLSHandover=Complete while the operator still could not own its own Certificates. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/concepts.md (1)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Four CEL rules" count.
The table this sentence introduces already lists well over a dozen rules, and this PR adds two more (
tls.client/tls.peeradd/remove and the handover-exception rules) to the same table, widening the drift further. Since the diff already touches this section, worth dropping the specific count or updating it.✏️ Suggested fix
-Four CEL `x-kubernetes-validations` rules on `EtcdClusterSpec` are evaluated at admission time. +Several CEL `x-kubernetes-validations` rules on `EtcdClusterSpec` are evaluated at admission time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/concepts.md` at line 180, Update the CEL validation description in the EtcdClusterSpec documentation to remove the stale “Four” count, while preserving the existing Kubernetes version guidance and references to quantity(). Use wording that accurately applies to all x-kubernetes-validations rules listed in the table.docs/operations.md (2)
218-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language hint to the fenced example block.
Flagged by markdownlint (MD040) — the
TLSHandover=False ...example fence has no language specified.✏️ Suggested fix
-``` +```text TLSHandover=False reason=Blocked cannot take over Certificate "etcd-peer": it exists but another controller owns it …</details> As per static analysis hints (markdownlint-cli2, MD040 fenced-code-language). <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/operations.mdaround lines 218 - 221, Update the fenced example block
containing the TLSHandover output to specify the text language, using the
existing content unchanged.</details> <!-- cr-comment:v1:b4bb2e609ffeb826173948e6 --> _Source: Linters/SAST tools_ --- `214-223`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **"Free up the names" omits the operator-client Certificate.** Per [Census of certs](concepts.md#census-of-certs) the operator can emit up to three Certificates (server, operator-client, peer). This runbook step only names `<cluster>-server` and `<cluster>-peer` — a chart-collision on `<cluster>-operator-client` (relevant whenever the handover target has mTLS on) would hit the same `Blocked` condition but isn't mentioned here, which could leave a reader stuck if that's the one that collides. As per coding guidelines the docs should stay consistent with the operator's actual behavior described in concepts.md. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/operations.mdaround lines 214 - 223, The “Free up the names” runbook
step omits the operator-client Certificate. Update the Certificate list and
collision example in this section to include -operator-client alongside
-server and -peer, matching the operator behavior documented
in the Census of certs section; preserve the existing Secret retention and
reissue guidance.</details> <!-- cr-comment:v1:e9bc5fbad5186870f086dc94 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Nitpick comments:
In@docs/concepts.md:
- Line 180: Update the CEL validation description in the EtcdClusterSpec
documentation to remove the stale “Four” count, while preserving the existing
Kubernetes version guidance and references to quantity(). Use wording that
accurately applies to all x-kubernetes-validations rules listed in the table.In
@docs/operations.md:
- Around line 218-221: Update the fenced example block containing the
TLSHandover output to specify the text language, using the existing content
unchanged.- Around line 214-223: The “Free up the names” runbook step omits the
operator-client Certificate. Update the Certificate list and collision example
in this section to include -operator-client alongside -server
and -peer, matching the operator behavior documented in the Census of
certs section; preserve the existing Secret retention and reissue guidance.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `25d7e38d-d503-449b-b204-e78fca3ff942` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9c5d8961fb8dc8c62be4136c50fd6679490d1a82 and a251e5bd44f1763e09bd81ac7f1b071c36bfdfc7. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `README.md` * `api/v1alpha2/cel_validation_test.go` * `api/v1alpha2/etcdcluster_types.go` * `charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdclusters.yaml` * `controllers/etcdcluster_controller.go` * `controllers/etcdcluster_controller_test.go` * `controllers/etcdmember_controller.go` * `controllers/tls_handover.go` * `controllers/tls_handover_test.go` * `docs/concepts.md` * `docs/installation.md` * `docs/operations.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Name the operator-client Certificate in the collision step — with client mTLS on the operator emits three Certificates, and a chart collision on that one hits the same Blocked path but was not mentioned. Note that only the first collision is reported per pass. Also drop the stale "Four CEL rules" count, which this branch widens, and label the example fence. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
tlsMountsOutOfDate keyed off the operator's own volume names (tls-client / tls-peer). A member adopted in place by etcd-migrate keeps the legacy StatefulSet's Pod — adoption rewrites labels and owner refs but never restarts it — so its volumes carry different names while its member spec does name TLS Secrets. The predicate read that as drift and ensurePod would have deleted every adopted Pod on the first reconcile after a migration, which is exactly the outcome in-place adoption exists to avoid. Treat a Pod carrying none of the operator's TLS volumes as out of scope: it is either plaintext or a shape the operator did not build. Adopted members keep their material until they are rolled onto the operator's own Pod shape. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
On the
|
|
Parking this as a draft. The TLS-reload research (what etcd can and cannot hot-reload) shows the simultaneous roll this PR is built around is both unnecessary and, on memory-backed clusters, unrecoverable:
Two further issues in the same area: pointing Rather than patch this incrementally, the rollout mechanism is being designed generally: The parts of it that stand on their own regardless — the CEL relaxation, the fail-closed conflict guard on foreign-owned Certificates, gating the repoint on material actually being populated, and not rebuilding Pods the operator did not author — will carry over. Note also that the red |
1 participant
Problem
A cluster installed with user-provided TLS Secrets has no route onto operator-managed cert-manager issuance.
spec.tlsis CEL-immutable post-create, so the only way to change where the material comes from is delete-and-recreate — which for etcd means destroying the data.That is a real dead end, not a theoretical one. Charts that predate the v1alpha2 operator issue their own
Certificates and hand the Secrets to the cluster viaserverSecretRef/secretRef. Those charts also tend to issue SANs that the operator'sGenerateNamed member names do not match, and there is currently no supported way to move such a cluster onto material the operator generates correctly.What this adds
Exactly one permitted post-create change to
spec.tls:client.serverSecretRef→client.certManagerpeer.secretRef→peer.certManagerEverything else about the subtree stays frozen. The handover is one-way, it may not flip whether client mTLS is in effect, and it cannot turn a plane on or off. Adding or removing
tls,tls.clientortls.peeris still rejected, as iscertManager→secretRef.The operator drives the move end to end:
Progress is reported on a new
TLSHandovercondition:AwaitingMaterial→RollingMembers→Complete, orBlocked.Three decisions worth reviewing
The roll is simultaneous, not rolling. The new material is signed by a different CA, so an old member and a new member cannot authenticate to each other at all — every intermediate state of a staggered roll is a split, quorum-less cluster. A one-at-a-time roll would not avoid the outage, it would stretch it across N pod startups instead of one. So all members are repointed together and come back on consistent material. A brief outage is the accepted cost of the operation.
Name conflicts fail closed. The operator's targets are
<cluster>-server/<cluster>-peer. For a cluster namedetcd— the common case — those are byte-identical to what a chart already emits, and Helm/Flux do not setownerReferenceson chart resources, so ownership is a clean discriminator. Adopting the object would mean this operator and the chart's controller reconciling one object forever, and would silently repoint a live cluster at TLS material the operator never issued. Instead it reports:The unblock is to stop the chart emitting it. cert-manager leaves the Secret behind when its Certificate is deleted, so the operator's replacement Certificate reissues into the same Secret with no gap.
A conflict does not abort the reconcile. The cluster is still serving on the material it holds, so the loop continues to
updateStatusand its health reporting stays live. Bailing out early would freezereadyMembersand the health conditions at whatever they were — a cluster that silently stops reporting is worse than one that reports it cannot converge.Notable behaviour change
TLS becomes the one mirrored member field that is re-templated after creation.
Storage,ResourcesandVersionare deliberately frozen per member — existing members are not re-templated when the cluster spec moves. TLS is different in kind: holding the wrong material does not degrade a member, it isolates it completely. Correspondingly,ensurePodnow tears down a Pod whose mounted TLS Secret names no longer match its member spec (Pod volumes are immutable, so a rebuild is the only route). Content changes — a cert-manager renewal writing a fresh leaf into the same Secret — deliberately do not trigger a rebuild.Testing
make manifests generate fmt vetclean;go test ./api/... ./controllers/...green.New coverage:
api/v1alpha2/cel_validation_test.go): handover accepted on both planes; reverse rejected; a handover that silently drops client mTLS rejected; the mTLS-preserving form accepted; adding atls.peersubtree post-create still rejected.controllers/tls_handover_test.go): no member is touched while material is missing; a Secret that exists but is missingtls.keycounts as not ready; all members repointed in one pass; each member gets its own copy of the derived TLS view; a conflict is reported without touching members and without taking over the reconcile; no condition appears on a cluster that never drifted; the in-flight condition settles toComplete.ensureCertificate): refuses a foreign-owned Certificate; the pre-existing create-once test was updated to seed an operator-owned Certificate, which is what makes it the "ours, leave it alone" case rather than the conflict case.Docs
Runbook at
docs/operations.md#handing-tls-over-to-the-operator, including the condition table and what to do when it stalls.README.md,docs/concepts.md(validation table and TLS section) anddocs/installation.mdall claimed the subtree was wholly immutable; corrected.Summary by CodeRabbit
New Features
Bug Fixes
Documentation