feat(api)!: configure peer and client TLS independently#376
Open
xrl wants to merge 8 commits into
Open
Conversation
etcd has three TLS surfaces -- peer<->peer, client->server, and the operator's own client identity -- but the alpha CRD conflated all three behind a single `spec.tls` toggle and a single shared issuer, so a user could neither serve peer TLS without client TLS (or vice versa) nor relax mutual client-cert auth per surface. The single predicate also forced `--client-cert-auth`/`--peer-client-cert-auth` on whenever TLS was set. Reshape `spec.tls` into two optional, independent surfaces -- `peer` and `client` -- each carrying its own provider, issuer, and `clientCertAuth` policy (default true). A nil surface serves/dials that surface in cleartext; both nil is fully cleartext and byte-identical to the pre-TLS path. The operator's own client identity follows the `client` surface. This is a clean break on the alpha API (no conversion webhook, which would re-introduce the conflation). The dead `caBundleSecret` knob is removed (type + cert interface; it was read nowhere). `IssuerGroup` is added to the cert-manager provider config (empty => cert-manager.io) so non-default issuer groups can be targeted. Threading (single source of truth, per surface): - peerScheme/clientScheme drive peer vs client URLs and flags independently - defaultArgs emits the server flag group iff client set, the peer flag group iff peer set, and each --*client-cert-auth gated on its surface - cert mounts: server secret iff client set, peer secret iff peer set (append-not-assign so they coexist with the storage data-dir mount) - cert provisioning: server+operator-client certs from the client surface, peer cert from the peer surface - etcdutils MemberList/ClusterHealth/AddMember/PromoteLearner/RemoveMember take an optional *tls.Config (nil => cleartext); the operator builds it from the client surface only, with a missing ca.crt now an explicit error Anti-misconfiguration guardrails: - apply-time CEL XValidation on each surface: provider/providerCfg coherence, mTLS-requires-a-CA, issuerKind/provider enums - a reconcile-time validateTLS backstop re-checks the spec-only rules and requeues on an invalid spec (for apiservers that don't enforce CEL) Also: the cert-site log.Printf calls move to the reconcile-scoped logger, and the "running without TLS" error-level log is demoted to Info (cleartext is a supported mode). The sample is rewritten to the two-tier shared-CA issuer pattern a multi-member peer-mTLS cluster actually needs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
GetCertificateConfig now echoes IssuerRef.Group into ExtraConfig under the issuerGroup key. The TestCertManagerProvider "Get certificate config" assertion built its expected config without that key, so reflect.DeepEqual failed. Add the empty-string issuerGroup to the expected config (no group set => cert-manager leaves Group == ""). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
A peer-TLS cluster could not form past the seed member. With
--peer-client-cert-auth, etcd verifies a joining peer's source IP against
its cert SANs via isHostInDNS (client/pkg transport), which forward-resolves
the cluster's headless-service DNS name. A joining member is not Ready until
it has actually joined the cluster, so without publishNotReadyAddresses its
pod IP is absent from the service's A record, the peer cert SAN check fails
("tls: <ip> does not match any of DNSNames"), the peer handshake is rejected
with EOF, and the joining member crashloops -- deadlocking the cluster at one
member.
Set PublishNotReadyAddresses=true on the headless service so not-yet-Ready
members are resolvable and peer mTLS can complete. Cleartext peer traffic does
not hit this verification path, so non-TLS clusters are unaffected.
Discovered by the T6 multi-member TLS e2e (first live exercise of multi-member
peer mTLS); verified on kind: 3-member both-surface TLS cluster reaches 3
voting members with this change and crashloops at 1 without it.
Belongs to: pr/tls-independence
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Xavier Lange <xrlange@gmail.com>
…aviour
Fill the two genuine documentation gaps the reshape left:
- spec.tls is effectively create-time; toggling it on a running cluster
drops quorum. Document the create-new-not-flip caveat on the
EtcdClusterSpec.TLS field where a spec author hits it (regenerated CRD).
- buildClientTLSConfig sets no ServerName, so verification relies on the
dialed pod FQDN matching the server cert SANs; custom AltNames must keep
covering *.{name}.{ns}.svc.cluster.local.
Comments only, no behaviour change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Xavier Lange <xrlange@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xrl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
This was referenced Jun 18, 2026
xrl
added a commit
to xrl/etcd-operator
that referenced
this pull request
Jun 19, 2026
…egration Brings the etcd-io#376 TLS reshape (clientEndpointForOrdinalIndex, etcdutils.ClusterHealth, per-surface TLS args) plus T5 and T6 e2e coverage. Cert-block conflict (T0 vs T5) resolved to log + Recorder.Eventf(reasonClientCertificateError) + requeue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
xrl
added a commit
to xrl/etcd-operator
that referenced
this pull request
Jun 19, 2026
…to downstream/integration Conflict in internal/controller/utils.go resolved (rerere) to keep both the reconcile-pool/QoS changes and the etcd-io#376 TLS reshape. Re-aligned the reconcile-pool test TestFetchAndValidateStateClientCertificateError to the reshaped EtcdClusterTLS{Client: &TLSSurface{...CertManagerCfg...}} type and added a FakeRecorder so the client-cert failure path's Eventf does not panic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
xrl
added a commit
to xrl/etcd-operator
that referenced
this pull request
Jun 19, 2026
…stream/integration Conflict in internal/controller/etcdcluster_controller.go resolved (rerere) to keep both quorum-loss recovery and the etcd-io#376 TLS reshape. Re-aligned quorum_recovery.go to the reshaped signatures (this branch was cut off main, blind to etcd-io#376): clientEndpointForOrdinalIndex now takes a per-surface scheme (clientScheme(ec)) and clusterHealth now takes (ctx, ec, eps). Behavior preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
xrl
added a commit
to xrl/etcd-operator
that referenced
this pull request
Jun 19, 2026
…ream/integration Brings the mutating+validating admission webhooks for EtcdCluster and the config/webhook + config/certmanager wiring. Conflict in cmd/main.go resolved (rerere) to keep all three controller registrations plus the webhook setup. Re-aligned the webhook (and its unit/envtest/e2e tests) from the pre-etcd-io#376 single-surface TLSCertificate type to the reshaped two-surface EtcdClusterTLS{Peer,Client}: validateTLS now fans out to a per-surface validateTLSSurface and the defaulter defaults each configured surface, so field-path messages gain a surface segment (spec.tls.client.*). Behavior preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
This was referenced Jul 2, 2026
This was referenced Jul 7, 2026
errcheck on a test listener Close, prealloc on validateTLS's error list, and an unparam on a test helper whose name argument was always "ec". Signed-off-by: Xavier Lange <xrlange@gmail.com>
… and AltNames interfaces.Config now carries the fields providers actually consume: IssuerRef (cert-manager's own cmmeta.IssuerReference), plain-string IPAddresses, Duration, and RenewBefore. The stringly-typed ExtraConfig map and the AltNames wrapper are gone, along with the comma-ok assertions that turned key typos into runtime errors. Behavior fixes that fall out of the typed shape: - user-supplied IP SANs now reach both providers (the cert-manager path serialized them through fmt.Sprint; the auto path dropped them) - GetCertificateConfig echoes real IPAddresses instead of a zeroed placeholder slice - an empty issuerRef.kind resolves to cert-manager's documented default, Issuer, instead of failing the issuer-existence check - renewBefore is passed through to Certificate.spec.renewBefore The controller's spec->Config mappers translate the existing CRD shape unchanged; the CRD itself is untouched here. Signed-off-by: Xavier Lange <xrlange@gmail.com>
The TLS surface config becomes a conventional discriminated union:
spec.tls.{peer,client}.provider (enum auto | cert-manager.io, default
auto) selects between sibling member blocks auto: and certManager:,
replacing the providerCfg wrapper. Provider identifiers are domain-style
(cf. StorageClass.provisioner), so cert-manager is addressed by its API
group.
The certManager block references cert-manager's own types: issuerRef is
cmmeta.IssuerReference ({name, kind, group}), replacing the ad-hoc
issuerKind/issuerName/issuerGroup strings. kind now defaults to Issuer
per cert-manager convention; a CEL rule keeps the value space closed to
Issuer|ClusterIssuer. Both member blocks carry the same curated
cert-manager-style passthrough set: commonName, organizations, dnsNames,
ipAddresses (literal strings), duration, renewBefore.
validityDuration and its day-suffix parsing are gone: duration and
renewBefore are metav1.Duration. Because the CRD renders these as bare
strings, CEL duration() guards on all four fields reject unparseable
values (e.g. "365d") at admission — without them the value would be
stored and wedge the controller's typed decode. The guards double as
floors: 8760h minimum for auto (etcd's SelfCert requirement), 1h for
cert-manager.
BREAKING CHANGE: existing tls stanzas must be rewritten; there is no
conversion webhook (pre-1.0 clean break). Note issuerKind was required
before — a port that drops it now gets a namespaced Issuer lookup.
Samples updated; the autocert sample also gains the peer/client split it
had missed, and a dangling empty tls: key is dropped from the minimal
sample.
Signed-off-by: Xavier Lange <xrlange@gmail.com>
Each TLS surface may reference a ConfigMap (fixed key ca.crt) of extra
PEM CAs. etcd takes exactly one trusted-CA file per surface, so the
operator composes <issued CA> + <user bundle> into an owned per-surface
ConfigMap ({cluster}-{server|peer}-trusted-ca), recomposed every
reconcile so issued-CA rotation is picked up, and points
--trusted-ca-file / --peer-trusted-ca-file at its mount. Without a
bundle the rendered args and volumes are byte-identical to before.
Intended for CA-rotation overlap windows: trust the incoming CA before
certificates from it appear.
Semantics chosen deliberately:
- The bundle broadens member INBOUND trust only. The operator keeps
pinning the issuing CA when dialing etcd; appending a user-writable
ConfigMap's CAs to the dial trust would let ConfigMap write access
mint certs that impersonate etcd to the operator.
- Bundles are strictly validated (every PEM block must be a parseable
certificate) because etcd's tlsutil.NewCertPool hard-errors on any
bad block: an unvalidated bundle admits fine, then crash-loops
members at their next restart. An invalid bundle fails the reconcile
and preserves the last good composition. Go's AppendCertsFromPEM is
NOT used for validation -- it silently skips bad blocks.
- etcd reads trusted-CA files at process start only, so trust changes
take effect per member on its next restart; docs/tls.md carries the
rollout-restart guidance. No automatic StatefulSet roll on trust
changes -- rolling a quorum-sensitive workload on trust bytes is a
separate decision.
docs/tls.md documents the surface model, the duration-format change,
the dedicated-CA-per-cluster guidance, and that etcd RBAC (auth enable)
is out of scope for v1alpha1.
The certs and composed trust ConfigMaps are also re-applied on the
steady-state reconcile path (the only path that skips
reconcileStatefulSet), so a bundle edit or issued-CA rotation on a
size-stable cluster recomposes before the user's rollout restart.
Signed-off-by: Xavier Lange <xrlange@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xrl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The operator already provisions TLS certificates —
pkg/certificatehas minted server, peer, and operator-client certs (auto and cert-manager providers) since the beginning — but nothing consumes them: the secrets are never mounted, the etcd URLs are hardcodedhttp://with no TLS flags emitted, and the operator's own etcd client dials cleartext (#371 / #372 / #373). This PR is the missing consumption half: it mounts the certs, emits the flags, and gives the operator a real mTLS client identity, completing the subsystem the codebase already carries.Wiring it exposed an API problem worth fixing in the same breaking change. etcd has three TLS surfaces — peer↔peer, client→server, and the operator's own client identity — but the alpha CRD conflated all three behind a single
spec.tlstoggle and a single shared issuer: no peer TLS without client TLS (or vice versa), no per-surface mutual-auth policy,--client-cert-auth/--peer-client-cert-authforced on whenever TLS was set.So
spec.tlsbecomes two optional, independent surfaces —spec.tls.peerandspec.tls.client— each carrying its own provider, issuer, andclientCertAuthpolicy (defaulttrue). A nil surface serves/dials that surface in cleartext; both nil is fully cleartext and byte-identical to the pre-TLS path. The operator's own client identity follows theclientsurface.Breaking alpha API change — and why
This is a clean break on the alpha API (
feat(api)!). There is no conversion webhook on purpose: a webhook mapping the old singlespec.tlsto the new surfaces would have to pick one surface or fan out to both, which re-introduces exactly the conflation this change removes. The deadcaBundleSecretknob (type + cert interface) is dropped — it was read nowhere.IssuerGroupis added to the cert-manager provider config (empty ⇒cert-manager.io) so non-default issuer groups can be targeted.Migration:
spec.tlsis effectively create-time — toggling it on a running cluster drops quorum, so a cleartext cluster is migrated by standing up a fresh TLS cluster, not flipped in place. Documented on the regenerated CRD field.Threading (single source of truth, per surface)
peerScheme/clientSchemedrive peer vs client URLs and flags independently.defaultArgsemits the server flag group iffclientset, the peer flag group iffpeerset, each--*client-cert-authgated on its surface.clientset, peer secret iffpeerset (append-not-assign, so they coexist with the storage data-dir mount).clientsurface, peer cert from thepeersurface.etcdutils(MemberList/ClusterHealth/AddMember/PromoteLearner/RemoveMember) take an optional*tls.Config(nil ⇒ cleartext); the operator builds it from theclientsurface only, a missingca.crtnow an explicit error.peer-TLS cluster setsPublishNotReadyAddresses=trueon the headless service so a not-yet-Ready joining member is DNS-resolvable and peer mTLS SAN verification (isHostInDNS) can complete — without it a peer-mTLS cluster deadlocks at one member.Guardrails
XValidationper surface: provider/providerCfg coherence, mTLS-requires-a-CA, issuerKind/provider enums.validateTLSbackstop re-checks the spec-only rules and requeues on an invalid spec (for apiservers that don't enforce CEL).Tests
tls_independence_test.goandtls_cel_test.gocover per-surface scheme/flag/mount/provisioning matrices and the CEL + reconcile validation rules;etcdutils_test.gocovers the optional*tls.Configthreading.go build ./...andgo vet ./...clean.Relationship to the prior TLS plan
This supersedes the earlier conflated, step-wise plan (T2 mounts / T3 flags+scheme / T4 client
*tls.Config): the reshape does all three, per surface, in one cohesive breaking change rather than layering them on the single-toggle API. It implements the intent of #371 / #372 / #373.Refs #371, #372, #373
PR series — operability fixes & TLS
Small single-purpose PRs from live kind-cluster testing of the operator. Each stands alone unless an After is listed. → = this PR.
events.k8s.ioRBAC so operator Events are actually recordedmembers[]/leaderIDfrom one health snapshot (consistent leader)altNames.ipAddressesinto certificatesvalidityDuration(365d,100d12h) as documentedDegradedcondition (was empty status)--max-concurrent-reconciles)podTemplate.specscheduling fields: topologySpread, resources, priorityClass, schedulerNameconfig/surfacespec.tls.{peer,client}surfaces (breaking alpha API)TLSReadycondition + TLS lifecycle EventsPeerCANotSharedEtcdMirrorCRD: one-way cross-cluster replication API (types + CEL)pkg/mirroragentreplication engine (fenced checkpoint-in-target)mirror-agentbinary: config/TLS-reload/statusz/metrics + kind e2e/metricsendpointEtcdBackupCR → object storage (S3/GCS)🟢 ready · ⚪ draft
Update: TLS config reshaped to a flattened, cert-manager-idiomatic union
The follow-up commits reshape the surface config into a conventional
discriminated union and make the cert-manager reference idiomatic:
Breaking changes (v1alpha1, no conversion webhook)
providerCfg.{autoCfg,certManagerCfg}-> flattenedauto:/certManager:memberblocks;
providerenum values are now domain-style:auto | cert-manager.io.issuerKind/issuerName/issuerGroup->certManager.issuerReftyped ascert-manager's
cmmeta.IssuerReference.kindis now optional and defaults toIssuer(namespaced) — a port that dropsissuerKind: ClusterIssuergets anamespaced lookup and
IssuerNotFound.validityDuration(day-suffix strings) ->duration/renewBeforeasmetav1.Duration. Go units only:"365d"->"8760h". CELduration()guardsreject unparseable values at admission (the CRD renders these fields as bare
strings, so without the guards a bad value would be stored and wedge the
controller's typed decode).
tls:stanzas must be deleted and re-created in the new shape.New
trustBundleConfigMapRefper surface: extra PEM CAs (ConfigMap keyca.crt)composed with the issued CA into
<cluster>-{server|peer}-trusted-caand wired to--trusted-ca-file/--peer-trusted-ca-file. Append-only, member-side trust only(the operator keeps pinning the issuing CA), strictly validated (etcd hard-errors
on any bad PEM block at startup, so invalid bundles are rejected and the last good
composition preserved). etcd reads CA files at startup only — trust changes need a
member restart; documented in docs/tls.md.
pkg/certificatecleanup: typedinterfaces.ConfigwithIssuerRef(stringly-typed
ExtraConfigmap deleted); user-supplied IP SANs now actuallyreach both providers;
renewBeforepassthrough.semantics, "etcd RBAC (
auth enable) out of scope for v1alpha1".On #377 (stacked)
ClientCABroadTrustWarning event (never a TLSReady failure) when client mTLSadmission is broader than a dedicated CA (shared client/peer issuer, or a client
trust bundle).
TrustBundleInvalidTLSReady=False reason for missing/invalid bundles.Validation
make test(unit + envtest incl. 18 CEL admission cases against a realapiserver) and
make verifygreen on every commit.TestTLSMultiMemberQuorum(3-member mTLS quorum, TLSReady=True,ClientCABroadTrust warning event) and
TestTLSPeerCANotSharedgreen at theladder tip.
#377also picks up theevents.k8s.ioRBAC grant (same fix asfix: grant RBAC for events.k8s.io Events emitted by the events/v1 recorder #391 — whichever lands first, the other rebases to a no-op) because the
TLSReady events it introduces are silently dropped without it.
Pre-existing issue surfaced while validating (NOT introduced here)
TestClusterAutoCertCreationfails at the base commit too: the auto provider mintsindependent self-signed certs via
transport.SelfCert, which are ServerAuth-onlyEKU and each their own root — so with
clientCertAuth: true(the default) theoperator's auto-minted client certificate can never verify against the server's
trust anchor, and the operator cannot scale the cluster past one member. The auto
provider needs a per-cluster CA model (mint one CA, sign server/peer/client from
it, pass ClientAuth EKU) — filed as #420; out of scope for this reshape, which
keeps auto-path behavior byte-identical.