feat(status): add TLSReady condition and TLS lifecycle events#377
Draft
xrl wants to merge 12 commits into
Draft
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. |
Author
This was referenced Jul 3, 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>
Surface the TLS lifecycle on the EtcdCluster CR. The independence reshape wired the EventRecorder and the buildClientTLSConfig/validateTLS surfaces but emitted zero Events and had no TLS status condition; this closes that observability gap (adversarial review section 5). - Add a single TLSReady status condition in updateConditions, reflecting both peer and client surfaces with a per-surface reason vocabulary: TLSNotConfigured (omitted when both surfaces nil), TLSReady, IssuerNotFound, PeerCANotShared, ClientServerCAMismatch, ClientCertificateError, SurfaceNotReady. False with the specific reason on each failure; True only when every configured surface is healthy. - Emit CR Events at the controller boundary (Recorder-free helpers return a typed verdict the boundary maps to Events): Warning on each failure with the matching reason, a single Normal TLSReady on transition into ready. - Implement the runtime checks the reshape deferred (not CEL-expressible): PeerCANotShared detects a self-signed-leaf cert-manager peer issuer that cannot establish shared peer trust, turning the silent cap-at-1-member hazard into a visible signal; client-surface issuer existence and operator-client secret usability are checked from cluster objects. ClientServerCAMismatch is in the vocabulary but unreachable from a single client surface (server + operator-client share one issuer by construction); left as a documented TODO. Unit tests drive every state->reason mapping, the peer-CA predicate against representative issuer inputs, and Event emission via record FakeRecorder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
…ason reuse Document that PeerCANotShared is a conservative WARNING rather than a hard member cap: the operator mounts ONE shared peer secret into every pod, so a self-signed-leaf peer cert still forms a multi-member quorum. Note the reuse of the ClientCertificateError reason on the validateTLS boundary, which runs before evaluateTLSReadiness and therefore has no per-surface verdict reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Xavier Lange <xrlange@gmail.com>
Two evaluateTLSReadiness additions for the broadened-trust cases the flattened union makes easy to configure: - ClientCABroadTrust: a Warning event, never a TLSReady failure. With client mTLS on, etcd's authorization is the CA boundary, so the operator says so out loud when that boundary is broader than a dedicated client CA: the client and peer surfaces share one issuer (any peer certificate then also authenticates as a client), or the client surface carries a trust bundle. Warnings ride the ready-transition gate (plus spec-generation changes), so steady-state clusters stay quiet. - TrustBundleInvalid: a referenced trust bundle that is missing, lacks ca.crt, or fails strict PEM validation now flips TLSReady=False with a distinct reason instead of surfacing only as a bare reconcile error. The composed trust ConfigMap keeps its last good content in this state. issuerRef comparisons resolve cert-manager's documented defaults (kind Issuer, group cert-manager.io) before comparing, so equivalent spellings of the same issuer are recognized as shared. Signed-off-by: Xavier Lange <xrlange@gmail.com>
The recorder returned by mgr.GetEventRecorder writes events.k8s.io/v1 Events, but role.yaml only granted events verbs in the core API group, so every TLSReady event (including the new ClientCABroadTrust warning) was silently dropped. Mirrors the standalone fix in etcd-io#391; whichever lands first, the other rebases to a no-op. Signed-off-by: Xavier Lange <xrlange@gmail.com>
xrl
force-pushed
the
pr/T5-tls-conditions
branch
from
July 11, 2026 18:49
e2364e3 to
d69599e
Compare
|
[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
Surface the TLS lifecycle on the
EtcdClusterCR. The independence reshape (#376) wired theEventRecorderand thebuildClientTLSConfig/validateTLSsurfaces but emitted zero Events and had no TLS status condition — so a misconfigured TLS spec failed silently. This closes that observability gap.TLSReadystatus condition inupdateConditions, reflecting both peer and client surfaces with a per-surface reason vocabulary:TLSNotConfigured(omitted when both surfaces nil),TLSReady,IssuerNotFound,PeerCANotShared,ClientServerCAMismatch,ClientCertificateError,SurfaceNotReady. False with the specific reason on each failure; True only when every configured surface is healthy.Warningon each failure with the matching reason, a singleNormalTLSReadyon transition into ready.PeerCANotShareddetects a self-signed-leaf cert-manager peer issuer that cannot establish shared peer trust — turning a silent cap-at-1-member hazard into a visible signal; client-surface issuer existence and operator-client secret usability are checked from cluster objects.ClientServerCAMismatchis in the vocabulary but unreachable from a single client surface (server + operator-client share one issuer by construction) — left as a documented TODO.Unit tests drive every state→reason mapping, the peer-CA predicate against representative issuer inputs, and Event emission via
record.FakeRecorder.go build/go vetclean.Stacking
Stacked on #376 (independent peer/client TLS). Review/merge after #376; this branch contains #376's commits and will shrink to its own two commits once #376 merges.
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