From 2e6fdaae75a667895c38958cb1591228a5f3f508 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 22:20:32 -0400 Subject: [PATCH 1/6] feat(api)!: configure peer and client TLS independently 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 Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 83 +++- api/v1alpha1/zz_generated.deepcopy.go | 42 +- .../bases/operator.etcd.io_etcdclusters.yaml | 354 ++++++++++---- .../sample_certmanager_etcdcluster.yaml | 73 ++- internal/controller/etcdcluster_controller.go | 47 +- internal/controller/tls_cel_test.go | 130 +++++ internal/controller/tls_independence_test.go | 447 ++++++++++++++++++ internal/controller/utils.go | 401 +++++++++++++--- internal/controller/utils_test.go | 102 ++-- internal/etcdutils/etcdutils.go | 22 +- internal/etcdutils/etcdutils_test.go | 100 +++- pkg/certificate/cert_manager/provider.go | 19 +- pkg/certificate/interfaces/interface.go | 1 - test/e2e/auto_provider_test.go | 13 +- test/e2e/cert_manager_test.go | 15 +- test/e2e/helpers_test.go | 35 ++ 16 files changed, 1595 insertions(+), 289 deletions(-) create mode 100644 internal/controller/tls_cel_test.go create mode 100644 internal/controller/tls_independence_test.go diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index f007ae04..7667e44d 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -43,8 +43,12 @@ type EtcdClusterSpec struct { Version string `json:"version"` // StorageSpec is the name of the StorageSpec to use for the etcd cluster. If not provided, then each POD just uses the temporary storage inside the container. StorageSpec *StorageSpec `json:"storageSpec,omitempty"` - // TLS is the TLS certificate configuration to use for the etcd cluster and etcd operator. - TLS *TLSCertificate `json:"tls,omitempty"` + // TLS configures etcd's two independent TLS surfaces (peer and client/server). + // Each surface is optional and configured fully independently; a nil surface + // means that surface is served/dialed in cleartext. When TLS itself is nil, the + // entire cluster (peer + client + operator client) is cleartext, byte-identical + // to a TLS-free deployment. + TLS *EtcdClusterTLS `json:"tls,omitempty"` // etcd configuration options are passed as command line arguments to the etcd container, refer to etcd documentation for configuration options applicable for the version of etcd being used. EtcdOptions []string `json:"etcdOptions,omitempty"` // PodTemplate is the pod template to use for the etcd cluster. @@ -68,9 +72,64 @@ type PodMetadata struct { Labels map[string]string `json:"labels,omitempty"` } -type TLSCertificate struct { - Provider string `json:"provider,omitempty"` // Defaults to Auto provider if not present +// EtcdClusterTLS configures etcd's two independent TLS surfaces. Each surface is +// optional; a nil surface means that surface is served/dialed in cleartext (http). +// The two surfaces are configured fully independently -- different providers, +// issuers, and client-cert-auth policy are allowed and expected. Both surfaces nil +// is legal and means fully-cleartext (today's default); it is intentional, not an +// error, so there is no "at least one surface" validation. +type EtcdClusterTLS struct { + // Peer configures etcd<->etcd (peer) TLS. When nil, peer traffic is cleartext. + // A configured peer surface REQUIRES a CA-capable issuer shared by all members + // so members can mutually verify; a self-signed *leaf* issuer cannot form a + // multi-member cluster. (CA-capability lives on the cert-manager Issuer object, + // not on this spec, so that check is enforced at reconcile time, not via CEL.) + // +optional + Peer *TLSSurface `json:"peer,omitempty"` + + // Client configures client->etcd (server) TLS AND, transitively, the operator's + // own etcd client identity (the operator authenticates to etcd as a client). + // When nil, client traffic is cleartext and the operator dials cleartext. + // +optional + Client *TLSSurface `json:"client,omitempty"` +} + +// TLSSurface is the full, independent TLS configuration for ONE surface (peer or +// client). It carries its own provider, provider config (issuer), and mutual +// client-cert-auth policy. +// +// The two XValidation rules below are the apply-time anti-misconfiguration +// guardrails (plan Decision 2.1/2.2): they reject incoherent provider/config +// combinations and mTLS-without-a-resolvable-CA at the API server, so a user +// "cannot misconfigure" these from the spec alone. Rules that require reading +// cluster objects (issuer existence, peer CA-capability, client/server CA match) +// cannot be expressed in CEL and are enforced at reconcile time instead (plan +// Decision 2.5-2.7, Decision 3) -- see validateTLSSurface and the cert-manager +// provider's validateCertificateConfig. +// +// +kubebuilder:validation:XValidation:rule="self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg)",message="provider 'cert-manager' requires providerCfg.certManagerCfg" +// +kubebuilder:validation:XValidation:rule="self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg)",message="providerCfg.certManagerCfg may only be set when provider is 'cert-manager'" +// +kubebuilder:validation:XValidation:rule="!self.clientCertAuth || self.provider != 'cert-manager' || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) > 0)",message="clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName" +type TLSSurface struct { + // Provider selects the certificate provider for THIS surface. + // Defaults to "auto" when empty. + // +kubebuilder:validation:Enum=auto;cert-manager + // +optional + Provider string `json:"provider,omitempty"` + + // ProviderCfg is the provider-specific config for THIS surface. + // +optional ProviderCfg ProviderConfig `json:"providerCfg,omitempty"` + + // ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + // --client-cert-auth for the client surface, --peer-client-cert-auth for the + // peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + // where clients authenticate by other means (password/token). When true with + // the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + // the XValidation rule above. + // +kubebuilder:default=true + // +optional + ClientCertAuth *bool `json:"clientCertAuth,omitempty"` } type ProviderConfig struct { @@ -109,13 +168,6 @@ type CommonConfig struct { // and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 // +optional ValidityDuration string `json:"validityDuration,omitempty"` - - // CABundleSecret is the expected secret name with CABundle present. It's used - // by each etcd POD to verify TLS communications with its peers or clients. If it isn't - // provided, the CA included in the secret generated by certificate provider will be - // used instead if present; otherwise, there is no way to verify TLS communications. - // +optional - CABundleSecret string `json:"caBundleSecret,omitempty"` } type ProviderAutoConfig struct { @@ -127,11 +179,18 @@ type ProviderCertManagerConfig struct { // CommonConfig is the struct of common fields required to create a certificate CommonConfig `json:",inline"` - // IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer" + // IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer". + // +kubebuilder:validation:Enum=Issuer;ClusterIssuer IssuerKind string `json:"issuerKind"` // IssuerName is the expected name of Issuer required to issue a certificate IssuerName string `json:"issuerName"` + + // IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + // Empty defaults to "cert-manager.io". Set this to target issuers served by an + // external/intermediate issuer group (e.g. an out-of-tree CA controller). + // +optional + IssuerGroup string `json:"issuerGroup,omitempty"` } // EtcdClusterStatus defines the observed state of EtcdCluster. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9db6ca12..fbab398c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -23,7 +23,7 @@ package v1alpha1 import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" netx "net" ) @@ -148,7 +148,7 @@ func (in *EtcdClusterSpec) DeepCopyInto(out *EtcdClusterSpec) { } if in.TLS != nil { in, out := &in.TLS, &out.TLS - *out = new(TLSCertificate) + *out = new(EtcdClusterTLS) (*in).DeepCopyInto(*out) } if in.EtcdOptions != nil { @@ -200,6 +200,31 @@ func (in *EtcdClusterStatus) DeepCopy() *EtcdClusterStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *EtcdClusterTLS) DeepCopyInto(out *EtcdClusterTLS) { + *out = *in + if in.Peer != nil { + in, out := &in.Peer, &out.Peer + *out = new(TLSSurface) + (*in).DeepCopyInto(*out) + } + if in.Client != nil { + in, out := &in.Client, &out.Client + *out = new(TLSSurface) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EtcdClusterTLS. +func (in *EtcdClusterTLS) DeepCopy() *EtcdClusterTLS { + if in == nil { + return nil + } + out := new(EtcdClusterTLS) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MemberStatus) DeepCopyInto(out *MemberStatus) { *out = *in @@ -378,17 +403,22 @@ func (in *StorageSpec) DeepCopy() *StorageSpec { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *TLSCertificate) DeepCopyInto(out *TLSCertificate) { +func (in *TLSSurface) DeepCopyInto(out *TLSSurface) { *out = *in in.ProviderCfg.DeepCopyInto(&out.ProviderCfg) + if in.ClientCertAuth != nil { + in, out := &in.ClientCertAuth, &out.ClientCertAuth + *out = new(bool) + **out = **in + } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSCertificate. -func (in *TLSCertificate) DeepCopy() *TLSCertificate { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSSurface. +func (in *TLSSurface) DeepCopy() *TLSSurface { if in == nil { return nil } - out := new(TLSCertificate) + out := new(TLSSurface) in.DeepCopyInto(out) return out } diff --git a/config/crd/bases/operator.etcd.io_etcdclusters.yaml b/config/crd/bases/operator.etcd.io_etcdclusters.yaml index 0ae1e80e..3843e523 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.20.1 + controller-gen.kubebuilder.io/version: v0.21.0 name: etcdclusters.operator.etcd.io spec: group: operator.etcd.io @@ -1069,119 +1069,295 @@ spec: - volumeSizeRequest type: object tls: - description: TLS is the TLS certificate configuration to use for the - etcd cluster and etcd operator. + description: |- + TLS configures etcd's two independent TLS surfaces (peer and client/server). + Each surface is optional and configured fully independently; a nil surface + means that surface is served/dialed in cleartext. When TLS itself is nil, the + entire cluster (peer + client + operator client) is cleartext, byte-identical + to a TLS-free deployment. properties: - provider: - type: string - providerCfg: + client: + description: |- + Client configures client->etcd (server) TLS AND, transitively, the operator's + own etcd client identity (the operator authenticates to etcd as a client). + When nil, client traffic is cleartext and the operator dials cleartext. properties: - autoCfg: + clientCertAuth: + default: true + description: |- + ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + --client-cert-auth for the client surface, --peer-client-cert-auth for the + peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + where clients authenticate by other means (password/token). When true with + the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + the XValidation rule above. + type: boolean + provider: + description: |- + Provider selects the certificate provider for THIS surface. + Defaults to "auto" when empty. + enum: + - auto + - cert-manager + type: string + providerCfg: + description: ProviderCfg is the provider-specific config for + THIS surface. properties: - altNames: - description: |- - AltNames contains the domain names and IP addresses that will be added - to the x509 certificate SubAltNames fields. The values will be passed - directly to the x509.Certificate object. + autoCfg: properties: - dnsNames: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: description: |- - DNSNames is the expected array of DNS subject alternative names. - if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array - ipAddresses: - description: IPs is the expected array of IP address - subject alternative names. + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + type: object + certManagerCfg: + properties: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: + description: |- + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + issuerGroup: + description: |- + IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + Empty defaults to "cert-manager.io". Set this to target issuers served by an + external/intermediate issuer group (e.g. an out-of-tree CA controller). + type: string + issuerKind: + description: IssuerKind is the expected kind of Issuer, + either "ClusterIssuer" or "Issuer". + enum: + - Issuer + - ClusterIssuer + type: string + issuerName: + description: IssuerName is the expected name of Issuer + required to issue a certificate + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + required: + - issuerKind + - issuerName type: object - caBundleSecret: - description: |- - CABundleSecret is the expected secret name with CABundle present. It's used - by each etcd POD to verify TLS communications with its peers or clients. If it isn't - provided, the CA included in the secret generated by certificate provider will be - used instead if present; otherwise, there is no way to verify TLS communications. - type: string - commonName: - description: |- - CommonName is the expected common name X509 certificate subject attribute. - Should have a length of 64 characters or fewer to avoid generating invalid CSRs. - type: string - organizations: - description: Organization is the expected array of Organization - names to be used on the Certificate. - items: - type: string - type: array - validityDuration: - description: |- - ValidityDuration is the expected duration until which the certificate will be valid, - expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager - and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 - type: string type: object - certManagerCfg: + type: object + x-kubernetes-validations: + - message: provider 'cert-manager' requires providerCfg.certManagerCfg + rule: self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg) + - message: providerCfg.certManagerCfg may only be set when provider + is 'cert-manager' + rule: self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg) + - message: 'clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName' + rule: '!self.clientCertAuth || self.provider != ''cert-manager'' + || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) + > 0)' + peer: + description: |- + Peer configures etcd<->etcd (peer) TLS. When nil, peer traffic is cleartext. + A configured peer surface REQUIRES a CA-capable issuer shared by all members + so members can mutually verify; a self-signed *leaf* issuer cannot form a + multi-member cluster. (CA-capability lives on the cert-manager Issuer object, + not on this spec, so that check is enforced at reconcile time, not via CEL.) + properties: + clientCertAuth: + default: true + description: |- + ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + --client-cert-auth for the client surface, --peer-client-cert-auth for the + peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + where clients authenticate by other means (password/token). When true with + the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + the XValidation rule above. + type: boolean + provider: + description: |- + Provider selects the certificate provider for THIS surface. + Defaults to "auto" when empty. + enum: + - auto + - cert-manager + type: string + providerCfg: + description: ProviderCfg is the provider-specific config for + THIS surface. properties: - altNames: - description: |- - AltNames contains the domain names and IP addresses that will be added - to the x509 certificate SubAltNames fields. The values will be passed - directly to the x509.Certificate object. + autoCfg: properties: - dnsNames: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: description: |- - DNSNames is the expected array of DNS subject alternative names. - if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array - ipAddresses: - description: IPs is the expected array of IP address - subject alternative names. + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + type: object + certManagerCfg: + properties: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: + description: |- + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + issuerGroup: + description: |- + IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + Empty defaults to "cert-manager.io". Set this to target issuers served by an + external/intermediate issuer group (e.g. an out-of-tree CA controller). + type: string + issuerKind: + description: IssuerKind is the expected kind of Issuer, + either "ClusterIssuer" or "Issuer". + enum: + - Issuer + - ClusterIssuer + type: string + issuerName: + description: IssuerName is the expected name of Issuer + required to issue a certificate + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + required: + - issuerKind + - issuerName type: object - caBundleSecret: - description: |- - CABundleSecret is the expected secret name with CABundle present. It's used - by each etcd POD to verify TLS communications with its peers or clients. If it isn't - provided, the CA included in the secret generated by certificate provider will be - used instead if present; otherwise, there is no way to verify TLS communications. - type: string - commonName: - description: |- - CommonName is the expected common name X509 certificate subject attribute. - Should have a length of 64 characters or fewer to avoid generating invalid CSRs. - type: string - issuerKind: - description: IssuerKind is the expected kind of Issuer, - either "ClusterIssuer" or "Issuer" - type: string - issuerName: - description: IssuerName is the expected name of Issuer - required to issue a certificate - type: string - organizations: - description: Organization is the expected array of Organization - names to be used on the Certificate. - items: - type: string - type: array - validityDuration: - description: |- - ValidityDuration is the expected duration until which the certificate will be valid, - expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager - and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 - type: string - required: - - issuerKind - - issuerName type: object type: object + x-kubernetes-validations: + - message: provider 'cert-manager' requires providerCfg.certManagerCfg + rule: self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg) + - message: providerCfg.certManagerCfg may only be set when provider + is 'cert-manager' + rule: self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg) + - message: 'clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName' + rule: '!self.clientCertAuth || self.provider != ''cert-manager'' + || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) + > 0)' type: object version: description: Version is the expected version of the etcd container diff --git a/config/samples/sample_certmanager_etcdcluster.yaml b/config/samples/sample_certmanager_etcdcluster.yaml index b515db39..282399b2 100644 --- a/config/samples/sample_certmanager_etcdcluster.yaml +++ b/config/samples/sample_certmanager_etcdcluster.yaml @@ -1,11 +1,53 @@ -# creating and managing the Issuer/ClusterIssuer is the users’ responsibility, while etcd-operator only needs to know which Issuer/ClusterIssuer to use. +# A multi-member TLS etcd cluster needs a SHARED CA so members can mutually verify +# each other's peer certs and the operator can verify the server certs. A bare +# self-signed ClusterIssuer mints CA:FALSE leaf certs that are each their own root, +# so peer mTLS cannot form across members and the cluster is effectively capped at +# one member. +# +# The supported pattern (and the one this sample models) is a two-tier CA: +# 1. a SelfSigned ClusterIssuer bootstraps +# 2. a CA Certificate (isCA: true), whose secret is signed by (1) +# 3. a CA ClusterIssuer built from that CA secret signs all of the leaf certs +# Then every issued secret's ca.crt is the same real CA and all verification chains +# succeed. Creating/managing the Issuer/ClusterIssuer is the user's responsibility; +# etcd-operator only needs to know which Issuer/ClusterIssuer to reference. apiVersion: cert-manager.io/v1 kind: ClusterIssuer metadata: - name: selfsigned + name: selfsigned-bootstrap spec: selfSigned: {} +--- +# CA Certificate: isCA so it can sign leaf certs. Its secret is consumed by the CA +# ClusterIssuer below. cert-manager resolves a ClusterIssuer's CA secret out of the +# cluster resource namespace (where cert-manager runs), so place it there. +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: etcd-ca + namespace: cert-manager +spec: + isCA: true + commonName: etcd-ca + secretName: etcd-ca + privateKey: + algorithm: ECDSA + size: 256 + issuerRef: + name: selfsigned-bootstrap + kind: ClusterIssuer + group: cert-manager.io + +--- +apiVersion: cert-manager.io/v1 +kind: ClusterIssuer +metadata: + name: etcd-ca-issuer +spec: + ca: + secretName: etcd-ca + --- apiVersion: operator.etcd.io/v1alpha1 kind: EtcdCluster @@ -17,11 +59,24 @@ metadata: spec: version: v3.5.21 size: 3 + # Peer and client/server TLS are configured independently. Here both surfaces use + # the shared CA issuer so peer mTLS forms across members and the operator can + # verify the server. Either surface may be omitted to serve that surface in + # cleartext, or pointed at a different issuer. tls: - provider: "cert-manager" - providerCfg: - certManagerCfg: - commonName: "etcd-operator-system" - validityDuration: "365d" - issuerKind: "ClusterIssuer" - issuerName: "selfsigned" + peer: + provider: "cert-manager" + providerCfg: + certManagerCfg: + commonName: "etcd-operator-system" + validityDuration: "365d" + issuerKind: "ClusterIssuer" + issuerName: "etcd-ca-issuer" + client: + provider: "cert-manager" + providerCfg: + certManagerCfg: + commonName: "etcd-operator-system" + validityDuration: "365d" + issuerKind: "ClusterIssuer" + issuerName: "etcd-ca-issuer" diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index 867886a3..3f1f1767 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -133,17 +133,26 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c ec.Spec.ImageRegistry = r.ImageRegistry } - // Ensure the operator has TLS credentials when the cluster requests TLS. - if ec.Spec.TLS != nil { + // Reconcile-time backstop for the apply-time CEL rules (see validateTLS). Reject + // an incoherent TLS spec by requeuing rather than silently proceeding into cert + // provisioning, which would otherwise fail deep with a less actionable error. + if errs := validateTLS(ec); len(errs) > 0 { + logger.Error(errs.ToAggregate(), "invalid TLS configuration; not reconciling until fixed", + "etcdCluster", ec.Name) + return nil, ctrl.Result{RequeueAfter: requeueDuration}, nil + } + + // The operator authenticates to etcd as a client, so it needs its own client + // identity iff the CLIENT surface is configured (independent of the peer surface). + if clientTLSEnabled(ec) { if err := createClientCertificate(ctx, ec, r.Client); err != nil { - logger.Error(err, "Failed to create Client Certificate.") + logger.Error(err, "Failed to create operator client certificate", "etcdCluster", ec.Name) } } else { - // TODO: instead of logging error, set default autoConfig - logger.Error(nil, fmt.Sprintf( - "missing TLS config for %s,\n running etcd-cluster without TLS protection is NOT recommended for production.", - ec.Name, - )) + // Cleartext (no client surface) is a supported mode; log at Info, not Error, + // so legitimately-cleartext clusters don't spam error logs every reconcile. + logger.Info("client TLS surface not configured; operator dials etcd in cleartext (not recommended for production)", + "etcdCluster", ec.Name) } logger.Info("Reconciling EtcdCluster", "spec", ec.Spec) @@ -260,7 +269,7 @@ func (r *EtcdClusterReconciler) performHealthChecks(ctx context.Context, s *reco logger := log.FromContext(ctx) logger.Info("Now checking health of the cluster members") var err error - s.memberListResp, s.memberHealth, err = healthCheck(s.sts, logger) + s.memberListResp, s.memberHealth, err = healthCheck(ctx, s.cluster, r.Client, s.sts, logger) if err != nil { return fmt.Errorf("health check failed: %w", err) } @@ -280,6 +289,14 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re targetReplica := *s.sts.Spec.Replicas var err error + // Build the operator's etcd-client TLS config once (nil when the client surface + // is cleartext) and derive client endpoints from the client surface's scheme. + clientTLSConfig, err := buildClientTLSConfig(ctx, s.cluster, r.Client) + if err != nil { + return ctrl.Result{}, err + } + cScheme := clientScheme(s.cluster) + // The number of replicas in the StatefulSet doesn't match the number of etcd members in the cluster. if int(targetReplica) != memberCnt { logger.Info("The expected number of replicas doesn't match the number of etcd members in the cluster", "targetReplica", targetReplica, "memberCnt", memberCnt) @@ -322,9 +339,9 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re if etcdutils.IsLearnerReady(leaderStatus, learnerStatus) { logger.Info("Learner is ready to be promoted to voting member", "learnerID", learner) logger.Info("Promoting the learner member", "learnerID", learner) - eps := clientEndpointsFromStatefulsets(s.sts) + eps := clientEndpointsFromStatefulsets(s.sts, cScheme) eps = eps[:(len(eps) - 1)] - if err := etcdutils.PromoteLearner(eps, learner); err != nil { + if err := etcdutils.PromoteLearner(eps, learner, clientTLSConfig); err != nil { // The member is not promoted yet, so we error out and requeue via the caller. return ctrl.Result{}, err } @@ -341,7 +358,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re return ctrl.Result{}, nil } - eps := clientEndpointsFromStatefulsets(s.sts) + eps := clientEndpointsFromStatefulsets(s.sts, cScheme) // If there are no learners left, we can proceed to scale the cluster towards the desired size. // When there are no members to add, the controller will requeue above and this block won't execute. @@ -350,7 +367,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re _, peerURL := peerEndpointForOrdinalIndex(s.cluster, int(targetReplica)) targetReplica++ logger.Info("[Scale out] adding a new learner member to etcd cluster", "peerURLs", peerURL) - if _, err := etcdutils.AddMember(eps, []string{peerURL}, true); err != nil { + if _, err := etcdutils.AddMember(eps, []string{peerURL}, true, clientTLSConfig); err != nil { return ctrl.Result{}, err } @@ -374,7 +391,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re logger.Info("[Scale in] removing one member", "memberID", memberID) eps = eps[:targetReplica] - if err := etcdutils.RemoveMember(eps, memberID); err != nil { + if err := etcdutils.RemoveMember(eps, memberID, clientTLSConfig); err != nil { return ctrl.Result{}, err } @@ -388,7 +405,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re } // Ensure every etcd member reports itself healthy before declaring success. - allMembersHealthy, err := areAllMembersHealthy(s.sts, logger) + allMembersHealthy, err := areAllMembersHealthy(ctx, s.cluster, r.Client, s.sts, logger) if err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/tls_cel_test.go b/internal/controller/tls_cel_test.go new file mode 100644 index 00000000..0b125b73 --- /dev/null +++ b/internal/controller/tls_cel_test.go @@ -0,0 +1,130 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// TestTLSCELValidation drives the apply-time CEL XValidation rules on TLSSurface +// against the real envtest apiserver (k8sClient applies the generated CRD). These +// are the rules that "cannot be applied wrong" -- the apiserver rejects them before +// the controller ever sees the object. The reconcile-time backstop is covered +// separately in TestValidateTLS. +func TestTLSCELValidation(t *testing.T) { + if k8sClient == nil { + t.Skip("envtest apiserver not available") + } + + cm := func(issuerName string) ecv1alpha1.ProviderConfig { + return ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", + IssuerName: issuerName, + }, + } + } + + tests := []struct { + name string + surface ecv1alpha1.TLSSurface + wantApply bool // true => apiserver should accept + }{ + { + name: "valid cert-manager mTLS surface accepted", + surface: ecv1alpha1.TLSSurface{Provider: "cert-manager", ProviderCfg: cm("etcd-ca-issuer")}, + wantApply: true, + }, + { + name: "auto provider surface accepted", + surface: ecv1alpha1.TLSSurface{Provider: "auto"}, + wantApply: true, + }, + { + name: "cert-manager provider without certManagerCfg rejected", + surface: ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + wantApply: false, + }, + { + name: "certManagerCfg under auto provider rejected", + surface: ecv1alpha1.TLSSurface{Provider: "auto", ProviderCfg: cm("etcd-ca-issuer")}, + wantApply: false, + }, + { + name: "clientCertAuth true with cert-manager but empty issuerName rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(true), + ProviderCfg: cm(""), + }, + wantApply: false, + }, + { + name: "server-only TLS (clientCertAuth false) without issuerName accepted", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(false), + ProviderCfg: cm("etcd-ca-issuer"), + }, + wantApply: true, + }, + { + name: "bad issuerKind rejected by enum", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "Bogus", + IssuerName: "etcd-ca-issuer", + }, + }, + }, + wantApply: false, + }, + { + name: "bad provider rejected by enum", + surface: ecv1alpha1.TLSSurface{ + Provider: "vault", + }, + wantApply: false, + }, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + surface := tt.surface + ec := &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "cel-test-", + Namespace: "default", + }, + Spec: ecv1alpha1.EtcdClusterSpec{ + Size: 3, + Version: "v3.6.12", + // Alternate which surface carries the config so both surfaces' + // XValidation rules are exercised across the table. + TLS: tlsForSurface(i%2 == 0, &surface), + }, + } + err := k8sClient.Create(t.Context(), ec) + if tt.wantApply { + require.NoError(t, err, "apiserver should accept a valid surface") + _ = k8sClient.Delete(t.Context(), ec, &client.DeleteOptions{}) + } else { + assert.Error(t, err, "apiserver should reject an invalid surface via CEL") + } + }) + } +} + +func tlsForSurface(onClient bool, s *ecv1alpha1.TLSSurface) *ecv1alpha1.EtcdClusterTLS { + if onClient { + return &ecv1alpha1.EtcdClusterTLS{Client: s} + } + return &ecv1alpha1.EtcdClusterTLS{Peer: s} +} diff --git a/internal/controller/tls_independence_test.go b/internal/controller/tls_independence_test.go new file mode 100644 index 00000000..f4a95531 --- /dev/null +++ b/internal/controller/tls_independence_test.go @@ -0,0 +1,447 @@ +package controller + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +func newScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, appsv1.AddToScheme(scheme)) + require.NoError(t, ecv1alpha1.AddToScheme(scheme)) + return scheme +} + +func discardLogger() logr.Logger { return logr.Discard() } + +func mustQuantity(s string) resource.Quantity { return resource.MustParse(s) } + +// genClientKeypair returns a (cert, key, ca) PEM triple suitable for +// buildClientTLSConfig: a self-signed leaf is its own CA here, which is fine for +// exercising the operator-side config building (CA-capability across members is an +// e2e concern, out of scope for this unit). +func genClientKeypair(t *testing.T) (certPEM, keyPEM, caPEM []byte) { + t.Helper() + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "etcd-operator-client"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + IsCA: true, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + require.NoError(t, err) + certPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) + + keyDER, err := x509.MarshalPKCS8PrivateKey(priv) + require.NoError(t, err) + keyPEM = pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}) + + return certPEM, keyPEM, certPEM // ca == the self-signed cert +} + +// boolPtr is a small helper for the *bool ClientCertAuth field. +func boolPtr(b bool) *bool { return &b } + +// cmSurface builds a cert-manager TLSSurface with mTLS on by default. +func cmSurface(clientCertAuth *bool) *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", + IssuerName: "etcd-ca-issuer", + }, + }, + ClientCertAuth: clientCertAuth, + } +} + +func clusterWithTLS(name string, tls *ecv1alpha1.EtcdClusterTLS) *ecv1alpha1.EtcdCluster { + return &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, + Spec: ecv1alpha1.EtcdClusterSpec{Size: 3, Version: "v3.6.12", TLS: tls}, + } +} + +// TestTLSIndependenceArgsMatrix asserts that the peer and client surfaces drive the +// rendered etcd args fully independently: peer flags/scheme follow the peer surface, +// server flags/scheme follow the client surface, and each --*client-cert-auth flag +// follows its own surface's toggle. +func TestTLSIndependenceArgsMatrix(t *testing.T) { + const name = "ec" + + cleartextArgs := []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=http://0.0.0.0:2380", + "--listen-client-urls=http://0.0.0.0:2379", + "--initial-advertise-peer-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + } + + serverFlags := []string{ + "--cert-file=/etc/etcd/server-tls/tls.crt", + "--key-file=/etc/etcd/server-tls/tls.key", + "--trusted-ca-file=/etc/etcd/server-tls/ca.crt", + } + peerFlags := []string{ + "--peer-cert-file=/etc/etcd/peer-tls/tls.crt", + "--peer-key-file=/etc/etcd/peer-tls/tls.key", + "--peer-trusted-ca-file=/etc/etcd/peer-tls/ca.crt", + } + + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + expected []string + }{ + { + name: "neither (cleartext) is byte-identical to today", + tls: nil, + expected: cleartextArgs, + }, + { + name: "peer-only: peer https + peer flags, client stays cleartext", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=http://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + { + name: "client-only: client https + server flags, peer stays cleartext", + tls: &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=http://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + }, + }, + { + name: "both: all eight flags, both schemes https", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + { + name: "peer mTLS opt-out: --peer-client-cert-auth absent, server mTLS still on", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurface(boolPtr(false)), + Client: cmSurface(boolPtr(true)), + }, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + peerFlags[0], peerFlags[1], peerFlags[2], + }, + }, + { + name: "client mTLS opt-out: --client-cert-auth absent, peer mTLS still on", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurface(boolPtr(true)), + Client: cmSurface(boolPtr(false)), + }, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS(name, tt.tls) + got := createArgs(name, nil, tlsArgsFor(ec)) + assert.Equal(t, tt.expected, got) + }) + } +} + +// TestTLSIndependenceSchemes asserts peerScheme/clientScheme are independent. +func TestTLSIndependenceSchemes(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantPeerScheme string + wantClientScheme string + }{ + {"neither", nil, "http", "http"}, + {"peer-only", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, "https", "http"}, + {"client-only", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, "http", "https"}, + {"both", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, "https", "https"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS("ec", tt.tls) + assert.Equal(t, tt.wantPeerScheme, peerScheme(ec), "peerScheme") + assert.Equal(t, tt.wantClientScheme, clientScheme(ec), "clientScheme") + // Endpoint generators must agree with the per-surface scheme. + _, peerURL := peerEndpointForOrdinalIndex(ec, 0) + assert.Contains(t, peerURL, tt.wantPeerScheme+"://", "peer endpoint scheme") + }) + } +} + +// stsContainer renders the StatefulSet for ec and returns the etcd container plus +// pod volumes, so cert mounts/volumes can be asserted per surface. +func stsContainer(t *testing.T, ec *ecv1alpha1.EtcdCluster) (corev1.Container, []corev1.Volume) { + t.Helper() + scheme := newScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + require.NoError(t, createOrPatchStatefulSet(t.Context(), discardLogger(), ec, c, 1, scheme)) + sts, err := getStatefulSet(t.Context(), c, ec.Name, ec.Namespace) + require.NoError(t, err) + require.Len(t, sts.Spec.Template.Spec.Containers, 1) + return sts.Spec.Template.Spec.Containers[0], sts.Spec.Template.Spec.Volumes +} + +func volumeNames(vols []corev1.Volume) []string { + out := make([]string, 0, len(vols)) + for _, v := range vols { + out = append(out, v.Name) + } + return out +} + +func mountNames(mounts []corev1.VolumeMount) []string { + out := make([]string, 0, len(mounts)) + for _, m := range mounts { + out = append(out, m.Name) + } + return out +} + +// TestTLSIndependenceMounts asserts the server-cert mount is gated on the client +// surface and the peer-cert mount on the peer surface, independently. +func TestTLSIndependenceMounts(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantServer bool + wantPeer bool + }{ + {"neither: no cert volumes/mounts", nil, false, false}, + {"peer-only: peer mount only", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, false, true}, + {"client-only: server mount only", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, true, false}, + {"both: server + peer mounts", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, true, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS("ec", tt.tls) + container, vols := stsContainer(t, ec) + vNames := volumeNames(vols) + mNames := mountNames(container.VolumeMounts) + + assert.Equal(t, tt.wantServer, contains(vNames, serverCertVolumeName), "server volume") + assert.Equal(t, tt.wantServer, contains(mNames, serverCertVolumeName), "server mount") + assert.Equal(t, tt.wantPeer, contains(vNames, peerCertVolumeName), "peer volume") + assert.Equal(t, tt.wantPeer, contains(mNames, peerCertVolumeName), "peer mount") + }) + } +} + +// TestTLSIndependenceMountsCoexistWithStorage asserts cert mounts and the storage +// data-dir mount coexist (append-not-assign). +func TestTLSIndependenceMountsCoexistWithStorage(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}) + ec.Spec.StorageSpec = &ecv1alpha1.StorageSpec{ + VolumeSizeRequest: mustQuantity("1Gi"), + } + container, _ := stsContainer(t, ec) + mNames := mountNames(container.VolumeMounts) + assert.Contains(t, mNames, volumeName, "storage data-dir mount") + assert.Contains(t, mNames, serverCertVolumeName, "server cert mount") + assert.Contains(t, mNames, peerCertVolumeName, "peer cert mount") +} + +// TestBuildClientTLSConfigGatedOnClientSurface asserts the operator's *tls.Config is +// built iff the client surface is configured, and nil otherwise (cleartext). +func TestBuildClientTLSConfigGatedOnClientSurface(t *testing.T) { + scheme := newScheme(t) + + // peer-only and neither: operator config must be nil (no secret read at all). + for _, tls := range []*ecv1alpha1.EtcdClusterTLS{ + nil, + {Peer: cmSurface(nil)}, + } { + ec := clusterWithTLS("ec", tls) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + cfg, err := buildClientTLSConfig(t.Context(), ec, c) + require.NoError(t, err) + assert.Nil(t, cfg, "operator client tls.Config must be nil without a client surface") + } + + // client surface set but secret missing => explicit error (not silent nil). + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + _, err := buildClientTLSConfig(t.Context(), ec, c) + require.Error(t, err, "missing client secret must be an error") + + // client surface set with a valid secret => non-nil config with the CA pinned. + cert, key, ca := genClientKeypair(t) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName("ec"), Namespace: "ns"}, + Data: map[string][]byte{ + tlsCertFile: cert, + tlsKeyFile: key, + tlsCAFile: ca, + }, + } + c = fake.NewClientBuilder().WithScheme(scheme).WithObjects(secret).Build() + cfg, err := buildClientTLSConfig(t.Context(), ec, c) + require.NoError(t, err) + require.NotNil(t, cfg) + assert.NotNil(t, cfg.RootCAs, "RootCAs must be pinned from the secret CA") + assert.Len(t, cfg.Certificates, 1) + + // client surface set, secret present but missing ca.crt => explicit error + // (not a silent fall-through to system roots). + secretNoCA := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName("ec"), Namespace: "ns"}, + Data: map[string][]byte{ + tlsCertFile: cert, + tlsKeyFile: key, + }, + } + c = fake.NewClientBuilder().WithScheme(scheme).WithObjects(secretNoCA).Build() + _, err = buildClientTLSConfig(t.Context(), ec, c) + require.Error(t, err, "missing ca.crt must be an explicit error") +} + +// TestValidateTLS covers the reconcile-time anti-misconfiguration backstop. +func TestValidateTLS(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantErr bool + }{ + {"nil tls is valid (fully cleartext)", nil, false}, + { + name: "valid cert-manager mTLS on both surfaces", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, + wantErr: false, + }, + { + name: "cert-manager provider without certManagerCfg is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + }, + wantErr: true, + }, + { + name: "certManagerCfg under auto provider is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "auto", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", IssuerName: "x", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "clientCertAuth (mTLS) with cert-manager but no issuerName is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(true), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer"}, + }, + }, + }, + wantErr: true, + }, + { + name: "server-only TLS (clientCertAuth false) without issuerName is allowed", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(false), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer", IssuerName: "x"}, + }, + }, + }, + wantErr: false, + }, + { + name: "auto provider on both surfaces is valid", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: &ecv1alpha1.TLSSurface{Provider: "auto"}, + Client: &ecv1alpha1.TLSSurface{Provider: "auto"}, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validateTLS(clusterWithTLS("ec", tt.tls)) + if tt.wantErr { + assert.NotEmpty(t, errs, "expected a validation error") + } else { + assert.Empty(t, errs, "expected no validation error: %v", errs) + } + }) + } +} + +func contains(s []string, v string) bool { + for _, x := range s { + if x == v { + return true + } + } + return false +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 3092c379..0ed33dca 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -2,9 +2,10 @@ package controller import ( "context" + "crypto/tls" + "crypto/x509" "errors" "fmt" - "log" "maps" "net" "slices" @@ -22,10 +23,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" "go.etcd.io/etcd-operator/internal/etcdutils" @@ -37,8 +40,124 @@ import ( const ( etcdDataDir = "/var/lib/etcd" volumeName = "etcd-data" + + // Cert mount points inside the etcd container. The server (client-surface) and + // peer secrets are mounted independently so a single-surface cluster only mounts + // the secret it actually needs. + serverCertVolumeName = "server-secret" + peerCertVolumeName = "peer-secret" + serverCertMountPath = "/etc/etcd/server-tls" + peerCertMountPath = "/etc/etcd/peer-tls" + + // Standard cert-secret keys (cert-manager and the auto provider both write these). + tlsCertFile = "tls.crt" + tlsKeyFile = "tls.key" + tlsCAFile = "ca.crt" + + schemeHTTP = "http" + schemeHTTPS = "https" ) +// peerScheme returns the URL scheme etcd serves/dials on its peer port. It is +// "https" iff the peer TLS surface is configured, otherwise "http". This is the +// single source of truth for the peer scheme: peer args and every peer-endpoint +// generator derive from it so the operator can never compute a peer URL with a +// scheme etcd does not serve. +func peerScheme(ec *ecv1alpha1.EtcdCluster) string { + if ec.Spec.TLS != nil && ec.Spec.TLS.Peer != nil { + return schemeHTTPS + } + return schemeHTTP +} + +// clientScheme returns the URL scheme etcd serves/dials on its client port. It is +// "https" iff the client TLS surface is configured, otherwise "http". Single source +// of truth for the client scheme (client args, client endpoints, and the operator's +// own dial scheme all derive from it). +func clientScheme(ec *ecv1alpha1.EtcdCluster) string { + if ec.Spec.TLS != nil && ec.Spec.TLS.Client != nil { + return schemeHTTPS + } + return schemeHTTP +} + +// peerTLSEnabled reports whether the peer TLS surface is configured. +func peerTLSEnabled(ec *ecv1alpha1.EtcdCluster) bool { + return ec.Spec.TLS != nil && ec.Spec.TLS.Peer != nil +} + +// clientTLSEnabled reports whether the client TLS surface is configured. +func clientTLSEnabled(ec *ecv1alpha1.EtcdCluster) bool { + return ec.Spec.TLS != nil && ec.Spec.TLS.Client != nil +} + +// clientCertAuthEnabled resolves a surface's ClientCertAuth toggle, defaulting to +// true (mTLS) when unset, matching the CRD default. A nil surface returns false. +func clientCertAuthEnabled(s *ecv1alpha1.TLSSurface) bool { + if s == nil { + return false + } + if s.ClientCertAuth == nil { + return true + } + return *s.ClientCertAuth +} + +// validateTLS is the reconcile-time anti-misconfiguration backstop for the two TLS +// surfaces. It re-checks the spec-only coherence rules that are *also* expressed as +// CEL XValidation markers on TLSSurface (api/v1alpha1/etcdcluster_types.go), so the +// operator still rejects an invalid spec on an apiserver that does not enforce CEL +// (e.g. pre-1.25, CEL feature-gated off, or a CR written directly to a fake client +// in tests). These are the spec-derivable rules ONLY: +// +// - provider 'cert-manager' => providerCfg.certManagerCfg must be set +// - provider auto/empty => providerCfg.certManagerCfg must NOT be set +// - clientCertAuth (mTLS) with cert-manager => issuerName must be set (a CA source) +// +// Rules that require reading cluster objects are intentionally NOT here: issuer +// existence and kind are enforced when the cert is provisioned (the cert-manager +// provider's validateCertificateConfig -> checkIssuerExists returns an error that +// requeues), and peer CA-capability / client-server CA equality live on the +// cert-manager Issuer and secret objects, not on this spec, so they cannot be +// checked purely from the EtcdCluster (plan Decision 2.5-2.7 / Decision 3). For the +// client surface specifically, the operator-client cert and the server cert both +// flow through the SINGLE client surface's issuer, so they share a CA by +// construction -- the cheap form of the client/server CA-match rule is satisfied +// without a runtime compare. +func validateTLS(ec *ecv1alpha1.EtcdCluster) field.ErrorList { + var errs field.ErrorList + if ec.Spec.TLS == nil { + return errs + } + base := field.NewPath("spec", "tls") + errs = append(errs, validateTLSSurface(base.Child("peer"), ec.Spec.TLS.Peer)...) + errs = append(errs, validateTLSSurface(base.Child("client"), ec.Spec.TLS.Client)...) + return errs +} + +func validateTLSSurface(path *field.Path, s *ecv1alpha1.TLSSurface) field.ErrorList { + var errs field.ErrorList + if s == nil { + return errs + } + hasCM := s.ProviderCfg.CertManagerCfg != nil + isCertManager := s.Provider == string(certificate.CertManager) + switch { + case isCertManager && !hasCM: + errs = append(errs, field.Required(path.Child("providerCfg", "certManagerCfg"), + "provider 'cert-manager' requires providerCfg.certManagerCfg")) + case !isCertManager && hasCM: + errs = append(errs, field.Invalid(path.Child("providerCfg", "certManagerCfg"), "", + "providerCfg.certManagerCfg may only be set when provider is 'cert-manager'")) + } + // mTLS requires a resolvable CA. With cert-manager that means an issuerName. + if clientCertAuthEnabled(s) && isCertManager && hasCM && s.ProviderCfg.CertManagerCfg.IssuerName == "" { + errs = append(errs, field.Required(path.Child("providerCfg", "certManagerCfg", "issuerName"), + "clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName")) + } + return errs +} + type etcdClusterState string const ( @@ -76,14 +195,73 @@ func reconcileStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1alpha return getStatefulSet(ctx, c, ec.Name, ec.Namespace) } -func defaultArgs(name string) []string { - return []string{ +// tlsArgs captures the per-surface TLS decisions that drive defaultArgs. Each +// surface is independent: the peer flag group and peer https scheme are gated on +// peerEnabled, the server flag group and client https scheme on clientEnabled, and +// each --*client-cert-auth flag on its own *CertAuth toggle. The zero value +// (everything false) yields byte-identical cleartext output to the pre-TLS args. +type tlsArgs struct { + peerEnabled bool + clientEnabled bool + peerCertAuth bool + clientCertAuth bool +} + +// tlsArgsFor derives tlsArgs from an EtcdCluster's two TLS surfaces. +func tlsArgsFor(ec *ecv1alpha1.EtcdCluster) tlsArgs { + var a tlsArgs + if ec.Spec.TLS != nil { + a.peerEnabled = ec.Spec.TLS.Peer != nil + a.clientEnabled = ec.Spec.TLS.Client != nil + a.peerCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Peer) + a.clientCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Client) + } + return a +} + +func defaultArgs(name string, tls tlsArgs) []string { + peerScheme := schemeHTTP + if tls.peerEnabled { + peerScheme = schemeHTTPS + } + clientScheme := schemeHTTP + if tls.clientEnabled { + clientScheme = schemeHTTPS + } + + args := []string{ "--name=$(POD_NAME)", - "--listen-peer-urls=http://0.0.0.0:2380", // TODO: only listen on 127.0.0.1 and host IP - "--listen-client-urls=http://0.0.0.0:2379", // TODO: only listen on 127.0.0.1 and host IP - fmt.Sprintf("--initial-advertise-peer-urls=http://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2380", name), - fmt.Sprintf("--advertise-client-urls=http://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2379", name), + fmt.Sprintf("--listen-peer-urls=%s://0.0.0.0:2380", peerScheme), // TODO: only listen on 127.0.0.1 and host IP + fmt.Sprintf("--listen-client-urls=%s://0.0.0.0:2379", clientScheme), // TODO: only listen on 127.0.0.1 and host IP + fmt.Sprintf("--initial-advertise-peer-urls=%s://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2380", peerScheme, name), + fmt.Sprintf("--advertise-client-urls=%s://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2379", clientScheme, name), + } + + // Server (client-surface) TLS flag group: emitted iff the client surface is set. + if tls.clientEnabled { + args = append(args, + fmt.Sprintf("--cert-file=%s/%s", serverCertMountPath, tlsCertFile), + fmt.Sprintf("--key-file=%s/%s", serverCertMountPath, tlsKeyFile), + fmt.Sprintf("--trusted-ca-file=%s/%s", serverCertMountPath, tlsCAFile), + ) + if tls.clientCertAuth { + args = append(args, "--client-cert-auth") + } } + + // Peer TLS flag group: emitted iff the peer surface is set. + if tls.peerEnabled { + args = append(args, + fmt.Sprintf("--peer-cert-file=%s/%s", peerCertMountPath, tlsCertFile), + fmt.Sprintf("--peer-key-file=%s/%s", peerCertMountPath, tlsKeyFile), + fmt.Sprintf("--peer-trusted-ca-file=%s/%s", peerCertMountPath, tlsCAFile), + ) + if tls.peerCertAuth { + args = append(args, "--peer-client-cert-auth") + } + } + + return args } func RemoveStringFromSlice(s []string, str string) []string { @@ -113,8 +291,8 @@ func getArgName(s string) string { return strings.TrimSpace(s) } -func createArgs(name string, etcdOptions []string) []string { - defaultArgs := defaultArgs(name) +func createArgs(name string, etcdOptions []string, tls tlsArgs) []string { + defaultArgs := defaultArgs(name, tls) if len(etcdOptions) > 0 { var argName string // Remove default arguments if conflicts with user supplied @@ -145,7 +323,7 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a { Name: "etcd", Command: []string{"/usr/local/bin/etcd"}, - Args: createArgs(ec.Name, ec.Spec.EtcdOptions), + Args: createArgs(ec.Name, ec.Spec.EtcdOptions, tlsArgsFor(ec)), Image: fmt.Sprintf("%s:%s", ec.Spec.ImageRegistry, ec.Spec.Version), Env: []corev1.EnvVar{ { @@ -188,28 +366,45 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a }, } - // mount server and peer certificate secret to each pods of the statefulset via PodSpec + // Mount the server and peer certificate secrets per surface, independently: the + // server (client-surface) secret iff the client surface is configured, the peer + // secret iff the peer surface is configured. A single-surface cluster only mounts + // the secret it needs. VolumeMounts are appended (not assigned) so they coexist + // with the storage data-dir mount added later. var certVolume []corev1.Volume - serverCertName := getServerCertName(ec.Name) - peerCertName := getPeerCertName(ec.Name) - if ec.Spec.TLS != nil { - serverCertVolume := corev1.Volume{ - Name: "server-secret", + var certMounts []corev1.VolumeMount + if clientTLSEnabled(ec) { + certVolume = append(certVolume, corev1.Volume{ + Name: serverCertVolumeName, VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{SecretName: serverCertName}, + Secret: &corev1.SecretVolumeSource{SecretName: getServerCertName(ec.Name)}, }, - } - peerCertVolume := corev1.Volume{ - Name: "peer-secret", + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: serverCertVolumeName, + MountPath: serverCertMountPath, + ReadOnly: true, + }) + } + if peerTLSEnabled(ec) { + certVolume = append(certVolume, corev1.Volume{ + Name: peerCertVolumeName, VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{SecretName: peerCertName}, + Secret: &corev1.SecretVolumeSource{SecretName: getPeerCertName(ec.Name)}, }, - } - certVolume = append(certVolume, serverCertVolume, peerCertVolume) + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: peerCertVolumeName, + MountPath: peerCertMountPath, + ReadOnly: true, + }) } if len(certVolume) != 0 { podSpec.Volumes = certVolume } + if len(certMounts) != 0 { + podSpec.Containers[0].VolumeMounts = append(podSpec.Containers[0].VolumeMounts, certMounts...) + } // Prepare pod template metadata podTemplateMetadata := metav1.ObjectMeta{ @@ -254,11 +449,15 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a if ec.Spec.StorageSpec != nil { - stsSpec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{ - Name: volumeName, - MountPath: etcdDataDir, - SubPathExpr: "$(POD_NAME)", - }} + // Append the storage data-dir mount so it coexists with any TLS cert mounts + // added above (assigning here would clobber them). + stsSpec.Template.Spec.Containers[0].VolumeMounts = append( + stsSpec.Template.Spec.Containers[0].VolumeMounts, + corev1.VolumeMount{ + Name: volumeName, + MountPath: etcdDataDir, + SubPathExpr: "$(POD_NAME)", + }) // Create a new volume claim template if ec.Spec.StorageSpec.VolumeSizeRequest.Cmp(resource.MustParse("1Mi")) < 0 { return fmt.Errorf("VolumeSizeRequest must be at least 1Mi") @@ -424,8 +623,8 @@ func configMapNameForEtcdCluster(ec *ecv1alpha1.EtcdCluster) string { func peerEndpointForOrdinalIndex(ec *ecv1alpha1.EtcdCluster, index int) (string, string) { name := fmt.Sprintf("%s-%d", ec.Name, index) - return name, fmt.Sprintf("http://%s-%d.%s.%s.svc.cluster.local:2380", - ec.Name, index, ec.Name, ec.Namespace) + return name, fmt.Sprintf("%s://%s-%d.%s.%s.svc.cluster.local:2380", + peerScheme(ec), ec.Name, index, ec.Name, ec.Namespace) } func newEtcdClusterState(ec *ecv1alpha1.EtcdCluster, replica int) *corev1.ConfigMap { @@ -478,9 +677,12 @@ func applyEtcdClusterState(ctx context.Context, ec *ecv1alpha1.EtcdCluster, repl return updateErr } -func clientEndpointForOrdinalIndex(sts *appsv1.StatefulSet, index int) string { - return fmt.Sprintf("http://%s-%d.%s.%s.svc.cluster.local:2379", - sts.Name, index, sts.Name, sts.Namespace) +// clientEndpointForOrdinalIndex builds the operator-facing client URL for one +// member. scheme ("http"/"https") MUST be the client surface's scheme (clientScheme) +// so the operator dials the same scheme etcd serves on its client port. +func clientEndpointForOrdinalIndex(sts *appsv1.StatefulSet, index int, scheme string) string { + return fmt.Sprintf("%s://%s-%d.%s.%s.svc.cluster.local:2379", + scheme, sts.Name, index, sts.Name, sts.Namespace) } func getStatefulSet(ctx context.Context, c client.Client, name, namespace string) (*appsv1.StatefulSet, error) { @@ -492,19 +694,21 @@ func getStatefulSet(ctx context.Context, c client.Client, name, namespace string return sts, nil } -func clientEndpointsFromStatefulsets(sts *appsv1.StatefulSet) []string { +// clientEndpointsFromStatefulsets builds the operator's client endpoints. scheme +// MUST be the client surface's scheme (clientScheme of the owning EtcdCluster). +func clientEndpointsFromStatefulsets(sts *appsv1.StatefulSet, scheme string) []string { var endpoints []string replica := int(*sts.Spec.Replicas) if replica > 0 { for i := 0; i < replica; i++ { - endpoints = append(endpoints, clientEndpointForOrdinalIndex(sts, i)) + endpoints = append(endpoints, clientEndpointForOrdinalIndex(sts, i, scheme)) } } return endpoints } -func areAllMembersHealthy(sts *appsv1.StatefulSet, logger logr.Logger) (bool, error) { - _, health, err := healthCheck(sts, logger) +func areAllMembersHealthy(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, sts *appsv1.StatefulSet, logger logr.Logger) (bool, error) { + _, health, err := healthCheck(ctx, ec, c, sts, logger) if err != nil { return false, err } @@ -519,16 +723,23 @@ func areAllMembersHealthy(sts *appsv1.StatefulSet, logger logr.Logger) (bool, er // healthCheck returns a memberList and an error. // If any member (excluding not yet started or already removed member) -// is unhealthy, the error won't be nil. -func healthCheck(sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListResponse, []etcdutils.EpHealth, error) { +// is unhealthy, the error won't be nil. The operator dials the client surface's +// scheme and, when the client surface is configured, presents its client TLS +// config; both derive from ec so the operator never mismatches what etcd serves. +func healthCheck(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListResponse, []etcdutils.EpHealth, error) { replica := int(*sts.Spec.Replicas) if replica == 0 { return nil, nil, nil } - endpoints := clientEndpointsFromStatefulsets(sts) + tlsConfig, err := buildClientTLSConfig(ctx, ec, c) + if err != nil { + return nil, nil, err + } + + endpoints := clientEndpointsFromStatefulsets(sts, clientScheme(ec)) - memberlistResp, err := etcdutils.MemberList(endpoints) + memberlistResp, err := etcdutils.MemberList(endpoints, tlsConfig) if err != nil { return nil, nil, err } @@ -544,7 +755,7 @@ func healthCheck(sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListR lg.Info("health checking", "replica", replica, "len(members)", memberCnt) endpoints = endpoints[:cnt] - healthInfos, err := etcdutils.ClusterHealth(endpoints) + healthInfos, err := etcdutils.ClusterHealth(endpoints, tlsConfig) if err != nil { return memberlistResp, nil, err } @@ -590,8 +801,8 @@ func parseValidityDuration(customizedDuration string, defaultDuration time.Durat return duration, nil } -func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) { - cmConfig := ec.Spec.TLS.ProviderCfg.CertManagerCfg +func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { + cmConfig := surface.ProviderCfg.CertManagerCfg if cmConfig == nil { return nil, fmt.Errorf("cert-manager configuration is not present") } @@ -626,15 +837,16 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Confi ValidityDuration: duration, AltNames: getAltNames, ExtraConfig: map[string]any{ - "issuerName": cmConfig.IssuerName, - "issuerKind": cmConfig.IssuerKind, + "issuerName": cmConfig.IssuerName, + "issuerKind": cmConfig.IssuerKind, + "issuerGroup": cmConfig.IssuerGroup, }, } return config, nil } -func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) { - autoConfig := ec.Spec.TLS.ProviderCfg.AutoCfg +func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { + autoConfig := surface.ProviderCfg.AutoCfg // Set default values for auto configuration if not present if autoConfig == nil { autoConfig = &ecv1alpha1.ProviderAutoConfig{ @@ -678,9 +890,14 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Con return config, nil } -func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client.Client, certName string) error { - // The TLS field is present but spec is empty - providerName := ec.Spec.TLS.Provider +// createCertificate provisions the certificate named certName from a SPECIFIC TLS +// surface (peer or client). The surface carries its own provider and provider +// config, so peer and client certs can flow through different issuers. +func createCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, surface *ecv1alpha1.TLSSurface, certName string) error { + logger := log.FromContext(ctx) + + // An empty provider on the surface defaults to the auto provider. + providerName := surface.Provider if providerName == "" { providerName = string(certificate.Auto) } @@ -693,12 +910,12 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client _, getCertError := cert.GetCertificateConfig(ctx, client.ObjectKey{Name: certName, Namespace: ec.Namespace}) if getCertError != nil { if k8serrors.IsNotFound(getCertError) { - log.Printf("Creating certificate: %s for etcd-operator: %s\n", certName, ec.Name) + logger.Info("Creating certificate", "certificate", certName, "etcdCluster", ec.Name) secretKey := client.ObjectKey{Name: certName, Namespace: ec.Namespace} switch certificate.ProviderType(providerName) { case certificate.Auto: - autoConfig, err := createAutoCertificateConfig(ec) + autoConfig, err := createAutoCertificateConfig(ec, surface) if err != nil { return fmt.Errorf("error creating auto certificate config: %w", err) } @@ -708,7 +925,7 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client } return nil case certificate.CertManager: - cmConfig, err := createCMCertificateConfig(ec) + cmConfig, err := createCMCertificateConfig(ec, surface) if err != nil { return fmt.Errorf("error creating cert-manager certificate config: %w", err) } @@ -719,7 +936,7 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client return nil default: // This should never happen // TODO: Use AuthProvider, since both AutoCfg and CertManagerCfg is not present - log.Printf("Error creating certificate, valid certificate provider not defined.") + logger.Info("Error creating certificate, valid certificate provider not defined", "certificate", certName) return nil } } else { @@ -730,9 +947,11 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client return nil } +// createClientCertificate provisions the operator's own client identity from the +// CLIENT surface. The operator authenticates to etcd's client port as a client. func createClientCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { certName := getClientCertName(ec.Name) - err := createCertificate(ec, ctx, c, certName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Client, certName) if err != nil { return err } @@ -743,9 +962,11 @@ func createClientCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c return err } +// createServerCertificate provisions etcd's server (client-facing) identity from +// the CLIENT surface. func createServerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { serverCertName := getServerCertName(ec.Name) - err := createCertificate(ec, ctx, c, serverCertName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Client, serverCertName) if err != nil { return err } @@ -756,9 +977,10 @@ func createServerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c return nil } +// createPeerCertificate provisions etcd's peer identity from the PEER surface. func createPeerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { peerCertName := getPeerCertName(ec.Name) - err := createCertificate(ec, ctx, c, peerCertName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Peer, peerCertName) if err != nil { return err } @@ -769,27 +991,78 @@ func createPeerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c cl return nil } +// applyEtcdMemberCerts provisions per-surface member certs independently: the +// server cert iff the client surface is configured, the peer cert iff the peer +// surface is configured. (The operator's own client cert is provisioned separately +// in fetchAndValidateState, also gated on the client surface.) func applyEtcdMemberCerts(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { - if ec.Spec.TLS != nil { - err := createServerCertificate(ctx, ec, c) - if err != nil { + if clientTLSEnabled(ec) { + if err := createServerCertificate(ctx, ec, c); err != nil { return err } - err = createPeerCertificate(ctx, ec, c) - if err != nil { + } + if peerTLSEnabled(ec) { + if err := createPeerCertificate(ctx, ec, c); err != nil { return err } } return nil } +// buildClientTLSConfig builds the operator's etcd-client *tls.Config from the +// CLIENT surface's secret. It returns (nil, nil) when the client surface is not +// configured, so the operator dials cleartext (byte-identical to the pre-TLS path). +// When configured it loads the operator client keypair and pins the server's CA; +// a missing ca.crt is an explicit error (rather than silently falling through to +// the system trust store, which would verify against the wrong roots and fail +// opaquely later). +func buildClientTLSConfig(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (*tls.Config, error) { + if !clientTLSEnabled(ec) { + return nil, nil + } + + secretName := getClientCertName(ec.Name) + secret := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: ec.Namespace}, secret); err != nil { + return nil, fmt.Errorf("failed to get operator client TLS secret %s/%s: %w", ec.Namespace, secretName, err) + } + + certPEM, ok := secret.Data[tlsCertFile] + if !ok || len(certPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q", ec.Namespace, secretName, tlsCertFile) + } + keyPEM, ok := secret.Data[tlsKeyFile] + if !ok || len(keyPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q", ec.Namespace, secretName, tlsKeyFile) + } + keyPair, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return nil, fmt.Errorf("failed to build operator client keypair from secret %s/%s: %w", ec.Namespace, secretName, err) + } + + caPEM, ok := secret.Data[tlsCAFile] + if !ok || len(caPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q (cannot verify the etcd server)", ec.Namespace, secretName, tlsCAFile) + } + caPool := x509.NewCertPool() + if !caPool.AppendCertsFromPEM(caPEM) { + return nil, fmt.Errorf("operator client TLS secret %s/%s has an unparseable %q", ec.Namespace, secretName, tlsCAFile) + } + + return &tls.Config{ + Certificates: []tls.Certificate{keyPair}, + RootCAs: caPool, + MinVersion: tls.VersionTLS12, + }, nil +} + func patchCertificateSecret(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, certSecretName string) error { getCertSecret := &corev1.Secret{} if err := c.Get(ctx, client.ObjectKey{Name: certSecretName, Namespace: ec.Namespace}, getCertSecret); err != nil { return err } - log.Printf("Setting ownerReference for certificate secret: %s", certSecretName) + log.FromContext(ctx).Info("Setting ownerReference for certificate secret", "secret", certSecretName) if err := controllerutil.SetControllerReference(ec, getCertSecret, c.Scheme()); err != nil { return err } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 67592316..0f27b7a3 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -205,7 +205,7 @@ func TestClientEndpointForOrdinalIndex(t *testing.T) { for _, tt := range tests { t.Run(fmt.Sprintf("index %d", tt.index), func(t *testing.T) { - result := clientEndpointForOrdinalIndex(sts, tt.index) + result := clientEndpointForOrdinalIndex(sts, tt.index, "http") assert.Equal(t, tt.expectedResult, result) }) } @@ -384,7 +384,7 @@ func TestClientEndpointsFromStatefulsets(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := clientEndpointsFromStatefulsets(tt.statefulSet) + result := clientEndpointsFromStatefulsets(tt.statefulSet, "http") assert.Equal(t, tt.expectedResult, result) }) } @@ -423,7 +423,13 @@ func TestAreAllMembersHealthy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { logger := logr.Discard() // Use a no-op logger for testing - result, err := areAllMembersHealthy(tt.statefulSet, logger) + // Cleartext cluster: buildClientTLSConfig returns nil and the dial + // times out, exactly as before the TLS-config threading. + ec := &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sts", Namespace: "default"}, + } + fakeClient := fake.NewClientBuilder().Build() + result, err := areAllMembersHealthy(t.Context(), ec, fakeClient, tt.statefulSet, logger) assert.Equal(t, tt.expectedResult, result) if tt.expectedError != nil { assert.Error(t, err) @@ -788,7 +794,7 @@ func TestCreatingArgs(t *testing.T) { } for _, tt := range tests { t.Run(tt.testName, func(t *testing.T) { - result := createArgs(tt.clusterName, tt.etcdOptions) + result := createArgs(tt.clusterName, tt.etcdOptions, tlsArgs{}) assert.Equal(t, tt.expectedResult, result) }) } @@ -910,6 +916,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -920,19 +927,17 @@ func TestCreateAutoCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: &ecv1alpha1.ProviderAutoConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "custom.example.com", - Organization: []string{"Test Org"}, - ValidityDuration: "720h", // 30 days - AltNames: ecv1alpha1.AltNames{ - DNSNames: []string{"custom1.example.com", "custom2.example.com"}, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.Auto), + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: &ecv1alpha1.ProviderAutoConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "custom.example.com", + Organization: []string{"Test Org"}, + ValidityDuration: "720h", // 30 days + AltNames: ecv1alpha1.AltNames{ + DNSNames: []string{"custom1.example.com", "custom2.example.com"}, }, }, }, @@ -956,13 +961,11 @@ func TestCreateAutoCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: nil, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.Auto), + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: nil, }, }, expected: &certInterface.Config{ @@ -982,7 +985,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := createAutoCertificateConfig(tt.ec) + result, err := createAutoCertificateConfig(tt.ec, tt.surface) if tt.wantErr { require.Error(t, err) @@ -1004,6 +1007,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -1014,23 +1018,22 @@ func TestCreateCMCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.CertManager), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "cm.example.com", - Organization: []string{"CM Org"}, - ValidityDuration: "1440h", // 60 days - AltNames: ecv1alpha1.AltNames{ - DNSNames: []string{"cm1.example.com", "cm2.example.com"}, - }, - }, - IssuerName: "test-issuer", - IssuerKind: "ClusterIssuer", + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.CertManager), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "cm.example.com", + Organization: []string{"CM Org"}, + ValidityDuration: "1440h", // 60 days + AltNames: ecv1alpha1.AltNames{ + DNSNames: []string{"cm1.example.com", "cm2.example.com"}, }, }, + IssuerName: "test-issuer", + IssuerKind: "ClusterIssuer", + IssuerGroup: "example.io", }, }, }, @@ -1043,8 +1046,9 @@ func TestCreateCMCertificateConfig(t *testing.T) { IPs: make([]net.IP, 2), }, ExtraConfig: map[string]any{ - "issuerName": "test-issuer", - "issuerKind": "ClusterIssuer", + "issuerName": "test-issuer", + "issuerKind": "ClusterIssuer", + "issuerGroup": "example.io", }, }, wantErr: false, @@ -1056,13 +1060,11 @@ func TestCreateCMCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.CertManager), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: nil, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.CertManager), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: nil, }, }, expected: nil, @@ -1072,7 +1074,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := createCMCertificateConfig(tt.ec) + result, err := createCMCertificateConfig(tt.ec, tt.surface) if tt.wantErr { require.Error(t, err) diff --git a/internal/etcdutils/etcdutils.go b/internal/etcdutils/etcdutils.go index 00f9493e..b061ed66 100644 --- a/internal/etcdutils/etcdutils.go +++ b/internal/etcdutils/etcdutils.go @@ -2,6 +2,7 @@ package etcdutils import ( "context" + "crypto/tls" "errors" "fmt" "sort" @@ -17,12 +18,19 @@ import ( clientv3 "go.etcd.io/etcd/client/v3" ) -func MemberList(eps []string) (*clientv3.MemberListResponse, error) { +// All etcd-client entry points take a trailing optional *tls.Config. When nil the +// client dials cleartext (byte-identical to the pre-TLS behaviour); when non-nil +// the config is set on clientv3.Config.TLS so the operator authenticates to a TLS +// etcd over its client port. The caller builds this from the client TLS surface +// only (see controller.buildClientTLSConfig). + +func MemberList(eps []string, tlsConfig *tls.Config) (*clientv3.MemberListResponse, error) { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -120,7 +128,7 @@ func FindLearnerStatus(healthInfos []EpHealth, logger logr.Logger) (uint64, *cli return learner, learnerStatus } -func ClusterHealth(eps []string) ([]EpHealth, error) { +func ClusterHealth(eps []string, tlsConfig *tls.Config) ([]EpHealth, error) { lg, err := logutil.CreateDefaultZapLogger(zap.InfoLevel) if err != nil { return nil, err @@ -133,6 +141,7 @@ func ClusterHealth(eps []string) ([]EpHealth, error) { DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } cfgs = append(cfgs, cfg) @@ -200,12 +209,13 @@ func ClusterHealth(eps []string) ([]EpHealth, error) { return healthList, nil } -func AddMember(eps []string, peerURLs []string, learner bool) (*clientv3.MemberAddResponse, error) { +func AddMember(eps []string, peerURLs []string, learner bool, tlsConfig *tls.Config) (*clientv3.MemberAddResponse, error) { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -230,12 +240,13 @@ func AddMember(eps []string, peerURLs []string, learner bool) (*clientv3.MemberA return c.MemberAdd(ctx, peerURLs) } -func PromoteLearner(eps []string, learnerId uint64) error { +func PromoteLearner(eps []string, learnerId uint64, tlsConfig *tls.Config) error { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -257,12 +268,13 @@ func PromoteLearner(eps []string, learnerId uint64) error { return err } -func RemoveMember(eps []string, memberID uint64) error { +func RemoveMember(eps []string, memberID uint64, tlsConfig *tls.Config) error { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) diff --git a/internal/etcdutils/etcdutils_test.go b/internal/etcdutils/etcdutils_test.go index a7cee425..1bb0eb9f 100644 --- a/internal/etcdutils/etcdutils_test.go +++ b/internal/etcdutils/etcdutils_test.go @@ -1,6 +1,14 @@ package etcdutils import ( + "crypto/ecdsa" + "crypto/elliptic" + crand "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "math/big" + "net" "testing" "time" @@ -60,7 +68,7 @@ func TestMemberList(t *testing.T) { assert.NoError(t, err) eps := []string{"http://localhost:2379"} - resp, err := MemberList(eps) + resp, err := MemberList(eps, nil) assert.NoError(t, err) assert.NotNil(t, resp) assert.Greater(t, len(resp.Members), 0) @@ -68,10 +76,82 @@ func TestMemberList(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - resp, err := MemberList(eps) + resp, err := MemberList(eps, nil) assert.Error(t, err) assert.Nil(t, resp) }) + + // The optional *tls.Config must actually be threaded onto clientv3.Config.TLS, + // not dropped. Point the client at a raw TLS listener over an https:// endpoint: + // a non-nil config drives a TLS handshake against the listener (which records + // it), proving the parameter reaches the dialer. + t.Run("NonNilTLSConfigIsApplied", func(t *testing.T) { + ln, err := tls.Listen("tcp", "127.0.0.1:0", selfSignedServerTLS(t)) + assert.NoError(t, err) + defer ln.Close() + + handshakeSeen := make(chan struct{}, 1) + go func() { + for { + conn, aerr := ln.Accept() + if aerr != nil { + return + } + if tc, ok := conn.(*tls.Conn); ok { + // A successful handshake means the client dialed TLS. + if tc.Handshake() == nil { + select { + case handshakeSeen <- struct{}{}: + default: + } + } + } + _ = conn.Close() + } + }() + + eps := []string{"https://" + ln.Addr().String()} + // The gRPC dial is lazy, so MemberList will error (the listener is not etcd), + // but the dial itself must perform a TLS handshake when a config is supplied. + // Run it in the background so the test returns as soon as the handshake lands + // rather than waiting out the client's dial-retry timeout. + go func() { + _, _ = MemberList(eps, &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}) + }() + + select { + case <-handshakeSeen: + // good: the *tls.Config drove a real TLS handshake. + case <-time.After(10 * time.Second): + t.Fatal("expected a TLS handshake from the threaded *tls.Config, saw none") + } + }) +} + +// selfSignedServerTLS returns a *tls.Config with a self-signed server cert for the +// raw TLS listener used to prove the client-side *tls.Config is applied. +func selfSignedServerTLS(t *testing.T) *tls.Config { + priv, err := ecdsa.GenerateKey(elliptic.P256(), crand.Reader) + if err != nil { + t.Fatalf("genkey: %v", err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "127.0.0.1"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + der, err := x509.CreateCertificate(crand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("createcert: %v", err) + } + return &tls.Config{ + Certificates: []tls.Certificate{{Certificate: [][]byte{der}, PrivateKey: priv}}, + MinVersion: tls.VersionTLS12, + } } func TestClusterHealth(t *testing.T) { @@ -80,7 +160,7 @@ func TestClusterHealth(t *testing.T) { t.Run("ReturnsHealthStatus", func(t *testing.T) { eps := []string{"http://localhost:2379"} - health, err := ClusterHealth(eps) + health, err := ClusterHealth(eps, nil) assert.NoError(t, err) assert.NotNil(t, health) assert.Greater(t, len(health), 0) @@ -89,7 +169,7 @@ func TestClusterHealth(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - health, err := ClusterHealth(eps) + health, err := ClusterHealth(eps, nil) assert.NoError(t, err) assert.Equal(t, "http://invalid:2379", health[0].Ep) assert.Equal(t, false, health[0].Health) @@ -104,7 +184,7 @@ func TestAddMember(t *testing.T) { t.Run("AddsNewMember", func(t *testing.T) { eps := []string{"http://localhost:2379"} peerURLs := []string{"http://127.0.0.1:2380"} - resp, err := AddMember(eps, peerURLs, false) + resp, err := AddMember(eps, peerURLs, false, nil) assert.NoError(t, err) assert.NotNil(t, resp) assert.Greater(t, len(resp.Members), 0) @@ -113,7 +193,7 @@ func TestAddMember(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} peerURLs := []string{"http://127.0.0.1:2380"} - resp, err := AddMember(eps, peerURLs, false) + resp, err := AddMember(eps, peerURLs, false, nil) assert.Error(t, err) assert.Nil(t, resp) }) @@ -126,17 +206,17 @@ func TestPromoteLearner(t *testing.T) { t.Run("PromotesMember", func(t *testing.T) { eps := []string{"http://localhost:2379"} peerURLs := []string{"http://test123:2380"} - addResp, err := AddMember(eps, peerURLs, true) + addResp, err := AddMember(eps, peerURLs, true, nil) assert.NoError(t, err) - err = PromoteLearner(eps, addResp.Member.ID) + err = PromoteLearner(eps, addResp.Member.ID, nil) assert.Error(t, err) assert.Equal(t, err.Error(), "etcdserver: can only promote a learner member which is in sync with leader") }) t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - err := PromoteLearner(eps, 12345) + err := PromoteLearner(eps, 12345, nil) assert.Error(t, err) }) } @@ -147,7 +227,7 @@ func TestRemoveMember(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - err := RemoveMember(eps, 12345) + err := RemoveMember(eps, 12345, nil) assert.Error(t, err) }) } diff --git a/pkg/certificate/cert_manager/provider.go b/pkg/certificate/cert_manager/provider.go index 84bf2c27..635890db 100644 --- a/pkg/certificate/cert_manager/provider.go +++ b/pkg/certificate/cert_manager/provider.go @@ -27,8 +27,9 @@ import ( ) const ( - IssuerNameKey = "issuerName" - IssuerKindKey = "issuerKind" + IssuerNameKey = "issuerName" + IssuerKindKey = "issuerKind" + IssuerGroupKey = "issuerGroup" ) type CertManagerProvider struct { @@ -234,8 +235,9 @@ func (cm *CertManagerProvider) GetCertificateConfig(ctx context.Context, }, ValidityDuration: cmCertificate.Spec.Duration.Duration, ExtraConfig: map[string]any{ - IssuerNameKey: cmCertificate.Spec.IssuerRef.Name, - IssuerKindKey: cmCertificate.Spec.IssuerRef.Kind, + IssuerNameKey: cmCertificate.Spec.IssuerRef.Name, + IssuerKindKey: cmCertificate.Spec.IssuerRef.Kind, + IssuerGroupKey: cmCertificate.Spec.IssuerRef.Group, }, } @@ -320,6 +322,10 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey if !isValid { return fmt.Errorf("value for %s not correctly provided, try again", IssuerKindKey) } + // issuerGroup is optional: empty (or absent) leaves IssuerRef.Group == "" so + // cert-manager defaults it to "cert-manager.io". Unlike issuerName/issuerKind, + // absence is legal here, so use comma-ok and do NOT error on a missing key. + issuerGroup, _ := cfg.ExtraConfig[IssuerGroupKey].(string) certificateResource := &certmanagerv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -335,8 +341,9 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey DNSNames: cfg.AltNames.DNSNames, IPAddresses: strings.Fields(strings.Trim(fmt.Sprint(cfg.AltNames.IPs), "[]")), IssuerRef: cmmeta.IssuerReference{ - Name: issuerName, - Kind: issuerKind, + Name: issuerName, + Kind: issuerKind, + Group: issuerGroup, }, Duration: &metav1.Duration{Duration: cfg.ValidityDuration}, }, diff --git a/pkg/certificate/interfaces/interface.go b/pkg/certificate/interfaces/interface.go index 90d06bfb..526cc28f 100644 --- a/pkg/certificate/interfaces/interface.go +++ b/pkg/certificate/interfaces/interface.go @@ -71,7 +71,6 @@ type Config struct { Organization []string AltNames AltNames ValidityDuration time.Duration - CABundleSecret string // ExtraConfig contains provider specific configurations. ExtraConfig map[string]any diff --git a/test/e2e/auto_provider_test.go b/test/e2e/auto_provider_test.go index a7ce8a4a..006bcaf8 100644 --- a/test/e2e/auto_provider_test.go +++ b/test/e2e/auto_provider_test.go @@ -137,16 +137,9 @@ func TestClusterAutoCertCreation(t *testing.T) { Spec: ecv1alpha1.EtcdClusterSpec{ Size: size, Version: etcdVersion, - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: &ecv1alpha1.ProviderAutoConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "etcd-operator-system", - ValidityDuration: "8760h", - }, - }, - }, + TLS: &ecv1alpha1.EtcdClusterTLS{ + Peer: autoSurface(), + Client: autoSurface(), }, }, } diff --git a/test/e2e/cert_manager_test.go b/test/e2e/cert_manager_test.go index 558f0238..df6d4e92 100644 --- a/test/e2e/cert_manager_test.go +++ b/test/e2e/cert_manager_test.go @@ -175,18 +175,9 @@ func TestClusterCertCreation(t *testing.T) { Spec: ecv1alpha1.EtcdClusterSpec{ Size: size, Version: etcdVersion, - TLS: &ecv1alpha1.TLSCertificate{ - Provider: "cert-manager", - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "etcd-operator-system", - ValidityDuration: "90h", - }, - IssuerKind: cmIssuerType, - IssuerName: cmIssuerName, - }, - }, + TLS: &ecv1alpha1.EtcdClusterTLS{ + Peer: certManagerSurface(cmIssuerType, cmIssuerName), + Client: certManagerSurface(cmIssuerType, cmIssuerName), }, }, } diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index 663155ee..e5b115a7 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -338,3 +338,38 @@ func httpViaProxy(ctx context.Context, r *rest.Request, pod corev1.Pod, failpoin Do(ctx) return result.Error() } + +// certManagerSurface returns a cert-manager TLSSurface for one TLS surface (peer +// or client). The peer/client split is configured independently in the new CRD +// shape; the e2e helpers set both surfaces to the same issuer to mirror the +// pre-split single-toggle behaviour. +func certManagerSurface(issuerKind, issuerName string) *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "etcd-operator-system", + ValidityDuration: "90h", + }, + IssuerKind: issuerKind, + IssuerName: issuerName, + }, + }, + } +} + +// autoSurface returns an auto-provider TLSSurface for one TLS surface. +func autoSurface() *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "auto", + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: &ecv1alpha1.ProviderAutoConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "etcd-operator-system", + ValidityDuration: "8760h", + }, + }, + }, + } +} From ddb14577c491ac199579b31b550deb13730773e3 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 23:52:33 -0400 Subject: [PATCH 2/6] fix(test): expect issuerGroup in cert-manager GetCertificateConfig 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 Signed-off-by: Xavier Lange --- test/e2e/cert_manager_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/e2e/cert_manager_test.go b/test/e2e/cert_manager_test.go index df6d4e92..2962b793 100644 --- a/test/e2e/cert_manager_test.go +++ b/test/e2e/cert_manager_test.go @@ -58,6 +58,10 @@ func TestCertManagerProvider(t *testing.T) { ExtraConfig: map[string]any{ "issuerName": cmIssuerName, "issuerKind": cmIssuerType, + // issuerGroup is optional; with no group set, cert-manager leaves + // IssuerRef.Group == "" and GetCertificateConfig echoes that back, so + // the expected ExtraConfig must include the empty key to DeepEqual-match. + "issuerGroup": "", }, } From 723e91e1288cf4f5322972224a039f6be265979c Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 23:48:26 -0400 Subject: [PATCH 3/6] fix(controller): publish not-ready addresses on the headless service 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: 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 Signed-off-by: Xavier Lange --- internal/controller/utils.go | 13 +++++++++++++ internal/controller/utils_test.go | 4 ++++ 2 files changed, 17 insertions(+) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 0ed33dca..c5f3628b 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -590,6 +590,19 @@ func createHeadlessServiceIfNotExist(ctx context.Context, logger logr.Logger, c Spec: corev1.ServiceSpec{ ClusterIP: "None", // Key for headless service Selector: labels, + // PublishNotReadyAddresses makes the headless service publish A + // records for pods that are not yet Ready. This is REQUIRED for a + // peer-TLS cluster to form: 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 DNS name. A joining member is not Ready until it has + // joined, so without this its IP is absent from the service's A + // record, the peer cert SAN check fails ("does not match any of + // DNSNames"), the peer handshake is rejected, and the member + // crashloops -- deadlocking the cluster at one member. (Cleartext + // peer traffic does not hit this path, so the pre-TLS behaviour is + // unchanged for non-TLS clusters.) + PublishNotReadyAddresses: true, }, } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 0f27b7a3..08e03890 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -170,6 +170,10 @@ func TestCreateHeadlessServiceIfNotExist(t *testing.T) { err = fakeClient.Get(ctx, client.ObjectKey{Name: "test-etcd", Namespace: "default"}, service) assert.NoError(t, err) assert.Equal(t, "None", service.Spec.ClusterIP) + // Required for peer-TLS cluster formation: a joining (not-yet-Ready) member + // must be resolvable so etcd's peer cert SAN check (isHostInDNS) accepts it. + assert.True(t, service.Spec.PublishNotReadyAddresses, + "headless service must publish not-ready addresses so peer-TLS members can join") assert.Equal(t, map[string]string{ "app": "test-etcd", "controller": "test-etcd", From 970c6689b3fde28ef458cef4996f41d2b558ceb8 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Thu, 18 Jun 2026 01:26:46 -0400 Subject: [PATCH 4/6] docs(tls): note create-not-flip migration and operator ServerName behaviour 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 Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 5 +++++ config/crd/bases/operator.etcd.io_etcdclusters.yaml | 5 +++++ internal/controller/utils.go | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index 7667e44d..b2d66659 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -48,6 +48,11 @@ type EtcdClusterSpec struct { // means that surface is served/dialed in cleartext. When TLS itself is nil, the // entire cluster (peer + client + operator client) is cleartext, byte-identical // to a TLS-free deployment. + // + // TLS is effectively create-time: it flows into the pod template and cert mounts. + // Toggling it on (or off) on a running cluster rolls the StatefulSet into a mixed + // http/https membership whose peers cannot connect, dropping quorum. The supported + // path is a NEW TLS cluster plus data migration, not an in-place flip. TLS *EtcdClusterTLS `json:"tls,omitempty"` // etcd configuration options are passed as command line arguments to the etcd container, refer to etcd documentation for configuration options applicable for the version of etcd being used. EtcdOptions []string `json:"etcdOptions,omitempty"` diff --git a/config/crd/bases/operator.etcd.io_etcdclusters.yaml b/config/crd/bases/operator.etcd.io_etcdclusters.yaml index 3843e523..59af77cf 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -1075,6 +1075,11 @@ spec: means that surface is served/dialed in cleartext. When TLS itself is nil, the entire cluster (peer + client + operator client) is cleartext, byte-identical to a TLS-free deployment. + + TLS is effectively create-time: it flows into the pod template and cert mounts. + Toggling it on (or off) on a running cluster rolls the StatefulSet into a mixed + http/https membership whose peers cannot connect, dropping quorum. The supported + path is a NEW TLS cluster plus data migration, not an in-place flip. properties: client: description: |- diff --git a/internal/controller/utils.go b/internal/controller/utils.go index c5f3628b..9de9a638 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -1029,6 +1029,11 @@ func applyEtcdMemberCerts(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c cli // a missing ca.crt is an explicit error (rather than silently falling through to // the system trust store, which would verify against the wrong roots and fail // opaquely later). +// +// No ServerName is set, so Go verifies the dialed pod FQDN against the server +// cert's SANs. The default SANs cover the pod FQDNs; if a user supplies custom +// AltNames they MUST still include *.{name}.{ns}.svc.cluster.local or every +// operator health check fails verification. func buildClientTLSConfig(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (*tls.Config, error) { if !clientTLSEnabled(ec) { return nil, nil From b4dd42ab1df92d2b6a04e583fe5829e061361547 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 22:40:21 -0400 Subject: [PATCH 5/6] feat(status): add TLSReady condition and TLS lifecycle events 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 Signed-off-by: Xavier Lange --- internal/controller/etcdcluster_controller.go | 57 ++- internal/controller/tls_status.go | 330 +++++++++++++++ internal/controller/tls_status_test.go | 393 ++++++++++++++++++ 3 files changed, 775 insertions(+), 5 deletions(-) create mode 100644 internal/controller/tls_status.go create mode 100644 internal/controller/tls_status_test.go diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index 3f1f1767..471926b1 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -60,6 +60,7 @@ type reconcileState struct { sts *appsv1.StatefulSet // associated StatefulSet for the cluster memberListResp *clientv3.MemberListResponse // member list fetched from the etcd cluster memberHealth []etcdutils.EpHealth // health information for each etcd member + tls tlsReadiness // verdict on the cluster's TLS surfaces (drives the TLSReady condition) } // +kubebuilder:rbac:groups=operator.etcd.io,resources=etcdclusters,verbs=get;list;watch;create;update;patch;delete @@ -137,8 +138,11 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c // an incoherent TLS spec by requeuing rather than silently proceeding into cert // provisioning, which would otherwise fail deep with a less actionable error. if errs := validateTLS(ec); len(errs) > 0 { - logger.Error(errs.ToAggregate(), "invalid TLS configuration; not reconciling until fixed", + agg := errs.ToAggregate() + logger.Error(agg, "invalid TLS configuration; not reconciling until fixed", "etcdCluster", ec.Name) + r.Recorder.Eventf(ec, nil, corev1.EventTypeWarning, reasonClientCertificateError, + "ValidateTLS", "invalid TLS configuration: %v", agg) return nil, ctrl.Result{RequeueAfter: requeueDuration}, nil } @@ -147,6 +151,8 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c if clientTLSEnabled(ec) { if err := createClientCertificate(ctx, ec, r.Client); err != nil { logger.Error(err, "Failed to create operator client certificate", "etcdCluster", ec.Name) + r.Recorder.Eventf(ec, nil, corev1.EventTypeWarning, reasonClientCertificateError, + "CreateClientCertificate", "failed to create operator client certificate: %v", err) } } else { // Cleartext (no client surface) is a supported mode; log at Info, not Error, @@ -155,6 +161,14 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c "etcdCluster", ec.Name) } + // Evaluate the configured TLS surfaces (Recorder-free verdict), emit any failure + // Event at this controller boundary, and stash the verdict for the TLSReady + // condition. This is the single place the runtime peer-CA / issuer / client-cert + // checks are surfaced as Events; the verdict itself is computed without the + // Recorder so the util/status helpers stay upstream-clean. + tlsState := evaluateTLSReadiness(ctx, ec, r.Client) + r.recordTLSEvent(ec, tlsState) + logger.Info("Reconciling EtcdCluster", "spec", ec.Spec) sts, err := getStatefulSet(ctx, r.Client, ec.Name, ec.Namespace) @@ -176,7 +190,7 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c // If the version to be reconciled is unsupported, throw an error. if len(sts.Spec.Template.Spec.Containers) == 0 { logger.Error(err, "StatefulSet has no containers yet") - return &reconcileState{cluster: ec, sts: sts}, ctrl.Result{}, nil + return &reconcileState{cluster: ec, sts: sts, tls: tlsState}, ctrl.Result{}, nil } stsImage := sts.Spec.Template.Spec.Containers[0].Image // Note: createOrPatchStatefulSet() only supports the "registry-path:image" format. @@ -186,7 +200,7 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c if idx == -1 { logger.Error(err, "could not extract image version from StatefulSet image", "image", stsImage) - return &reconcileState{cluster: ec, sts: sts}, ctrl.Result{}, nil + return &reconcileState{cluster: ec, sts: sts, tls: tlsState}, ctrl.Result{}, nil } currentVersion := stsImage[idx+1:] targetVersion := ec.Spec.Version @@ -204,7 +218,7 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c "target", targetVersion, "error", err, ) - return &reconcileState{cluster: ec, sts: sts}, ctrl.Result{}, nil + return &reconcileState{cluster: ec, sts: sts, tls: tlsState}, ctrl.Result{}, nil } else { if err != nil { logger.Error(err, "unsupported upgrade path between current and target versions", @@ -220,7 +234,7 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c } } - return &reconcileState{cluster: ec, sts: sts}, ctrl.Result{}, nil + return &reconcileState{cluster: ec, sts: sts, tls: tlsState}, ctrl.Result{}, nil } // bootstrapStatefulSet ensures that the foundational Kubernetes objects for @@ -293,6 +307,8 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re // is cleartext) and derive client endpoints from the client surface's scheme. clientTLSConfig, err := buildClientTLSConfig(ctx, s.cluster, r.Client) if err != nil { + r.Recorder.Eventf(s.cluster, nil, corev1.EventTypeWarning, reasonClientCertificateError, + "BuildClientTLSConfig", "failed to build operator client TLS config: %v", err) return ctrl.Result{}, err } cScheme := clientScheme(s.cluster) @@ -585,6 +601,37 @@ func (r *EtcdClusterReconciler) updateConditions(s *reconcileState) { meta.SetStatusCondition(&s.cluster.Status.Conditions, availableCondition) meta.SetStatusCondition(&s.cluster.Status.Conditions, progressingCondition) meta.SetStatusCondition(&s.cluster.Status.Conditions, degradedCondition) + + // TLSReady reflects the health of the configured TLS surfaces. When no surface + // is configured (TLSNotConfigured) the condition is omitted entirely -- no TLS, + // no TLS condition -- and any stale prior condition is removed so flipping a + // cluster back to cleartext doesn't leave a dangling TLSReady. + if s.tls.configured { + meta.SetStatusCondition(&s.cluster.Status.Conditions, s.tls.condition(s.cluster.Generation)) + } else { + meta.RemoveStatusCondition(&s.cluster.Status.Conditions, tlsReadyConditionType) + } +} + +// recordTLSEvent emits a CR Event reflecting the TLS verdict computed by +// evaluateTLSReadiness. It is the controller boundary that turns the Recorder-free +// verdict into a Kubernetes Event: a Warning (reason == the verdict reason) for any +// failure, and a single Normal "TLSReady" only when a configured cluster transitions +// INTO the ready state (so a steady-state healthy cluster doesn't emit a TLSReady +// event every reconcile). The reason strings match the TLSReady condition reasons. +func (r *EtcdClusterReconciler) recordTLSEvent(ec *ecv1alpha1.EtcdCluster, t tlsReadiness) { + if !t.configured { + return + } + if !t.ready { + r.Recorder.Eventf(ec, nil, corev1.EventTypeWarning, t.reason, "TLSReady", "%s", t.message) + return + } + // Ready: only emit on transition (the prior TLSReady condition was not already True). + prior := meta.FindStatusCondition(ec.Status.Conditions, tlsReadyConditionType) + if prior == nil || prior.Status != metav1.ConditionTrue { + r.Recorder.Eventf(ec, nil, corev1.EventTypeNormal, reasonTLSReady, "TLSReady", "%s", t.message) + } } // isCertManagerCRDPresent checks if cert-manager CRDs are installed in the cluster diff --git a/internal/controller/tls_status.go b/internal/controller/tls_status.go new file mode 100644 index 00000000..ce17af9e --- /dev/null +++ b/internal/controller/tls_status.go @@ -0,0 +1,330 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "fmt" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// tlsReadyConditionType is the status condition Type that reflects the health of +// the cluster's configured TLS surfaces (peer and/or client). A single condition +// reflects BOTH surfaces; its Reason/Message carry which surface failed. +const tlsReadyConditionType = "TLSReady" + +// TLSReady reason vocabulary. These strings are shared verbatim between the +// status condition Reason and the CR Event Reason, so a `kubectl describe` shows +// the same vocabulary in both the conditions table and the events list. +// +// State -> reason mapping (see evaluateTLSReadiness): +// +// both surfaces nil ............................... TLSNotConfigured (omit condition) +// all configured surfaces healthy ................ TLSReady (Status=True) +// cert-manager issuer missing/wrong-kind ......... IssuerNotFound (Status=False) +// peer issuer is a self-signed leaf (no shared CA) PeerCANotShared (Status=False) +// operator client cert/secret not usable ......... ClientCertificateError (Status=False) +// operator-client CA != server CA ................ ClientServerCAMismatch (Status=False) +// a configured surface's cert/secret not yet ready SurfaceNotReady (Status=False) +const ( + // reasonTLSNotConfigured indicates no TLS surface is configured (cleartext). + // When this is the reason the TLSReady condition is omitted entirely (no TLS, + // no TLS condition) rather than reported as a failure. + reasonTLSNotConfigured = "TLSNotConfigured" + // reasonTLSReady indicates every configured surface is provisioned, the + // issuer(s) are CA-capable, and (for the client surface) the operator's own + // client *tls.Config built and verified. + reasonTLSReady = "TLSReady" + // reasonIssuerNotFound indicates a cert-manager surface references an Issuer + // or ClusterIssuer that does not exist (or is the wrong kind). + reasonIssuerNotFound = "IssuerNotFound" + // reasonPeerCANotShared indicates the peer surface uses a non-CA / self-signed + // *leaf* issuer that cannot establish shared peer trust, so members cannot + // mutually verify and the cluster would silently cap at one member. + reasonPeerCANotShared = "PeerCANotShared" + // reasonClientServerCAMismatch indicates the operator's client CA does not + // match the CA that signs the etcd server cert. + reasonClientServerCAMismatch = "ClientServerCAMismatch" + // reasonClientCertificateError indicates the operator's own client cert/secret + // could not be provisioned or built into a usable *tls.Config. + reasonClientCertificateError = "ClientCertificateError" + // reasonSurfaceNotReady indicates a configured surface's cert/secret has not + // yet materialized (a transient bring-up state, resolved by requeue). + reasonSurfaceNotReady = "SurfaceNotReady" +) + +// tlsReadiness is the Recorder-free evaluation of a cluster's TLS surfaces. It is +// the single source of truth that both updateConditions (the TLSReady condition) +// and the controller boundary (CR Events) consume, so the condition Reason and the +// Event Reason can never drift. A nil *tlsReadiness (configured == false) means no +// TLS surface is configured and the condition is omitted. +type tlsReadiness struct { + // configured is true iff at least one surface (peer or client) is set. + configured bool + // ready is true iff every configured surface is healthy. + ready bool + // reason is one of the reason* constants above. + reason string + // message is a human-readable detail for the condition/event. + message string +} + +// condition renders the tlsReadiness as a metav1.Condition for SetStatusCondition. +// It must only be called when configured == true (the caller omits the condition +// otherwise). +func (t tlsReadiness) condition(generation int64) metav1.Condition { + status := metav1.ConditionFalse + if t.ready { + status = metav1.ConditionTrue + } + return metav1.Condition{ + Type: tlsReadyConditionType, + Status: status, + ObservedGeneration: generation, + LastTransitionTime: metav1.Now(), + Reason: t.reason, + Message: t.message, + } +} + +// evaluateTLSReadiness inspects a cluster's configured TLS surfaces against live +// cluster objects (cert-manager Issuers and the operator's client secret) and +// returns a typed verdict. It is deliberately Recorder-free and read-only: it +// performs Gets but emits no Events and mutates nothing, so it is safe to call +// from both the reconcile boundary (which turns the verdict into Events) and from +// unit tests (which assert the verdict directly). +// +// The runtime checks implemented here are the ones the independence reshape +// deferred because they are NOT expressible in CEL (they read issuer/secret +// objects, not the EtcdCluster spec): +// +// - PeerCANotShared: a configured cert-manager peer surface whose issuer is a +// self-signed leaf (Issuer.Spec.SelfSigned != nil) cannot establish shared +// peer trust. We detect it from the Issuer object's kind/spec. +// - ClientServerCAMismatch: see note below — with the independent-surface shape +// the operator-client and server certs both flow through the SINGLE client +// surface's issuer, so they share a CA by construction. A genuine cross-issuer +// mismatch is therefore not reachable from a single client surface; the +// residual runtime compare is left as a documented TODO. +func evaluateTLSReadiness(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) tlsReadiness { + if !peerTLSEnabled(ec) && !clientTLSEnabled(ec) { + return tlsReadiness{configured: false, reason: reasonTLSNotConfigured, message: "no TLS surface configured"} + } + + // Peer surface: require a CA-capable shared issuer. A self-signed leaf issuer + // mints CA:FALSE certs that are each their own root, so members cannot mutually + // verify (the silent "stuck at 1 member" hazard). + if peerTLSEnabled(ec) { + if r, ok := checkPeerCAShared(ctx, ec, c); !ok { + return r + } + } + + // Client surface: the operator must be able to build a usable client *tls.Config + // from its own client secret (keypair + pinned server CA). A missing/unusable + // secret during bring-up surfaces as SurfaceNotReady; a malformed cert surfaces + // as ClientCertificateError. + if clientTLSEnabled(ec) { + // First confirm the cert-manager issuer (if any) for the client surface + // resolves, so a missing issuer reports IssuerNotFound rather than the + // downstream "secret not ready". + if r, ok := checkSurfaceIssuer(ctx, ec, c, "client", ec.Spec.TLS.Client); !ok { + return r + } + if r, ok := checkClientSecretUsable(ctx, ec, c); !ok { + return r + } + } else if peerTLSEnabled(ec) { + // Peer-only cluster: also confirm the peer issuer resolves (the CA-shared + // check above already read it, but checkSurfaceIssuer reports a missing + // issuer distinctly from a non-CA one). + if r, ok := checkSurfaceIssuer(ctx, ec, c, "peer", ec.Spec.TLS.Peer); !ok { + return r + } + } + + return tlsReadiness{ + configured: true, + ready: true, + reason: reasonTLSReady, + message: tlsReadyMessage(ec), + } +} + +// tlsReadyMessage describes which surfaces are healthy. +func tlsReadyMessage(ec *ecv1alpha1.EtcdCluster) string { + switch { + case peerTLSEnabled(ec) && clientTLSEnabled(ec): + return "peer and client TLS surfaces provisioned and verified" + case peerTLSEnabled(ec): + return "peer TLS surface provisioned and verified" + default: + return "client TLS surface provisioned and verified; operator client identity verified" + } +} + +// checkSurfaceIssuer confirms that a cert-manager surface's issuer exists and is a +// supported kind. Non-cert-manager (auto) surfaces always pass (the operator mints +// a CA itself). Returns (verdict, ok=false) on failure. +func checkSurfaceIssuer(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, + surfaceName string, surface *ecv1alpha1.TLSSurface) (tlsReadiness, bool) { + cm := surface.ProviderCfg.CertManagerCfg + if surface.Provider != "cert-manager" || cm == nil { + return tlsReadiness{}, true + } + if err := issuerResolves(ctx, c, cm.IssuerName, cm.IssuerKind, ec.Namespace); err != nil { + return tlsReadiness{ + configured: true, + ready: false, + reason: reasonIssuerNotFound, + message: fmt.Sprintf("%s surface: %v", surfaceName, err), + }, false + } + return tlsReadiness{}, true +} + +// checkPeerCAShared inspects the peer surface's cert-manager issuer and fails with +// PeerCANotShared when it is a self-signed leaf issuer (no shared CA). Auto-provider +// peer surfaces pass: the operator's auto provider mints a shared CA itself. +func checkPeerCAShared(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (tlsReadiness, bool) { + peer := ec.Spec.TLS.Peer + cm := peer.ProviderCfg.CertManagerCfg + if peer.Provider != "cert-manager" || cm == nil { + return tlsReadiness{}, true + } + + caCapable, err := issuerIsCACapable(ctx, c, cm.IssuerName, cm.IssuerKind, ec.Namespace) + if err != nil { + // Issuer missing/unreadable: report it as IssuerNotFound (checkSurfaceIssuer + // would say the same); fold it here so a single read drives both. + return tlsReadiness{ + configured: true, + ready: false, + reason: reasonIssuerNotFound, + message: fmt.Sprintf("peer surface: %v", err), + }, false + } + if !caCapable { + return tlsReadiness{ + configured: true, + ready: false, + reason: reasonPeerCANotShared, + message: fmt.Sprintf("peer TLS issuer %q (%s) is a self-signed leaf; members cannot mutually "+ + "verify. Use a CA-capable issuer (SelfSigned -> CA Certificate isCA:true -> CA Issuer)", + cm.IssuerName, cm.IssuerKind), + }, false + } + return tlsReadiness{}, true +} + +// checkClientSecretUsable confirms the operator's client secret yields a usable +// *tls.Config. A NotFound/transient bring-up state reports SurfaceNotReady (resolved +// by requeue); a malformed secret reports ClientCertificateError. +func checkClientSecretUsable(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (tlsReadiness, bool) { + _, err := buildClientTLSConfig(ctx, ec, c) + if err == nil { + return tlsReadiness{}, true + } + // A not-yet-materialized secret is a transient bring-up state, not a misconfig. + secretName := getClientCertName(ec.Name) + notReady := &corev1.Secret{} + getErr := c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: ec.Namespace}, notReady) + if k8serrors.IsNotFound(getErr) { + return tlsReadiness{ + configured: true, + ready: false, + reason: reasonSurfaceNotReady, + message: fmt.Sprintf("client surface: operator client secret %q not yet materialized", secretName), + }, false + } + return tlsReadiness{ + configured: true, + ready: false, + reason: reasonClientCertificateError, + message: fmt.Sprintf("client surface: operator client TLS not usable: %v", err), + }, false +} + +// issuerResolves returns nil iff the named cert-manager (cluster)issuer exists and +// the kind is supported. +func issuerResolves(ctx context.Context, c client.Client, name, kind, namespace string) error { + switch kind { + case "Issuer": + issuer := &certmanagerv1.Issuer{} + if err := c.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, issuer); err != nil { + if k8serrors.IsNotFound(err) { + return fmt.Errorf("issuer %q not found in namespace %s", name, namespace) + } + return fmt.Errorf("reading issuer %q: %w", name, err) + } + case "ClusterIssuer": + clusterIssuer := &certmanagerv1.ClusterIssuer{} + if err := c.Get(ctx, client.ObjectKey{Name: name}, clusterIssuer); err != nil { + if k8serrors.IsNotFound(err) { + return fmt.Errorf("clusterIssuer %q not found", name) + } + return fmt.Errorf("reading clusterIssuer %q: %w", name, err) + } + default: + return fmt.Errorf("unsupported issuer kind: %q", kind) + } + return nil +} + +// issuerIsCACapable reports whether the named cert-manager (cluster)issuer can act +// as a shared CA for peer trust. A SelfSigned issuer (Spec.SelfSigned != nil) mints +// self-signed *leaf* certs and is NOT CA-capable; every other issuer config (CA, +// ACME, Vault, Venafi, external) issues certs chained to a CA shared across members +// and IS considered CA-capable. The error is non-nil only when the issuer cannot be +// read (treated as IssuerNotFound by the caller). +func issuerIsCACapable(ctx context.Context, c client.Client, name, kind, namespace string) (bool, error) { + var spec certmanagerv1.IssuerSpec + switch kind { + case "Issuer": + issuer := &certmanagerv1.Issuer{} + if err := c.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, issuer); err != nil { + if k8serrors.IsNotFound(err) { + return false, fmt.Errorf("issuer %q not found in namespace %s", name, namespace) + } + return false, fmt.Errorf("reading issuer %q: %w", name, err) + } + spec = issuer.Spec + case "ClusterIssuer": + clusterIssuer := &certmanagerv1.ClusterIssuer{} + if err := c.Get(ctx, client.ObjectKey{Name: name}, clusterIssuer); err != nil { + if k8serrors.IsNotFound(err) { + return false, fmt.Errorf("clusterIssuer %q not found", name) + } + return false, fmt.Errorf("reading clusterIssuer %q: %w", name, err) + } + spec = clusterIssuer.Spec + default: + return false, fmt.Errorf("unsupported issuer kind: %q", kind) + } + // A self-signed issuer mints CA:FALSE leaves that are each their own root. + if spec.SelfSigned != nil { + return false, nil + } + return true, nil +} diff --git a/internal/controller/tls_status_test.go b/internal/controller/tls_status_test.go new file mode 100644 index 00000000..6b710bf1 --- /dev/null +++ b/internal/controller/tls_status_test.go @@ -0,0 +1,393 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// schemeWithCertManager extends the base test scheme with cert-manager Issuer / +// ClusterIssuer so the fake client can serve the runtime issuer reads. +func schemeWithCertManager(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := newScheme(t) + require.NoError(t, certmanagerv1.AddToScheme(scheme)) + return scheme +} + +// caClusterIssuer is a CA-capable ClusterIssuer (peer trust can be shared). +func caClusterIssuer(name string) *certmanagerv1.ClusterIssuer { + return &certmanagerv1.ClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: certmanagerv1.IssuerSpec{ + IssuerConfig: certmanagerv1.IssuerConfig{CA: &certmanagerv1.CAIssuer{SecretName: name + "-ca"}}, + }, + } +} + +// selfSignedClusterIssuer is a self-signed *leaf* ClusterIssuer (NOT CA-capable). +func selfSignedClusterIssuer(name string) *certmanagerv1.ClusterIssuer { + return &certmanagerv1.ClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: certmanagerv1.IssuerSpec{ + IssuerConfig: certmanagerv1.IssuerConfig{SelfSigned: &certmanagerv1.SelfSignedIssuer{}}, + }, + } +} + +// cmSurfaceFor builds a cert-manager surface pointing at a named ClusterIssuer. +func cmSurfaceFor(issuer string) *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", + IssuerName: issuer, + }, + }, + } +} + +// clientSecretFor returns the operator's client secret (valid keypair + CA) for ec. +func clientSecretFor(t *testing.T, ec *ecv1alpha1.EtcdCluster) *corev1.Secret { + t.Helper() + certPEM, keyPEM, caPEM := genClientKeypair(t) + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName(ec.Name), Namespace: ec.Namespace}, + Data: map[string][]byte{ + tlsCertFile: certPEM, + tlsKeyFile: keyPEM, + tlsCAFile: caPEM, + }, + } +} + +// TestEvaluateTLSReadiness drives the Recorder-free verdict through every state in +// the reason vocabulary: both-nil, fully-wired ready, and each failure reason. +func TestEvaluateTLSReadiness(t *testing.T) { + scheme := schemeWithCertManager(t) + + t.Run("both surfaces nil => TLSNotConfigured, not configured", func(t *testing.T) { + ec := clusterWithTLS("ec", nil) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.False(t, got.configured) + assert.Equal(t, reasonTLSNotConfigured, got.reason) + }) + + t.Run("client surface fully wired => TLSReady=True", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurfaceFor("ca-issuer")}) + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(caClusterIssuer("ca-issuer"), clientSecretFor(t, ec)).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.True(t, got.configured) + assert.True(t, got.ready) + assert.Equal(t, reasonTLSReady, got.reason) + }) + + t.Run("peer+client fully wired => TLSReady=True", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurfaceFor("ca-issuer"), + Client: cmSurfaceFor("ca-issuer"), + }) + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(caClusterIssuer("ca-issuer"), clientSecretFor(t, ec)).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.True(t, got.ready) + assert.Equal(t, reasonTLSReady, got.reason) + }) + + t.Run("client issuer missing => IssuerNotFound", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurfaceFor("nope")}) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.True(t, got.configured) + assert.False(t, got.ready) + assert.Equal(t, reasonIssuerNotFound, got.reason) + }) + + t.Run("peer issuer is self-signed leaf => PeerCANotShared", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurfaceFor("selfsigned")}) + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(selfSignedClusterIssuer("selfsigned")).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.False(t, got.ready) + assert.Equal(t, reasonPeerCANotShared, got.reason) + }) + + t.Run("peer issuer missing => IssuerNotFound (not PeerCANotShared)", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurfaceFor("nope")}) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.Equal(t, reasonIssuerNotFound, got.reason) + }) + + t.Run("client secret not yet materialized => SurfaceNotReady", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurfaceFor("ca-issuer")}) + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(caClusterIssuer("ca-issuer")).Build() // issuer present, secret absent + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.False(t, got.ready) + assert.Equal(t, reasonSurfaceNotReady, got.reason) + }) + + t.Run("client secret malformed => ClientCertificateError", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurfaceFor("ca-issuer")}) + badSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName(ec.Name), Namespace: ec.Namespace}, + Data: map[string][]byte{ + tlsCertFile: []byte("not a pem"), + tlsKeyFile: []byte("not a pem"), + tlsCAFile: []byte("not a pem"), + }, + } + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(caClusterIssuer("ca-issuer"), badSecret).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.False(t, got.ready) + assert.Equal(t, reasonClientCertificateError, got.reason) + }) + + t.Run("peer-only with auto provider => TLSReady (auto mints a shared CA)", func(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{ + Peer: &ecv1alpha1.TLSSurface{Provider: "auto"}, + }) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + got := evaluateTLSReadiness(context.Background(), ec, c) + assert.True(t, got.ready) + assert.Equal(t, reasonTLSReady, got.reason) + }) +} + +// TestIssuerIsCACapable unit-tests the peer-CA predicate against representative +// issuer inputs: a self-signed leaf is NOT CA-capable; a CA issuer is; a missing +// issuer is an error (folded into IssuerNotFound by the caller). +func TestIssuerIsCACapable(t *testing.T) { + scheme := schemeWithCertManager(t) + ctx := context.Background() + + t.Run("self-signed leaf is not CA-capable", func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(selfSignedClusterIssuer("ss")).Build() + ok, err := issuerIsCACapable(ctx, c, "ss", "ClusterIssuer", "ns") + require.NoError(t, err) + assert.False(t, ok) + }) + + t.Run("CA issuer is CA-capable", func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(caClusterIssuer("ca")).Build() + ok, err := issuerIsCACapable(ctx, c, "ca", "ClusterIssuer", "ns") + require.NoError(t, err) + assert.True(t, ok) + }) + + t.Run("namespaced self-signed Issuer is not CA-capable", func(t *testing.T) { + iss := &certmanagerv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{Name: "ss", Namespace: "ns"}, + Spec: certmanagerv1.IssuerSpec{ + IssuerConfig: certmanagerv1.IssuerConfig{SelfSigned: &certmanagerv1.SelfSignedIssuer{}}, + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(iss).Build() + ok, err := issuerIsCACapable(ctx, c, "ss", "Issuer", "ns") + require.NoError(t, err) + assert.False(t, ok) + }) + + t.Run("missing issuer errors", func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme).Build() + _, err := issuerIsCACapable(ctx, c, "gone", "ClusterIssuer", "ns") + assert.Error(t, err) + }) + + t.Run("unsupported kind errors", func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme).Build() + _, err := issuerIsCACapable(ctx, c, "x", "Bogus", "ns") + assert.Error(t, err) + }) +} + +// TestTLSReadyCondition drives updateConditions and asserts the TLSReady condition +// is set/omitted with the correct Status and Reason for each verdict. +func TestTLSReadyCondition(t *testing.T) { + r := &EtcdClusterReconciler{} + + newStateWithTLS := func(t tlsReadiness) *reconcileState { + return &reconcileState{ + cluster: &ecv1alpha1.EtcdCluster{ObjectMeta: metav1.ObjectMeta{Name: "ec", Namespace: "ns", Generation: 7}}, + tls: t, + } + } + + t.Run("not configured => no TLSReady condition", func(t *testing.T) { + s := newStateWithTLS(tlsReadiness{configured: false, reason: reasonTLSNotConfigured}) + r.updateConditions(s) + assert.Nil(t, meta.FindStatusCondition(s.cluster.Status.Conditions, tlsReadyConditionType)) + }) + + t.Run("ready => TLSReady True", func(t *testing.T) { + s := newStateWithTLS(tlsReadiness{configured: true, ready: true, reason: reasonTLSReady, message: "ok"}) + r.updateConditions(s) + cond := meta.FindStatusCondition(s.cluster.Status.Conditions, tlsReadyConditionType) + require.NotNil(t, cond) + assert.Equal(t, metav1.ConditionTrue, cond.Status) + assert.Equal(t, reasonTLSReady, cond.Reason) + assert.Equal(t, int64(7), cond.ObservedGeneration) + }) + + failureReasons := []string{ + reasonIssuerNotFound, + reasonPeerCANotShared, + reasonClientServerCAMismatch, + reasonClientCertificateError, + reasonSurfaceNotReady, + } + for _, reason := range failureReasons { + t.Run("failure "+reason+" => TLSReady False with reason", func(t *testing.T) { + s := newStateWithTLS(tlsReadiness{configured: true, ready: false, reason: reason, message: "boom"}) + r.updateConditions(s) + cond := meta.FindStatusCondition(s.cluster.Status.Conditions, tlsReadyConditionType) + require.NotNil(t, cond) + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, reason, cond.Reason) + }) + } + + t.Run("flipping back to cleartext removes a stale TLSReady", func(t *testing.T) { + s := newStateWithTLS(tlsReadiness{configured: true, ready: true, reason: reasonTLSReady}) + r.updateConditions(s) + require.NotNil(t, meta.FindStatusCondition(s.cluster.Status.Conditions, tlsReadyConditionType)) + // Now reconcile the same cluster with no TLS surface. + s.tls = tlsReadiness{configured: false, reason: reasonTLSNotConfigured} + r.updateConditions(s) + assert.Nil(t, meta.FindStatusCondition(s.cluster.Status.Conditions, tlsReadyConditionType)) + }) +} + +// drain collects all currently-buffered events from a FakeRecorder. +func drain(rec *events.FakeRecorder) []string { + var out []string + for { + select { + case e := <-rec.Events: + out = append(out, e) + default: + return out + } + } +} + +func hasEventWith(recorded []string, eventType, reason string) bool { + for _, e := range recorded { + // FakeRecorder formats as " ". + if len(e) >= len(eventType)+1+len(reason) && + e[:len(eventType)] == eventType && + e[len(eventType)+1:len(eventType)+1+len(reason)] == reason { + return true + } + } + return false +} + +// TestRecordTLSEvent asserts the controller boundary emits the expected Event +// (type + reason) for each verdict, and that a steady-state ready cluster does NOT +// re-emit the Normal TLSReady event. +func TestRecordTLSEvent(t *testing.T) { + ec := &ecv1alpha1.EtcdCluster{ObjectMeta: metav1.ObjectMeta{Name: "ec", Namespace: "ns"}} + + t.Run("not configured => no event", func(t *testing.T) { + rec := events.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Recorder: rec} + r.recordTLSEvent(ec, tlsReadiness{configured: false, reason: reasonTLSNotConfigured}) + assert.Empty(t, drain(rec)) + }) + + failureCases := []string{ + reasonIssuerNotFound, + reasonPeerCANotShared, + reasonClientCertificateError, + reasonSurfaceNotReady, + reasonClientServerCAMismatch, + } + for _, reason := range failureCases { + t.Run("failure "+reason+" => Warning event with matching reason", func(t *testing.T) { + rec := events.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Recorder: rec} + r.recordTLSEvent(ec, tlsReadiness{configured: true, ready: false, reason: reason, message: "m"}) + got := drain(rec) + assert.True(t, hasEventWith(got, corev1.EventTypeWarning, reason), "events=%v", got) + }) + } + + t.Run("ready (fresh) => Normal TLSReady event", func(t *testing.T) { + rec := events.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Recorder: rec} + fresh := &ecv1alpha1.EtcdCluster{ObjectMeta: metav1.ObjectMeta{Name: "ec", Namespace: "ns"}} + r.recordTLSEvent(fresh, tlsReadiness{configured: true, ready: true, reason: reasonTLSReady, message: "m"}) + got := drain(rec) + assert.True(t, hasEventWith(got, corev1.EventTypeNormal, reasonTLSReady), "events=%v", got) + }) + + t.Run("ready (already True) => no repeat event", func(t *testing.T) { + rec := events.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Recorder: rec} + steady := &ecv1alpha1.EtcdCluster{ObjectMeta: metav1.ObjectMeta{Name: "ec", Namespace: "ns"}} + meta.SetStatusCondition(&steady.Status.Conditions, metav1.Condition{ + Type: tlsReadyConditionType, Status: metav1.ConditionTrue, Reason: reasonTLSReady, + }) + r.recordTLSEvent(steady, tlsReadiness{configured: true, ready: true, reason: reasonTLSReady, message: "m"}) + assert.Empty(t, drain(rec)) + }) +} + +// TestFetchAndValidateStateEmitsTLSEvents exercises the integration at the +// fetchAndValidateState boundary: a self-signed-leaf peer issuer must produce a +// PeerCANotShared Warning Event AND stash a False verdict on the returned state. +func TestFetchAndValidateStateEmitsTLSEvents(t *testing.T) { + scheme := schemeWithCertManager(t) + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurfaceFor("selfsigned")}) + ec.Spec.Version = "v3.6.12" + + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(ec, selfSignedClusterIssuer("selfsigned")).Build() + rec := events.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Client: c, Scheme: scheme, Recorder: rec} + + state, _, err := r.fetchAndValidateState(context.Background(), + ctrl.Request{NamespacedName: types.NamespacedName{Name: "ec", Namespace: "ns"}}) + require.NoError(t, err) + require.NotNil(t, state) + assert.Equal(t, reasonPeerCANotShared, state.tls.reason) + assert.False(t, state.tls.ready) + assert.True(t, hasEventWith(drain(rec), corev1.EventTypeWarning, reasonPeerCANotShared)) +} From e2364e3566fe362b24e5583900872fbc7389d9b1 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Thu, 18 Jun 2026 01:29:13 -0400 Subject: [PATCH 6/6] docs(tls): clarify PeerCANotShared is conservative and ValidateTLS reason 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 Signed-off-by: Xavier Lange --- internal/controller/etcdcluster_controller.go | 3 +++ internal/controller/tls_status.go | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index 471926b1..b29140fd 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -141,6 +141,9 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c agg := errs.ToAggregate() logger.Error(agg, "invalid TLS configuration; not reconciling until fixed", "etcdCluster", ec.Name) + // Reuses the ClientCertificateError reason: this path predates evaluateTLSReadiness + // (the spec is too malformed to evaluate surfaces), so there is no per-surface verdict + // reason to emit; the aggregated CEL message in the note carries the specifics. r.Recorder.Eventf(ec, nil, corev1.EventTypeWarning, reasonClientCertificateError, "ValidateTLS", "invalid TLS configuration: %v", agg) return nil, ctrl.Result{RequeueAfter: requeueDuration}, nil diff --git a/internal/controller/tls_status.go b/internal/controller/tls_status.go index ce17af9e..1d0483c7 100644 --- a/internal/controller/tls_status.go +++ b/internal/controller/tls_status.go @@ -60,8 +60,12 @@ const ( // or ClusterIssuer that does not exist (or is the wrong kind). reasonIssuerNotFound = "IssuerNotFound" // reasonPeerCANotShared indicates the peer surface uses a non-CA / self-signed - // *leaf* issuer that cannot establish shared peer trust, so members cannot - // mutually verify and the cluster would silently cap at one member. + // *leaf* issuer that cannot establish shared peer trust by chain verification. + // This is a conservative WARNING, not a hard cap: the operator mounts ONE shared + // peer secret (getPeerCertName) into every pod, so even a self-signed leaf is in + // every member's --peer-trusted-ca-file and a multi-member quorum still forms. + // We still flag it because that arrangement breaks the moment peer certs are + // rotated per-member or the secret stops being shared. reasonPeerCANotShared = "PeerCANotShared" // reasonClientServerCAMismatch indicates the operator's client CA does not // match the CA that signs the etcd server cert.