diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index f007ae04..b2d66659 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -43,8 +43,17 @@ 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 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"` // PodTemplate is the pod template to use for the etcd cluster. @@ -68,9 +77,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 +173,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 +184,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..59af77cf 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,300 @@ 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. + + 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: - 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..b29140fd 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 @@ -133,19 +134,44 @@ 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 { + 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 + } + + // 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) + r.Recorder.Eventf(ec, nil, corev1.EventTypeWarning, reasonClientCertificateError, + "CreateClientCertificate", "failed to create operator client certificate: %v", err) } } 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) } + // 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) @@ -167,7 +193,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. @@ -177,7 +203,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 @@ -195,7 +221,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", @@ -211,7 +237,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 @@ -260,7 +286,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 +306,16 @@ 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 { + 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) + // 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 +358,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 +377,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 +386,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 +410,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 +424,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 } @@ -568,6 +604,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_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/tls_status.go b/internal/controller/tls_status.go new file mode 100644 index 00000000..1d0483c7 --- /dev/null +++ b/internal/controller/tls_status.go @@ -0,0 +1,334 @@ +/* +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 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. + 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)) +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 3092c379..9de9a638 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") @@ -391,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, }, } @@ -424,8 +636,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 +690,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 +707,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 +736,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 + } - memberlistResp, err := etcdutils.MemberList(endpoints) + endpoints := clientEndpointsFromStatefulsets(sts, clientScheme(ec)) + + memberlistResp, err := etcdutils.MemberList(endpoints, tlsConfig) if err != nil { return nil, nil, err } @@ -544,7 +768,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 +814,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 +850,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 +903,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 +923,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 +938,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 +949,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 +960,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 +975,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 +990,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 +1004,83 @@ 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). +// +// 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 + } + + 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..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", @@ -205,7 +209,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 +388,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 +427,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 +798,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 +920,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -920,19 +931,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 +965,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 +989,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 +1011,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -1014,23 +1022,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 +1050,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 +1064,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 +1078,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..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": "", }, } @@ -175,18 +179,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", + }, + }, + }, + } +} diff --git a/test/e2e/tls_multimember_test.go b/test/e2e/tls_multimember_test.go new file mode 100644 index 00000000..9df48600 --- /dev/null +++ b/test/e2e/tls_multimember_test.go @@ -0,0 +1,548 @@ +/* +Copyright 2025. + +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 e2e + +import ( + "context" + "encoding/json" + "fmt" + "log" + "strings" + "testing" + "time" + + certv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apiextensionsV1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/e2e-framework/klient/k8s" + "sigs.k8s.io/e2e-framework/klient/wait" + "sigs.k8s.io/e2e-framework/pkg/envconf" + "sigs.k8s.io/e2e-framework/pkg/features" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" + test_utils "go.etcd.io/etcd-operator/test/utils" + "go.etcd.io/etcd/api/v3/etcdserverpb" +) + +// The two-tier shared CA issuer that a multi-member peer-TLS cluster REQUIRES. +// A bare SelfSigned issuer (cmIssuer in cert_manager_test.go) mints CA:FALSE leaf +// certs that are each their own root, so members cannot mutually verify and the +// cluster caps at one member -- which is exactly the negative case asserted by +// TestTLSPeerCANotShared below. This shared CA chain is: +// +// SelfSigned ClusterIssuer -> CA Certificate (isCA:true) -> CA ClusterIssuer +// +// referenced by both the peer and client surfaces so every member's cert chains +// to one shared root. +const ( + caBootstrapIssuerName = "etcd-tls-e2e-selfsigned-bootstrap" + caCertificateName = "etcd-tls-e2e-ca" + caCertSecretName = "etcd-tls-e2e-ca-tls" + caIssuerName = "etcd-tls-e2e-ca-issuer" + caIssuerType = "ClusterIssuer" + caCertNamespace = "cert-manager" +) + +// ensureClusterCAIssuer creates (idempotently) the SelfSigned->CA Certificate->CA +// ClusterIssuer chain, returning once the CA ClusterIssuer is Ready so certs +// referencing it can actually be issued. +func ensureClusterCAIssuer(ctx context.Context, t *testing.T, cfg *envconf.Config) { + t.Helper() + r := cfg.Client().Resources() + + bootstrap := &certv1.ClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: caBootstrapIssuerName}, + Spec: certv1.IssuerSpec{ + IssuerConfig: certv1.IssuerConfig{SelfSigned: &certv1.SelfSignedIssuer{}}, + }, + } + createIfAbsent(ctx, t, cfg, bootstrap, caBootstrapIssuerName, "") + + caCert := &certv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: caCertificateName, Namespace: caCertNamespace}, + Spec: certv1.CertificateSpec{ + IsCA: true, + CommonName: "etcd-tls-e2e-ca", + SecretName: caCertSecretName, + PrivateKey: &certv1.CertificatePrivateKey{Algorithm: certv1.ECDSAKeyAlgorithm, Size: 256}, + IssuerRef: cmmeta.IssuerReference{ + Name: caBootstrapIssuerName, + Kind: "ClusterIssuer", + Group: "cert-manager.io", + }, + }, + } + createIfAbsent(ctx, t, cfg, caCert, caCertificateName, caCertNamespace) + + caIssuer := &certv1.ClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: caIssuerName}, + Spec: certv1.IssuerSpec{ + IssuerConfig: certv1.IssuerConfig{ + CA: &certv1.CAIssuer{SecretName: caCertSecretName}, + }, + }, + } + createIfAbsent(ctx, t, cfg, caIssuer, caIssuerName, "") + + // Wait for the CA ClusterIssuer to be Ready -- otherwise member certs cannot + // be minted and the cluster never comes up (this is the failure the test is + // here to prove does NOT happen with a shared CA). + if err := wait.For(func(ctx context.Context) (bool, error) { + var iss certv1.ClusterIssuer + if err := r.Get(ctx, caIssuerName, "", &iss); err != nil { + return false, nil + } + for _, c := range iss.Status.Conditions { + if c.Type == certv1.IssuerConditionReady && c.Status == cmmeta.ConditionTrue { + return true, nil + } + } + return false, nil + }, wait.WithTimeout(3*time.Minute), wait.WithInterval(5*time.Second)); err != nil { + t.Fatalf("CA ClusterIssuer %q never became Ready: %v", caIssuerName, err) + } +} + +// createIfAbsent gets obj by name/ns and returns if it already exists, otherwise +// creates it, retrying past the transient cert-manager webhook errors below. +func createIfAbsent(ctx context.Context, t *testing.T, cfg *envconf.Config, obj k8s.Object, name, ns string) { + t.Helper() + r := cfg.Client().Resources() + if err := r.Get(ctx, name, ns, obj); err == nil { + return // already exists + } else if !k8serrors.IsNotFound(err) { + t.Fatalf("failed to get %T %q: %v", obj, name, err) + } + + // Create with retry: cert-manager's validating webhook is registered before its + // serving-cert CA bundle is injected (by cainjector), so an immediate Create can + // fail transiently with "failed calling webhook ... x509: certificate signed by + // unknown authority". Retry until the webhook is genuinely serving. + if err := wait.For(func(ctx context.Context) (bool, error) { + err := r.Create(ctx, obj) + if err == nil || k8serrors.IsAlreadyExists(err) { + return true, nil + } + if isTransientWebhookError(err) { + return false, nil + } + return false, err + }, wait.WithTimeout(2*time.Minute), wait.WithInterval(5*time.Second)); err != nil { + t.Fatalf("failed to create %T %q: %v", obj, name, err) + } +} + +// isTransientWebhookError reports whether err is the well-known transient failure of +// calling the cert-manager admission webhook before its CA bundle is injected. +func isTransientWebhookError(err error) bool { + if err == nil { + return false + } + msg := err.Error() + return strings.Contains(msg, "failed calling webhook") || + strings.Contains(msg, "certificate signed by unknown authority") || + strings.Contains(msg, "connection refused") || + strings.Contains(msg, "no endpoints available") +} + +// installCertManagerForTLS installs cert-manager and registers the schemes the +// TLS tests need, mirroring the setup in cert_manager_test.go. +func installCertManagerForTLS(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + t.Helper() + log.Println("Installing cert-manager...") + if err := test_utils.InstallCertManager(); err != nil { + log.Printf("Unable to install Cert Manager: %s", err) + } + client := cfg.Client() + _ = appsv1.AddToScheme(client.Resources().GetScheme()) + _ = corev1.AddToScheme(client.Resources().GetScheme()) + _ = certv1.AddToScheme(client.Resources().GetScheme()) + _ = apiextensionsV1.AddToScheme(client.Resources().GetScheme()) + return ctx +} + +// waitForSTSCreated blocks until the operator has created the named StatefulSet. +// The shared waitForSTSReadiness treats a Get error (including NotFound) as fatal, +// so it must only be called once the STS exists; with a fresh cert-manager install +// the operator can take longer than usual to mint certs and create the STS. +func waitForSTSCreated(t *testing.T, c *envconf.Config, name string, waitFor time.Duration) { + t.Helper() + err := wait.For(func(ctx context.Context) (bool, error) { + var sts appsv1.StatefulSet + if err := c.Client().Resources().Get(ctx, name, namespace, &sts); err != nil { + if k8serrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil + }, wait.WithTimeout(waitFor), wait.WithInterval(5*time.Second)) + if err != nil { + t.Fatalf("StatefulSet %s was never created by the operator: %v", name, err) + } +} + +// podFQDN returns the in-cluster DNS name etcd advertises for a member, which is +// what the server cert's wildcard SAN (*.{cluster}.{ns}.svc.cluster.local) covers. +// etcdctl MUST dial this name (not 127.0.0.1) or Go TLS hostname verification fails. +func podFQDN(clusterName string, ordinal int) string { + return fmt.Sprintf("%s-%d.%s.%s.svc.cluster.local", clusterName, ordinal, clusterName, namespace) +} + +// tlsEtcdctl runs etcdctl inside the given pod with explicit mTLS flags pointed at +// the server cert material mounted at serverCertMountPath, dialing the pod's own +// advertised https endpoint. +func tlsEtcdctl(t *testing.T, c *envconf.Config, podName, clusterName string, ordinal int, args ...string) (string, string, error) { + t.Helper() + base := []string{ + "etcdctl", + "--cacert=/etc/etcd/server-tls/ca.crt", + "--cert=/etc/etcd/server-tls/tls.crt", + "--key=/etc/etcd/server-tls/tls.key", + fmt.Sprintf("--endpoints=https://%s:2379", podFQDN(clusterName, ordinal)), + } + return execInPod(t, c, podName, namespace, append(base, args...)) +} + +// getEtcdMemberListTLS returns the member list parsed from `etcdctl member list -w +// json`, run with mTLS flags. The shared getEtcdMemberListPB uses a bare etcdctl +// (no certs, 127.0.0.1) which a client-cert-auth TLS server resets, so TLS clusters +// need this variant. +func getEtcdMemberListTLS(t *testing.T, c *envconf.Config, clusterName string, ordinal int) *etcdserverpb.MemberListResponse { + t.Helper() + podName := fmt.Sprintf("%s-%d", clusterName, ordinal) + stdout, stderr, err := tlsEtcdctl(t, c, podName, clusterName, ordinal, "member", "list", "-w", "json") + if err != nil { + t.Fatalf("Failed to get etcd member list over mTLS: %v, stderr: %s", err, stderr) + } + var ml etcdserverpb.MemberListResponse + if err := json.Unmarshal([]byte(stdout), &ml); err != nil { + t.Fatalf("Failed to parse mTLS member list JSON: %v. Raw: %s", err, stdout) + } + return &ml +} + +// waitForNoLearnersTLS is the mTLS counterpart of waitForNoLearners: it waits until +// the cluster reports exactly expectedMembers, all voting (no learners), querying +// over mTLS. +func waitForNoLearnersTLS(t *testing.T, c *envconf.Config, clusterName string, ordinal, expectedMembers int, waitFor time.Duration) { + t.Helper() + err := wait.For(func(ctx context.Context) (bool, error) { + ml := getEtcdMemberListTLS(t, c, clusterName, ordinal) + if len(ml.Members) != expectedMembers { + return false, nil + } + for _, m := range ml.Members { + if m.IsLearner { + return false, nil + } + } + return true, nil + }, wait.WithTimeout(waitFor), wait.WithInterval(5*time.Second)) + if err != nil { + t.Fatalf("Timeout waiting for %d voting members (no learners) over mTLS: %v", expectedMembers, err) + } +} + +// waitForTLSReadyCondition blocks until the EtcdCluster's TLSReady condition has +// the wanted status, returning the observed reason. It fails the test on timeout. +func waitForTLSReadyCondition(t *testing.T, c *envconf.Config, name string, + want metav1.ConditionStatus, waitFor time.Duration) string { + t.Helper() + var observedReason string + err := wait.For(func(ctx context.Context) (bool, error) { + var ec ecv1alpha1.EtcdCluster + if err := c.Client().Resources().Get(ctx, name, namespace, &ec); err != nil { + return false, nil + } + for _, cond := range ec.Status.Conditions { + if cond.Type == "TLSReady" { + observedReason = cond.Reason + return cond.Status == want, nil + } + } + return false, nil + }, wait.WithTimeout(waitFor), wait.WithInterval(5*time.Second)) + if err != nil { + // Surface the current conditions for debugging. + var ec ecv1alpha1.EtcdCluster + _ = c.Client().Resources().Get(t.Context(), name, namespace, &ec) + t.Fatalf("TLSReady never reached status=%s for %q (last reason=%q); conditions=%+v", + want, name, observedReason, ec.Status.Conditions) + } + return observedReason +} + +// TestTLSMultiMemberQuorum is the headline T6 proof: a 3-member EtcdCluster with +// BOTH TLS surfaces enabled against a shared CA issuer forms and holds quorum, +// reports TLSReady=True, serves real mTLS (put/get succeeds) and genuinely +// enforces it (a no-cert / cleartext attempt fails). +func TestTLSMultiMemberQuorum(t *testing.T) { + feature := features.New("tls-multi-member-quorum").WithLabel("app", "cert-manager") + + const clusterName = "etcd-tls-quorum" + const size = 3 + + etcdCluster := &ecv1alpha1.EtcdCluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "operator.etcd.io/v1alpha1", + Kind: "EtcdCluster", + }, + ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}, + Spec: ecv1alpha1.EtcdClusterSpec{ + Size: size, + Version: etcdVersion, + TLS: &ecv1alpha1.EtcdClusterTLS{ + Peer: certManagerSurface(caIssuerType, caIssuerName), + Client: certManagerSurface(caIssuerType, caIssuerName), + }, + }, + } + + feature.Setup(func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + ctx = installCertManagerForTLS(ctx, t, cfg) + ensureClusterCAIssuer(ctx, t, cfg) + if err := cfg.Client().Resources().Create(ctx, etcdCluster); err != nil { + t.Fatalf("unable to create etcd cluster: %s", err) + } + return ctx + }) + + // THE quorum assertion: all 3 replicas ready + 3 voting members, no learners. + feature.Assess("3-member TLS cluster forms and holds quorum", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + // Wait for the StatefulSet to bring all 3 replicas to Ready. With a + // self-signed leaf peer issuer this would hang at 1; with the shared CA + // it must reach 3. Wait for the operator to create the STS first (cert + // issuance on a fresh cert-manager can lag), then for full readiness. + waitForSTSCreated(t, c, clusterName, 3*time.Minute) + waitForSTSReadiness(t, c, clusterName, size) + + // All 3 members registered and promoted to voting (no stuck learners), + // queried over mTLS (the cleartext helper can't talk to a TLS server). + waitForNoLearnersTLS(t, c, clusterName, 0, size, 5*time.Minute) + + ml := getEtcdMemberListTLS(t, c, clusterName, 0) + if len(ml.Members) != size { + t.Fatalf("expected %d members, got %d: %+v", size, len(ml.Members), ml.Members) + } + for _, m := range ml.Members { + if m.IsLearner { + t.Errorf("member %s (%d) is still a learner -- quorum not fully formed", m.Name, m.ID) + } + } + t.Logf("etcd member list (%d voting members):", len(ml.Members)) + for _, m := range ml.Members { + t.Logf(" - %s id=%x learner=%v peerURLs=%v", m.Name, m.ID, m.IsLearner, m.PeerURLs) + } + return ctx + }) + + feature.Assess("EtcdCluster reports TLSReady=True", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + reason := waitForTLSReadyCondition(t, c, clusterName, metav1.ConditionTrue, 3*time.Minute) + if reason != "TLSReady" { + t.Errorf("expected TLSReady reason=TLSReady, got %q", reason) + } + return ctx + }) + + feature.Assess("peer traffic is https (peer cert flags + https peer URLs)", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + var sts appsv1.StatefulSet + if err := c.Client().Resources().Get(ctx, clusterName, namespace, &sts); err != nil { + t.Fatalf("failed to get StatefulSet: %v", err) + } + args := strings.Join(sts.Spec.Template.Spec.Containers[0].Args, " ") + for _, want := range []string{ + "--listen-peer-urls=https://", + "--initial-advertise-peer-urls=https://", + "--peer-cert-file=/etc/etcd/peer-tls/tls.crt", + "--peer-trusted-ca-file=/etc/etcd/peer-tls/ca.crt", + "--peer-client-cert-auth", + "--listen-client-urls=https://", + "--cert-file=/etc/etcd/server-tls/tls.crt", + "--client-cert-auth", + } { + if !strings.Contains(args, want) { + t.Errorf("expected etcd args to contain %q; full args: %s", want, args) + } + } + return ctx + }) + + // mTLS genuinely works AND is genuinely enforced. + feature.Assess("mTLS put/get succeeds across the cluster", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + podName := fmt.Sprintf("%s-0", clusterName) + // Write with mTLS to member 0. + if _, stderr, err := tlsEtcdctl(t, c, podName, clusterName, 0, "put", "tls-key", "tls-value"); err != nil { + t.Fatalf("mTLS put failed: %v, stderr: %s", err, stderr) + } + // Read it back through member 2 (proves replication across the TLS cluster). + pod2 := fmt.Sprintf("%s-2", clusterName) + stdout, stderr, err := tlsEtcdctl(t, c, pod2, clusterName, 2, "get", "tls-key") + if err != nil { + t.Fatalf("mTLS get on member 2 failed: %v, stderr: %s", err, stderr) + } + lines := strings.Split(strings.TrimSpace(stdout), "\n") + if len(lines) < 2 || lines[0] != "tls-key" || lines[1] != "tls-value" { + t.Errorf("expected tls-key=tls-value replicated to member 2, got: %s", stdout) + } + return ctx + }) + + feature.Assess("cleartext / no-cert access is rejected (TLS is not theater)", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + podName := fmt.Sprintf("%s-0", clusterName) + fqdn := podFQDN(clusterName, 0) + + // 1) http:// against an https listener must fail. + stdout, stderr, err := execInPod(t, c, podName, namespace, []string{ + "etcdctl", fmt.Sprintf("--endpoints=http://%s:2379", fqdn), + "--command-timeout=5s", "put", "should", "fail", + }) + if err == nil { + t.Errorf("cleartext (http) put unexpectedly SUCCEEDED -- TLS is not enforced. stdout=%s stderr=%s", + stdout, stderr) + } + + // 2) https but WITHOUT a client cert must fail because --client-cert-auth + // requires a presented client certificate. + stdout, stderr, err = execInPod(t, c, podName, namespace, []string{ + "etcdctl", + "--cacert=/etc/etcd/server-tls/ca.crt", + fmt.Sprintf("--endpoints=https://%s:2379", fqdn), + "--command-timeout=5s", "put", "should", "fail", + }) + if err == nil { + t.Errorf("no-client-cert put unexpectedly SUCCEEDED -- client-cert-auth not enforced. stdout=%s stderr=%s", + stdout, stderr) + } + return ctx + }) + + feature.Teardown(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + cleanupEtcdCluster(ctx, t, c, clusterName) + return ctx + }) + + _ = testEnv.Test(t, feature.Feature()) +} + +// TestTLSPeerCANotShared is the negative companion: a cluster whose PEER surface +// uses a bare self-signed leaf issuer must surface TLSReady=False/PeerCANotShared -- +// a visible, actionable signal -- and must NOT report TLSReady=True. The client +// surface uses the shared CA so the verdict is unambiguously attributable to the +// peer surface. +// +// IMPORTANT empirical finding (verified live on kind, 2026-06-17): the plan's +// Decision 3 / the adversarial review assumed a self-signed *leaf* peer issuer +// makes members unable to mutually verify and caps the cluster at one member. That +// is FALSE for this operator's architecture: the operator provisions a SINGLE +// shared peer Certificate/secret ({cluster}-peer-tls) mounted into every member, so +// every member presents and trusts the *identical* self-signed cert and peer mTLS +// succeeds -- a healthy 3-member quorum still forms. PeerCANotShared is therefore a +// conservative WARNING signal (the user's issuer is not a real shared CA, which is a +// fragile/unintended config), not a hard barrier to formation, and the operator does +// not gate scale-out on it. This test asserts the signal fires and the condition is +// not falsely True; it deliberately does NOT assert the cluster fails to form, +// because live evidence shows it does form. See the T6 report for the full analysis. +func TestTLSPeerCANotShared(t *testing.T) { + feature := features.New("tls-peer-ca-not-shared").WithLabel("app", "cert-manager") + + const clusterName = "etcd-tls-peercheck" + const size = 3 + + etcdCluster := &ecv1alpha1.EtcdCluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "operator.etcd.io/v1alpha1", + Kind: "EtcdCluster", + }, + ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: namespace}, + Spec: ecv1alpha1.EtcdClusterSpec{ + Size: size, + Version: etcdVersion, + TLS: &ecv1alpha1.EtcdClusterTLS{ + // Peer on a bare self-signed leaf issuer (the broken pattern). + Peer: certManagerSurface(cmIssuerType, cmIssuerName), + // Client on the shared CA, so any failure is the peer surface's. + Client: certManagerSurface(caIssuerType, caIssuerName), + }, + }, + } + + feature.Setup(func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { + ctx = installCertManagerForTLS(ctx, t, cfg) + ensureClusterCAIssuer(ctx, t, cfg) + // The bare self-signed ClusterIssuer (cmIssuer) the peer surface points at. + createIfAbsent(ctx, t, cfg, cmIssuer, cmIssuerName, "") + if err := cfg.Client().Resources().Create(ctx, etcdCluster); err != nil { + t.Fatalf("unable to create etcd cluster: %s", err) + } + return ctx + }) + + feature.Assess("operator surfaces TLSReady=False / reason=PeerCANotShared", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + reason := waitForTLSReadyCondition(t, c, clusterName, metav1.ConditionFalse, 3*time.Minute) + if reason != "PeerCANotShared" { + t.Fatalf("expected TLSReady=False reason=PeerCANotShared, got reason=%q", reason) + } + return ctx + }) + + feature.Assess("PeerCANotShared persists; the operator never falsely reports TLSReady=True", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + // The signal must be stable, not flap to True. The shared-peer-secret + // design means the cluster CAN still form (see the test doc), so we do + // not assert non-formation; we assert the WARNING stays put -- the user + // keeps a visible, actionable signal that their peer issuer is not a real + // shared CA. A flip to TLSReady=True here would be the real bug (the guard + // silently clearing itself). + deadline := time.Now().Add(60 * time.Second) + for time.Now().Before(deadline) { + var ec ecv1alpha1.EtcdCluster + if err := c.Client().Resources().Get(ctx, clusterName, namespace, &ec); err == nil { + for _, cond := range ec.Status.Conditions { + if cond.Type == "TLSReady" { + if cond.Status == metav1.ConditionTrue { + t.Fatalf("TLSReady flipped to True for a self-signed-leaf peer issuer; "+ + "the PeerCANotShared guard cleared itself (reason=%q)", cond.Reason) + } + if cond.Reason != "PeerCANotShared" { + t.Fatalf("TLSReady reason drifted off PeerCANotShared to %q", cond.Reason) + } + } + } + } + time.Sleep(10 * time.Second) + } + return ctx + }) + + feature.Teardown(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + cleanupEtcdCluster(ctx, t, c, clusterName) + return ctx + }) + + _ = testEnv.Test(t, feature.Feature()) +}