Skip to content

feat: mount server/peer cert secrets into the etcd container#375

Closed
xrl wants to merge 1 commit into
etcd-io:mainfrom
xrl:pr/T2-mount-cert-secrets
Closed

feat: mount server/peer cert secrets into the etcd container#375
xrl wants to merge 1 commit into
etcd-io:mainfrom
xrl:pr/T2-mount-cert-secrets

Conversation

@xrl

@xrl xrl commented Jun 17, 2026

Copy link
Copy Markdown

What

Mount the already-provisioned server and peer cert secrets into the etcd container.

When spec.tls is set, the operator already provisions the server/peer
Certificates and adds their secret Volumes to the StatefulSet pod spec
(createOrPatchStatefulSet). However, the etcd container never mounts those
volumes, so the certificates are present in the pod spec but invisible to
etcd
. This PR adds the missing VolumeMounts.

Change

  • Add VolumeMounts on the etcd container for the server and peer cert secrets:
    • server-secret/etc/etcd/server-tls/
    • peer-secret/etc/etcd/peer-tls/
  • Gated on spec.tls != nil, and appended independently of StorageSpec so
    a TLS cluster without persistent storage still receives the cert mounts.
  • The data-dir mount inside the StorageSpec != nil branch now appends
    rather than assigns the VolumeMounts slice, so the cert mounts are preserved
    when both TLS and storage are configured.
  • Volume names reuse the existing cert Volume names; paths are factored into
    constants so the follow-up TLS-flags change references exactly the same paths.

Why this is safe / why now

Mounting cert files with no TLS flags is a no-op: etcd ignores files it is
not told to use. This keeps the change independently shippable and the cluster
always bootable. It is the first step of wiring TLS into the etcd data path; the
flags that consume these mount paths (--cert-file, --peer-cert-file, …,
plus the https:// scheme flip) land in a separate follow-up PR so each step
stays independently reviewable.

Migration note

The StatefulSet pod template is immutable for existing clusters, so this takes
effect for new or recreated clusters. TLS is a create-time property — a
running cleartext cluster should be migrated to a fresh TLS cluster rather than
flipped in place.

Test

Extended the controller unit tests with TestCreateOrPatchStatefulSetTLSCertMounts,
which renders the StatefulSet through the real createOrPatchStatefulSet path
(controller-runtime fake client) and asserts both cert VolumeMounts are present
with the correct names and mount paths when spec.tls != nil, absent when nil,
and that the data-dir mount coexists with the cert mounts when StorageSpec is
also set. Verified the test fails against the unmodified code and passes after
the change.

Refs #371


Related work — etcd TLS data-path enablement

The operator provisions cert-manager certificates but currently runs etcd in cleartext. This PR is one step of a small, dependency-ordered series wiring TLS through the data path ( marks this PR):

Step Change Issue PR
T0 stop swallowing the client-certificate error #370 #374
T2 mount server/peer cert secrets into the etcd container #371 #375
T3 emit etcd TLS flags + https endpoints (single scheme source-of-truth) #372 pending
T4 thread an optional *tls.Config through the operator's etcd client #373 pending
kind stress/e2e harness (the TLS end-to-end test will extend it) #369

The server and peer Certificate secrets are already provisioned and their
Volumes are added to the StatefulSet pod spec when spec.tls is set, but the
etcd container never mounts them, so the certs are invisible to etcd.

Add VolumeMounts on the etcd container for the server and peer cert secrets
(/etc/etcd/server-tls/ and /etc/etcd/peer-tls/), gated on spec.tls != nil and
appended independently of StorageSpec so a TLS cluster without persistent
storage still receives the mounts. The data-dir mount in the StorageSpec branch
now appends rather than assigns, preserving the cert mounts when both are set.

This is a safe no-op without the TLS flags (etcd ignores unused files) and is
the first step of wiring TLS into the etcd data path; the flags that reference
these paths land in a follow-up.

Note: the StatefulSet pod template is immutable for existing clusters, so this
takes effect for new or recreated clusters; TLS is a create-time property and a
live cleartext cluster should be migrated to a new TLS cluster, not flipped in
place.

Refs etcd-io#371

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Xavier Lange <xrlange@gmail.com>
@k8s-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xrl
Once this PR has been reviewed and has the lgtm label, please assign hakman for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot

Copy link
Copy Markdown

Hi @xrl. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@xrl

xrl commented Jun 18, 2026

Copy link
Copy Markdown
Author

Superseding this PR. The TLS work was reshaped into independent spec.tls.{peer,client} surfaces (#376), and the per-surface cert VolumeMounts this PR added now live there — server secret mounted iff the client surface is set, peer secret iff peer is set, appended (not assigned) so they coexist with the storage data-dir mount. The standalone single-toggle approach here is subsumed by that breaking alpha API change, so closing in favor of #376. Leaving the branch in place for reference. Thanks!

@xrl xrl closed this Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants