From 2e6fdaae75a667895c38958cb1591228a5f3f508 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 22:20:32 -0400 Subject: [PATCH 1/8] feat(api)!: configure peer and client TLS independently etcd has three TLS surfaces -- peer<->peer, client->server, and the operator's own client identity -- but the alpha CRD conflated all three behind a single `spec.tls` toggle and a single shared issuer, so a user could neither serve peer TLS without client TLS (or vice versa) nor relax mutual client-cert auth per surface. The single predicate also forced `--client-cert-auth`/`--peer-client-cert-auth` on whenever TLS was set. Reshape `spec.tls` into two optional, independent surfaces -- `peer` and `client` -- each carrying its own provider, issuer, and `clientCertAuth` policy (default true). A nil surface serves/dials that surface in cleartext; both nil is fully cleartext and byte-identical to the pre-TLS path. The operator's own client identity follows the `client` surface. This is a clean break on the alpha API (no conversion webhook, which would re-introduce the conflation). The dead `caBundleSecret` knob is removed (type + cert interface; it was read nowhere). `IssuerGroup` is added to the cert-manager provider config (empty => cert-manager.io) so non-default issuer groups can be targeted. Threading (single source of truth, per surface): - peerScheme/clientScheme drive peer vs client URLs and flags independently - defaultArgs emits the server flag group iff client set, the peer flag group iff peer set, and each --*client-cert-auth gated on its surface - cert mounts: server secret iff client set, peer secret iff peer set (append-not-assign so they coexist with the storage data-dir mount) - cert provisioning: server+operator-client certs from the client surface, peer cert from the peer surface - etcdutils MemberList/ClusterHealth/AddMember/PromoteLearner/RemoveMember take an optional *tls.Config (nil => cleartext); the operator builds it from the client surface only, with a missing ca.crt now an explicit error Anti-misconfiguration guardrails: - apply-time CEL XValidation on each surface: provider/providerCfg coherence, mTLS-requires-a-CA, issuerKind/provider enums - a reconcile-time validateTLS backstop re-checks the spec-only rules and requeues on an invalid spec (for apiservers that don't enforce CEL) Also: the cert-site log.Printf calls move to the reconcile-scoped logger, and the "running without TLS" error-level log is demoted to Info (cleartext is a supported mode). The sample is rewritten to the two-tier shared-CA issuer pattern a multi-member peer-mTLS cluster actually needs. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 83 +++- api/v1alpha1/zz_generated.deepcopy.go | 42 +- .../bases/operator.etcd.io_etcdclusters.yaml | 354 ++++++++++---- .../sample_certmanager_etcdcluster.yaml | 73 ++- internal/controller/etcdcluster_controller.go | 47 +- internal/controller/tls_cel_test.go | 130 +++++ internal/controller/tls_independence_test.go | 447 ++++++++++++++++++ internal/controller/utils.go | 401 +++++++++++++--- internal/controller/utils_test.go | 102 ++-- internal/etcdutils/etcdutils.go | 22 +- internal/etcdutils/etcdutils_test.go | 100 +++- pkg/certificate/cert_manager/provider.go | 19 +- pkg/certificate/interfaces/interface.go | 1 - test/e2e/auto_provider_test.go | 13 +- test/e2e/cert_manager_test.go | 15 +- test/e2e/helpers_test.go | 35 ++ 16 files changed, 1595 insertions(+), 289 deletions(-) create mode 100644 internal/controller/tls_cel_test.go create mode 100644 internal/controller/tls_independence_test.go diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index f007ae04..7667e44d 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -43,8 +43,12 @@ type EtcdClusterSpec struct { Version string `json:"version"` // StorageSpec is the name of the StorageSpec to use for the etcd cluster. If not provided, then each POD just uses the temporary storage inside the container. StorageSpec *StorageSpec `json:"storageSpec,omitempty"` - // TLS is the TLS certificate configuration to use for the etcd cluster and etcd operator. - TLS *TLSCertificate `json:"tls,omitempty"` + // TLS configures etcd's two independent TLS surfaces (peer and client/server). + // Each surface is optional and configured fully independently; a nil surface + // means that surface is served/dialed in cleartext. When TLS itself is nil, the + // entire cluster (peer + client + operator client) is cleartext, byte-identical + // to a TLS-free deployment. + TLS *EtcdClusterTLS `json:"tls,omitempty"` // etcd configuration options are passed as command line arguments to the etcd container, refer to etcd documentation for configuration options applicable for the version of etcd being used. EtcdOptions []string `json:"etcdOptions,omitempty"` // PodTemplate is the pod template to use for the etcd cluster. @@ -68,9 +72,64 @@ type PodMetadata struct { Labels map[string]string `json:"labels,omitempty"` } -type TLSCertificate struct { - Provider string `json:"provider,omitempty"` // Defaults to Auto provider if not present +// EtcdClusterTLS configures etcd's two independent TLS surfaces. Each surface is +// optional; a nil surface means that surface is served/dialed in cleartext (http). +// The two surfaces are configured fully independently -- different providers, +// issuers, and client-cert-auth policy are allowed and expected. Both surfaces nil +// is legal and means fully-cleartext (today's default); it is intentional, not an +// error, so there is no "at least one surface" validation. +type EtcdClusterTLS struct { + // Peer configures etcd<->etcd (peer) TLS. When nil, peer traffic is cleartext. + // A configured peer surface REQUIRES a CA-capable issuer shared by all members + // so members can mutually verify; a self-signed *leaf* issuer cannot form a + // multi-member cluster. (CA-capability lives on the cert-manager Issuer object, + // not on this spec, so that check is enforced at reconcile time, not via CEL.) + // +optional + Peer *TLSSurface `json:"peer,omitempty"` + + // Client configures client->etcd (server) TLS AND, transitively, the operator's + // own etcd client identity (the operator authenticates to etcd as a client). + // When nil, client traffic is cleartext and the operator dials cleartext. + // +optional + Client *TLSSurface `json:"client,omitempty"` +} + +// TLSSurface is the full, independent TLS configuration for ONE surface (peer or +// client). It carries its own provider, provider config (issuer), and mutual +// client-cert-auth policy. +// +// The two XValidation rules below are the apply-time anti-misconfiguration +// guardrails (plan Decision 2.1/2.2): they reject incoherent provider/config +// combinations and mTLS-without-a-resolvable-CA at the API server, so a user +// "cannot misconfigure" these from the spec alone. Rules that require reading +// cluster objects (issuer existence, peer CA-capability, client/server CA match) +// cannot be expressed in CEL and are enforced at reconcile time instead (plan +// Decision 2.5-2.7, Decision 3) -- see validateTLSSurface and the cert-manager +// provider's validateCertificateConfig. +// +// +kubebuilder:validation:XValidation:rule="self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg)",message="provider 'cert-manager' requires providerCfg.certManagerCfg" +// +kubebuilder:validation:XValidation:rule="self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg)",message="providerCfg.certManagerCfg may only be set when provider is 'cert-manager'" +// +kubebuilder:validation:XValidation:rule="!self.clientCertAuth || self.provider != 'cert-manager' || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) > 0)",message="clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName" +type TLSSurface struct { + // Provider selects the certificate provider for THIS surface. + // Defaults to "auto" when empty. + // +kubebuilder:validation:Enum=auto;cert-manager + // +optional + Provider string `json:"provider,omitempty"` + + // ProviderCfg is the provider-specific config for THIS surface. + // +optional ProviderCfg ProviderConfig `json:"providerCfg,omitempty"` + + // ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + // --client-cert-auth for the client surface, --peer-client-cert-auth for the + // peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + // where clients authenticate by other means (password/token). When true with + // the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + // the XValidation rule above. + // +kubebuilder:default=true + // +optional + ClientCertAuth *bool `json:"clientCertAuth,omitempty"` } type ProviderConfig struct { @@ -109,13 +168,6 @@ type CommonConfig struct { // and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 // +optional ValidityDuration string `json:"validityDuration,omitempty"` - - // CABundleSecret is the expected secret name with CABundle present. It's used - // by each etcd POD to verify TLS communications with its peers or clients. If it isn't - // provided, the CA included in the secret generated by certificate provider will be - // used instead if present; otherwise, there is no way to verify TLS communications. - // +optional - CABundleSecret string `json:"caBundleSecret,omitempty"` } type ProviderAutoConfig struct { @@ -127,11 +179,18 @@ type ProviderCertManagerConfig struct { // CommonConfig is the struct of common fields required to create a certificate CommonConfig `json:",inline"` - // IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer" + // IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer". + // +kubebuilder:validation:Enum=Issuer;ClusterIssuer IssuerKind string `json:"issuerKind"` // IssuerName is the expected name of Issuer required to issue a certificate IssuerName string `json:"issuerName"` + + // IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + // Empty defaults to "cert-manager.io". Set this to target issuers served by an + // external/intermediate issuer group (e.g. an out-of-tree CA controller). + // +optional + IssuerGroup string `json:"issuerGroup,omitempty"` } // EtcdClusterStatus defines the observed state of EtcdCluster. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9db6ca12..fbab398c 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -23,7 +23,7 @@ package v1alpha1 import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" netx "net" ) @@ -148,7 +148,7 @@ func (in *EtcdClusterSpec) DeepCopyInto(out *EtcdClusterSpec) { } if in.TLS != nil { in, out := &in.TLS, &out.TLS - *out = new(TLSCertificate) + *out = new(EtcdClusterTLS) (*in).DeepCopyInto(*out) } if in.EtcdOptions != nil { @@ -200,6 +200,31 @@ func (in *EtcdClusterStatus) DeepCopy() *EtcdClusterStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *EtcdClusterTLS) DeepCopyInto(out *EtcdClusterTLS) { + *out = *in + if in.Peer != nil { + in, out := &in.Peer, &out.Peer + *out = new(TLSSurface) + (*in).DeepCopyInto(*out) + } + if in.Client != nil { + in, out := &in.Client, &out.Client + *out = new(TLSSurface) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EtcdClusterTLS. +func (in *EtcdClusterTLS) DeepCopy() *EtcdClusterTLS { + if in == nil { + return nil + } + out := new(EtcdClusterTLS) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MemberStatus) DeepCopyInto(out *MemberStatus) { *out = *in @@ -378,17 +403,22 @@ func (in *StorageSpec) DeepCopy() *StorageSpec { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *TLSCertificate) DeepCopyInto(out *TLSCertificate) { +func (in *TLSSurface) DeepCopyInto(out *TLSSurface) { *out = *in in.ProviderCfg.DeepCopyInto(&out.ProviderCfg) + if in.ClientCertAuth != nil { + in, out := &in.ClientCertAuth, &out.ClientCertAuth + *out = new(bool) + **out = **in + } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSCertificate. -func (in *TLSCertificate) DeepCopy() *TLSCertificate { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSSurface. +func (in *TLSSurface) DeepCopy() *TLSSurface { if in == nil { return nil } - out := new(TLSCertificate) + out := new(TLSSurface) in.DeepCopyInto(out) return out } diff --git a/config/crd/bases/operator.etcd.io_etcdclusters.yaml b/config/crd/bases/operator.etcd.io_etcdclusters.yaml index 0ae1e80e..3843e523 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.20.1 + controller-gen.kubebuilder.io/version: v0.21.0 name: etcdclusters.operator.etcd.io spec: group: operator.etcd.io @@ -1069,119 +1069,295 @@ spec: - volumeSizeRequest type: object tls: - description: TLS is the TLS certificate configuration to use for the - etcd cluster and etcd operator. + description: |- + TLS configures etcd's two independent TLS surfaces (peer and client/server). + Each surface is optional and configured fully independently; a nil surface + means that surface is served/dialed in cleartext. When TLS itself is nil, the + entire cluster (peer + client + operator client) is cleartext, byte-identical + to a TLS-free deployment. properties: - provider: - type: string - providerCfg: + client: + description: |- + Client configures client->etcd (server) TLS AND, transitively, the operator's + own etcd client identity (the operator authenticates to etcd as a client). + When nil, client traffic is cleartext and the operator dials cleartext. properties: - autoCfg: + clientCertAuth: + default: true + description: |- + ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + --client-cert-auth for the client surface, --peer-client-cert-auth for the + peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + where clients authenticate by other means (password/token). When true with + the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + the XValidation rule above. + type: boolean + provider: + description: |- + Provider selects the certificate provider for THIS surface. + Defaults to "auto" when empty. + enum: + - auto + - cert-manager + type: string + providerCfg: + description: ProviderCfg is the provider-specific config for + THIS surface. properties: - altNames: - description: |- - AltNames contains the domain names and IP addresses that will be added - to the x509 certificate SubAltNames fields. The values will be passed - directly to the x509.Certificate object. + autoCfg: properties: - dnsNames: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: description: |- - DNSNames is the expected array of DNS subject alternative names. - if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array - ipAddresses: - description: IPs is the expected array of IP address - subject alternative names. + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + type: object + certManagerCfg: + properties: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: + description: |- + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + issuerGroup: + description: |- + IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + Empty defaults to "cert-manager.io". Set this to target issuers served by an + external/intermediate issuer group (e.g. an out-of-tree CA controller). + type: string + issuerKind: + description: IssuerKind is the expected kind of Issuer, + either "ClusterIssuer" or "Issuer". + enum: + - Issuer + - ClusterIssuer + type: string + issuerName: + description: IssuerName is the expected name of Issuer + required to issue a certificate + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + required: + - issuerKind + - issuerName type: object - caBundleSecret: - description: |- - CABundleSecret is the expected secret name with CABundle present. It's used - by each etcd POD to verify TLS communications with its peers or clients. If it isn't - provided, the CA included in the secret generated by certificate provider will be - used instead if present; otherwise, there is no way to verify TLS communications. - type: string - commonName: - description: |- - CommonName is the expected common name X509 certificate subject attribute. - Should have a length of 64 characters or fewer to avoid generating invalid CSRs. - type: string - organizations: - description: Organization is the expected array of Organization - names to be used on the Certificate. - items: - type: string - type: array - validityDuration: - description: |- - ValidityDuration is the expected duration until which the certificate will be valid, - expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager - and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 - type: string type: object - certManagerCfg: + type: object + x-kubernetes-validations: + - message: provider 'cert-manager' requires providerCfg.certManagerCfg + rule: self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg) + - message: providerCfg.certManagerCfg may only be set when provider + is 'cert-manager' + rule: self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg) + - message: 'clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName' + rule: '!self.clientCertAuth || self.provider != ''cert-manager'' + || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) + > 0)' + peer: + description: |- + Peer configures etcd<->etcd (peer) TLS. When nil, peer traffic is cleartext. + A configured peer surface REQUIRES a CA-capable issuer shared by all members + so members can mutually verify; a self-signed *leaf* issuer cannot form a + multi-member cluster. (CA-capability lives on the cert-manager Issuer object, + not on this spec, so that check is enforced at reconcile time, not via CEL.) + properties: + clientCertAuth: + default: true + description: |- + ClientCertAuth toggles mutual cert auth for THIS surface (etcd's + --client-cert-auth for the client surface, --peer-client-cert-auth for the + peer surface). Defaults to true (mTLS). Set false to serve server-only TLS + where clients authenticate by other means (password/token). When true with + the cert-manager provider a trusted CA (issuerName) is REQUIRED, enforced by + the XValidation rule above. + type: boolean + provider: + description: |- + Provider selects the certificate provider for THIS surface. + Defaults to "auto" when empty. + enum: + - auto + - cert-manager + type: string + providerCfg: + description: ProviderCfg is the provider-specific config for + THIS surface. properties: - altNames: - description: |- - AltNames contains the domain names and IP addresses that will be added - to the x509 certificate SubAltNames fields. The values will be passed - directly to the x509.Certificate object. + autoCfg: properties: - dnsNames: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: description: |- - DNSNames is the expected array of DNS subject alternative names. - if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array - ipAddresses: - description: IPs is the expected array of IP address - subject alternative names. + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + type: object + certManagerCfg: + properties: + altNames: + description: |- + AltNames contains the domain names and IP addresses that will be added + to the x509 certificate SubAltNames fields. The values will be passed + directly to the x509.Certificate object. + properties: + dnsNames: + description: |- + DNSNames is the expected array of DNS subject alternative names. + if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local + items: + type: string + type: array + ipAddresses: + description: IPs is the expected array of IP address + subject alternative names. + items: + type: string + type: array + type: object + commonName: + description: |- + CommonName is the expected common name X509 certificate subject attribute. + Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + type: string + issuerGroup: + description: |- + IssuerGroup is the API group of the issuer referenced by IssuerKind/IssuerName. + Empty defaults to "cert-manager.io". Set this to target issuers served by an + external/intermediate issuer group (e.g. an out-of-tree CA controller). + type: string + issuerKind: + description: IssuerKind is the expected kind of Issuer, + either "ClusterIssuer" or "Issuer". + enum: + - Issuer + - ClusterIssuer + type: string + issuerName: + description: IssuerName is the expected name of Issuer + required to issue a certificate + type: string + organizations: + description: Organization is the expected array of + Organization names to be used on the Certificate. items: type: string type: array + validityDuration: + description: |- + ValidityDuration is the expected duration until which the certificate will be valid, + expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager + and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 + type: string + required: + - issuerKind + - issuerName type: object - caBundleSecret: - description: |- - CABundleSecret is the expected secret name with CABundle present. It's used - by each etcd POD to verify TLS communications with its peers or clients. If it isn't - provided, the CA included in the secret generated by certificate provider will be - used instead if present; otherwise, there is no way to verify TLS communications. - type: string - commonName: - description: |- - CommonName is the expected common name X509 certificate subject attribute. - Should have a length of 64 characters or fewer to avoid generating invalid CSRs. - type: string - issuerKind: - description: IssuerKind is the expected kind of Issuer, - either "ClusterIssuer" or "Issuer" - type: string - issuerName: - description: IssuerName is the expected name of Issuer - required to issue a certificate - type: string - organizations: - description: Organization is the expected array of Organization - names to be used on the Certificate. - items: - type: string - type: array - validityDuration: - description: |- - ValidityDuration is the expected duration until which the certificate will be valid, - expects in human-readable duration: 100d12h, if empty defaults to 90d for cert-manager - and 365d for auto as per: https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275 - type: string - required: - - issuerKind - - issuerName type: object type: object + x-kubernetes-validations: + - message: provider 'cert-manager' requires providerCfg.certManagerCfg + rule: self.provider != 'cert-manager' || has(self.providerCfg.certManagerCfg) + - message: providerCfg.certManagerCfg may only be set when provider + is 'cert-manager' + rule: self.provider == 'cert-manager' || !has(self.providerCfg.certManagerCfg) + - message: 'clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName' + rule: '!self.clientCertAuth || self.provider != ''cert-manager'' + || (has(self.providerCfg.certManagerCfg) && size(self.providerCfg.certManagerCfg.issuerName) + > 0)' type: object version: description: Version is the expected version of the etcd container diff --git a/config/samples/sample_certmanager_etcdcluster.yaml b/config/samples/sample_certmanager_etcdcluster.yaml index b515db39..282399b2 100644 --- a/config/samples/sample_certmanager_etcdcluster.yaml +++ b/config/samples/sample_certmanager_etcdcluster.yaml @@ -1,11 +1,53 @@ -# creating and managing the Issuer/ClusterIssuer is the users’ responsibility, while etcd-operator only needs to know which Issuer/ClusterIssuer to use. +# A multi-member TLS etcd cluster needs a SHARED CA so members can mutually verify +# each other's peer certs and the operator can verify the server certs. A bare +# self-signed ClusterIssuer mints CA:FALSE leaf certs that are each their own root, +# so peer mTLS cannot form across members and the cluster is effectively capped at +# one member. +# +# The supported pattern (and the one this sample models) is a two-tier CA: +# 1. a SelfSigned ClusterIssuer bootstraps +# 2. a CA Certificate (isCA: true), whose secret is signed by (1) +# 3. a CA ClusterIssuer built from that CA secret signs all of the leaf certs +# Then every issued secret's ca.crt is the same real CA and all verification chains +# succeed. Creating/managing the Issuer/ClusterIssuer is the user's responsibility; +# etcd-operator only needs to know which Issuer/ClusterIssuer to reference. apiVersion: cert-manager.io/v1 kind: ClusterIssuer metadata: - name: selfsigned + name: selfsigned-bootstrap spec: selfSigned: {} +--- +# CA Certificate: isCA so it can sign leaf certs. Its secret is consumed by the CA +# ClusterIssuer below. cert-manager resolves a ClusterIssuer's CA secret out of the +# cluster resource namespace (where cert-manager runs), so place it there. +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: etcd-ca + namespace: cert-manager +spec: + isCA: true + commonName: etcd-ca + secretName: etcd-ca + privateKey: + algorithm: ECDSA + size: 256 + issuerRef: + name: selfsigned-bootstrap + kind: ClusterIssuer + group: cert-manager.io + +--- +apiVersion: cert-manager.io/v1 +kind: ClusterIssuer +metadata: + name: etcd-ca-issuer +spec: + ca: + secretName: etcd-ca + --- apiVersion: operator.etcd.io/v1alpha1 kind: EtcdCluster @@ -17,11 +59,24 @@ metadata: spec: version: v3.5.21 size: 3 + # Peer and client/server TLS are configured independently. Here both surfaces use + # the shared CA issuer so peer mTLS forms across members and the operator can + # verify the server. Either surface may be omitted to serve that surface in + # cleartext, or pointed at a different issuer. tls: - provider: "cert-manager" - providerCfg: - certManagerCfg: - commonName: "etcd-operator-system" - validityDuration: "365d" - issuerKind: "ClusterIssuer" - issuerName: "selfsigned" + peer: + provider: "cert-manager" + providerCfg: + certManagerCfg: + commonName: "etcd-operator-system" + validityDuration: "365d" + issuerKind: "ClusterIssuer" + issuerName: "etcd-ca-issuer" + client: + provider: "cert-manager" + providerCfg: + certManagerCfg: + commonName: "etcd-operator-system" + validityDuration: "365d" + issuerKind: "ClusterIssuer" + issuerName: "etcd-ca-issuer" diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index 867886a3..3f1f1767 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -133,17 +133,26 @@ func (r *EtcdClusterReconciler) fetchAndValidateState(ctx context.Context, req c ec.Spec.ImageRegistry = r.ImageRegistry } - // Ensure the operator has TLS credentials when the cluster requests TLS. - if ec.Spec.TLS != nil { + // Reconcile-time backstop for the apply-time CEL rules (see validateTLS). Reject + // an incoherent TLS spec by requeuing rather than silently proceeding into cert + // provisioning, which would otherwise fail deep with a less actionable error. + if errs := validateTLS(ec); len(errs) > 0 { + logger.Error(errs.ToAggregate(), "invalid TLS configuration; not reconciling until fixed", + "etcdCluster", ec.Name) + return nil, ctrl.Result{RequeueAfter: requeueDuration}, nil + } + + // The operator authenticates to etcd as a client, so it needs its own client + // identity iff the CLIENT surface is configured (independent of the peer surface). + if clientTLSEnabled(ec) { if err := createClientCertificate(ctx, ec, r.Client); err != nil { - logger.Error(err, "Failed to create Client Certificate.") + logger.Error(err, "Failed to create operator client certificate", "etcdCluster", ec.Name) } } else { - // TODO: instead of logging error, set default autoConfig - logger.Error(nil, fmt.Sprintf( - "missing TLS config for %s,\n running etcd-cluster without TLS protection is NOT recommended for production.", - ec.Name, - )) + // Cleartext (no client surface) is a supported mode; log at Info, not Error, + // so legitimately-cleartext clusters don't spam error logs every reconcile. + logger.Info("client TLS surface not configured; operator dials etcd in cleartext (not recommended for production)", + "etcdCluster", ec.Name) } logger.Info("Reconciling EtcdCluster", "spec", ec.Spec) @@ -260,7 +269,7 @@ func (r *EtcdClusterReconciler) performHealthChecks(ctx context.Context, s *reco logger := log.FromContext(ctx) logger.Info("Now checking health of the cluster members") var err error - s.memberListResp, s.memberHealth, err = healthCheck(s.sts, logger) + s.memberListResp, s.memberHealth, err = healthCheck(ctx, s.cluster, r.Client, s.sts, logger) if err != nil { return fmt.Errorf("health check failed: %w", err) } @@ -280,6 +289,14 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re targetReplica := *s.sts.Spec.Replicas var err error + // Build the operator's etcd-client TLS config once (nil when the client surface + // is cleartext) and derive client endpoints from the client surface's scheme. + clientTLSConfig, err := buildClientTLSConfig(ctx, s.cluster, r.Client) + if err != nil { + return ctrl.Result{}, err + } + cScheme := clientScheme(s.cluster) + // The number of replicas in the StatefulSet doesn't match the number of etcd members in the cluster. if int(targetReplica) != memberCnt { logger.Info("The expected number of replicas doesn't match the number of etcd members in the cluster", "targetReplica", targetReplica, "memberCnt", memberCnt) @@ -322,9 +339,9 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re if etcdutils.IsLearnerReady(leaderStatus, learnerStatus) { logger.Info("Learner is ready to be promoted to voting member", "learnerID", learner) logger.Info("Promoting the learner member", "learnerID", learner) - eps := clientEndpointsFromStatefulsets(s.sts) + eps := clientEndpointsFromStatefulsets(s.sts, cScheme) eps = eps[:(len(eps) - 1)] - if err := etcdutils.PromoteLearner(eps, learner); err != nil { + if err := etcdutils.PromoteLearner(eps, learner, clientTLSConfig); err != nil { // The member is not promoted yet, so we error out and requeue via the caller. return ctrl.Result{}, err } @@ -341,7 +358,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re return ctrl.Result{}, nil } - eps := clientEndpointsFromStatefulsets(s.sts) + eps := clientEndpointsFromStatefulsets(s.sts, cScheme) // If there are no learners left, we can proceed to scale the cluster towards the desired size. // When there are no members to add, the controller will requeue above and this block won't execute. @@ -350,7 +367,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re _, peerURL := peerEndpointForOrdinalIndex(s.cluster, int(targetReplica)) targetReplica++ logger.Info("[Scale out] adding a new learner member to etcd cluster", "peerURLs", peerURL) - if _, err := etcdutils.AddMember(eps, []string{peerURL}, true); err != nil { + if _, err := etcdutils.AddMember(eps, []string{peerURL}, true, clientTLSConfig); err != nil { return ctrl.Result{}, err } @@ -374,7 +391,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re logger.Info("[Scale in] removing one member", "memberID", memberID) eps = eps[:targetReplica] - if err := etcdutils.RemoveMember(eps, memberID); err != nil { + if err := etcdutils.RemoveMember(eps, memberID, clientTLSConfig); err != nil { return ctrl.Result{}, err } @@ -388,7 +405,7 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re } // Ensure every etcd member reports itself healthy before declaring success. - allMembersHealthy, err := areAllMembersHealthy(s.sts, logger) + allMembersHealthy, err := areAllMembersHealthy(ctx, s.cluster, r.Client, s.sts, logger) if err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/tls_cel_test.go b/internal/controller/tls_cel_test.go new file mode 100644 index 00000000..0b125b73 --- /dev/null +++ b/internal/controller/tls_cel_test.go @@ -0,0 +1,130 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// TestTLSCELValidation drives the apply-time CEL XValidation rules on TLSSurface +// against the real envtest apiserver (k8sClient applies the generated CRD). These +// are the rules that "cannot be applied wrong" -- the apiserver rejects them before +// the controller ever sees the object. The reconcile-time backstop is covered +// separately in TestValidateTLS. +func TestTLSCELValidation(t *testing.T) { + if k8sClient == nil { + t.Skip("envtest apiserver not available") + } + + cm := func(issuerName string) ecv1alpha1.ProviderConfig { + return ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", + IssuerName: issuerName, + }, + } + } + + tests := []struct { + name string + surface ecv1alpha1.TLSSurface + wantApply bool // true => apiserver should accept + }{ + { + name: "valid cert-manager mTLS surface accepted", + surface: ecv1alpha1.TLSSurface{Provider: "cert-manager", ProviderCfg: cm("etcd-ca-issuer")}, + wantApply: true, + }, + { + name: "auto provider surface accepted", + surface: ecv1alpha1.TLSSurface{Provider: "auto"}, + wantApply: true, + }, + { + name: "cert-manager provider without certManagerCfg rejected", + surface: ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + wantApply: false, + }, + { + name: "certManagerCfg under auto provider rejected", + surface: ecv1alpha1.TLSSurface{Provider: "auto", ProviderCfg: cm("etcd-ca-issuer")}, + wantApply: false, + }, + { + name: "clientCertAuth true with cert-manager but empty issuerName rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(true), + ProviderCfg: cm(""), + }, + wantApply: false, + }, + { + name: "server-only TLS (clientCertAuth false) without issuerName accepted", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(false), + ProviderCfg: cm("etcd-ca-issuer"), + }, + wantApply: true, + }, + { + name: "bad issuerKind rejected by enum", + surface: ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "Bogus", + IssuerName: "etcd-ca-issuer", + }, + }, + }, + wantApply: false, + }, + { + name: "bad provider rejected by enum", + surface: ecv1alpha1.TLSSurface{ + Provider: "vault", + }, + wantApply: false, + }, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + surface := tt.surface + ec := &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "cel-test-", + Namespace: "default", + }, + Spec: ecv1alpha1.EtcdClusterSpec{ + Size: 3, + Version: "v3.6.12", + // Alternate which surface carries the config so both surfaces' + // XValidation rules are exercised across the table. + TLS: tlsForSurface(i%2 == 0, &surface), + }, + } + err := k8sClient.Create(t.Context(), ec) + if tt.wantApply { + require.NoError(t, err, "apiserver should accept a valid surface") + _ = k8sClient.Delete(t.Context(), ec, &client.DeleteOptions{}) + } else { + assert.Error(t, err, "apiserver should reject an invalid surface via CEL") + } + }) + } +} + +func tlsForSurface(onClient bool, s *ecv1alpha1.TLSSurface) *ecv1alpha1.EtcdClusterTLS { + if onClient { + return &ecv1alpha1.EtcdClusterTLS{Client: s} + } + return &ecv1alpha1.EtcdClusterTLS{Peer: s} +} diff --git a/internal/controller/tls_independence_test.go b/internal/controller/tls_independence_test.go new file mode 100644 index 00000000..f4a95531 --- /dev/null +++ b/internal/controller/tls_independence_test.go @@ -0,0 +1,447 @@ +package controller + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +func newScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, appsv1.AddToScheme(scheme)) + require.NoError(t, ecv1alpha1.AddToScheme(scheme)) + return scheme +} + +func discardLogger() logr.Logger { return logr.Discard() } + +func mustQuantity(s string) resource.Quantity { return resource.MustParse(s) } + +// genClientKeypair returns a (cert, key, ca) PEM triple suitable for +// buildClientTLSConfig: a self-signed leaf is its own CA here, which is fine for +// exercising the operator-side config building (CA-capability across members is an +// e2e concern, out of scope for this unit). +func genClientKeypair(t *testing.T) (certPEM, keyPEM, caPEM []byte) { + t.Helper() + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "etcd-operator-client"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + IsCA: true, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + require.NoError(t, err) + certPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) + + keyDER, err := x509.MarshalPKCS8PrivateKey(priv) + require.NoError(t, err) + keyPEM = pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}) + + return certPEM, keyPEM, certPEM // ca == the self-signed cert +} + +// boolPtr is a small helper for the *bool ClientCertAuth field. +func boolPtr(b bool) *bool { return &b } + +// cmSurface builds a cert-manager TLSSurface with mTLS on by default. +func cmSurface(clientCertAuth *bool) *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", + IssuerName: "etcd-ca-issuer", + }, + }, + ClientCertAuth: clientCertAuth, + } +} + +func clusterWithTLS(name string, tls *ecv1alpha1.EtcdClusterTLS) *ecv1alpha1.EtcdCluster { + return &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, + Spec: ecv1alpha1.EtcdClusterSpec{Size: 3, Version: "v3.6.12", TLS: tls}, + } +} + +// TestTLSIndependenceArgsMatrix asserts that the peer and client surfaces drive the +// rendered etcd args fully independently: peer flags/scheme follow the peer surface, +// server flags/scheme follow the client surface, and each --*client-cert-auth flag +// follows its own surface's toggle. +func TestTLSIndependenceArgsMatrix(t *testing.T) { + const name = "ec" + + cleartextArgs := []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=http://0.0.0.0:2380", + "--listen-client-urls=http://0.0.0.0:2379", + "--initial-advertise-peer-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + } + + serverFlags := []string{ + "--cert-file=/etc/etcd/server-tls/tls.crt", + "--key-file=/etc/etcd/server-tls/tls.key", + "--trusted-ca-file=/etc/etcd/server-tls/ca.crt", + } + peerFlags := []string{ + "--peer-cert-file=/etc/etcd/peer-tls/tls.crt", + "--peer-key-file=/etc/etcd/peer-tls/tls.key", + "--peer-trusted-ca-file=/etc/etcd/peer-tls/ca.crt", + } + + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + expected []string + }{ + { + name: "neither (cleartext) is byte-identical to today", + tls: nil, + expected: cleartextArgs, + }, + { + name: "peer-only: peer https + peer flags, client stays cleartext", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=http://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + { + name: "client-only: client https + server flags, peer stays cleartext", + tls: &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=http://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=http://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + }, + }, + { + name: "both: all eight flags, both schemes https", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + { + name: "peer mTLS opt-out: --peer-client-cert-auth absent, server mTLS still on", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurface(boolPtr(false)), + Client: cmSurface(boolPtr(true)), + }, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], "--client-cert-auth", + peerFlags[0], peerFlags[1], peerFlags[2], + }, + }, + { + name: "client mTLS opt-out: --client-cert-auth absent, peer mTLS still on", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurface(boolPtr(true)), + Client: cmSurface(boolPtr(false)), + }, + expected: []string{ + "--name=$(POD_NAME)", + "--listen-peer-urls=https://0.0.0.0:2380", + "--listen-client-urls=https://0.0.0.0:2379", + "--initial-advertise-peer-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2380", + "--advertise-client-urls=https://$(POD_NAME).ec.$(POD_NAMESPACE).svc.cluster.local:2379", + serverFlags[0], serverFlags[1], serverFlags[2], + peerFlags[0], peerFlags[1], peerFlags[2], "--peer-client-cert-auth", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS(name, tt.tls) + got := createArgs(name, nil, tlsArgsFor(ec)) + assert.Equal(t, tt.expected, got) + }) + } +} + +// TestTLSIndependenceSchemes asserts peerScheme/clientScheme are independent. +func TestTLSIndependenceSchemes(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantPeerScheme string + wantClientScheme string + }{ + {"neither", nil, "http", "http"}, + {"peer-only", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, "https", "http"}, + {"client-only", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, "http", "https"}, + {"both", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, "https", "https"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS("ec", tt.tls) + assert.Equal(t, tt.wantPeerScheme, peerScheme(ec), "peerScheme") + assert.Equal(t, tt.wantClientScheme, clientScheme(ec), "clientScheme") + // Endpoint generators must agree with the per-surface scheme. + _, peerURL := peerEndpointForOrdinalIndex(ec, 0) + assert.Contains(t, peerURL, tt.wantPeerScheme+"://", "peer endpoint scheme") + }) + } +} + +// stsContainer renders the StatefulSet for ec and returns the etcd container plus +// pod volumes, so cert mounts/volumes can be asserted per surface. +func stsContainer(t *testing.T, ec *ecv1alpha1.EtcdCluster) (corev1.Container, []corev1.Volume) { + t.Helper() + scheme := newScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + require.NoError(t, createOrPatchStatefulSet(t.Context(), discardLogger(), ec, c, 1, scheme)) + sts, err := getStatefulSet(t.Context(), c, ec.Name, ec.Namespace) + require.NoError(t, err) + require.Len(t, sts.Spec.Template.Spec.Containers, 1) + return sts.Spec.Template.Spec.Containers[0], sts.Spec.Template.Spec.Volumes +} + +func volumeNames(vols []corev1.Volume) []string { + out := make([]string, 0, len(vols)) + for _, v := range vols { + out = append(out, v.Name) + } + return out +} + +func mountNames(mounts []corev1.VolumeMount) []string { + out := make([]string, 0, len(mounts)) + for _, m := range mounts { + out = append(out, m.Name) + } + return out +} + +// TestTLSIndependenceMounts asserts the server-cert mount is gated on the client +// surface and the peer-cert mount on the peer surface, independently. +func TestTLSIndependenceMounts(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantServer bool + wantPeer bool + }{ + {"neither: no cert volumes/mounts", nil, false, false}, + {"peer-only: peer mount only", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil)}, false, true}, + {"client-only: server mount only", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}, true, false}, + {"both: server + peer mounts", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, true, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ec := clusterWithTLS("ec", tt.tls) + container, vols := stsContainer(t, ec) + vNames := volumeNames(vols) + mNames := mountNames(container.VolumeMounts) + + assert.Equal(t, tt.wantServer, contains(vNames, serverCertVolumeName), "server volume") + assert.Equal(t, tt.wantServer, contains(mNames, serverCertVolumeName), "server mount") + assert.Equal(t, tt.wantPeer, contains(vNames, peerCertVolumeName), "peer volume") + assert.Equal(t, tt.wantPeer, contains(mNames, peerCertVolumeName), "peer mount") + }) + } +} + +// TestTLSIndependenceMountsCoexistWithStorage asserts cert mounts and the storage +// data-dir mount coexist (append-not-assign). +func TestTLSIndependenceMountsCoexistWithStorage(t *testing.T) { + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}) + ec.Spec.StorageSpec = &ecv1alpha1.StorageSpec{ + VolumeSizeRequest: mustQuantity("1Gi"), + } + container, _ := stsContainer(t, ec) + mNames := mountNames(container.VolumeMounts) + assert.Contains(t, mNames, volumeName, "storage data-dir mount") + assert.Contains(t, mNames, serverCertVolumeName, "server cert mount") + assert.Contains(t, mNames, peerCertVolumeName, "peer cert mount") +} + +// TestBuildClientTLSConfigGatedOnClientSurface asserts the operator's *tls.Config is +// built iff the client surface is configured, and nil otherwise (cleartext). +func TestBuildClientTLSConfigGatedOnClientSurface(t *testing.T) { + scheme := newScheme(t) + + // peer-only and neither: operator config must be nil (no secret read at all). + for _, tls := range []*ecv1alpha1.EtcdClusterTLS{ + nil, + {Peer: cmSurface(nil)}, + } { + ec := clusterWithTLS("ec", tls) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + cfg, err := buildClientTLSConfig(t.Context(), ec, c) + require.NoError(t, err) + assert.Nil(t, cfg, "operator client tls.Config must be nil without a client surface") + } + + // client surface set but secret missing => explicit error (not silent nil). + ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + _, err := buildClientTLSConfig(t.Context(), ec, c) + require.Error(t, err, "missing client secret must be an error") + + // client surface set with a valid secret => non-nil config with the CA pinned. + cert, key, ca := genClientKeypair(t) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName("ec"), Namespace: "ns"}, + Data: map[string][]byte{ + tlsCertFile: cert, + tlsKeyFile: key, + tlsCAFile: ca, + }, + } + c = fake.NewClientBuilder().WithScheme(scheme).WithObjects(secret).Build() + cfg, err := buildClientTLSConfig(t.Context(), ec, c) + require.NoError(t, err) + require.NotNil(t, cfg) + assert.NotNil(t, cfg.RootCAs, "RootCAs must be pinned from the secret CA") + assert.Len(t, cfg.Certificates, 1) + + // client surface set, secret present but missing ca.crt => explicit error + // (not a silent fall-through to system roots). + secretNoCA := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getClientCertName("ec"), Namespace: "ns"}, + Data: map[string][]byte{ + tlsCertFile: cert, + tlsKeyFile: key, + }, + } + c = fake.NewClientBuilder().WithScheme(scheme).WithObjects(secretNoCA).Build() + _, err = buildClientTLSConfig(t.Context(), ec, c) + require.Error(t, err, "missing ca.crt must be an explicit error") +} + +// TestValidateTLS covers the reconcile-time anti-misconfiguration backstop. +func TestValidateTLS(t *testing.T) { + tests := []struct { + name string + tls *ecv1alpha1.EtcdClusterTLS + wantErr bool + }{ + {"nil tls is valid (fully cleartext)", nil, false}, + { + name: "valid cert-manager mTLS on both surfaces", + tls: &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}, + wantErr: false, + }, + { + name: "cert-manager provider without certManagerCfg is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + }, + wantErr: true, + }, + { + name: "certManagerCfg under auto provider is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "auto", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + IssuerKind: "ClusterIssuer", IssuerName: "x", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "clientCertAuth (mTLS) with cert-manager but no issuerName is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(true), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer"}, + }, + }, + }, + wantErr: true, + }, + { + name: "server-only TLS (clientCertAuth false) without issuerName is allowed", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ClientCertAuth: boolPtr(false), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer", IssuerName: "x"}, + }, + }, + }, + wantErr: false, + }, + { + name: "auto provider on both surfaces is valid", + tls: &ecv1alpha1.EtcdClusterTLS{ + Peer: &ecv1alpha1.TLSSurface{Provider: "auto"}, + Client: &ecv1alpha1.TLSSurface{Provider: "auto"}, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validateTLS(clusterWithTLS("ec", tt.tls)) + if tt.wantErr { + assert.NotEmpty(t, errs, "expected a validation error") + } else { + assert.Empty(t, errs, "expected no validation error: %v", errs) + } + }) + } +} + +func contains(s []string, v string) bool { + for _, x := range s { + if x == v { + return true + } + } + return false +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 3092c379..0ed33dca 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -2,9 +2,10 @@ package controller import ( "context" + "crypto/tls" + "crypto/x509" "errors" "fmt" - "log" "maps" "net" "slices" @@ -22,10 +23,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" "go.etcd.io/etcd-operator/internal/etcdutils" @@ -37,8 +40,124 @@ import ( const ( etcdDataDir = "/var/lib/etcd" volumeName = "etcd-data" + + // Cert mount points inside the etcd container. The server (client-surface) and + // peer secrets are mounted independently so a single-surface cluster only mounts + // the secret it actually needs. + serverCertVolumeName = "server-secret" + peerCertVolumeName = "peer-secret" + serverCertMountPath = "/etc/etcd/server-tls" + peerCertMountPath = "/etc/etcd/peer-tls" + + // Standard cert-secret keys (cert-manager and the auto provider both write these). + tlsCertFile = "tls.crt" + tlsKeyFile = "tls.key" + tlsCAFile = "ca.crt" + + schemeHTTP = "http" + schemeHTTPS = "https" ) +// peerScheme returns the URL scheme etcd serves/dials on its peer port. It is +// "https" iff the peer TLS surface is configured, otherwise "http". This is the +// single source of truth for the peer scheme: peer args and every peer-endpoint +// generator derive from it so the operator can never compute a peer URL with a +// scheme etcd does not serve. +func peerScheme(ec *ecv1alpha1.EtcdCluster) string { + if ec.Spec.TLS != nil && ec.Spec.TLS.Peer != nil { + return schemeHTTPS + } + return schemeHTTP +} + +// clientScheme returns the URL scheme etcd serves/dials on its client port. It is +// "https" iff the client TLS surface is configured, otherwise "http". Single source +// of truth for the client scheme (client args, client endpoints, and the operator's +// own dial scheme all derive from it). +func clientScheme(ec *ecv1alpha1.EtcdCluster) string { + if ec.Spec.TLS != nil && ec.Spec.TLS.Client != nil { + return schemeHTTPS + } + return schemeHTTP +} + +// peerTLSEnabled reports whether the peer TLS surface is configured. +func peerTLSEnabled(ec *ecv1alpha1.EtcdCluster) bool { + return ec.Spec.TLS != nil && ec.Spec.TLS.Peer != nil +} + +// clientTLSEnabled reports whether the client TLS surface is configured. +func clientTLSEnabled(ec *ecv1alpha1.EtcdCluster) bool { + return ec.Spec.TLS != nil && ec.Spec.TLS.Client != nil +} + +// clientCertAuthEnabled resolves a surface's ClientCertAuth toggle, defaulting to +// true (mTLS) when unset, matching the CRD default. A nil surface returns false. +func clientCertAuthEnabled(s *ecv1alpha1.TLSSurface) bool { + if s == nil { + return false + } + if s.ClientCertAuth == nil { + return true + } + return *s.ClientCertAuth +} + +// validateTLS is the reconcile-time anti-misconfiguration backstop for the two TLS +// surfaces. It re-checks the spec-only coherence rules that are *also* expressed as +// CEL XValidation markers on TLSSurface (api/v1alpha1/etcdcluster_types.go), so the +// operator still rejects an invalid spec on an apiserver that does not enforce CEL +// (e.g. pre-1.25, CEL feature-gated off, or a CR written directly to a fake client +// in tests). These are the spec-derivable rules ONLY: +// +// - provider 'cert-manager' => providerCfg.certManagerCfg must be set +// - provider auto/empty => providerCfg.certManagerCfg must NOT be set +// - clientCertAuth (mTLS) with cert-manager => issuerName must be set (a CA source) +// +// Rules that require reading cluster objects are intentionally NOT here: issuer +// existence and kind are enforced when the cert is provisioned (the cert-manager +// provider's validateCertificateConfig -> checkIssuerExists returns an error that +// requeues), and peer CA-capability / client-server CA equality live on the +// cert-manager Issuer and secret objects, not on this spec, so they cannot be +// checked purely from the EtcdCluster (plan Decision 2.5-2.7 / Decision 3). For the +// client surface specifically, the operator-client cert and the server cert both +// flow through the SINGLE client surface's issuer, so they share a CA by +// construction -- the cheap form of the client/server CA-match rule is satisfied +// without a runtime compare. +func validateTLS(ec *ecv1alpha1.EtcdCluster) field.ErrorList { + var errs field.ErrorList + if ec.Spec.TLS == nil { + return errs + } + base := field.NewPath("spec", "tls") + errs = append(errs, validateTLSSurface(base.Child("peer"), ec.Spec.TLS.Peer)...) + errs = append(errs, validateTLSSurface(base.Child("client"), ec.Spec.TLS.Client)...) + return errs +} + +func validateTLSSurface(path *field.Path, s *ecv1alpha1.TLSSurface) field.ErrorList { + var errs field.ErrorList + if s == nil { + return errs + } + hasCM := s.ProviderCfg.CertManagerCfg != nil + isCertManager := s.Provider == string(certificate.CertManager) + switch { + case isCertManager && !hasCM: + errs = append(errs, field.Required(path.Child("providerCfg", "certManagerCfg"), + "provider 'cert-manager' requires providerCfg.certManagerCfg")) + case !isCertManager && hasCM: + errs = append(errs, field.Invalid(path.Child("providerCfg", "certManagerCfg"), "", + "providerCfg.certManagerCfg may only be set when provider is 'cert-manager'")) + } + // mTLS requires a resolvable CA. With cert-manager that means an issuerName. + if clientCertAuthEnabled(s) && isCertManager && hasCM && s.ProviderCfg.CertManagerCfg.IssuerName == "" { + errs = append(errs, field.Required(path.Child("providerCfg", "certManagerCfg", "issuerName"), + "clientCertAuth requires a trusted CA: set providerCfg.certManagerCfg.issuerName")) + } + return errs +} + type etcdClusterState string const ( @@ -76,14 +195,73 @@ func reconcileStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1alpha return getStatefulSet(ctx, c, ec.Name, ec.Namespace) } -func defaultArgs(name string) []string { - return []string{ +// tlsArgs captures the per-surface TLS decisions that drive defaultArgs. Each +// surface is independent: the peer flag group and peer https scheme are gated on +// peerEnabled, the server flag group and client https scheme on clientEnabled, and +// each --*client-cert-auth flag on its own *CertAuth toggle. The zero value +// (everything false) yields byte-identical cleartext output to the pre-TLS args. +type tlsArgs struct { + peerEnabled bool + clientEnabled bool + peerCertAuth bool + clientCertAuth bool +} + +// tlsArgsFor derives tlsArgs from an EtcdCluster's two TLS surfaces. +func tlsArgsFor(ec *ecv1alpha1.EtcdCluster) tlsArgs { + var a tlsArgs + if ec.Spec.TLS != nil { + a.peerEnabled = ec.Spec.TLS.Peer != nil + a.clientEnabled = ec.Spec.TLS.Client != nil + a.peerCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Peer) + a.clientCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Client) + } + return a +} + +func defaultArgs(name string, tls tlsArgs) []string { + peerScheme := schemeHTTP + if tls.peerEnabled { + peerScheme = schemeHTTPS + } + clientScheme := schemeHTTP + if tls.clientEnabled { + clientScheme = schemeHTTPS + } + + args := []string{ "--name=$(POD_NAME)", - "--listen-peer-urls=http://0.0.0.0:2380", // TODO: only listen on 127.0.0.1 and host IP - "--listen-client-urls=http://0.0.0.0:2379", // TODO: only listen on 127.0.0.1 and host IP - fmt.Sprintf("--initial-advertise-peer-urls=http://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2380", name), - fmt.Sprintf("--advertise-client-urls=http://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2379", name), + fmt.Sprintf("--listen-peer-urls=%s://0.0.0.0:2380", peerScheme), // TODO: only listen on 127.0.0.1 and host IP + fmt.Sprintf("--listen-client-urls=%s://0.0.0.0:2379", clientScheme), // TODO: only listen on 127.0.0.1 and host IP + fmt.Sprintf("--initial-advertise-peer-urls=%s://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2380", peerScheme, name), + fmt.Sprintf("--advertise-client-urls=%s://$(POD_NAME).%s.$(POD_NAMESPACE).svc.cluster.local:2379", clientScheme, name), + } + + // Server (client-surface) TLS flag group: emitted iff the client surface is set. + if tls.clientEnabled { + args = append(args, + fmt.Sprintf("--cert-file=%s/%s", serverCertMountPath, tlsCertFile), + fmt.Sprintf("--key-file=%s/%s", serverCertMountPath, tlsKeyFile), + fmt.Sprintf("--trusted-ca-file=%s/%s", serverCertMountPath, tlsCAFile), + ) + if tls.clientCertAuth { + args = append(args, "--client-cert-auth") + } } + + // Peer TLS flag group: emitted iff the peer surface is set. + if tls.peerEnabled { + args = append(args, + fmt.Sprintf("--peer-cert-file=%s/%s", peerCertMountPath, tlsCertFile), + fmt.Sprintf("--peer-key-file=%s/%s", peerCertMountPath, tlsKeyFile), + fmt.Sprintf("--peer-trusted-ca-file=%s/%s", peerCertMountPath, tlsCAFile), + ) + if tls.peerCertAuth { + args = append(args, "--peer-client-cert-auth") + } + } + + return args } func RemoveStringFromSlice(s []string, str string) []string { @@ -113,8 +291,8 @@ func getArgName(s string) string { return strings.TrimSpace(s) } -func createArgs(name string, etcdOptions []string) []string { - defaultArgs := defaultArgs(name) +func createArgs(name string, etcdOptions []string, tls tlsArgs) []string { + defaultArgs := defaultArgs(name, tls) if len(etcdOptions) > 0 { var argName string // Remove default arguments if conflicts with user supplied @@ -145,7 +323,7 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a { Name: "etcd", Command: []string{"/usr/local/bin/etcd"}, - Args: createArgs(ec.Name, ec.Spec.EtcdOptions), + Args: createArgs(ec.Name, ec.Spec.EtcdOptions, tlsArgsFor(ec)), Image: fmt.Sprintf("%s:%s", ec.Spec.ImageRegistry, ec.Spec.Version), Env: []corev1.EnvVar{ { @@ -188,28 +366,45 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a }, } - // mount server and peer certificate secret to each pods of the statefulset via PodSpec + // Mount the server and peer certificate secrets per surface, independently: the + // server (client-surface) secret iff the client surface is configured, the peer + // secret iff the peer surface is configured. A single-surface cluster only mounts + // the secret it needs. VolumeMounts are appended (not assigned) so they coexist + // with the storage data-dir mount added later. var certVolume []corev1.Volume - serverCertName := getServerCertName(ec.Name) - peerCertName := getPeerCertName(ec.Name) - if ec.Spec.TLS != nil { - serverCertVolume := corev1.Volume{ - Name: "server-secret", + var certMounts []corev1.VolumeMount + if clientTLSEnabled(ec) { + certVolume = append(certVolume, corev1.Volume{ + Name: serverCertVolumeName, VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{SecretName: serverCertName}, + Secret: &corev1.SecretVolumeSource{SecretName: getServerCertName(ec.Name)}, }, - } - peerCertVolume := corev1.Volume{ - Name: "peer-secret", + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: serverCertVolumeName, + MountPath: serverCertMountPath, + ReadOnly: true, + }) + } + if peerTLSEnabled(ec) { + certVolume = append(certVolume, corev1.Volume{ + Name: peerCertVolumeName, VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{SecretName: peerCertName}, + Secret: &corev1.SecretVolumeSource{SecretName: getPeerCertName(ec.Name)}, }, - } - certVolume = append(certVolume, serverCertVolume, peerCertVolume) + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: peerCertVolumeName, + MountPath: peerCertMountPath, + ReadOnly: true, + }) } if len(certVolume) != 0 { podSpec.Volumes = certVolume } + if len(certMounts) != 0 { + podSpec.Containers[0].VolumeMounts = append(podSpec.Containers[0].VolumeMounts, certMounts...) + } // Prepare pod template metadata podTemplateMetadata := metav1.ObjectMeta{ @@ -254,11 +449,15 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a if ec.Spec.StorageSpec != nil { - stsSpec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{ - Name: volumeName, - MountPath: etcdDataDir, - SubPathExpr: "$(POD_NAME)", - }} + // Append the storage data-dir mount so it coexists with any TLS cert mounts + // added above (assigning here would clobber them). + stsSpec.Template.Spec.Containers[0].VolumeMounts = append( + stsSpec.Template.Spec.Containers[0].VolumeMounts, + corev1.VolumeMount{ + Name: volumeName, + MountPath: etcdDataDir, + SubPathExpr: "$(POD_NAME)", + }) // Create a new volume claim template if ec.Spec.StorageSpec.VolumeSizeRequest.Cmp(resource.MustParse("1Mi")) < 0 { return fmt.Errorf("VolumeSizeRequest must be at least 1Mi") @@ -424,8 +623,8 @@ func configMapNameForEtcdCluster(ec *ecv1alpha1.EtcdCluster) string { func peerEndpointForOrdinalIndex(ec *ecv1alpha1.EtcdCluster, index int) (string, string) { name := fmt.Sprintf("%s-%d", ec.Name, index) - return name, fmt.Sprintf("http://%s-%d.%s.%s.svc.cluster.local:2380", - ec.Name, index, ec.Name, ec.Namespace) + return name, fmt.Sprintf("%s://%s-%d.%s.%s.svc.cluster.local:2380", + peerScheme(ec), ec.Name, index, ec.Name, ec.Namespace) } func newEtcdClusterState(ec *ecv1alpha1.EtcdCluster, replica int) *corev1.ConfigMap { @@ -478,9 +677,12 @@ func applyEtcdClusterState(ctx context.Context, ec *ecv1alpha1.EtcdCluster, repl return updateErr } -func clientEndpointForOrdinalIndex(sts *appsv1.StatefulSet, index int) string { - return fmt.Sprintf("http://%s-%d.%s.%s.svc.cluster.local:2379", - sts.Name, index, sts.Name, sts.Namespace) +// clientEndpointForOrdinalIndex builds the operator-facing client URL for one +// member. scheme ("http"/"https") MUST be the client surface's scheme (clientScheme) +// so the operator dials the same scheme etcd serves on its client port. +func clientEndpointForOrdinalIndex(sts *appsv1.StatefulSet, index int, scheme string) string { + return fmt.Sprintf("%s://%s-%d.%s.%s.svc.cluster.local:2379", + scheme, sts.Name, index, sts.Name, sts.Namespace) } func getStatefulSet(ctx context.Context, c client.Client, name, namespace string) (*appsv1.StatefulSet, error) { @@ -492,19 +694,21 @@ func getStatefulSet(ctx context.Context, c client.Client, name, namespace string return sts, nil } -func clientEndpointsFromStatefulsets(sts *appsv1.StatefulSet) []string { +// clientEndpointsFromStatefulsets builds the operator's client endpoints. scheme +// MUST be the client surface's scheme (clientScheme of the owning EtcdCluster). +func clientEndpointsFromStatefulsets(sts *appsv1.StatefulSet, scheme string) []string { var endpoints []string replica := int(*sts.Spec.Replicas) if replica > 0 { for i := 0; i < replica; i++ { - endpoints = append(endpoints, clientEndpointForOrdinalIndex(sts, i)) + endpoints = append(endpoints, clientEndpointForOrdinalIndex(sts, i, scheme)) } } return endpoints } -func areAllMembersHealthy(sts *appsv1.StatefulSet, logger logr.Logger) (bool, error) { - _, health, err := healthCheck(sts, logger) +func areAllMembersHealthy(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, sts *appsv1.StatefulSet, logger logr.Logger) (bool, error) { + _, health, err := healthCheck(ctx, ec, c, sts, logger) if err != nil { return false, err } @@ -519,16 +723,23 @@ func areAllMembersHealthy(sts *appsv1.StatefulSet, logger logr.Logger) (bool, er // healthCheck returns a memberList and an error. // If any member (excluding not yet started or already removed member) -// is unhealthy, the error won't be nil. -func healthCheck(sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListResponse, []etcdutils.EpHealth, error) { +// is unhealthy, the error won't be nil. The operator dials the client surface's +// scheme and, when the client surface is configured, presents its client TLS +// config; both derive from ec so the operator never mismatches what etcd serves. +func healthCheck(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListResponse, []etcdutils.EpHealth, error) { replica := int(*sts.Spec.Replicas) if replica == 0 { return nil, nil, nil } - endpoints := clientEndpointsFromStatefulsets(sts) + tlsConfig, err := buildClientTLSConfig(ctx, ec, c) + if err != nil { + return nil, nil, err + } + + endpoints := clientEndpointsFromStatefulsets(sts, clientScheme(ec)) - memberlistResp, err := etcdutils.MemberList(endpoints) + memberlistResp, err := etcdutils.MemberList(endpoints, tlsConfig) if err != nil { return nil, nil, err } @@ -544,7 +755,7 @@ func healthCheck(sts *appsv1.StatefulSet, lg klog.Logger) (*clientv3.MemberListR lg.Info("health checking", "replica", replica, "len(members)", memberCnt) endpoints = endpoints[:cnt] - healthInfos, err := etcdutils.ClusterHealth(endpoints) + healthInfos, err := etcdutils.ClusterHealth(endpoints, tlsConfig) if err != nil { return memberlistResp, nil, err } @@ -590,8 +801,8 @@ func parseValidityDuration(customizedDuration string, defaultDuration time.Durat return duration, nil } -func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) { - cmConfig := ec.Spec.TLS.ProviderCfg.CertManagerCfg +func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { + cmConfig := surface.ProviderCfg.CertManagerCfg if cmConfig == nil { return nil, fmt.Errorf("cert-manager configuration is not present") } @@ -626,15 +837,16 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Confi ValidityDuration: duration, AltNames: getAltNames, ExtraConfig: map[string]any{ - "issuerName": cmConfig.IssuerName, - "issuerKind": cmConfig.IssuerKind, + "issuerName": cmConfig.IssuerName, + "issuerKind": cmConfig.IssuerKind, + "issuerGroup": cmConfig.IssuerGroup, }, } return config, nil } -func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) { - autoConfig := ec.Spec.TLS.ProviderCfg.AutoCfg +func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { + autoConfig := surface.ProviderCfg.AutoCfg // Set default values for auto configuration if not present if autoConfig == nil { autoConfig = &ecv1alpha1.ProviderAutoConfig{ @@ -678,9 +890,14 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Con return config, nil } -func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client.Client, certName string) error { - // The TLS field is present but spec is empty - providerName := ec.Spec.TLS.Provider +// createCertificate provisions the certificate named certName from a SPECIFIC TLS +// surface (peer or client). The surface carries its own provider and provider +// config, so peer and client certs can flow through different issuers. +func createCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, surface *ecv1alpha1.TLSSurface, certName string) error { + logger := log.FromContext(ctx) + + // An empty provider on the surface defaults to the auto provider. + providerName := surface.Provider if providerName == "" { providerName = string(certificate.Auto) } @@ -693,12 +910,12 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client _, getCertError := cert.GetCertificateConfig(ctx, client.ObjectKey{Name: certName, Namespace: ec.Namespace}) if getCertError != nil { if k8serrors.IsNotFound(getCertError) { - log.Printf("Creating certificate: %s for etcd-operator: %s\n", certName, ec.Name) + logger.Info("Creating certificate", "certificate", certName, "etcdCluster", ec.Name) secretKey := client.ObjectKey{Name: certName, Namespace: ec.Namespace} switch certificate.ProviderType(providerName) { case certificate.Auto: - autoConfig, err := createAutoCertificateConfig(ec) + autoConfig, err := createAutoCertificateConfig(ec, surface) if err != nil { return fmt.Errorf("error creating auto certificate config: %w", err) } @@ -708,7 +925,7 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client } return nil case certificate.CertManager: - cmConfig, err := createCMCertificateConfig(ec) + cmConfig, err := createCMCertificateConfig(ec, surface) if err != nil { return fmt.Errorf("error creating cert-manager certificate config: %w", err) } @@ -719,7 +936,7 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client return nil default: // This should never happen // TODO: Use AuthProvider, since both AutoCfg and CertManagerCfg is not present - log.Printf("Error creating certificate, valid certificate provider not defined.") + logger.Info("Error creating certificate, valid certificate provider not defined", "certificate", certName) return nil } } else { @@ -730,9 +947,11 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client return nil } +// createClientCertificate provisions the operator's own client identity from the +// CLIENT surface. The operator authenticates to etcd's client port as a client. func createClientCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { certName := getClientCertName(ec.Name) - err := createCertificate(ec, ctx, c, certName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Client, certName) if err != nil { return err } @@ -743,9 +962,11 @@ func createClientCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c return err } +// createServerCertificate provisions etcd's server (client-facing) identity from +// the CLIENT surface. func createServerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { serverCertName := getServerCertName(ec.Name) - err := createCertificate(ec, ctx, c, serverCertName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Client, serverCertName) if err != nil { return err } @@ -756,9 +977,10 @@ func createServerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c return nil } +// createPeerCertificate provisions etcd's peer identity from the PEER surface. func createPeerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { peerCertName := getPeerCertName(ec.Name) - err := createCertificate(ec, ctx, c, peerCertName) + err := createCertificate(ctx, ec, c, ec.Spec.TLS.Peer, peerCertName) if err != nil { return err } @@ -769,27 +991,78 @@ func createPeerCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c cl return nil } +// applyEtcdMemberCerts provisions per-surface member certs independently: the +// server cert iff the client surface is configured, the peer cert iff the peer +// surface is configured. (The operator's own client cert is provisioned separately +// in fetchAndValidateState, also gated on the client surface.) func applyEtcdMemberCerts(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { - if ec.Spec.TLS != nil { - err := createServerCertificate(ctx, ec, c) - if err != nil { + if clientTLSEnabled(ec) { + if err := createServerCertificate(ctx, ec, c); err != nil { return err } - err = createPeerCertificate(ctx, ec, c) - if err != nil { + } + if peerTLSEnabled(ec) { + if err := createPeerCertificate(ctx, ec, c); err != nil { return err } } return nil } +// buildClientTLSConfig builds the operator's etcd-client *tls.Config from the +// CLIENT surface's secret. It returns (nil, nil) when the client surface is not +// configured, so the operator dials cleartext (byte-identical to the pre-TLS path). +// When configured it loads the operator client keypair and pins the server's CA; +// a missing ca.crt is an explicit error (rather than silently falling through to +// the system trust store, which would verify against the wrong roots and fail +// opaquely later). +func buildClientTLSConfig(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (*tls.Config, error) { + if !clientTLSEnabled(ec) { + return nil, nil + } + + secretName := getClientCertName(ec.Name) + secret := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKey{Name: secretName, Namespace: ec.Namespace}, secret); err != nil { + return nil, fmt.Errorf("failed to get operator client TLS secret %s/%s: %w", ec.Namespace, secretName, err) + } + + certPEM, ok := secret.Data[tlsCertFile] + if !ok || len(certPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q", ec.Namespace, secretName, tlsCertFile) + } + keyPEM, ok := secret.Data[tlsKeyFile] + if !ok || len(keyPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q", ec.Namespace, secretName, tlsKeyFile) + } + keyPair, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return nil, fmt.Errorf("failed to build operator client keypair from secret %s/%s: %w", ec.Namespace, secretName, err) + } + + caPEM, ok := secret.Data[tlsCAFile] + if !ok || len(caPEM) == 0 { + return nil, fmt.Errorf("operator client TLS secret %s/%s missing %q (cannot verify the etcd server)", ec.Namespace, secretName, tlsCAFile) + } + caPool := x509.NewCertPool() + if !caPool.AppendCertsFromPEM(caPEM) { + return nil, fmt.Errorf("operator client TLS secret %s/%s has an unparseable %q", ec.Namespace, secretName, tlsCAFile) + } + + return &tls.Config{ + Certificates: []tls.Certificate{keyPair}, + RootCAs: caPool, + MinVersion: tls.VersionTLS12, + }, nil +} + func patchCertificateSecret(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, certSecretName string) error { getCertSecret := &corev1.Secret{} if err := c.Get(ctx, client.ObjectKey{Name: certSecretName, Namespace: ec.Namespace}, getCertSecret); err != nil { return err } - log.Printf("Setting ownerReference for certificate secret: %s", certSecretName) + log.FromContext(ctx).Info("Setting ownerReference for certificate secret", "secret", certSecretName) if err := controllerutil.SetControllerReference(ec, getCertSecret, c.Scheme()); err != nil { return err } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 67592316..0f27b7a3 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -205,7 +205,7 @@ func TestClientEndpointForOrdinalIndex(t *testing.T) { for _, tt := range tests { t.Run(fmt.Sprintf("index %d", tt.index), func(t *testing.T) { - result := clientEndpointForOrdinalIndex(sts, tt.index) + result := clientEndpointForOrdinalIndex(sts, tt.index, "http") assert.Equal(t, tt.expectedResult, result) }) } @@ -384,7 +384,7 @@ func TestClientEndpointsFromStatefulsets(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := clientEndpointsFromStatefulsets(tt.statefulSet) + result := clientEndpointsFromStatefulsets(tt.statefulSet, "http") assert.Equal(t, tt.expectedResult, result) }) } @@ -423,7 +423,13 @@ func TestAreAllMembersHealthy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { logger := logr.Discard() // Use a no-op logger for testing - result, err := areAllMembersHealthy(tt.statefulSet, logger) + // Cleartext cluster: buildClientTLSConfig returns nil and the dial + // times out, exactly as before the TLS-config threading. + ec := &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sts", Namespace: "default"}, + } + fakeClient := fake.NewClientBuilder().Build() + result, err := areAllMembersHealthy(t.Context(), ec, fakeClient, tt.statefulSet, logger) assert.Equal(t, tt.expectedResult, result) if tt.expectedError != nil { assert.Error(t, err) @@ -788,7 +794,7 @@ func TestCreatingArgs(t *testing.T) { } for _, tt := range tests { t.Run(tt.testName, func(t *testing.T) { - result := createArgs(tt.clusterName, tt.etcdOptions) + result := createArgs(tt.clusterName, tt.etcdOptions, tlsArgs{}) assert.Equal(t, tt.expectedResult, result) }) } @@ -910,6 +916,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -920,19 +927,17 @@ func TestCreateAutoCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: &ecv1alpha1.ProviderAutoConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "custom.example.com", - Organization: []string{"Test Org"}, - ValidityDuration: "720h", // 30 days - AltNames: ecv1alpha1.AltNames{ - DNSNames: []string{"custom1.example.com", "custom2.example.com"}, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.Auto), + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: &ecv1alpha1.ProviderAutoConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "custom.example.com", + Organization: []string{"Test Org"}, + ValidityDuration: "720h", // 30 days + AltNames: ecv1alpha1.AltNames{ + DNSNames: []string{"custom1.example.com", "custom2.example.com"}, }, }, }, @@ -956,13 +961,11 @@ func TestCreateAutoCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: nil, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.Auto), + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: nil, }, }, expected: &certInterface.Config{ @@ -982,7 +985,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := createAutoCertificateConfig(tt.ec) + result, err := createAutoCertificateConfig(tt.ec, tt.surface) if tt.wantErr { require.Error(t, err) @@ -1004,6 +1007,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { tests := []struct { name string ec *ecv1alpha1.EtcdCluster + surface *ecv1alpha1.TLSSurface expected *certInterface.Config wantErr bool }{ @@ -1014,23 +1018,22 @@ func TestCreateCMCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.CertManager), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "cm.example.com", - Organization: []string{"CM Org"}, - ValidityDuration: "1440h", // 60 days - AltNames: ecv1alpha1.AltNames{ - DNSNames: []string{"cm1.example.com", "cm2.example.com"}, - }, - }, - IssuerName: "test-issuer", - IssuerKind: "ClusterIssuer", + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.CertManager), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "cm.example.com", + Organization: []string{"CM Org"}, + ValidityDuration: "1440h", // 60 days + AltNames: ecv1alpha1.AltNames{ + DNSNames: []string{"cm1.example.com", "cm2.example.com"}, }, }, + IssuerName: "test-issuer", + IssuerKind: "ClusterIssuer", + IssuerGroup: "example.io", }, }, }, @@ -1043,8 +1046,9 @@ func TestCreateCMCertificateConfig(t *testing.T) { IPs: make([]net.IP, 2), }, ExtraConfig: map[string]any{ - "issuerName": "test-issuer", - "issuerKind": "ClusterIssuer", + "issuerName": "test-issuer", + "issuerKind": "ClusterIssuer", + "issuerGroup": "example.io", }, }, wantErr: false, @@ -1056,13 +1060,11 @@ func TestCreateCMCertificateConfig(t *testing.T) { Name: "test-cluster", Namespace: "test-namespace", }, - Spec: ecv1alpha1.EtcdClusterSpec{ - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.CertManager), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: nil, - }, - }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: string(certificate.CertManager), + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: nil, }, }, expected: nil, @@ -1072,7 +1074,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := createCMCertificateConfig(tt.ec) + result, err := createCMCertificateConfig(tt.ec, tt.surface) if tt.wantErr { require.Error(t, err) diff --git a/internal/etcdutils/etcdutils.go b/internal/etcdutils/etcdutils.go index 00f9493e..b061ed66 100644 --- a/internal/etcdutils/etcdutils.go +++ b/internal/etcdutils/etcdutils.go @@ -2,6 +2,7 @@ package etcdutils import ( "context" + "crypto/tls" "errors" "fmt" "sort" @@ -17,12 +18,19 @@ import ( clientv3 "go.etcd.io/etcd/client/v3" ) -func MemberList(eps []string) (*clientv3.MemberListResponse, error) { +// All etcd-client entry points take a trailing optional *tls.Config. When nil the +// client dials cleartext (byte-identical to the pre-TLS behaviour); when non-nil +// the config is set on clientv3.Config.TLS so the operator authenticates to a TLS +// etcd over its client port. The caller builds this from the client TLS surface +// only (see controller.buildClientTLSConfig). + +func MemberList(eps []string, tlsConfig *tls.Config) (*clientv3.MemberListResponse, error) { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -120,7 +128,7 @@ func FindLearnerStatus(healthInfos []EpHealth, logger logr.Logger) (uint64, *cli return learner, learnerStatus } -func ClusterHealth(eps []string) ([]EpHealth, error) { +func ClusterHealth(eps []string, tlsConfig *tls.Config) ([]EpHealth, error) { lg, err := logutil.CreateDefaultZapLogger(zap.InfoLevel) if err != nil { return nil, err @@ -133,6 +141,7 @@ func ClusterHealth(eps []string) ([]EpHealth, error) { DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } cfgs = append(cfgs, cfg) @@ -200,12 +209,13 @@ func ClusterHealth(eps []string) ([]EpHealth, error) { return healthList, nil } -func AddMember(eps []string, peerURLs []string, learner bool) (*clientv3.MemberAddResponse, error) { +func AddMember(eps []string, peerURLs []string, learner bool, tlsConfig *tls.Config) (*clientv3.MemberAddResponse, error) { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -230,12 +240,13 @@ func AddMember(eps []string, peerURLs []string, learner bool) (*clientv3.MemberA return c.MemberAdd(ctx, peerURLs) } -func PromoteLearner(eps []string, learnerId uint64) error { +func PromoteLearner(eps []string, learnerId uint64, tlsConfig *tls.Config) error { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) @@ -257,12 +268,13 @@ func PromoteLearner(eps []string, learnerId uint64) error { return err } -func RemoveMember(eps []string, memberID uint64) error { +func RemoveMember(eps []string, memberID uint64, tlsConfig *tls.Config) error { cfg := clientv3.Config{ Endpoints: eps, DialTimeout: 2 * time.Second, DialKeepAliveTime: 2 * time.Second, DialKeepAliveTimeout: 6 * time.Second, + TLS: tlsConfig, } c, err := clientv3.New(cfg) diff --git a/internal/etcdutils/etcdutils_test.go b/internal/etcdutils/etcdutils_test.go index a7cee425..1bb0eb9f 100644 --- a/internal/etcdutils/etcdutils_test.go +++ b/internal/etcdutils/etcdutils_test.go @@ -1,6 +1,14 @@ package etcdutils import ( + "crypto/ecdsa" + "crypto/elliptic" + crand "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "math/big" + "net" "testing" "time" @@ -60,7 +68,7 @@ func TestMemberList(t *testing.T) { assert.NoError(t, err) eps := []string{"http://localhost:2379"} - resp, err := MemberList(eps) + resp, err := MemberList(eps, nil) assert.NoError(t, err) assert.NotNil(t, resp) assert.Greater(t, len(resp.Members), 0) @@ -68,10 +76,82 @@ func TestMemberList(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - resp, err := MemberList(eps) + resp, err := MemberList(eps, nil) assert.Error(t, err) assert.Nil(t, resp) }) + + // The optional *tls.Config must actually be threaded onto clientv3.Config.TLS, + // not dropped. Point the client at a raw TLS listener over an https:// endpoint: + // a non-nil config drives a TLS handshake against the listener (which records + // it), proving the parameter reaches the dialer. + t.Run("NonNilTLSConfigIsApplied", func(t *testing.T) { + ln, err := tls.Listen("tcp", "127.0.0.1:0", selfSignedServerTLS(t)) + assert.NoError(t, err) + defer ln.Close() + + handshakeSeen := make(chan struct{}, 1) + go func() { + for { + conn, aerr := ln.Accept() + if aerr != nil { + return + } + if tc, ok := conn.(*tls.Conn); ok { + // A successful handshake means the client dialed TLS. + if tc.Handshake() == nil { + select { + case handshakeSeen <- struct{}{}: + default: + } + } + } + _ = conn.Close() + } + }() + + eps := []string{"https://" + ln.Addr().String()} + // The gRPC dial is lazy, so MemberList will error (the listener is not etcd), + // but the dial itself must perform a TLS handshake when a config is supplied. + // Run it in the background so the test returns as soon as the handshake lands + // rather than waiting out the client's dial-retry timeout. + go func() { + _, _ = MemberList(eps, &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}) + }() + + select { + case <-handshakeSeen: + // good: the *tls.Config drove a real TLS handshake. + case <-time.After(10 * time.Second): + t.Fatal("expected a TLS handshake from the threaded *tls.Config, saw none") + } + }) +} + +// selfSignedServerTLS returns a *tls.Config with a self-signed server cert for the +// raw TLS listener used to prove the client-side *tls.Config is applied. +func selfSignedServerTLS(t *testing.T) *tls.Config { + priv, err := ecdsa.GenerateKey(elliptic.P256(), crand.Reader) + if err != nil { + t.Fatalf("genkey: %v", err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "127.0.0.1"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + der, err := x509.CreateCertificate(crand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("createcert: %v", err) + } + return &tls.Config{ + Certificates: []tls.Certificate{{Certificate: [][]byte{der}, PrivateKey: priv}}, + MinVersion: tls.VersionTLS12, + } } func TestClusterHealth(t *testing.T) { @@ -80,7 +160,7 @@ func TestClusterHealth(t *testing.T) { t.Run("ReturnsHealthStatus", func(t *testing.T) { eps := []string{"http://localhost:2379"} - health, err := ClusterHealth(eps) + health, err := ClusterHealth(eps, nil) assert.NoError(t, err) assert.NotNil(t, health) assert.Greater(t, len(health), 0) @@ -89,7 +169,7 @@ func TestClusterHealth(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - health, err := ClusterHealth(eps) + health, err := ClusterHealth(eps, nil) assert.NoError(t, err) assert.Equal(t, "http://invalid:2379", health[0].Ep) assert.Equal(t, false, health[0].Health) @@ -104,7 +184,7 @@ func TestAddMember(t *testing.T) { t.Run("AddsNewMember", func(t *testing.T) { eps := []string{"http://localhost:2379"} peerURLs := []string{"http://127.0.0.1:2380"} - resp, err := AddMember(eps, peerURLs, false) + resp, err := AddMember(eps, peerURLs, false, nil) assert.NoError(t, err) assert.NotNil(t, resp) assert.Greater(t, len(resp.Members), 0) @@ -113,7 +193,7 @@ func TestAddMember(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} peerURLs := []string{"http://127.0.0.1:2380"} - resp, err := AddMember(eps, peerURLs, false) + resp, err := AddMember(eps, peerURLs, false, nil) assert.Error(t, err) assert.Nil(t, resp) }) @@ -126,17 +206,17 @@ func TestPromoteLearner(t *testing.T) { t.Run("PromotesMember", func(t *testing.T) { eps := []string{"http://localhost:2379"} peerURLs := []string{"http://test123:2380"} - addResp, err := AddMember(eps, peerURLs, true) + addResp, err := AddMember(eps, peerURLs, true, nil) assert.NoError(t, err) - err = PromoteLearner(eps, addResp.Member.ID) + err = PromoteLearner(eps, addResp.Member.ID, nil) assert.Error(t, err) assert.Equal(t, err.Error(), "etcdserver: can only promote a learner member which is in sync with leader") }) t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - err := PromoteLearner(eps, 12345) + err := PromoteLearner(eps, 12345, nil) assert.Error(t, err) }) } @@ -147,7 +227,7 @@ func TestRemoveMember(t *testing.T) { t.Run("ReturnsErrorForInvalidEndpoint", func(t *testing.T) { eps := []string{"http://invalid:2379"} - err := RemoveMember(eps, 12345) + err := RemoveMember(eps, 12345, nil) assert.Error(t, err) }) } diff --git a/pkg/certificate/cert_manager/provider.go b/pkg/certificate/cert_manager/provider.go index 84bf2c27..635890db 100644 --- a/pkg/certificate/cert_manager/provider.go +++ b/pkg/certificate/cert_manager/provider.go @@ -27,8 +27,9 @@ import ( ) const ( - IssuerNameKey = "issuerName" - IssuerKindKey = "issuerKind" + IssuerNameKey = "issuerName" + IssuerKindKey = "issuerKind" + IssuerGroupKey = "issuerGroup" ) type CertManagerProvider struct { @@ -234,8 +235,9 @@ func (cm *CertManagerProvider) GetCertificateConfig(ctx context.Context, }, ValidityDuration: cmCertificate.Spec.Duration.Duration, ExtraConfig: map[string]any{ - IssuerNameKey: cmCertificate.Spec.IssuerRef.Name, - IssuerKindKey: cmCertificate.Spec.IssuerRef.Kind, + IssuerNameKey: cmCertificate.Spec.IssuerRef.Name, + IssuerKindKey: cmCertificate.Spec.IssuerRef.Kind, + IssuerGroupKey: cmCertificate.Spec.IssuerRef.Group, }, } @@ -320,6 +322,10 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey if !isValid { return fmt.Errorf("value for %s not correctly provided, try again", IssuerKindKey) } + // issuerGroup is optional: empty (or absent) leaves IssuerRef.Group == "" so + // cert-manager defaults it to "cert-manager.io". Unlike issuerName/issuerKind, + // absence is legal here, so use comma-ok and do NOT error on a missing key. + issuerGroup, _ := cfg.ExtraConfig[IssuerGroupKey].(string) certificateResource := &certmanagerv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -335,8 +341,9 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey DNSNames: cfg.AltNames.DNSNames, IPAddresses: strings.Fields(strings.Trim(fmt.Sprint(cfg.AltNames.IPs), "[]")), IssuerRef: cmmeta.IssuerReference{ - Name: issuerName, - Kind: issuerKind, + Name: issuerName, + Kind: issuerKind, + Group: issuerGroup, }, Duration: &metav1.Duration{Duration: cfg.ValidityDuration}, }, diff --git a/pkg/certificate/interfaces/interface.go b/pkg/certificate/interfaces/interface.go index 90d06bfb..526cc28f 100644 --- a/pkg/certificate/interfaces/interface.go +++ b/pkg/certificate/interfaces/interface.go @@ -71,7 +71,6 @@ type Config struct { Organization []string AltNames AltNames ValidityDuration time.Duration - CABundleSecret string // ExtraConfig contains provider specific configurations. ExtraConfig map[string]any diff --git a/test/e2e/auto_provider_test.go b/test/e2e/auto_provider_test.go index a7ce8a4a..006bcaf8 100644 --- a/test/e2e/auto_provider_test.go +++ b/test/e2e/auto_provider_test.go @@ -137,16 +137,9 @@ func TestClusterAutoCertCreation(t *testing.T) { Spec: ecv1alpha1.EtcdClusterSpec{ Size: size, Version: etcdVersion, - TLS: &ecv1alpha1.TLSCertificate{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: &ecv1alpha1.ProviderAutoConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "etcd-operator-system", - ValidityDuration: "8760h", - }, - }, - }, + TLS: &ecv1alpha1.EtcdClusterTLS{ + Peer: autoSurface(), + Client: autoSurface(), }, }, } diff --git a/test/e2e/cert_manager_test.go b/test/e2e/cert_manager_test.go index 558f0238..df6d4e92 100644 --- a/test/e2e/cert_manager_test.go +++ b/test/e2e/cert_manager_test.go @@ -175,18 +175,9 @@ func TestClusterCertCreation(t *testing.T) { Spec: ecv1alpha1.EtcdClusterSpec{ Size: size, Version: etcdVersion, - TLS: &ecv1alpha1.TLSCertificate{ - Provider: "cert-manager", - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: "etcd-operator-system", - ValidityDuration: "90h", - }, - IssuerKind: cmIssuerType, - IssuerName: cmIssuerName, - }, - }, + TLS: &ecv1alpha1.EtcdClusterTLS{ + Peer: certManagerSurface(cmIssuerType, cmIssuerName), + Client: certManagerSurface(cmIssuerType, cmIssuerName), }, }, } diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index 663155ee..e5b115a7 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -338,3 +338,38 @@ func httpViaProxy(ctx context.Context, r *rest.Request, pod corev1.Pod, failpoin Do(ctx) return result.Error() } + +// certManagerSurface returns a cert-manager TLSSurface for one TLS surface (peer +// or client). The peer/client split is configured independently in the new CRD +// shape; the e2e helpers set both surfaces to the same issuer to mirror the +// pre-split single-toggle behaviour. +func certManagerSurface(issuerKind, issuerName string) *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "cert-manager", + ProviderCfg: ecv1alpha1.ProviderConfig{ + CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "etcd-operator-system", + ValidityDuration: "90h", + }, + IssuerKind: issuerKind, + IssuerName: issuerName, + }, + }, + } +} + +// autoSurface returns an auto-provider TLSSurface for one TLS surface. +func autoSurface() *ecv1alpha1.TLSSurface { + return &ecv1alpha1.TLSSurface{ + Provider: "auto", + ProviderCfg: ecv1alpha1.ProviderConfig{ + AutoCfg: &ecv1alpha1.ProviderAutoConfig{ + CommonConfig: ecv1alpha1.CommonConfig{ + CommonName: "etcd-operator-system", + ValidityDuration: "8760h", + }, + }, + }, + } +} From ddb14577c491ac199579b31b550deb13730773e3 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 23:52:33 -0400 Subject: [PATCH 2/8] fix(test): expect issuerGroup in cert-manager GetCertificateConfig GetCertificateConfig now echoes IssuerRef.Group into ExtraConfig under the issuerGroup key. The TestCertManagerProvider "Get certificate config" assertion built its expected config without that key, so reflect.DeepEqual failed. Add the empty-string issuerGroup to the expected config (no group set => cert-manager leaves Group == ""). Co-Authored-By: Claude Opus 4.8 Signed-off-by: Xavier Lange --- test/e2e/cert_manager_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/e2e/cert_manager_test.go b/test/e2e/cert_manager_test.go index df6d4e92..2962b793 100644 --- a/test/e2e/cert_manager_test.go +++ b/test/e2e/cert_manager_test.go @@ -58,6 +58,10 @@ func TestCertManagerProvider(t *testing.T) { ExtraConfig: map[string]any{ "issuerName": cmIssuerName, "issuerKind": cmIssuerType, + // issuerGroup is optional; with no group set, cert-manager leaves + // IssuerRef.Group == "" and GetCertificateConfig echoes that back, so + // the expected ExtraConfig must include the empty key to DeepEqual-match. + "issuerGroup": "", }, } From 723e91e1288cf4f5322972224a039f6be265979c Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 17 Jun 2026 23:48:26 -0400 Subject: [PATCH 3/8] fix(controller): publish not-ready addresses on the headless service A peer-TLS cluster could not form past the seed member. With --peer-client-cert-auth, etcd verifies a joining peer's source IP against its cert SANs via isHostInDNS (client/pkg transport), which forward-resolves the cluster's headless-service DNS name. A joining member is not Ready until it has actually joined the cluster, so without publishNotReadyAddresses its pod IP is absent from the service's A record, the peer cert SAN check fails ("tls: does not match any of DNSNames"), the peer handshake is rejected with EOF, and the joining member crashloops -- deadlocking the cluster at one member. Set PublishNotReadyAddresses=true on the headless service so not-yet-Ready members are resolvable and peer mTLS can complete. Cleartext peer traffic does not hit this verification path, so non-TLS clusters are unaffected. Discovered by the T6 multi-member TLS e2e (first live exercise of multi-member peer mTLS); verified on kind: 3-member both-surface TLS cluster reaches 3 voting members with this change and crashloops at 1 without it. Belongs to: pr/tls-independence Co-Authored-By: Claude Opus 4.8 Signed-off-by: Xavier Lange --- internal/controller/utils.go | 13 +++++++++++++ internal/controller/utils_test.go | 4 ++++ 2 files changed, 17 insertions(+) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 0ed33dca..c5f3628b 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -590,6 +590,19 @@ func createHeadlessServiceIfNotExist(ctx context.Context, logger logr.Logger, c Spec: corev1.ServiceSpec{ ClusterIP: "None", // Key for headless service Selector: labels, + // PublishNotReadyAddresses makes the headless service publish A + // records for pods that are not yet Ready. This is REQUIRED for a + // peer-TLS cluster to form: with --peer-client-cert-auth, etcd + // verifies a joining peer's source IP against its cert SANs via + // isHostInDNS (client/pkg transport), which forward-resolves the + // cluster's DNS name. A joining member is not Ready until it has + // joined, so without this its IP is absent from the service's A + // record, the peer cert SAN check fails ("does not match any of + // DNSNames"), the peer handshake is rejected, and the member + // crashloops -- deadlocking the cluster at one member. (Cleartext + // peer traffic does not hit this path, so the pre-TLS behaviour is + // unchanged for non-TLS clusters.) + PublishNotReadyAddresses: true, }, } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 0f27b7a3..08e03890 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -170,6 +170,10 @@ func TestCreateHeadlessServiceIfNotExist(t *testing.T) { err = fakeClient.Get(ctx, client.ObjectKey{Name: "test-etcd", Namespace: "default"}, service) assert.NoError(t, err) assert.Equal(t, "None", service.Spec.ClusterIP) + // Required for peer-TLS cluster formation: a joining (not-yet-Ready) member + // must be resolvable so etcd's peer cert SAN check (isHostInDNS) accepts it. + assert.True(t, service.Spec.PublishNotReadyAddresses, + "headless service must publish not-ready addresses so peer-TLS members can join") assert.Equal(t, map[string]string{ "app": "test-etcd", "controller": "test-etcd", From 970c6689b3fde28ef458cef4996f41d2b558ceb8 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Thu, 18 Jun 2026 01:26:46 -0400 Subject: [PATCH 4/8] docs(tls): note create-not-flip migration and operator ServerName behaviour Fill the two genuine documentation gaps the reshape left: - spec.tls is effectively create-time; toggling it on a running cluster drops quorum. Document the create-new-not-flip caveat on the EtcdClusterSpec.TLS field where a spec author hits it (regenerated CRD). - buildClientTLSConfig sets no ServerName, so verification relies on the dialed pod FQDN matching the server cert SANs; custom AltNames must keep covering *.{name}.{ns}.svc.cluster.local. Comments only, no behaviour change. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 5 +++++ config/crd/bases/operator.etcd.io_etcdclusters.yaml | 5 +++++ internal/controller/utils.go | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index 7667e44d..b2d66659 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -48,6 +48,11 @@ type EtcdClusterSpec struct { // means that surface is served/dialed in cleartext. When TLS itself is nil, the // entire cluster (peer + client + operator client) is cleartext, byte-identical // to a TLS-free deployment. + // + // TLS is effectively create-time: it flows into the pod template and cert mounts. + // Toggling it on (or off) on a running cluster rolls the StatefulSet into a mixed + // http/https membership whose peers cannot connect, dropping quorum. The supported + // path is a NEW TLS cluster plus data migration, not an in-place flip. TLS *EtcdClusterTLS `json:"tls,omitempty"` // etcd configuration options are passed as command line arguments to the etcd container, refer to etcd documentation for configuration options applicable for the version of etcd being used. EtcdOptions []string `json:"etcdOptions,omitempty"` diff --git a/config/crd/bases/operator.etcd.io_etcdclusters.yaml b/config/crd/bases/operator.etcd.io_etcdclusters.yaml index 3843e523..59af77cf 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -1075,6 +1075,11 @@ spec: means that surface is served/dialed in cleartext. When TLS itself is nil, the entire cluster (peer + client + operator client) is cleartext, byte-identical to a TLS-free deployment. + + TLS is effectively create-time: it flows into the pod template and cert mounts. + Toggling it on (or off) on a running cluster rolls the StatefulSet into a mixed + http/https membership whose peers cannot connect, dropping quorum. The supported + path is a NEW TLS cluster plus data migration, not an in-place flip. properties: client: description: |- diff --git a/internal/controller/utils.go b/internal/controller/utils.go index c5f3628b..9de9a638 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -1029,6 +1029,11 @@ func applyEtcdMemberCerts(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c cli // a missing ca.crt is an explicit error (rather than silently falling through to // the system trust store, which would verify against the wrong roots and fail // opaquely later). +// +// No ServerName is set, so Go verifies the dialed pod FQDN against the server +// cert's SANs. The default SANs cover the pod FQDNs; if a user supplies custom +// AltNames they MUST still include *.{name}.{ns}.svc.cluster.local or every +// operator health check fails verification. func buildClientTLSConfig(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) (*tls.Config, error) { if !clientTLSEnabled(ec) { return nil, nil From f0e3564987ff1832c2331dc94ab9f6732c304b8b Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Sat, 11 Jul 2026 02:48:13 -0400 Subject: [PATCH 5/8] fix(lint): appease golangci-lint v2.12 findings on the TLS branch errcheck on a test listener Close, prealloc on validateTLS's error list, and an unparam on a test helper whose name argument was always "ec". Signed-off-by: Xavier Lange --- internal/controller/tls_independence_test.go | 18 +++++++++--------- internal/controller/utils.go | 4 ++-- internal/etcdutils/etcdutils_test.go | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/controller/tls_independence_test.go b/internal/controller/tls_independence_test.go index f4a95531..cdfac606 100644 --- a/internal/controller/tls_independence_test.go +++ b/internal/controller/tls_independence_test.go @@ -82,9 +82,9 @@ func cmSurface(clientCertAuth *bool) *ecv1alpha1.TLSSurface { } } -func clusterWithTLS(name string, tls *ecv1alpha1.EtcdClusterTLS) *ecv1alpha1.EtcdCluster { +func clusterWithTLS(tls *ecv1alpha1.EtcdClusterTLS) *ecv1alpha1.EtcdCluster { return &ecv1alpha1.EtcdCluster{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, + ObjectMeta: metav1.ObjectMeta{Name: "ec", Namespace: "ns"}, Spec: ecv1alpha1.EtcdClusterSpec{Size: 3, Version: "v3.6.12", TLS: tls}, } } @@ -198,7 +198,7 @@ func TestTLSIndependenceArgsMatrix(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ec := clusterWithTLS(name, tt.tls) + ec := clusterWithTLS(tt.tls) got := createArgs(name, nil, tlsArgsFor(ec)) assert.Equal(t, tt.expected, got) }) @@ -220,7 +220,7 @@ func TestTLSIndependenceSchemes(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ec := clusterWithTLS("ec", tt.tls) + ec := clusterWithTLS(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. @@ -275,7 +275,7 @@ func TestTLSIndependenceMounts(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ec := clusterWithTLS("ec", tt.tls) + ec := clusterWithTLS(tt.tls) container, vols := stsContainer(t, ec) vNames := volumeNames(vols) mNames := mountNames(container.VolumeMounts) @@ -291,7 +291,7 @@ func TestTLSIndependenceMounts(t *testing.T) { // 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 := clusterWithTLS(&ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}) ec.Spec.StorageSpec = &ecv1alpha1.StorageSpec{ VolumeSizeRequest: mustQuantity("1Gi"), } @@ -312,7 +312,7 @@ func TestBuildClientTLSConfigGatedOnClientSurface(t *testing.T) { nil, {Peer: cmSurface(nil)}, } { - ec := clusterWithTLS("ec", tls) + ec := clusterWithTLS(tls) c := fake.NewClientBuilder().WithScheme(scheme).Build() cfg, err := buildClientTLSConfig(t.Context(), ec, c) require.NoError(t, err) @@ -320,7 +320,7 @@ func TestBuildClientTLSConfigGatedOnClientSurface(t *testing.T) { } // client surface set but secret missing => explicit error (not silent nil). - ec := clusterWithTLS("ec", &ecv1alpha1.EtcdClusterTLS{Client: cmSurface(nil)}) + ec := clusterWithTLS(&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") @@ -427,7 +427,7 @@ func TestValidateTLS(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errs := validateTLS(clusterWithTLS("ec", tt.tls)) + errs := validateTLS(clusterWithTLS(tt.tls)) if tt.wantErr { assert.NotEmpty(t, errs, "expected a validation error") } else { diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 9de9a638..f2714869 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -125,10 +125,10 @@ func clientCertAuthEnabled(s *ecv1alpha1.TLSSurface) bool { // 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 + return nil } + errs := make(field.ErrorList, 0, 2) 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)...) diff --git a/internal/etcdutils/etcdutils_test.go b/internal/etcdutils/etcdutils_test.go index 1bb0eb9f..c45f9984 100644 --- a/internal/etcdutils/etcdutils_test.go +++ b/internal/etcdutils/etcdutils_test.go @@ -88,7 +88,7 @@ func TestMemberList(t *testing.T) { 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() + defer func() { _ = ln.Close() }() handshakeSeen := make(chan struct{}, 1) go func() { From 6cfd5693e60c42b32d3ca2da26b9a41a2ecaa6f0 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Sat, 11 Jul 2026 02:48:53 -0400 Subject: [PATCH 6/8] refactor(certificate)!: typed Config with issuerRef; drop ExtraConfig and AltNames interfaces.Config now carries the fields providers actually consume: IssuerRef (cert-manager's own cmmeta.IssuerReference), plain-string IPAddresses, Duration, and RenewBefore. The stringly-typed ExtraConfig map and the AltNames wrapper are gone, along with the comma-ok assertions that turned key typos into runtime errors. Behavior fixes that fall out of the typed shape: - user-supplied IP SANs now reach both providers (the cert-manager path serialized them through fmt.Sprint; the auto path dropped them) - GetCertificateConfig echoes real IPAddresses instead of a zeroed placeholder slice - an empty issuerRef.kind resolves to cert-manager's documented default, Issuer, instead of failing the issuer-existence check - renewBefore is passed through to Certificate.spec.renewBefore The controller's spec->Config mappers translate the existing CRD shape unchanged; the CRD itself is untouched here. Signed-off-by: Xavier Lange --- internal/controller/utils.go | 88 +++++++-------- internal/controller/utils_test.go | 59 ++++------ pkg/certificate/auto/provider.go | 41 ++++--- pkg/certificate/auto/provider_test.go | 94 ++++++++++++++++ pkg/certificate/cert_manager/provider.go | 93 ++++++---------- pkg/certificate/cert_manager/provider_test.go | 102 ++++++++++++++++++ pkg/certificate/interfaces/interface.go | 32 +++--- test/e2e/auto_provider_test.go | 4 +- test/e2e/cert_manager_test.go | 17 ++- 9 files changed, 343 insertions(+), 187 deletions(-) create mode 100644 pkg/certificate/auto/provider_test.go create mode 100644 pkg/certificate/cert_manager/provider_test.go diff --git a/internal/controller/utils.go b/internal/controller/utils.go index f2714869..34fc40f7 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -13,6 +13,7 @@ import ( "strings" "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/coreos/go-semver/semver" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" @@ -814,6 +815,31 @@ func parseValidityDuration(customizedDuration string, defaultDuration time.Durat return duration, nil } +// defaultCertDNSNames returns the user-provided DNS SANs, or wildcard DNS for +// the cluster's headless service to cover all pods (pod-0, pod-1, etc.). +func defaultCertDNSNames(ec *ecv1alpha1.EtcdCluster, dnsNames []string) []string { + if dnsNames != nil { + return dnsNames + } + return []string{ + fmt.Sprintf("*.%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), + fmt.Sprintf("%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), + } +} + +// certIPStrings renders the CRD's parsed IP SANs to the literal-string form +// used by certInterface.Config, preserving nil when none are set. +func certIPStrings(ips []net.IP) []string { + if len(ips) == 0 { + return nil + } + out := make([]string, 0, len(ips)) + for _, ip := range ips { + out = append(out, ip.String()) + } + return out +} + func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { cmConfig := surface.ProviderCfg.CertManagerCfg if cmConfig == nil { @@ -826,33 +852,16 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.T return nil, err } - var getAltNames certInterface.AltNames - if cmConfig.AltNames.DNSNames != nil { - getAltNames = certInterface.AltNames{ - DNSNames: cmConfig.AltNames.DNSNames, - IPs: make([]net.IP, len(cmConfig.AltNames.DNSNames)), - } - } else { - // Use wildcard DNS for the cluster's headless service to cover all pods - // This allows the certificate to work for pod-0, pod-1, etc. - defaultDNSNames := []string{ - fmt.Sprintf("*.%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), - fmt.Sprintf("%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), - } - getAltNames = certInterface.AltNames{ - DNSNames: defaultDNSNames, - } - } - config := &certInterface.Config{ - CommonName: cmConfig.CommonName, - Organization: cmConfig.Organization, - ValidityDuration: duration, - AltNames: getAltNames, - ExtraConfig: map[string]any{ - "issuerName": cmConfig.IssuerName, - "issuerKind": cmConfig.IssuerKind, - "issuerGroup": cmConfig.IssuerGroup, + CommonName: cmConfig.CommonName, + Organizations: cmConfig.Organization, + DNSNames: defaultCertDNSNames(ec, cmConfig.AltNames.DNSNames), + IPAddresses: certIPStrings(cmConfig.AltNames.IPs), + Duration: duration, + IssuerRef: &cmmeta.IssuerReference{ + Name: cmConfig.IssuerName, + Kind: cmConfig.IssuerKind, + Group: cmConfig.IssuerGroup, }, } return config, nil @@ -876,29 +885,12 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1 return nil, err } - var altNames certInterface.AltNames - if autoConfig.AltNames.DNSNames != nil { - altNames = certInterface.AltNames{ - DNSNames: autoConfig.AltNames.DNSNames, - IPs: make([]net.IP, len(autoConfig.AltNames.DNSNames)), - } - } else { - // Use wildcard DNS for the cluster's headless service to cover all pods - // This allows the certificate to work for pod-0, pod-1, etc. - defaultDNSNames := []string{ - fmt.Sprintf("*.%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), - fmt.Sprintf("%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), - } - altNames = certInterface.AltNames{ - DNSNames: defaultDNSNames, - } - } - config := &certInterface.Config{ - CommonName: autoConfig.CommonName, - Organization: autoConfig.Organization, - ValidityDuration: duration, - AltNames: altNames, + CommonName: autoConfig.CommonName, + Organizations: autoConfig.Organization, + DNSNames: defaultCertDNSNames(ec, autoConfig.AltNames.DNSNames), + IPAddresses: certIPStrings(autoConfig.AltNames.IPs), + Duration: duration, } return config, nil } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 08e03890..2f4d4fdd 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -3,10 +3,10 @@ package controller import ( "errors" "fmt" - "net" "testing" "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/coreos/go-semver/semver" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" @@ -948,13 +948,10 @@ func TestCreateAutoCertificateConfig(t *testing.T) { }, }, expected: &certInterface.Config{ - CommonName: "custom.example.com", - Organization: []string{"Test Org"}, - ValidityDuration: 720 * time.Hour, // 30 days - AltNames: certInterface.AltNames{ - DNSNames: []string{"custom1.example.com", "custom2.example.com"}, - IPs: make([]net.IP, 2), - }, + CommonName: "custom.example.com", + Organizations: []string{"Test Org"}, + Duration: 720 * time.Hour, // 30 days + DNSNames: []string{"custom1.example.com", "custom2.example.com"}, }, wantErr: false, }, @@ -973,14 +970,12 @@ func TestCreateAutoCertificateConfig(t *testing.T) { }, }, expected: &certInterface.Config{ - CommonName: "test-cluster.test-namespace.svc.cluster.local", - Organization: nil, - ValidityDuration: certInterface.DefaultAutoValidity, - AltNames: certInterface.AltNames{ - DNSNames: []string{ - "*.test-cluster.test-namespace.svc.cluster.local", - "test-cluster.test-namespace.svc.cluster.local", - }, + CommonName: "test-cluster.test-namespace.svc.cluster.local", + Organizations: nil, + Duration: certInterface.DefaultAutoValidity, + DNSNames: []string{ + "*.test-cluster.test-namespace.svc.cluster.local", + "test-cluster.test-namespace.svc.cluster.local", }, }, wantErr: false, @@ -997,11 +992,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, result) - assert.Equal(t, tt.expected.CommonName, result.CommonName) - assert.Equal(t, tt.expected.Organization, result.Organization) - assert.Equal(t, tt.expected.ValidityDuration, result.ValidityDuration) - assert.Equal(t, tt.expected.AltNames.DNSNames, result.AltNames.DNSNames) - assert.Equal(t, tt.expected.AltNames.IPs, result.AltNames.IPs) + assert.Equal(t, tt.expected, result) } }) } @@ -1042,17 +1033,14 @@ func TestCreateCMCertificateConfig(t *testing.T) { }, }, expected: &certInterface.Config{ - CommonName: "cm.example.com", - Organization: []string{"CM Org"}, - ValidityDuration: 1440 * time.Hour, // 60 days - AltNames: certInterface.AltNames{ - DNSNames: []string{"cm1.example.com", "cm2.example.com"}, - IPs: make([]net.IP, 2), - }, - ExtraConfig: map[string]any{ - "issuerName": "test-issuer", - "issuerKind": "ClusterIssuer", - "issuerGroup": "example.io", + CommonName: "cm.example.com", + Organizations: []string{"CM Org"}, + Duration: 1440 * time.Hour, // 60 days + DNSNames: []string{"cm1.example.com", "cm2.example.com"}, + IssuerRef: &cmmeta.IssuerReference{ + Name: "test-issuer", + Kind: "ClusterIssuer", + Group: "example.io", }, }, wantErr: false, @@ -1086,12 +1074,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { } else { require.NoError(t, err) require.NotNil(t, result) - assert.Equal(t, tt.expected.CommonName, result.CommonName) - assert.Equal(t, tt.expected.Organization, result.Organization) - assert.Equal(t, tt.expected.ValidityDuration, result.ValidityDuration) - assert.Equal(t, tt.expected.AltNames.DNSNames, result.AltNames.DNSNames) - assert.Equal(t, tt.expected.AltNames.IPs, result.AltNames.IPs) - assert.Equal(t, tt.expected.ExtraConfig, result.ExtraConfig) + assert.Equal(t, tt.expected, result) } }) } diff --git a/pkg/certificate/auto/provider.go b/pkg/certificate/auto/provider.go index cf7c7526..1fc0f8d0 100644 --- a/pkg/certificate/auto/provider.go +++ b/pkg/certificate/auto/provider.go @@ -167,17 +167,28 @@ func (ac *Provider) GetCertificateConfig(ctx context.Context, // Extract config from the certificate config := &interfaces.Config{ - CommonName: cert.Subject.CommonName, - Organization: cert.Subject.Organization, - AltNames: interfaces.AltNames{ - DNSNames: cert.DNSNames, - IPs: cert.IPAddresses, - }, + CommonName: cert.Subject.CommonName, + Organizations: cert.Subject.Organization, + DNSNames: cert.DNSNames, + IPAddresses: ipStrings(cert.IPAddresses), } return config, nil } +// ipStrings renders parsed IP SANs back to the literal-string form used by +// interfaces.Config, preserving nil for certificates without IP SANs. +func ipStrings(ips []net.IP) []string { + if len(ips) == 0 { + return nil + } + out := make([]string, 0, len(ips)) + for _, ip := range ips { + out = append(out, ip.String()) + } + return out +} + // parseCertificateFromSecret extracts and parses the x509 certificate from a Kubernetes secret. func parseCertificateFromSecret(secret *corev1.Secret) (*x509.Certificate, error) { certData, ok := secret.Data[corev1.TLSCertKey] @@ -248,8 +259,8 @@ func checkKeyPair(cert *x509.Certificate, privateKey crypto.PrivateKey) error { func (ac *Provider) createNewSecret(ctx context.Context, secretKey client.ObjectKey, cfg *interfaces.Config) error { validity := interfaces.DefaultAutoValidity - if cfg.ValidityDuration != 0 { - validity = cfg.ValidityDuration + if cfg.Duration != 0 { + validity = cfg.Duration } // Validate validity duration: minimum is 1 year as required by etcd @@ -276,11 +287,15 @@ func (ac *Provider) createNewSecret(ctx context.Context, secretKey client.Object hostPort := net.JoinHostPort(cfg.CommonName, "5500") hosts = append(hosts, hostPort) } - if len(cfg.AltNames.DNSNames) > 0 { - for _, dnsName := range cfg.AltNames.DNSNames { - hostPort := net.JoinHostPort(dnsName, "5500") - hosts = append(hosts, hostPort) - } + for _, dnsName := range cfg.DNSNames { + hostPort := net.JoinHostPort(dnsName, "5500") + hosts = append(hosts, hostPort) + } + // transport.SelfCert splits each host and sorts it into an IP or DNS + // SAN via net.ParseIP, so IP SANs ride the same hosts list. + for _, ip := range cfg.IPAddresses { + hostPort := net.JoinHostPort(ip, "5500") + hosts = append(hosts, hostPort) } } diff --git a/pkg/certificate/auto/provider_test.go b/pkg/certificate/auto/provider_test.go new file mode 100644 index 00000000..6f83a4a0 --- /dev/null +++ b/pkg/certificate/auto/provider_test.go @@ -0,0 +1,94 @@ +package auto + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + interfaces "go.etcd.io/etcd-operator/pkg/certificate/interfaces" +) + +// TestEnsureCertificateSecretIPSANs verifies that user-supplied IP SANs land in +// the generated self-signed certificate alongside DNS SANs. +func TestEnsureCertificateSecretIPSANs(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + provider := New(cl) + + secretKey := client.ObjectKey{Name: "ip-san-cert", Namespace: "default"} + cfg := &interfaces.Config{ + CommonName: "etcd.default.svc.cluster.local", + DNSNames: []string{"member.etcd.default.svc.cluster.local"}, + IPAddresses: []string{"10.0.0.5", "192.168.1.9"}, + Duration: interfaces.DefaultAutoValidity, + } + + if err := provider.EnsureCertificateSecret(t.Context(), secretKey, cfg); err != nil { + t.Fatalf("EnsureCertificateSecret failed: %v", err) + } + + var secret corev1.Secret + if err := cl.Get(t.Context(), secretKey, &secret); err != nil { + t.Fatalf("generated secret not found: %v", err) + } + cert, err := parseCertificateFromSecret(&secret) + if err != nil { + t.Fatalf("failed to parse generated certificate: %v", err) + } + + gotIPs := map[string]bool{} + for _, ip := range cert.IPAddresses { + gotIPs[ip.String()] = true + } + for _, want := range cfg.IPAddresses { + if !gotIPs[want] { + t.Errorf("IP SAN %s missing from generated certificate, got %v", want, cert.IPAddresses) + } + } + + gotDNS := map[string]bool{} + for _, name := range cert.DNSNames { + gotDNS[name] = true + } + if !gotDNS["member.etcd.default.svc.cluster.local"] { + t.Errorf("DNS SAN missing from generated certificate, got %v", cert.DNSNames) + } +} + +// TestGetCertificateConfigRoundTrip verifies that a config used for creation is +// echoed back by GetCertificateConfig, including IP SANs as literal strings. +func TestGetCertificateConfigRoundTrip(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + provider := New(cl) + + secretKey := client.ObjectKey{Name: "roundtrip-cert", Namespace: "default"} + cfg := &interfaces.Config{ + CommonName: "etcd.default.svc.cluster.local", + IPAddresses: []string{"10.0.0.5"}, + Duration: interfaces.DefaultAutoValidity, + } + if err := provider.EnsureCertificateSecret(t.Context(), secretKey, cfg); err != nil { + t.Fatalf("EnsureCertificateSecret failed: %v", err) + } + + got, err := provider.GetCertificateConfig(t.Context(), secretKey) + if err != nil { + t.Fatalf("GetCertificateConfig failed: %v", err) + } + if got.CommonName != cfg.CommonName { + t.Errorf("CommonName = %q, want %q", got.CommonName, cfg.CommonName) + } + if len(got.IPAddresses) != 1 || got.IPAddresses[0] != "10.0.0.5" { + t.Errorf("IPAddresses = %v, want [10.0.0.5]", got.IPAddresses) + } +} diff --git a/pkg/certificate/cert_manager/provider.go b/pkg/certificate/cert_manager/provider.go index 635890db..add5d44b 100644 --- a/pkg/certificate/cert_manager/provider.go +++ b/pkg/certificate/cert_manager/provider.go @@ -12,12 +12,9 @@ import ( "errors" "fmt" "log" - "net" - "strings" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,12 +23,6 @@ import ( interfaces "go.etcd.io/etcd-operator/pkg/certificate/interfaces" ) -const ( - IssuerNameKey = "issuerName" - IssuerKindKey = "issuerKind" - IssuerGroupKey = "issuerGroup" -) - type CertManagerProvider struct { client.Client } @@ -219,26 +210,18 @@ func (cm *CertManagerProvider) GetCertificateConfig(ctx context.Context, return nil, fmt.Errorf("failed to get certificate: %w", err) } - var ipAddresses []net.IP - if len(cmCertificate.Spec.IPAddresses) != 0 { - ipAddresses = make([]net.IP, len(cmCertificate.Spec.IPAddresses)) - } else { - ipAddresses = nil - } - cfg := &interfaces.Config{ - CommonName: cmCertificate.Spec.CommonName, - Organization: cmCertificate.Spec.Subject.Organizations, - AltNames: interfaces.AltNames{ - DNSNames: cmCertificate.Spec.DNSNames, - IPs: ipAddresses, - }, - ValidityDuration: cmCertificate.Spec.Duration.Duration, - ExtraConfig: map[string]any{ - IssuerNameKey: cmCertificate.Spec.IssuerRef.Name, - IssuerKindKey: cmCertificate.Spec.IssuerRef.Kind, - IssuerGroupKey: cmCertificate.Spec.IssuerRef.Group, - }, + CommonName: cmCertificate.Spec.CommonName, + DNSNames: cmCertificate.Spec.DNSNames, + IPAddresses: cmCertificate.Spec.IPAddresses, + RenewBefore: cmCertificate.Spec.RenewBefore, + IssuerRef: &cmCertificate.Spec.IssuerRef, + } + if cmCertificate.Spec.Subject != nil { + cfg.Organizations = cmCertificate.Spec.Subject.Organizations + } + if cmCertificate.Spec.Duration != nil { + cfg.Duration = cmCertificate.Spec.Duration.Duration } return cfg, nil @@ -269,9 +252,18 @@ func (cm *CertManagerProvider) checkCertificateStatus(certificateName, namespace return nil } +// effectiveIssuerKind resolves an empty issuerRef.kind to cert-manager's +// documented default, "Issuer". +func effectiveIssuerKind(kind string) string { + if kind == "" { + return "Issuer" + } + return kind +} + // checkIssuerExists checks for if the provided issuer is present in the namespace/cluster func (cm *CertManagerProvider) checkIssuerExists(issuerName, issuerKind, namespace string, ctx context.Context) error { - switch issuerKind { + switch effectiveIssuerKind(issuerKind) { case "Issuer": issuer := &certmanagerv1.Issuer{} err := cm.Get(ctx, client.ObjectKey{Name: issuerName, Namespace: namespace}, issuer) @@ -293,19 +285,10 @@ func (cm *CertManagerProvider) checkIssuerExists(issuerName, issuerKind, namespa // validateCertificateConfig checks if the config passed is valid func (cm *CertManagerProvider) validateCertificateConfig(ctx context.Context, namespace string, cfg *interfaces.Config) error { - issuerName, isValid := cfg.ExtraConfig[IssuerNameKey].(string) - if !isValid { - return fmt.Errorf("value for %s not correctly provided, try again", IssuerNameKey) - } - issuerKind, isValid := cfg.ExtraConfig[IssuerKindKey].(string) - if !isValid { - return fmt.Errorf("value for %s not correctly provided, try again", IssuerKindKey) + if cfg.IssuerRef == nil { + return errors.New("issuerRef not provided for cert-manager certificate") } - checkIssuerExist := cm.checkIssuerExists(issuerName, issuerKind, namespace, ctx) - if checkIssuerExist != nil { - return checkIssuerExist - } - return nil + return cm.checkIssuerExists(cfg.IssuerRef.Name, cfg.IssuerRef.Kind, namespace, ctx) } // createCertificate creates a cert-manager Certificate resource in the specified namespace. @@ -314,18 +297,9 @@ func (cm *CertManagerProvider) validateCertificateConfig(ctx context.Context, na // returns an error if the Certificate resource cannot be created. func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey client.ObjectKey, cfg *interfaces.Config) error { - issuerName, isValid := cfg.ExtraConfig[IssuerNameKey].(string) - if !isValid { - return fmt.Errorf("value for %s not correctly provided, try again", IssuerNameKey) + if cfg.IssuerRef == nil { + return errors.New("issuerRef not provided for cert-manager certificate") } - issuerKind, isValid := cfg.ExtraConfig[IssuerKindKey].(string) - 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,17 +309,14 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey Spec: certmanagerv1.CertificateSpec{ CommonName: cfg.CommonName, Subject: &certmanagerv1.X509Subject{ - Organizations: cfg.Organization, + Organizations: cfg.Organizations, }, SecretName: secretKey.Name, - DNSNames: cfg.AltNames.DNSNames, - IPAddresses: strings.Fields(strings.Trim(fmt.Sprint(cfg.AltNames.IPs), "[]")), - IssuerRef: cmmeta.IssuerReference{ - Name: issuerName, - Kind: issuerKind, - Group: issuerGroup, - }, - Duration: &metav1.Duration{Duration: cfg.ValidityDuration}, + DNSNames: cfg.DNSNames, + IPAddresses: cfg.IPAddresses, + IssuerRef: *cfg.IssuerRef, + Duration: &metav1.Duration{Duration: cfg.Duration}, + RenewBefore: cfg.RenewBefore, }, } diff --git a/pkg/certificate/cert_manager/provider_test.go b/pkg/certificate/cert_manager/provider_test.go new file mode 100644 index 00000000..31edda93 --- /dev/null +++ b/pkg/certificate/cert_manager/provider_test.go @@ -0,0 +1,102 @@ +package cert_manager + +import ( + "testing" + "time" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + interfaces "go.etcd.io/etcd-operator/pkg/certificate/interfaces" +) + +func newFakeProvider(t *testing.T, objs ...client.Object) (*CertManagerProvider, client.Client) { + t.Helper() + scheme := runtime.NewScheme() + if err := certmanagerv1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add cert-manager scheme: %v", err) + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + return &CertManagerProvider{cl}, cl +} + +// TestCreateCertificateSpec verifies the typed Config fields reach the +// generated cert-manager Certificate spec. +func TestCreateCertificateSpec(t *testing.T) { + provider, cl := newFakeProvider(t) + + renewBefore := &metav1.Duration{Duration: 360 * time.Hour} + secretKey := client.ObjectKey{Name: "typed-cert", Namespace: "default"} + cfg := &interfaces.Config{ + CommonName: "etcd.default.svc.cluster.local", + Organizations: []string{"etcd-operator"}, + DNSNames: []string{"*.etcd.default.svc.cluster.local"}, + IPAddresses: []string{"10.0.0.5"}, + Duration: 2160 * time.Hour, + RenewBefore: renewBefore, + IssuerRef: &cmmeta.IssuerReference{ + Name: "etcd-ca", + Kind: "ClusterIssuer", + }, + } + + if err := provider.createCertificate(t.Context(), secretKey, cfg); err != nil { + t.Fatalf("createCertificate failed: %v", err) + } + + var cert certmanagerv1.Certificate + if err := cl.Get(t.Context(), secretKey, &cert); err != nil { + t.Fatalf("Certificate not created: %v", err) + } + if cert.Spec.IssuerRef != *cfg.IssuerRef { + t.Errorf("IssuerRef = %+v, want %+v", cert.Spec.IssuerRef, *cfg.IssuerRef) + } + if cert.Spec.RenewBefore == nil || cert.Spec.RenewBefore.Duration != renewBefore.Duration { + t.Errorf("RenewBefore = %v, want %v", cert.Spec.RenewBefore, renewBefore) + } + if len(cert.Spec.IPAddresses) != 1 || cert.Spec.IPAddresses[0] != "10.0.0.5" { + t.Errorf("IPAddresses = %v, want [10.0.0.5]", cert.Spec.IPAddresses) + } + if cert.Spec.Duration == nil || cert.Spec.Duration.Duration != cfg.Duration { + t.Errorf("Duration = %v, want %v", cert.Spec.Duration, cfg.Duration) + } +} + +// TestCreateCertificateRequiresIssuerRef verifies a nil IssuerRef is rejected +// before any Certificate object is created. +func TestCreateCertificateRequiresIssuerRef(t *testing.T) { + provider, _ := newFakeProvider(t) + + secretKey := client.ObjectKey{Name: "no-issuer", Namespace: "default"} + err := provider.createCertificate(t.Context(), secretKey, &interfaces.Config{CommonName: "x"}) + if err == nil { + t.Fatal("createCertificate accepted a nil IssuerRef") + } +} + +// TestValidateCertificateConfigKindDefault verifies an empty issuerRef.kind is +// resolved to cert-manager's documented default, "Issuer". +func TestValidateCertificateConfigKindDefault(t *testing.T) { + issuer := &certmanagerv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-issuer", Namespace: "default"}, + } + provider, _ := newFakeProvider(t, issuer) + + cfg := &interfaces.Config{ + IssuerRef: &cmmeta.IssuerReference{Name: "ns-issuer"}, // Kind omitted + } + if err := provider.validateCertificateConfig(t.Context(), "default", cfg); err != nil { + t.Fatalf("empty issuerRef.kind should default to Issuer, got error: %v", err) + } + + missing := &interfaces.Config{ + IssuerRef: &cmmeta.IssuerReference{Name: "absent"}, + } + if err := provider.validateCertificateConfig(t.Context(), "default", missing); err == nil { + t.Fatal("validateCertificateConfig accepted a missing namespaced Issuer") + } +} diff --git a/pkg/certificate/interfaces/interface.go b/pkg/certificate/interfaces/interface.go index 526cc28f..f6494bd3 100644 --- a/pkg/certificate/interfaces/interface.go +++ b/pkg/certificate/interfaces/interface.go @@ -3,9 +3,10 @@ package certificate import ( "context" "errors" - "net" "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -57,23 +58,22 @@ const ( DefaultDomainName = "svc.cluster.local" ) -// 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. -type AltNames struct { - DNSNames []string - IPs []net.IP -} - // Config contains the basic fields required for creating a certificate type Config struct { - CommonName string - Organization []string - AltNames AltNames - ValidityDuration time.Duration - - // ExtraConfig contains provider specific configurations. - ExtraConfig map[string]any + CommonName string + Organizations []string + DNSNames []string + // IPAddresses are IP subject alternative names as literal IP strings. + IPAddresses []string + // Duration is the requested certificate lifetime, already resolved by the + // caller (providers do not apply their own default). + Duration time.Duration + // RenewBefore is passed through to providers that support renewal; + // nil means the provider default. + RenewBefore *metav1.Duration + // IssuerRef selects the cert-manager issuer signing the certificate. + // It is nil for providers that mint their own certificates. + IssuerRef *cmmeta.IssuerReference } type Provider interface { diff --git a/test/e2e/auto_provider_test.go b/test/e2e/auto_provider_test.go index 006bcaf8..c7f33e3b 100644 --- a/test/e2e/auto_provider_test.go +++ b/test/e2e/auto_provider_test.go @@ -35,8 +35,8 @@ func TestAutoProvider(t *testing.T) { feature := features.New("Auto Provider Certificate").WithLabel("app", string(certificate.Auto)) cmConfig := &interfaces.Config{ - CommonName: autoCertificateName, - ValidityDuration: autoCertificateValidity, + CommonName: autoCertificateName, + Duration: autoCertificateValidity, } feature.Setup( diff --git a/test/e2e/cert_manager_test.go b/test/e2e/cert_manager_test.go index 2962b793..12e8e6b6 100644 --- a/test/e2e/cert_manager_test.go +++ b/test/e2e/cert_manager_test.go @@ -9,6 +9,7 @@ import ( "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" @@ -53,15 +54,13 @@ func TestCertManagerProvider(t *testing.T) { feature := features.New("Cert-Manager Certificate").WithLabel("app", "cert-manager") cmConfig := &interfaces.Config{ - CommonName: cmCertificateName, - ValidityDuration: cmCertificateValidity, - 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": "", + CommonName: cmCertificateName, + Duration: cmCertificateValidity, + // Group is optional; cert-manager stores IssuerRef.Group == "" and + // GetCertificateConfig echoes that back, so the zero value DeepEqual-matches. + IssuerRef: &cmmeta.IssuerReference{ + Name: cmIssuerName, + Kind: cmIssuerType, }, } From e8a18b6e0f5591102f8f0249515eb0e7ad3df250 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Sat, 11 Jul 2026 03:04:43 -0400 Subject: [PATCH 7/8] feat(api)!: flatten TLSSurface into a provider-discriminated union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TLS surface config becomes a conventional discriminated union: spec.tls.{peer,client}.provider (enum auto | cert-manager.io, default auto) selects between sibling member blocks auto: and certManager:, replacing the providerCfg wrapper. Provider identifiers are domain-style (cf. StorageClass.provisioner), so cert-manager is addressed by its API group. The certManager block references cert-manager's own types: issuerRef is cmmeta.IssuerReference ({name, kind, group}), replacing the ad-hoc issuerKind/issuerName/issuerGroup strings. kind now defaults to Issuer per cert-manager convention; a CEL rule keeps the value space closed to Issuer|ClusterIssuer. Both member blocks carry the same curated cert-manager-style passthrough set: commonName, organizations, dnsNames, ipAddresses (literal strings), duration, renewBefore. validityDuration and its day-suffix parsing are gone: duration and renewBefore are metav1.Duration. Because the CRD renders these as bare strings, CEL duration() guards on all four fields reject unparseable values (e.g. "365d") at admission — without them the value would be stored and wedge the controller's typed decode. The guards double as floors: 8760h minimum for auto (etcd's SelfCert requirement), 1h for cert-manager. BREAKING CHANGE: existing tls stanzas must be rewritten; there is no conversion webhook (pre-1.0 clean break). Note issuerKind was required before — a port that drops it now gets a namespaced Issuer lookup. Samples updated; the autocert sample also gains the peer/client split it had missed, and a dangling empty tls: key is dropped from the minimal sample. Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 175 +++--- api/v1alpha1/zz_generated.deepcopy.go | 164 +++--- .../bases/operator.etcd.io_etcdclusters.yaml | 530 ++++++++++-------- .../operator_v1alpha1_etcdcluster.yaml | 1 - .../samples/sample_autocert_etcdcluster.yaml | 15 +- .../sample_certmanager_etcdcluster.yaml | 29 +- docs/api-references/docs.md | 174 +++--- internal/controller/tls_cel_test.go | 145 ++++- internal/controller/tls_independence_test.go | 58 +- internal/controller/utils.go | 125 ++--- internal/controller/utils_test.go | 96 ++-- pkg/certificate/certificate.go | 6 +- test/e2e/helpers_test.go | 29 +- 13 files changed, 889 insertions(+), 658 deletions(-) diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index b2d66659..9f307169 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -17,8 +17,7 @@ limitations under the License. package v1alpha1 import ( - "net" - + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -99,103 +98,153 @@ type EtcdClusterTLS struct { Client *TLSSurface `json:"client,omitempty"` } +// TLSProvider names the certificate provider for one TLS surface. It is the +// discriminator of the flattened union on TLSSurface. Values are domain-style +// identifiers (cf. StorageClass.provisioner): the operator's built-in +// self-signed provider is "auto"; cert-manager is addressed by its API group. +// +kubebuilder:validation:Enum=auto;cert-manager.io +type TLSProvider string + +const ( + // TLSProviderAuto selects the operator's built-in self-signed provider. + TLSProviderAuto TLSProvider = "auto" + // TLSProviderCertManager selects cert-manager issuance. + TLSProviderCertManager TLSProvider = "cert-manager.io" +) + // 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. +// client): a provider-discriminated union (provider selects which member block +// below is honored), the surface's mutual client-cert-auth policy, and an +// optional additional trust bundle. // -// 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. +// The XValidation rules below are the apply-time anti-misconfiguration +// guardrails: they reject incoherent provider/member-block 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 -- 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" +// +kubebuilder:validation:XValidation:rule="self.provider != 'cert-manager.io' || has(self.certManager)",message="provider 'cert-manager.io' requires the certManager block" +// +kubebuilder:validation:XValidation:rule="self.provider == 'cert-manager.io' || !has(self.certManager)",message="certManager may only be set when provider is 'cert-manager.io'" +// +kubebuilder:validation:XValidation:rule="self.provider == 'auto' || !has(self.auto)",message="auto may only be set when provider is 'auto'" +// +kubebuilder:validation:XValidation:rule="!self.clientCertAuth || self.provider != 'cert-manager.io' || (has(self.certManager) && size(self.certManager.issuerRef.name) > 0)",message="clientCertAuth requires a trusted CA: set certManager.issuerRef.name" type TLSSurface struct { - // Provider selects the certificate provider for THIS surface. - // Defaults to "auto" when empty. - // +kubebuilder:validation:Enum=auto;cert-manager + // Provider selects the certificate provider for THIS surface and names + // which member block below is honored. Defaults to "auto". + // +kubebuilder:default=auto + // +optional + Provider TLSProvider `json:"provider,omitempty"` + + // Auto configures the operator's built-in self-signed provider for THIS + // surface. Only valid when provider is "auto". // +optional - Provider string `json:"provider,omitempty"` + Auto *TLSAutoProvider `json:"auto,omitempty"` - // ProviderCfg is the provider-specific config for THIS surface. + // CertManager configures cert-manager issuance for THIS surface. + // Required when provider is "cert-manager.io"; forbidden otherwise. // +optional - ProviderCfg ProviderConfig `json:"providerCfg,omitempty"` + CertManager *TLSCertManagerProvider `json:"certManager,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. + // the cert-manager provider a trusted CA (issuerRef.name) is REQUIRED, + // enforced by the XValidation rule above. // +kubebuilder:default=true // +optional ClientCertAuth *bool `json:"clientCertAuth,omitempty"` } -type ProviderConfig struct { - AutoCfg *ProviderAutoConfig `json:"autoCfg,omitempty"` - CertManagerCfg *ProviderCertManagerConfig `json:"certManagerCfg,omitempty"` +// EffectiveProvider resolves the surface's provider, defaulting to "auto" when +// empty (objects written by an apiserver are already defaulted; this covers +// fake-client / envtest-less code paths). +func (s *TLSSurface) EffectiveProvider() TLSProvider { + if s == nil || s.Provider == "" { + return TLSProviderAuto + } + return s.Provider } -type AltNames struct { - // DNSNames is the expected array of DNS subject alternative names. - // if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local +// TLSAutoProvider tunes the built-in self-signed provider for one surface. +// All fields are optional; empty values derive defaults from the cluster. +type TLSAutoProvider struct { + // CommonName is the X509 subject CN. Keep to 64 characters or fewer to + // avoid generating invalid CSRs. + // +kubebuilder:validation:MaxLength=64 // +optional - DNSNames []string `json:"dnsNames,omitempty"` + CommonName string `json:"commonName,omitempty"` - // IPs is the expected array of IP address subject alternative names. + // Organizations are the X509 subject O values. // +optional - IPs []net.IP `json:"ipAddresses,omitempty"` -} + Organizations []string `json:"organizations,omitempty"` -type CommonConfig struct { - // CommonName is the expected common name X509 certificate subject attribute. - // Should have a length of 64 characters or fewer to avoid generating invalid CSRs. + // DNSNames are the DNS subject alternative names. Empty defaults to + // *...svc.cluster.local and ..svc.cluster.local + // (required for the operator's hostname verification of members). // +optional - CommonName string `json:"commonName,omitempty"` + DNSNames []string `json:"dnsNames,omitempty"` - // Organization is the expected array of Organization names to be used on the Certificate. + // IPAddresses are IP subject alternative names, as literal IP strings. // +optional - Organization []string `json:"organizations,omitempty"` + IPAddresses []string `json:"ipAddresses,omitempty"` - // 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. - AltNames AltNames `json:"altNames,omitempty"` + // Duration is the requested certificate lifetime. The auto provider + // requires at least 8760h (365 days); empty defaults to 8760h. Go duration + // units only (h/m/s); day suffixes like "365d" are not accepted. + // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('8760h')",message="auto provider certificates must be valid for at least 8760h (365 days); use Go duration units h/m/s (day suffixes like '365d' are not accepted)" + // +optional + Duration *metav1.Duration `json:"duration,omitempty"` - // 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 + // RenewBefore is reserved: the auto provider does not yet renew + // certificates. Accepted for parity with the certManager block. + // +kubebuilder:validation:XValidation:rule="duration(self) > duration('0s')",message="renewBefore must be a positive Go duration using h/m/s units (day suffixes are not accepted)" // +optional - ValidityDuration string `json:"validityDuration,omitempty"` + RenewBefore *metav1.Duration `json:"renewBefore,omitempty"` } -type ProviderAutoConfig struct { - // CommonConfig is the struct of common fields required to create a certificate - CommonConfig `json:",inline"` -} +// TLSCertManagerProvider configures cert-manager issuance for one surface. +type TLSCertManagerProvider struct { + // IssuerRef references the cert-manager Issuer or ClusterIssuer that signs + // this surface's certificates. kind defaults to "Issuer"; group defaults to + // "cert-manager.io". Point it at a CA Issuer to issue from a custom CA. + // +kubebuilder:validation:XValidation:rule="!has(self.kind) || self.kind in ['Issuer', 'ClusterIssuer']",message="issuerRef.kind must be 'Issuer' or 'ClusterIssuer'" + IssuerRef cmmeta.IssuerReference `json:"issuerRef"` + + // CommonName is the X509 subject CN. Keep to 64 characters or fewer to + // avoid generating invalid CSRs. + // +kubebuilder:validation:MaxLength=64 + // +optional + CommonName string `json:"commonName,omitempty"` + + // Organizations are the X509 subject O values. + // +optional + Organizations []string `json:"organizations,omitempty"` -type ProviderCertManagerConfig struct { - // CommonConfig is the struct of common fields required to create a certificate - CommonConfig `json:",inline"` + // DNSNames are the DNS subject alternative names. Empty defaults to + // *...svc.cluster.local and ..svc.cluster.local + // (required for the operator's hostname verification of members). + // +optional + DNSNames []string `json:"dnsNames,omitempty"` - // IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer". - // +kubebuilder:validation:Enum=Issuer;ClusterIssuer - IssuerKind string `json:"issuerKind"` + // IPAddresses are IP subject alternative names, as literal IP strings. + // +optional + IPAddresses []string `json:"ipAddresses,omitempty"` - // IssuerName is the expected name of Issuer required to issue a certificate - IssuerName string `json:"issuerName"` + // Duration is the requested certificate lifetime, passed through to + // Certificate.spec.duration. Empty defaults to 2160h (90 days). Go duration + // units only (h/m/s); day suffixes like "90d" are not accepted, and + // cert-manager's minimum is 1h. + // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('1h')",message="duration must be at least 1h (cert-manager minimum) using Go duration units h/m/s (day suffixes like '90d' are not accepted)" + // +optional + Duration *metav1.Duration `json:"duration,omitempty"` - // 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). + // RenewBefore is passed through to Certificate.spec.renewBefore; empty + // leaves cert-manager's default renewal policy in place. + // +kubebuilder:validation:XValidation:rule="duration(self) > duration('0s')",message="renewBefore must be a positive Go duration using h/m/s units (day suffixes are not accepted)" // +optional - IssuerGroup string `json:"issuerGroup,omitempty"` + RenewBefore *metav1.Duration `json:"renewBefore,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 fbab398c..4e06d734 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -24,61 +24,8 @@ import ( "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - netx "net" ) -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *AltNames) DeepCopyInto(out *AltNames) { - *out = *in - if in.DNSNames != nil { - in, out := &in.DNSNames, &out.DNSNames - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.IPs != nil { - in, out := &in.IPs, &out.IPs - *out = make([]netx.IP, len(*in)) - for i := range *in { - if (*in)[i] != nil { - in, out := &(*in)[i], &(*out)[i] - *out = make(netx.IP, len(*in)) - copy(*out, *in) - } - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AltNames. -func (in *AltNames) DeepCopy() *AltNames { - if in == nil { - return nil - } - out := new(AltNames) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CommonConfig) DeepCopyInto(out *CommonConfig) { - *out = *in - if in.Organization != nil { - in, out := &in.Organization, &out.Organization - *out = make([]string, len(*in)) - copy(*out, *in) - } - in.AltNames.DeepCopyInto(&out.AltNames) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CommonConfig. -func (in *CommonConfig) DeepCopy() *CommonConfig { - if in == nil { - return nil - } - out := new(CommonConfig) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EtcdCluster) DeepCopyInto(out *EtcdCluster) { *out = *in @@ -329,75 +276,99 @@ func (in *PodTemplate) DeepCopy() *PodTemplate { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ProviderAutoConfig) DeepCopyInto(out *ProviderAutoConfig) { +func (in *StorageSpec) DeepCopyInto(out *StorageSpec) { *out = *in - in.CommonConfig.DeepCopyInto(&out.CommonConfig) + out.VolumeSizeRequest = in.VolumeSizeRequest.DeepCopy() + out.VolumeSizeLimit = in.VolumeSizeLimit.DeepCopy() } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderAutoConfig. -func (in *ProviderAutoConfig) DeepCopy() *ProviderAutoConfig { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new StorageSpec. +func (in *StorageSpec) DeepCopy() *StorageSpec { if in == nil { return nil } - out := new(ProviderAutoConfig) + out := new(StorageSpec) in.DeepCopyInto(out) return out } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ProviderCertManagerConfig) DeepCopyInto(out *ProviderCertManagerConfig) { +func (in *TLSAutoProvider) DeepCopyInto(out *TLSAutoProvider) { *out = *in - in.CommonConfig.DeepCopyInto(&out.CommonConfig) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderCertManagerConfig. -func (in *ProviderCertManagerConfig) DeepCopy() *ProviderCertManagerConfig { - if in == nil { - return nil + if in.Organizations != nil { + in, out := &in.Organizations, &out.Organizations + *out = make([]string, len(*in)) + copy(*out, *in) } - out := new(ProviderCertManagerConfig) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ProviderConfig) DeepCopyInto(out *ProviderConfig) { - *out = *in - if in.AutoCfg != nil { - in, out := &in.AutoCfg, &out.AutoCfg - *out = new(ProviderAutoConfig) - (*in).DeepCopyInto(*out) + if in.DNSNames != nil { + in, out := &in.DNSNames, &out.DNSNames + *out = make([]string, len(*in)) + copy(*out, *in) } - if in.CertManagerCfg != nil { - in, out := &in.CertManagerCfg, &out.CertManagerCfg - *out = new(ProviderCertManagerConfig) - (*in).DeepCopyInto(*out) + if in.IPAddresses != nil { + in, out := &in.IPAddresses, &out.IPAddresses + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Duration != nil { + in, out := &in.Duration, &out.Duration + *out = new(metav1.Duration) + **out = **in + } + if in.RenewBefore != nil { + in, out := &in.RenewBefore, &out.RenewBefore + *out = new(metav1.Duration) + **out = **in } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ProviderConfig. -func (in *ProviderConfig) DeepCopy() *ProviderConfig { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSAutoProvider. +func (in *TLSAutoProvider) DeepCopy() *TLSAutoProvider { if in == nil { return nil } - out := new(ProviderConfig) + out := new(TLSAutoProvider) in.DeepCopyInto(out) return out } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *StorageSpec) DeepCopyInto(out *StorageSpec) { +func (in *TLSCertManagerProvider) DeepCopyInto(out *TLSCertManagerProvider) { *out = *in - out.VolumeSizeRequest = in.VolumeSizeRequest.DeepCopy() - out.VolumeSizeLimit = in.VolumeSizeLimit.DeepCopy() + out.IssuerRef = in.IssuerRef + if in.Organizations != nil { + in, out := &in.Organizations, &out.Organizations + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.DNSNames != nil { + in, out := &in.DNSNames, &out.DNSNames + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.IPAddresses != nil { + in, out := &in.IPAddresses, &out.IPAddresses + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Duration != nil { + in, out := &in.Duration, &out.Duration + *out = new(metav1.Duration) + **out = **in + } + if in.RenewBefore != nil { + in, out := &in.RenewBefore, &out.RenewBefore + *out = new(metav1.Duration) + **out = **in + } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new StorageSpec. -func (in *StorageSpec) DeepCopy() *StorageSpec { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSCertManagerProvider. +func (in *TLSCertManagerProvider) DeepCopy() *TLSCertManagerProvider { if in == nil { return nil } - out := new(StorageSpec) + out := new(TLSCertManagerProvider) in.DeepCopyInto(out) return out } @@ -405,7 +376,16 @@ func (in *StorageSpec) DeepCopy() *StorageSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TLSSurface) DeepCopyInto(out *TLSSurface) { *out = *in - in.ProviderCfg.DeepCopyInto(&out.ProviderCfg) + if in.Auto != nil { + in, out := &in.Auto, &out.Auto + *out = new(TLSAutoProvider) + (*in).DeepCopyInto(*out) + } + if in.CertManager != nil { + in, out := &in.CertManager, &out.CertManager + *out = new(TLSCertManagerProvider) + (*in).DeepCopyInto(*out) + } if in.ClientCertAuth != nil { in, out := &in.ClientCertAuth, &out.ClientCertAuth *out = new(bool) diff --git a/config/crd/bases/operator.etcd.io_etcdclusters.yaml b/config/crd/bases/operator.etcd.io_etcdclusters.yaml index 59af77cf..294c5629 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -1087,6 +1087,137 @@ spec: own etcd client identity (the operator authenticates to etcd as a client). When nil, client traffic is cleartext and the operator dials cleartext. properties: + auto: + description: |- + Auto configures the operator's built-in self-signed provider for THIS + surface. Only valid when provider is "auto". + properties: + commonName: + description: |- + CommonName is the X509 subject CN. Keep to 64 characters or fewer to + avoid generating invalid CSRs. + maxLength: 64 + type: string + dnsNames: + description: |- + DNSNames are the DNS subject alternative names. Empty defaults to + *...svc.cluster.local and ..svc.cluster.local + (required for the operator's hostname verification of members). + items: + type: string + type: array + duration: + description: |- + Duration is the requested certificate lifetime. The auto provider + requires at least 8760h (365 days); empty defaults to 8760h. Go duration + units only (h/m/s); day suffixes like "365d" are not accepted. + type: string + x-kubernetes-validations: + - message: auto provider certificates must be valid for + at least 8760h (365 days); use Go duration units h/m/s + (day suffixes like '365d' are not accepted) + rule: duration(self) >= duration('8760h') + ipAddresses: + description: IPAddresses are IP subject alternative names, + as literal IP strings. + items: + type: string + type: array + organizations: + description: Organizations are the X509 subject O values. + items: + type: string + type: array + renewBefore: + description: |- + RenewBefore is reserved: the auto provider does not yet renew + certificates. Accepted for parity with the certManager block. + type: string + x-kubernetes-validations: + - message: renewBefore must be a positive Go duration + using h/m/s units (day suffixes are not accepted) + rule: duration(self) > duration('0s') + type: object + certManager: + description: |- + CertManager configures cert-manager issuance for THIS surface. + Required when provider is "cert-manager.io"; forbidden otherwise. + properties: + commonName: + description: |- + CommonName is the X509 subject CN. Keep to 64 characters or fewer to + avoid generating invalid CSRs. + maxLength: 64 + type: string + dnsNames: + description: |- + DNSNames are the DNS subject alternative names. Empty defaults to + *...svc.cluster.local and ..svc.cluster.local + (required for the operator's hostname verification of members). + items: + type: string + type: array + duration: + description: |- + Duration is the requested certificate lifetime, passed through to + Certificate.spec.duration. Empty defaults to 2160h (90 days). Go duration + units only (h/m/s); day suffixes like "90d" are not accepted, and + cert-manager's minimum is 1h. + type: string + x-kubernetes-validations: + - message: duration must be at least 1h (cert-manager + minimum) using Go duration units h/m/s (day suffixes + like '90d' are not accepted) + rule: duration(self) >= duration('1h') + ipAddresses: + description: IPAddresses are IP subject alternative names, + as literal IP strings. + items: + type: string + type: array + issuerRef: + description: |- + IssuerRef references the cert-manager Issuer or ClusterIssuer that signs + this surface's certificates. kind defaults to "Issuer"; group defaults to + "cert-manager.io". Point it at a CA Issuer to issue from a custom CA. + properties: + group: + description: |- + Group of the issuer being referred to. + Defaults to 'cert-manager.io'. + type: string + kind: + description: |- + Kind of the issuer being referred to. + Defaults to 'Issuer'. + type: string + name: + description: Name of the issuer being referred to. + type: string + required: + - name + type: object + x-kubernetes-validations: + - message: issuerRef.kind must be 'Issuer' or 'ClusterIssuer' + rule: '!has(self.kind) || self.kind in [''Issuer'', + ''ClusterIssuer'']' + organizations: + description: Organizations are the X509 subject O values. + items: + type: string + type: array + renewBefore: + description: |- + RenewBefore is passed through to Certificate.spec.renewBefore; empty + leaves cert-manager's default renewal policy in place. + type: string + x-kubernetes-validations: + - message: renewBefore must be a positive Go duration + using h/m/s units (day suffixes are not accepted) + rule: duration(self) > duration('0s') + required: + - issuerRef + type: object clientCertAuth: default: true description: |- @@ -1094,132 +1225,30 @@ spec: --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. + the cert-manager provider a trusted CA (issuerRef.name) is REQUIRED, + enforced by the XValidation rule above. type: boolean provider: + default: auto description: |- - Provider selects the certificate provider for THIS surface. - Defaults to "auto" when empty. + Provider selects the certificate provider for THIS surface and names + which member block below is honored. Defaults to "auto". enum: - auto - - cert-manager + - cert-manager.io type: string - providerCfg: - description: ProviderCfg is the provider-specific config for - THIS surface. - properties: - autoCfg: - 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 - 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: - 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 - 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) + - message: provider 'cert-manager.io' requires the certManager + block + rule: self.provider != 'cert-manager.io' || has(self.certManager) + - message: certManager may only be set when provider is 'cert-manager.io' + rule: self.provider == 'cert-manager.io' || !has(self.certManager) + - message: auto may only be set when provider is 'auto' + rule: self.provider == 'auto' || !has(self.auto) + - message: 'clientCertAuth requires a trusted CA: set certManager.issuerRef.name' + rule: '!self.clientCertAuth || self.provider != ''cert-manager.io'' + || (has(self.certManager) && size(self.certManager.issuerRef.name) > 0)' peer: description: |- @@ -1229,6 +1258,137 @@ spec: 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: + auto: + description: |- + Auto configures the operator's built-in self-signed provider for THIS + surface. Only valid when provider is "auto". + properties: + commonName: + description: |- + CommonName is the X509 subject CN. Keep to 64 characters or fewer to + avoid generating invalid CSRs. + maxLength: 64 + type: string + dnsNames: + description: |- + DNSNames are the DNS subject alternative names. Empty defaults to + *...svc.cluster.local and ..svc.cluster.local + (required for the operator's hostname verification of members). + items: + type: string + type: array + duration: + description: |- + Duration is the requested certificate lifetime. The auto provider + requires at least 8760h (365 days); empty defaults to 8760h. Go duration + units only (h/m/s); day suffixes like "365d" are not accepted. + type: string + x-kubernetes-validations: + - message: auto provider certificates must be valid for + at least 8760h (365 days); use Go duration units h/m/s + (day suffixes like '365d' are not accepted) + rule: duration(self) >= duration('8760h') + ipAddresses: + description: IPAddresses are IP subject alternative names, + as literal IP strings. + items: + type: string + type: array + organizations: + description: Organizations are the X509 subject O values. + items: + type: string + type: array + renewBefore: + description: |- + RenewBefore is reserved: the auto provider does not yet renew + certificates. Accepted for parity with the certManager block. + type: string + x-kubernetes-validations: + - message: renewBefore must be a positive Go duration + using h/m/s units (day suffixes are not accepted) + rule: duration(self) > duration('0s') + type: object + certManager: + description: |- + CertManager configures cert-manager issuance for THIS surface. + Required when provider is "cert-manager.io"; forbidden otherwise. + properties: + commonName: + description: |- + CommonName is the X509 subject CN. Keep to 64 characters or fewer to + avoid generating invalid CSRs. + maxLength: 64 + type: string + dnsNames: + description: |- + DNSNames are the DNS subject alternative names. Empty defaults to + *...svc.cluster.local and ..svc.cluster.local + (required for the operator's hostname verification of members). + items: + type: string + type: array + duration: + description: |- + Duration is the requested certificate lifetime, passed through to + Certificate.spec.duration. Empty defaults to 2160h (90 days). Go duration + units only (h/m/s); day suffixes like "90d" are not accepted, and + cert-manager's minimum is 1h. + type: string + x-kubernetes-validations: + - message: duration must be at least 1h (cert-manager + minimum) using Go duration units h/m/s (day suffixes + like '90d' are not accepted) + rule: duration(self) >= duration('1h') + ipAddresses: + description: IPAddresses are IP subject alternative names, + as literal IP strings. + items: + type: string + type: array + issuerRef: + description: |- + IssuerRef references the cert-manager Issuer or ClusterIssuer that signs + this surface's certificates. kind defaults to "Issuer"; group defaults to + "cert-manager.io". Point it at a CA Issuer to issue from a custom CA. + properties: + group: + description: |- + Group of the issuer being referred to. + Defaults to 'cert-manager.io'. + type: string + kind: + description: |- + Kind of the issuer being referred to. + Defaults to 'Issuer'. + type: string + name: + description: Name of the issuer being referred to. + type: string + required: + - name + type: object + x-kubernetes-validations: + - message: issuerRef.kind must be 'Issuer' or 'ClusterIssuer' + rule: '!has(self.kind) || self.kind in [''Issuer'', + ''ClusterIssuer'']' + organizations: + description: Organizations are the X509 subject O values. + items: + type: string + type: array + renewBefore: + description: |- + RenewBefore is passed through to Certificate.spec.renewBefore; empty + leaves cert-manager's default renewal policy in place. + type: string + x-kubernetes-validations: + - message: renewBefore must be a positive Go duration + using h/m/s units (day suffixes are not accepted) + rule: duration(self) > duration('0s') + required: + - issuerRef + type: object clientCertAuth: default: true description: |- @@ -1236,132 +1396,30 @@ spec: --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. + the cert-manager provider a trusted CA (issuerRef.name) is REQUIRED, + enforced by the XValidation rule above. type: boolean provider: + default: auto description: |- - Provider selects the certificate provider for THIS surface. - Defaults to "auto" when empty. + Provider selects the certificate provider for THIS surface and names + which member block below is honored. Defaults to "auto". enum: - auto - - cert-manager + - cert-manager.io type: string - providerCfg: - description: ProviderCfg is the provider-specific config for - THIS surface. - properties: - autoCfg: - 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 - 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: - 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 - 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) + - message: provider 'cert-manager.io' requires the certManager + block + rule: self.provider != 'cert-manager.io' || has(self.certManager) + - message: certManager may only be set when provider is 'cert-manager.io' + rule: self.provider == 'cert-manager.io' || !has(self.certManager) + - message: auto may only be set when provider is 'auto' + rule: self.provider == 'auto' || !has(self.auto) + - message: 'clientCertAuth requires a trusted CA: set certManager.issuerRef.name' + rule: '!self.clientCertAuth || self.provider != ''cert-manager.io'' + || (has(self.certManager) && size(self.certManager.issuerRef.name) > 0)' type: object version: diff --git a/config/samples/operator_v1alpha1_etcdcluster.yaml b/config/samples/operator_v1alpha1_etcdcluster.yaml index a18672b7..f97122d1 100644 --- a/config/samples/operator_v1alpha1_etcdcluster.yaml +++ b/config/samples/operator_v1alpha1_etcdcluster.yaml @@ -8,4 +8,3 @@ metadata: spec: version: v3.5.21 size: 3 - tls: diff --git a/config/samples/sample_autocert_etcdcluster.yaml b/config/samples/sample_autocert_etcdcluster.yaml index 9d7bc126..c111c0d8 100644 --- a/config/samples/sample_autocert_etcdcluster.yaml +++ b/config/samples/sample_autocert_etcdcluster.yaml @@ -8,9 +8,16 @@ metadata: spec: version: v3.5.21 size: 3 + # Both surfaces use the built-in self-signed provider. The auto provider + # requires a duration of at least 8760h (365 days); Go duration units only. tls: - provider: "auto" - providerCfg: - autoCfg: + peer: + provider: "auto" + auto: commonName: "etcd-operator-system" - validityDuration: 365d + duration: "8760h" + client: + provider: "auto" + auto: + commonName: "etcd-operator-system" + duration: "8760h" diff --git a/config/samples/sample_certmanager_etcdcluster.yaml b/config/samples/sample_certmanager_etcdcluster.yaml index 282399b2..a9fc01c5 100644 --- a/config/samples/sample_certmanager_etcdcluster.yaml +++ b/config/samples/sample_certmanager_etcdcluster.yaml @@ -65,18 +65,19 @@ spec: # cleartext, or pointed at a different issuer. tls: peer: - provider: "cert-manager" - providerCfg: - certManagerCfg: - commonName: "etcd-operator-system" - validityDuration: "365d" - issuerKind: "ClusterIssuer" - issuerName: "etcd-ca-issuer" + provider: "cert-manager.io" + certManager: + commonName: "etcd-operator-system" + # Go duration units only (h/m/s); day suffixes like "365d" are rejected. + duration: "8760h" + issuerRef: + kind: ClusterIssuer + name: etcd-ca-issuer client: - provider: "cert-manager" - providerCfg: - certManagerCfg: - commonName: "etcd-operator-system" - validityDuration: "365d" - issuerKind: "ClusterIssuer" - issuerName: "etcd-ca-issuer" + provider: "cert-manager.io" + certManager: + commonName: "etcd-operator-system" + duration: "8760h" + issuerRef: + kind: ClusterIssuer + name: etcd-ca-issuer diff --git a/docs/api-references/docs.md b/docs/api-references/docs.md index 6a32be4e..5e6f6cca 100644 --- a/docs/api-references/docs.md +++ b/docs/api-references/docs.md @@ -14,46 +14,6 @@ Package v1alpha1 contains API Schema definitions for the operator v1alpha1 API g -#### AltNames - - - - - - - -_Appears in:_ -- [CommonConfig](#commonconfig) -- [ProviderAutoConfig](#providerautoconfig) -- [ProviderCertManagerConfig](#providercertmanagerconfig) - -| Field | Description | Default | Validation | -| --- | --- | --- | --- | -| `dnsNames` _string array_ | DNSNames is the expected array of DNS subject alternative names.
if empty defaults to $(POD_NAME).$(ETCD_CLUSTER_NAME).$(POD_NAMESPACE).svc.cluster.local | | Optional: \{\}
| -| `ipAddresses` _IP array_ | IPs is the expected array of IP address subject alternative names. | | Optional: \{\}
| - - -#### CommonConfig - - - - - - - -_Appears in:_ -- [ProviderAutoConfig](#providerautoconfig) -- [ProviderCertManagerConfig](#providercertmanagerconfig) - -| Field | Description | Default | Validation | -| --- | --- | --- | --- | -| `commonName` _string_ | CommonName is the expected common name X509 certificate subject attribute.
Should have a length of 64 characters or fewer to avoid generating invalid CSRs. | | Optional: \{\}
| -| `organizations` _string array_ | Organization is the expected array of Organization names to be used on the Certificate. | | Optional: \{\}
| -| `altNames` _[AltNames](#altnames)_ | 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. | | | -| `validityDuration` _string_ | 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 | | Optional: \{\}
| -| `caBundleSecret` _string_ | 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: \{\}
| - - #### EtcdCluster @@ -108,13 +68,35 @@ _Appears in:_ | `imageRegistry` _string_ | ImageRegistry specifies the container registry that hosts the etcd images.
If unset, it defaults to the value provided via the controller's
--image-registry flag, which itself defaults to "gcr.io/etcd-development/etcd". | | | | `version` _string_ | Version is the expected version of the etcd container image. | | | | `storageSpec` _[StorageSpec](#storagespec)_ | 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. | | | -| `tls` _[TLSCertificate](#tlscertificate)_ | TLS is the TLS certificate configuration to use for the etcd cluster and etcd operator. | | | +| `tls` _[EtcdClusterTLS](#etcdclustertls)_ | 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. | | | | `etcdOptions` _string array_ | 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. | | | | `podTemplate` _[PodTemplate](#podtemplate)_ | PodTemplate is the pod template to use for the etcd cluster. | | | +#### EtcdClusterTLS + + + +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. + + + +_Appears in:_ +- [EtcdClusterSpec](#etcdclusterspec) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `peer` _[TLSSurface](#tlssurface)_ | 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: \{\}
| +| `client` _[TLSSurface](#tlssurface)_ | 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: \{\}
| + + #### MemberStatus @@ -153,7 +135,7 @@ _Appears in:_ | `labels` _object (keys:string, values:string)_ | | | | -#### PodTemplate +#### PodSpec @@ -162,14 +144,16 @@ _Appears in:_ _Appears in:_ -- [EtcdClusterSpec](#etcdclusterspec) +- [PodTemplate](#podtemplate) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `metadata` _[PodMetadata](#podmetadata)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | +| `affinity` _[Affinity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#affinity-v1-core)_ | | | | +| `nodeSelector` _object (keys:string, values:string)_ | | | | +| `tolerations` _[Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#toleration-v1-core) array_ | | | | -#### ProviderAutoConfig +#### PodTemplate @@ -178,18 +162,15 @@ _Appears in:_ _Appears in:_ -- [ProviderConfig](#providerconfig) +- [EtcdClusterSpec](#etcdclusterspec) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `commonName` _string_ | CommonName is the expected common name X509 certificate subject attribute.
Should have a length of 64 characters or fewer to avoid generating invalid CSRs. | | Optional: \{\}
| -| `organizations` _string array_ | Organization is the expected array of Organization names to be used on the Certificate. | | Optional: \{\}
| -| `altNames` _[AltNames](#altnames)_ | 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. | | | -| `validityDuration` _string_ | 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 | | Optional: \{\}
| -| `caBundleSecret` _string_ | 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: \{\}
| +| `metadata` _[PodMetadata](#podmetadata)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | +| `spec` _[PodSpec](#podspec)_ | | | | -#### ProviderCertManagerConfig +#### StorageSpec @@ -198,70 +179,109 @@ _Appears in:_ _Appears in:_ -- [ProviderConfig](#providerconfig) +- [EtcdClusterSpec](#etcdclusterspec) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `commonName` _string_ | CommonName is the expected common name X509 certificate subject attribute.
Should have a length of 64 characters or fewer to avoid generating invalid CSRs. | | Optional: \{\}
| -| `organizations` _string array_ | Organization is the expected array of Organization names to be used on the Certificate. | | Optional: \{\}
| -| `altNames` _[AltNames](#altnames)_ | 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. | | | -| `validityDuration` _string_ | 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 | | Optional: \{\}
| -| `caBundleSecret` _string_ | 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: \{\}
| -| `issuerKind` _string_ | IssuerKind is the expected kind of Issuer, either "ClusterIssuer" or "Issuer" | | | -| `issuerName` _string_ | IssuerName is the expected name of Issuer required to issue a certificate | | | - +| `accessModes` _[PersistentVolumeAccessMode](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#persistentvolumeaccessmode-v1-core)_ | | | | +| `storageClassName` _string_ | | | | +| `pvcName` _string_ | | | | +| `volumeSizeRequest` _[Quantity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#quantity-resource-api)_ | | | | +| `volumeSizeLimit` _[Quantity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#quantity-resource-api)_ | | | | -#### ProviderConfig +#### TLSAutoProvider +TLSAutoProvider tunes the built-in self-signed provider for one surface. +All fields are optional; empty values derive defaults from the cluster. _Appears in:_ -- [TLSCertificate](#tlscertificate) +- [TLSSurface](#tlssurface) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `autoCfg` _[ProviderAutoConfig](#providerautoconfig)_ | | | | -| `certManagerCfg` _[ProviderCertManagerConfig](#providercertmanagerconfig)_ | | | | +| `commonName` _string_ | CommonName is the X509 subject CN. Keep to 64 characters or fewer to
avoid generating invalid CSRs. | | MaxLength: 64
Optional: \{\}
| +| `organizations` _string array_ | Organizations are the X509 subject O values. | | Optional: \{\}
| +| `dnsNames` _string array_ | DNSNames are the DNS subject alternative names. Empty defaults to
*...svc.cluster.local and ..svc.cluster.local
(required for the operator's hostname verification of members). | | Optional: \{\}
| +| `ipAddresses` _string array_ | IPAddresses are IP subject alternative names, as literal IP strings. | | Optional: \{\}
| +| `duration` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#duration-v1-meta)_ | Duration is the requested certificate lifetime. The auto provider
requires at least 8760h (365 days); empty defaults to 8760h. Go duration
units only (h/m/s); day suffixes like "365d" are not accepted. | | Optional: \{\}
| +| `renewBefore` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#duration-v1-meta)_ | RenewBefore is reserved: the auto provider does not yet renew
certificates. Accepted for parity with the certManager block. | | Optional: \{\}
| -#### StorageSpec - +#### TLSCertManagerProvider +TLSCertManagerProvider configures cert-manager issuance for one surface. _Appears in:_ -- [EtcdClusterSpec](#etcdclusterspec) +- [TLSSurface](#tlssurface) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `accessModes` _[PersistentVolumeAccessMode](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#persistentvolumeaccessmode-v1-core)_ | | | | -| `storageClassName` _string_ | | | | -| `pvcName` _string_ | | | | -| `volumeSizeRequest` _[Quantity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#quantity-resource-api)_ | | | | -| `volumeSizeLimit` _[Quantity](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#quantity-resource-api)_ | | | | +| `issuerRef` _[IssuerReference](#issuerreference)_ | IssuerRef references the cert-manager Issuer or ClusterIssuer that signs
this surface's certificates. kind defaults to "Issuer"; group defaults to
"cert-manager.io". Point it at a CA Issuer to issue from a custom CA. | | | +| `commonName` _string_ | CommonName is the X509 subject CN. Keep to 64 characters or fewer to
avoid generating invalid CSRs. | | MaxLength: 64
Optional: \{\}
| +| `organizations` _string array_ | Organizations are the X509 subject O values. | | Optional: \{\}
| +| `dnsNames` _string array_ | DNSNames are the DNS subject alternative names. Empty defaults to
*...svc.cluster.local and ..svc.cluster.local
(required for the operator's hostname verification of members). | | Optional: \{\}
| +| `ipAddresses` _string array_ | IPAddresses are IP subject alternative names, as literal IP strings. | | Optional: \{\}
| +| `duration` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#duration-v1-meta)_ | Duration is the requested certificate lifetime, passed through to
Certificate.spec.duration. Empty defaults to 2160h (90 days). Go duration
units only (h/m/s); day suffixes like "90d" are not accepted, and
cert-manager's minimum is 1h. | | Optional: \{\}
| +| `renewBefore` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v/#duration-v1-meta)_ | RenewBefore is passed through to Certificate.spec.renewBefore; empty
leaves cert-manager's default renewal policy in place. | | Optional: \{\}
| -#### TLSCertificate +#### TLSProvider +_Underlying type:_ _string_ +TLSProvider names the certificate provider for one TLS surface. It is the +discriminator of the flattened union on TLSSurface. Values are domain-style +identifiers (cf. StorageClass.provisioner): the operator's built-in +self-signed provider is "auto"; cert-manager is addressed by its API group. +_Validation:_ +- Enum: [auto cert-manager.io] +_Appears in:_ +- [TLSSurface](#tlssurface) + +| Field | Description | +| --- | --- | +| `auto` | TLSProviderAuto selects the operator's built-in self-signed provider.
| +| `cert-manager.io` | TLSProviderCertManager selects cert-manager issuance.
| + + +#### TLSSurface + + + +TLSSurface is the full, independent TLS configuration for ONE surface (peer or +client): a provider-discriminated union (provider selects which member block +below is honored), the surface's mutual client-cert-auth policy, and an +optional additional trust bundle. + +The XValidation rules below are the apply-time anti-misconfiguration +guardrails: they reject incoherent provider/member-block 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 -- see +validateTLSSurface and the cert-manager provider's validateCertificateConfig. _Appears in:_ -- [EtcdClusterSpec](#etcdclusterspec) +- [EtcdClusterTLS](#etcdclustertls) | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `provider` _string_ | | | | -| `providerCfg` _[ProviderConfig](#providerconfig)_ | | | | +| `provider` _[TLSProvider](#tlsprovider)_ | Provider selects the certificate provider for THIS surface and names
which member block below is honored. Defaults to "auto". | auto | Enum: [auto cert-manager.io]
Optional: \{\}
| +| `auto` _[TLSAutoProvider](#tlsautoprovider)_ | Auto configures the operator's built-in self-signed provider for THIS
surface. Only valid when provider is "auto". | | Optional: \{\}
| +| `certManager` _[TLSCertManagerProvider](#tlscertmanagerprovider)_ | CertManager configures cert-manager issuance for THIS surface.
Required when provider is "cert-manager.io"; forbidden otherwise. | | Optional: \{\}
| +| `clientCertAuth` _boolean_ | 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 (issuerRef.name) is REQUIRED,
enforced by the XValidation rule above. | true | Optional: \{\}
| diff --git a/internal/controller/tls_cel_test.go b/internal/controller/tls_cel_test.go index 0b125b73..18d3ec06 100644 --- a/internal/controller/tls_cel_test.go +++ b/internal/controller/tls_cel_test.go @@ -2,10 +2,13 @@ package controller import ( "testing" + "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" @@ -21,14 +24,15 @@ func TestTLSCELValidation(t *testing.T) { t.Skip("envtest apiserver not available") } - cm := func(issuerName string) ecv1alpha1.ProviderConfig { - return ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - IssuerKind: "ClusterIssuer", - IssuerName: issuerName, + cm := func(issuerName string) *ecv1alpha1.TLSCertManagerProvider { + return &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{ + Kind: "ClusterIssuer", + Name: issuerName, }, } } + dur := func(d time.Duration) *metav1.Duration { return &metav1.Duration{Duration: d} } tests := []struct { name string @@ -37,51 +41,67 @@ func TestTLSCELValidation(t *testing.T) { }{ { name: "valid cert-manager mTLS surface accepted", - surface: ecv1alpha1.TLSSurface{Provider: "cert-manager", ProviderCfg: cm("etcd-ca-issuer")}, + surface: ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderCertManager, CertManager: cm("etcd-ca-issuer")}, wantApply: true, }, { name: "auto provider surface accepted", - surface: ecv1alpha1.TLSSurface{Provider: "auto"}, + surface: ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderAuto}, wantApply: true, }, { - name: "cert-manager provider without certManagerCfg rejected", - surface: ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + name: "cert-manager provider without the certManager block rejected", + surface: ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderCertManager}, wantApply: false, }, { - name: "certManagerCfg under auto provider rejected", - surface: ecv1alpha1.TLSSurface{Provider: "auto", ProviderCfg: cm("etcd-ca-issuer")}, + name: "certManager block under auto provider rejected", + surface: ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderAuto, CertManager: cm("etcd-ca-issuer")}, wantApply: false, }, { - name: "clientCertAuth true with cert-manager but empty issuerName rejected", + name: "auto block under cert-manager provider rejected", surface: ecv1alpha1.TLSSurface{ - Provider: "cert-manager", + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: cm("etcd-ca-issuer"), + Auto: &ecv1alpha1.TLSAutoProvider{}, + }, + wantApply: false, + }, + { + name: "clientCertAuth true with cert-manager but empty issuerRef.name rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderCertManager, ClientCertAuth: boolPtr(true), - ProviderCfg: cm(""), + CertManager: cm(""), }, wantApply: false, }, { - name: "server-only TLS (clientCertAuth false) without issuerName accepted", + name: "server-only TLS (clientCertAuth false) accepted", surface: ecv1alpha1.TLSSurface{ - Provider: "cert-manager", + Provider: ecv1alpha1.TLSProviderCertManager, ClientCertAuth: boolPtr(false), - ProviderCfg: cm("etcd-ca-issuer"), + CertManager: cm("etcd-ca-issuer"), + }, + wantApply: true, + }, + { + name: "issuerRef.kind omitted accepted (defaults to Issuer)", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Name: "etcd-ca-issuer"}, + }, }, wantApply: true, }, { - name: "bad issuerKind rejected by enum", + name: "bad issuerRef.kind rejected by CEL", surface: ecv1alpha1.TLSSurface{ - Provider: "cert-manager", - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - IssuerKind: "Bogus", - IssuerName: "etcd-ca-issuer", - }, + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Kind: "Bogus", Name: "etcd-ca-issuer"}, }, }, wantApply: false, @@ -93,6 +113,48 @@ func TestTLSCELValidation(t *testing.T) { }, wantApply: false, }, + { + name: "cert-manager duration below 1h rejected", + surface: func() ecv1alpha1.TLSSurface { + s := ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderCertManager, CertManager: cm("etcd-ca-issuer")} + s.CertManager.Duration = dur(30 * time.Minute) + return s + }(), + wantApply: false, + }, + { + name: "cert-manager duration of 90 days accepted", + surface: func() ecv1alpha1.TLSSurface { + s := ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderCertManager, CertManager: cm("etcd-ca-issuer")} + s.CertManager.Duration = dur(2160 * time.Hour) + return s + }(), + wantApply: true, + }, + { + name: "auto duration below 365 days rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{Duration: dur(720 * time.Hour)}, + }, + wantApply: false, + }, + { + name: "auto duration of 365 days accepted", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{Duration: dur(8760 * time.Hour)}, + }, + wantApply: true, + }, + { + name: "zero renewBefore rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{RenewBefore: dur(0)}, + }, + wantApply: false, + }, } for i, tt := range tests { @@ -122,6 +184,41 @@ func TestTLSCELValidation(t *testing.T) { } } +// TestTLSDaySuffixDurationRejected asserts the CEL duration() guards reject +// day-suffix strings at ADMISSION. The CRD renders *metav1.Duration as a bare +// string schema, so without these guards "365d" would be admitted and stored, +// then wedge the controller's typed decode (time.ParseDuration has no 'd' +// unit). Raw JSON is used because the Go client cannot even marshal an +// unparseable metav1.Duration. +func TestTLSDaySuffixDurationRejected(t *testing.T) { + if k8sClient == nil { + t.Skip("envtest apiserver not available") + } + + for _, tc := range []struct { + name string + block string + }{ + {"auto 365d", `"provider":"auto","auto":{"duration":"365d"}`}, + {"certManager 90d", `"provider":"cert-manager.io","certManager":{"issuerRef":{"name":"x"},"duration":"90d"}`}, + {"renewBefore 30d", `"provider":"auto","auto":{"renewBefore":"30d"}`}, + } { + t.Run(tc.name, func(t *testing.T) { + u := &unstructured.Unstructured{} + raw := `{ + "apiVersion": "operator.etcd.io/v1alpha1", + "kind": "EtcdCluster", + "metadata": {"generateName": "cel-day-", "namespace": "default"}, + "spec": {"size": 3, "version": "v3.6.12", + "tls": {"client": {` + tc.block + `}}} + }` + require.NoError(t, u.UnmarshalJSON([]byte(raw))) + err := k8sClient.Create(t.Context(), u) + assert.Error(t, err, "day-suffix duration must be rejected at admission") + }) + } +} + func tlsForSurface(onClient bool, s *ecv1alpha1.TLSSurface) *ecv1alpha1.EtcdClusterTLS { if onClient { return &ecv1alpha1.EtcdClusterTLS{Client: s} diff --git a/internal/controller/tls_independence_test.go b/internal/controller/tls_independence_test.go index cdfac606..1b7b2619 100644 --- a/internal/controller/tls_independence_test.go +++ b/internal/controller/tls_independence_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -71,11 +72,11 @@ 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", + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{ + Kind: "ClusterIssuer", + Name: "etcd-ca-issuer", }, }, ClientCertAuth: clientCertAuth, @@ -370,47 +371,58 @@ func TestValidateTLS(t *testing.T) { wantErr: false, }, { - name: "cert-manager provider without certManagerCfg is rejected", + name: "cert-manager provider without the certManager block is rejected", tls: &ecv1alpha1.EtcdClusterTLS{ - Client: &ecv1alpha1.TLSSurface{Provider: "cert-manager"}, + Client: &ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderCertManager}, }, wantErr: true, }, { - name: "certManagerCfg under auto provider is rejected", + name: "certManager block under auto provider is rejected", tls: &ecv1alpha1.EtcdClusterTLS{ Client: &ecv1alpha1.TLSSurface{ - Provider: "auto", - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{ - IssuerKind: "ClusterIssuer", IssuerName: "x", - }, + Provider: ecv1alpha1.TLSProviderAuto, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Kind: "ClusterIssuer", Name: "x"}, }, }, }, wantErr: true, }, { - name: "clientCertAuth (mTLS) with cert-manager but no issuerName is rejected", + name: "auto block under cert-manager provider is rejected", tls: &ecv1alpha1.EtcdClusterTLS{ Client: &ecv1alpha1.TLSSurface{ - Provider: "cert-manager", + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Kind: "ClusterIssuer", Name: "x"}, + }, + Auto: &ecv1alpha1.TLSAutoProvider{}, + }, + }, + wantErr: true, + }, + { + name: "clientCertAuth (mTLS) with cert-manager but no issuerRef.name is rejected", + tls: &ecv1alpha1.EtcdClusterTLS{ + Client: &ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderCertManager, ClientCertAuth: boolPtr(true), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer"}, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Kind: "ClusterIssuer"}, }, }, }, wantErr: true, }, { - name: "server-only TLS (clientCertAuth false) without issuerName is allowed", + name: "server-only TLS (clientCertAuth false) with an issuerRef is allowed", tls: &ecv1alpha1.EtcdClusterTLS{ Client: &ecv1alpha1.TLSSurface{ - Provider: "cert-manager", + Provider: ecv1alpha1.TLSProviderCertManager, ClientCertAuth: boolPtr(false), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: &ecv1alpha1.ProviderCertManagerConfig{IssuerKind: "ClusterIssuer", IssuerName: "x"}, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Kind: "ClusterIssuer", Name: "x"}, }, }, }, @@ -419,8 +431,8 @@ func TestValidateTLS(t *testing.T) { { name: "auto provider on both surfaces is valid", tls: &ecv1alpha1.EtcdClusterTLS{ - Peer: &ecv1alpha1.TLSSurface{Provider: "auto"}, - Client: &ecv1alpha1.TLSSurface{Provider: "auto"}, + Peer: &ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderAuto}, + Client: &ecv1alpha1.TLSSurface{Provider: ecv1alpha1.TLSProviderAuto}, }, wantErr: false, }, diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 34fc40f7..521503cd 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -13,7 +13,6 @@ import ( "strings" "time" - cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/coreos/go-semver/semver" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" @@ -111,9 +110,10 @@ func clientCertAuthEnabled(s *ecv1alpha1.TLSSurface) bool { // (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) +// - provider 'cert-manager.io' => the certManager block must be set +// - provider auto/empty => the certManager block must NOT be set +// - provider 'cert-manager.io' => the auto block must NOT be set (and vice versa) +// - clientCertAuth (mTLS) with cert-manager => issuerRef.name 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 @@ -141,20 +141,24 @@ func validateTLSSurface(path *field.Path, s *ecv1alpha1.TLSSurface) field.ErrorL if s == nil { return errs } - hasCM := s.ProviderCfg.CertManagerCfg != nil - isCertManager := s.Provider == string(certificate.CertManager) + hasCM := s.CertManager != nil + isCertManager := s.EffectiveProvider() == ecv1alpha1.TLSProviderCertManager switch { case isCertManager && !hasCM: - errs = append(errs, field.Required(path.Child("providerCfg", "certManagerCfg"), - "provider 'cert-manager' requires providerCfg.certManagerCfg")) + errs = append(errs, field.Required(path.Child("certManager"), + "provider 'cert-manager.io' requires the certManager block")) case !isCertManager && hasCM: - errs = append(errs, field.Invalid(path.Child("providerCfg", "certManagerCfg"), "", - "providerCfg.certManagerCfg may only be set when provider is 'cert-manager'")) + errs = append(errs, field.Invalid(path.Child("certManager"), "", + "certManager may only be set when provider is 'cert-manager.io'")) } - // 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")) + if isCertManager && s.Auto != nil { + errs = append(errs, field.Invalid(path.Child("auto"), "", + "auto may only be set when provider is 'auto'")) + } + // mTLS requires a resolvable CA. With cert-manager that means an issuerRef.name. + if clientCertAuthEnabled(s) && isCertManager && hasCM && s.CertManager.IssuerRef.Name == "" { + errs = append(errs, field.Required(path.Child("certManager", "issuerRef", "name"), + "clientCertAuth requires a trusted CA: set certManager.issuerRef.name")) } return errs } @@ -801,20 +805,6 @@ func getPeerCertName(etcdClusterName string) string { return peerCertName } -// parseValidityDuration parses a duration string and returns the parsed duration. -// If the customizedDuration is empty, it returns the defaultDuration. -// Returns an error if the duration string cannot be parsed. -func parseValidityDuration(customizedDuration string, defaultDuration time.Duration) (time.Duration, error) { - if customizedDuration == "" { - return defaultDuration, nil - } - duration, err := time.ParseDuration(customizedDuration) - if err != nil { - return 0, fmt.Errorf("failed to parse ValidityDuration: %w", err) - } - return duration, nil -} - // defaultCertDNSNames returns the user-provided DNS SANs, or wildcard DNS for // the cluster's headless service to cover all pods (pod-0, pod-1, etc.). func defaultCertDNSNames(ec *ecv1alpha1.EtcdCluster, dnsNames []string) []string { @@ -827,70 +817,68 @@ func defaultCertDNSNames(ec *ecv1alpha1.EtcdCluster, dnsNames []string) []string } } -// certIPStrings renders the CRD's parsed IP SANs to the literal-string form -// used by certInterface.Config, preserving nil when none are set. -func certIPStrings(ips []net.IP) []string { - if len(ips) == 0 { - return nil - } - out := make([]string, 0, len(ips)) +// validateCertIPAddresses rejects IP SANs that do not parse as literal IPs. +// This runs at reconcile time because the CRD cannot use CEL's isIP() on the +// apiserver versions the operator still supports. +func validateCertIPAddresses(ips []string) error { for _, ip := range ips { - out = append(out, ip.String()) + if net.ParseIP(ip) == nil { + return field.Invalid(field.NewPath("ipAddresses"), ip, "must be a literal IP address") + } + } + return nil +} + +// resolveCertDuration resolves an optional CR duration against the provider +// default. +func resolveCertDuration(d *metav1.Duration, defaultDuration time.Duration) time.Duration { + if d == nil { + return defaultDuration } - return out + return d.Duration } func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { - cmConfig := surface.ProviderCfg.CertManagerCfg + cmConfig := surface.CertManager if cmConfig == nil { return nil, fmt.Errorf("cert-manager configuration is not present") } - - // Set default duration to 90 days for cert-manager if not provided - duration, err := parseValidityDuration(cmConfig.ValidityDuration, certInterface.DefaultCertManagerValidity) - if err != nil { + if err := validateCertIPAddresses(cmConfig.IPAddresses); err != nil { return nil, err } + issuerRef := cmConfig.IssuerRef config := &certInterface.Config{ CommonName: cmConfig.CommonName, - Organizations: cmConfig.Organization, - DNSNames: defaultCertDNSNames(ec, cmConfig.AltNames.DNSNames), - IPAddresses: certIPStrings(cmConfig.AltNames.IPs), - Duration: duration, - IssuerRef: &cmmeta.IssuerReference{ - Name: cmConfig.IssuerName, - Kind: cmConfig.IssuerKind, - Group: cmConfig.IssuerGroup, - }, + Organizations: cmConfig.Organizations, + DNSNames: defaultCertDNSNames(ec, cmConfig.DNSNames), + IPAddresses: cmConfig.IPAddresses, + Duration: resolveCertDuration(cmConfig.Duration, certInterface.DefaultCertManagerValidity), + RenewBefore: cmConfig.RenewBefore, + IssuerRef: &issuerRef, } return config, nil } func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster, surface *ecv1alpha1.TLSSurface) (*certInterface.Config, error) { - autoConfig := surface.ProviderCfg.AutoCfg - // Set default values for auto configuration if not present + autoConfig := surface.Auto + // The auto block is optional: defaults are derived from the cluster. if autoConfig == nil { - autoConfig = &ecv1alpha1.ProviderAutoConfig{ - CommonConfig: ecv1alpha1.CommonConfig{ - CommonName: fmt.Sprintf("%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), - ValidityDuration: certInterface.DefaultAutoValidity.String(), - }, + autoConfig = &ecv1alpha1.TLSAutoProvider{ + CommonName: fmt.Sprintf("%s.%s.%s", ec.Name, ec.Namespace, certInterface.DefaultDomainName), } } - - // Set default duration to 365 days for auto provider if not provided - duration, err := parseValidityDuration(autoConfig.ValidityDuration, certInterface.DefaultAutoValidity) - if err != nil { + if err := validateCertIPAddresses(autoConfig.IPAddresses); err != nil { return nil, err } config := &certInterface.Config{ CommonName: autoConfig.CommonName, - Organizations: autoConfig.Organization, - DNSNames: defaultCertDNSNames(ec, autoConfig.AltNames.DNSNames), - IPAddresses: certIPStrings(autoConfig.AltNames.IPs), - Duration: duration, + Organizations: autoConfig.Organizations, + DNSNames: defaultCertDNSNames(ec, autoConfig.DNSNames), + IPAddresses: autoConfig.IPAddresses, + Duration: resolveCertDuration(autoConfig.Duration, certInterface.DefaultAutoValidity), + RenewBefore: autoConfig.RenewBefore, } return config, nil } @@ -902,10 +890,7 @@ func createCertificate(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client logger := log.FromContext(ctx) // An empty provider on the surface defaults to the auto provider. - providerName := surface.Provider - if providerName == "" { - providerName = string(certificate.Auto) - } + providerName := surface.EffectiveProvider() cert, certErr := certificate.NewProvider(certificate.ProviderType(providerName), c) if certErr != nil { diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 2f4d4fdd..d2db6e84 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -21,7 +21,6 @@ import ( ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" "go.etcd.io/etcd-operator/internal/etcdutils" - "go.etcd.io/etcd-operator/pkg/certificate" certInterface "go.etcd.io/etcd-operator/pkg/certificate/interfaces" "go.etcd.io/etcd/api/v3/etcdserverpb" clientv3 "go.etcd.io/etcd/client/v3" @@ -933,18 +932,13 @@ func TestCreateAutoCertificateConfig(t *testing.T) { }, }, 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"}, - }, - }, - }, + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{ + CommonName: "custom.example.com", + Organizations: []string{"Test Org"}, + Duration: &metav1.Duration{Duration: 720 * time.Hour}, // 30 days + DNSNames: []string{"custom1.example.com", "custom2.example.com"}, + IPAddresses: []string{"10.0.0.9"}, }, }, expected: &certInterface.Config{ @@ -952,11 +946,12 @@ func TestCreateAutoCertificateConfig(t *testing.T) { Organizations: []string{"Test Org"}, Duration: 720 * time.Hour, // 30 days DNSNames: []string{"custom1.example.com", "custom2.example.com"}, + IPAddresses: []string{"10.0.0.9"}, }, wantErr: false, }, { - name: "auto config with nil AutoCfg - should use defaults", + name: "auto config with a malformed IP SAN is rejected", ec: &ecv1alpha1.EtcdCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", @@ -964,11 +959,23 @@ func TestCreateAutoCertificateConfig(t *testing.T) { }, }, surface: &ecv1alpha1.TLSSurface{ - Provider: string(certificate.Auto), - ProviderCfg: ecv1alpha1.ProviderConfig{ - AutoCfg: nil, + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{IPAddresses: []string{"not-an-ip"}}, + }, + expected: nil, + wantErr: true, + }, + { + name: "auto config with nil AutoCfg - should use defaults", + ec: &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-namespace", }, }, + surface: &ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + }, expected: &certInterface.Config{ CommonName: "test-cluster.test-namespace.svc.cluster.local", Organizations: nil, @@ -1015,20 +1022,17 @@ func TestCreateCMCertificateConfig(t *testing.T) { }, }, 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", + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + CommonName: "cm.example.com", + Organizations: []string{"CM Org"}, + Duration: &metav1.Duration{Duration: 1440 * time.Hour}, // 60 days + RenewBefore: &metav1.Duration{Duration: 360 * time.Hour}, + DNSNames: []string{"cm1.example.com", "cm2.example.com"}, + IssuerRef: cmmeta.IssuerReference{ + Name: "test-issuer", + Kind: "ClusterIssuer", + Group: "example.io", }, }, }, @@ -1036,6 +1040,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { CommonName: "cm.example.com", Organizations: []string{"CM Org"}, Duration: 1440 * time.Hour, // 60 days + RenewBefore: &metav1.Duration{Duration: 360 * time.Hour}, DNSNames: []string{"cm1.example.com", "cm2.example.com"}, IssuerRef: &cmmeta.IssuerReference{ Name: "test-issuer", @@ -1046,7 +1051,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { wantErr: false, }, { - name: "cert-manager config with nil CertManagerCfg", + name: "cert-manager config with defaulted duration", ec: &ecv1alpha1.EtcdCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", @@ -1054,11 +1059,32 @@ func TestCreateCMCertificateConfig(t *testing.T) { }, }, surface: &ecv1alpha1.TLSSurface{ - Provider: string(certificate.CertManager), - ProviderCfg: ecv1alpha1.ProviderConfig{ - CertManagerCfg: nil, + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + IssuerRef: cmmeta.IssuerReference{Name: "test-issuer"}, }, }, + expected: &certInterface.Config{ + Duration: certInterface.DefaultCertManagerValidity, + DNSNames: []string{ + "*.test-cluster.test-namespace.svc.cluster.local", + "test-cluster.test-namespace.svc.cluster.local", + }, + IssuerRef: &cmmeta.IssuerReference{Name: "test-issuer"}, + }, + wantErr: false, + }, + { + name: "cert-manager config with nil certManager block", + ec: &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-namespace", + }, + }, + surface: &ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderCertManager, + }, expected: nil, wantErr: true, }, diff --git a/pkg/certificate/certificate.go b/pkg/certificate/certificate.go index 48e2f0be..59dd2abb 100644 --- a/pkg/certificate/certificate.go +++ b/pkg/certificate/certificate.go @@ -13,8 +13,10 @@ import ( type ProviderType string const ( - Auto ProviderType = "auto" - CertManager ProviderType = "cert-manager" + Auto ProviderType = "auto" + // CertManager is addressed by cert-manager's API group, matching the CRD's + // spec.tls.{peer,client}.provider enum. + CertManager ProviderType = "cert-manager.io" // add more ... ) diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index e5b115a7..a9bdc599 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -26,6 +26,7 @@ import ( "testing" "time" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -345,15 +346,13 @@ func httpViaProxy(ctx context.Context, r *rest.Request, pod corev1.Pod, failpoin // 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, + Provider: ecv1alpha1.TLSProviderCertManager, + CertManager: &ecv1alpha1.TLSCertManagerProvider{ + CommonName: "etcd-operator-system", + Duration: &metav1.Duration{Duration: 90 * time.Hour}, + IssuerRef: cmmeta.IssuerReference{ + Kind: issuerKind, + Name: issuerName, }, }, } @@ -362,14 +361,10 @@ func certManagerSurface(issuerKind, issuerName string) *ecv1alpha1.TLSSurface { // 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", - }, - }, + Provider: ecv1alpha1.TLSProviderAuto, + Auto: &ecv1alpha1.TLSAutoProvider{ + CommonName: "etcd-operator-system", + Duration: &metav1.Duration{Duration: 8760 * time.Hour}, }, } } From fa4ffd8c4e14a26e56061f0c83cc7f72c5fcd771 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Sat, 11 Jul 2026 03:14:57 -0400 Subject: [PATCH 8/8] feat(api): per-surface trustBundleConfigMapRef appended to trusted CAs Each TLS surface may reference a ConfigMap (fixed key ca.crt) of extra PEM CAs. etcd takes exactly one trusted-CA file per surface, so the operator composes + into an owned per-surface ConfigMap ({cluster}-{server|peer}-trusted-ca), recomposed every reconcile so issued-CA rotation is picked up, and points --trusted-ca-file / --peer-trusted-ca-file at its mount. Without a bundle the rendered args and volumes are byte-identical to before. Intended for CA-rotation overlap windows: trust the incoming CA before certificates from it appear. Semantics chosen deliberately: - The bundle broadens member INBOUND trust only. The operator keeps pinning the issuing CA when dialing etcd; appending a user-writable ConfigMap's CAs to the dial trust would let ConfigMap write access mint certs that impersonate etcd to the operator. - Bundles are strictly validated (every PEM block must be a parseable certificate) because etcd's tlsutil.NewCertPool hard-errors on any bad block: an unvalidated bundle admits fine, then crash-loops members at their next restart. An invalid bundle fails the reconcile and preserves the last good composition. Go's AppendCertsFromPEM is NOT used for validation -- it silently skips bad blocks. - etcd reads trusted-CA files at process start only, so trust changes take effect per member on its next restart; docs/tls.md carries the rollout-restart guidance. No automatic StatefulSet roll on trust changes -- rolling a quorum-sensitive workload on trust bytes is a separate decision. docs/tls.md documents the surface model, the duration-format change, the dedicated-CA-per-cluster guidance, and that etcd RBAC (auth enable) is out of scope for v1alpha1. The certs and composed trust ConfigMaps are also re-applied on the steady-state reconcile path (the only path that skips reconcileStatefulSet), so a bundle edit or issued-CA rotation on a size-stable cluster recomposes before the user's rollout restart. Signed-off-by: Xavier Lange --- api/v1alpha1/etcdcluster_types.go | 21 ++ api/v1alpha1/zz_generated.deepcopy.go | 20 ++ .../bases/operator.etcd.io_etcdclusters.yaml | 40 ++++ docs/api-references/docs.md | 18 ++ docs/tls.md | 115 ++++++++++ internal/controller/etcdcluster_controller.go | 7 + internal/controller/tls_cel_test.go | 16 ++ internal/controller/tls_trustbundle_test.go | 211 ++++++++++++++++++ internal/controller/trustbundle.go | 152 +++++++++++++ internal/controller/utils.go | 64 +++++- 10 files changed, 656 insertions(+), 8 deletions(-) create mode 100644 docs/tls.md create mode 100644 internal/controller/tls_trustbundle_test.go create mode 100644 internal/controller/trustbundle.go diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go index 9f307169..3cc70d7c 100644 --- a/api/v1alpha1/etcdcluster_types.go +++ b/api/v1alpha1/etcdcluster_types.go @@ -155,6 +155,27 @@ type TLSSurface struct { // +kubebuilder:default=true // +optional ClientCertAuth *bool `json:"clientCertAuth,omitempty"` + + // TrustBundleConfigMapRef references a ConfigMap in the EtcdCluster's + // namespace carrying one or more additional PEM CA certificates under the + // fixed key "ca.crt". The bundle is APPENDED to (never replaces) this + // surface's MEMBER-SIDE trusted CA set: the operator concatenates it with + // the issued CA into the file behind --trusted-ca-file / + // --peer-trusted-ca-file. It broadens which client/peer certificates etcd + // members accept, NOT which servers the operator trusts when dialing etcd + // (the operator pins the issuing CA only). etcd reads trusted-CA files at + // process start only, so trust changes take effect per member on its next + // restart. Intended for CA-rotation overlap windows. + // +optional + TrustBundleConfigMapRef *TrustBundleConfigMapRef `json:"trustBundleConfigMapRef,omitempty"` +} + +// TrustBundleConfigMapRef is a LocalObjectReference-style pointer to a +// ConfigMap holding extra trusted CAs under the key "ca.crt". +type TrustBundleConfigMapRef struct { + // Name of the ConfigMap. + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` } // EffectiveProvider resolves the surface's provider, defaulting to "auto" when diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4e06d734..8b53895b 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -391,6 +391,11 @@ func (in *TLSSurface) DeepCopyInto(out *TLSSurface) { *out = new(bool) **out = **in } + if in.TrustBundleConfigMapRef != nil { + in, out := &in.TrustBundleConfigMapRef, &out.TrustBundleConfigMapRef + *out = new(TrustBundleConfigMapRef) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TLSSurface. @@ -402,3 +407,18 @@ func (in *TLSSurface) DeepCopy() *TLSSurface { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *TrustBundleConfigMapRef) DeepCopyInto(out *TrustBundleConfigMapRef) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TrustBundleConfigMapRef. +func (in *TrustBundleConfigMapRef) DeepCopy() *TrustBundleConfigMapRef { + if in == nil { + return nil + } + out := new(TrustBundleConfigMapRef) + 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 294c5629..e8056e96 100644 --- a/config/crd/bases/operator.etcd.io_etcdclusters.yaml +++ b/config/crd/bases/operator.etcd.io_etcdclusters.yaml @@ -1237,6 +1237,26 @@ spec: - auto - cert-manager.io type: string + trustBundleConfigMapRef: + description: |- + TrustBundleConfigMapRef references a ConfigMap in the EtcdCluster's + namespace carrying one or more additional PEM CA certificates under the + fixed key "ca.crt". The bundle is APPENDED to (never replaces) this + surface's MEMBER-SIDE trusted CA set: the operator concatenates it with + the issued CA into the file behind --trusted-ca-file / + --peer-trusted-ca-file. It broadens which client/peer certificates etcd + members accept, NOT which servers the operator trusts when dialing etcd + (the operator pins the issuing CA only). etcd reads trusted-CA files at + process start only, so trust changes take effect per member on its next + restart. Intended for CA-rotation overlap windows. + properties: + name: + description: Name of the ConfigMap. + minLength: 1 + type: string + required: + - name + type: object type: object x-kubernetes-validations: - message: provider 'cert-manager.io' requires the certManager @@ -1408,6 +1428,26 @@ spec: - auto - cert-manager.io type: string + trustBundleConfigMapRef: + description: |- + TrustBundleConfigMapRef references a ConfigMap in the EtcdCluster's + namespace carrying one or more additional PEM CA certificates under the + fixed key "ca.crt". The bundle is APPENDED to (never replaces) this + surface's MEMBER-SIDE trusted CA set: the operator concatenates it with + the issued CA into the file behind --trusted-ca-file / + --peer-trusted-ca-file. It broadens which client/peer certificates etcd + members accept, NOT which servers the operator trusts when dialing etcd + (the operator pins the issuing CA only). etcd reads trusted-CA files at + process start only, so trust changes take effect per member on its next + restart. Intended for CA-rotation overlap windows. + properties: + name: + description: Name of the ConfigMap. + minLength: 1 + type: string + required: + - name + type: object type: object x-kubernetes-validations: - message: provider 'cert-manager.io' requires the certManager diff --git a/docs/api-references/docs.md b/docs/api-references/docs.md index 5e6f6cca..a88f858b 100644 --- a/docs/api-references/docs.md +++ b/docs/api-references/docs.md @@ -283,5 +283,23 @@ _Appears in:_ | `auto` _[TLSAutoProvider](#tlsautoprovider)_ | Auto configures the operator's built-in self-signed provider for THIS
surface. Only valid when provider is "auto". | | Optional: \{\}
| | `certManager` _[TLSCertManagerProvider](#tlscertmanagerprovider)_ | CertManager configures cert-manager issuance for THIS surface.
Required when provider is "cert-manager.io"; forbidden otherwise. | | Optional: \{\}
| | `clientCertAuth` _boolean_ | 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 (issuerRef.name) is REQUIRED,
enforced by the XValidation rule above. | true | Optional: \{\}
| +| `trustBundleConfigMapRef` _[TrustBundleConfigMapRef](#trustbundleconfigmapref)_ | TrustBundleConfigMapRef references a ConfigMap in the EtcdCluster's
namespace carrying one or more additional PEM CA certificates under the
fixed key "ca.crt". The bundle is APPENDED to (never replaces) this
surface's MEMBER-SIDE trusted CA set: the operator concatenates it with
the issued CA into the file behind --trusted-ca-file /
--peer-trusted-ca-file. It broadens which client/peer certificates etcd
members accept, NOT which servers the operator trusts when dialing etcd
(the operator pins the issuing CA only). etcd reads trusted-CA files at
process start only, so trust changes take effect per member on its next
restart. Intended for CA-rotation overlap windows. | | Optional: \{\}
| + + +#### TrustBundleConfigMapRef + + + +TrustBundleConfigMapRef is a LocalObjectReference-style pointer to a +ConfigMap holding extra trusted CAs under the key "ca.crt". + + + +_Appears in:_ +- [TLSSurface](#tlssurface) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `name` _string_ | Name of the ConfigMap. | | MinLength: 1
| diff --git a/docs/tls.md b/docs/tls.md new file mode 100644 index 00000000..2ef58a47 --- /dev/null +++ b/docs/tls.md @@ -0,0 +1,115 @@ +# TLS + +`spec.tls` configures etcd's two independent TLS surfaces. Each surface is +optional; a nil surface is served (and, for the client surface, dialed by the +operator) in cleartext. Omitting `tls` entirely yields a fully-cleartext +cluster, byte-identical to a TLS-free deployment — there is no separate opt-out +knob. + +```yaml +spec: + tls: + peer: # etcd <-> etcd + provider: cert-manager.io # auto | cert-manager.io (default: auto) + certManager: + issuerRef: # cert-manager Issuer or ClusterIssuer + name: etcd-ca-issuer + kind: ClusterIssuer # defaults to Issuer (namespaced!) + commonName: my-etcd + duration: 8760h # Go units only; "365d" is rejected + renewBefore: 360h + trustBundleConfigMapRef: # optional extra trusted CAs (see below) + name: extra-cas + clientCertAuth: true # default true (mTLS) + client: # client -> etcd, and the operator's own dial identity + provider: auto # built-in self-signed provider + auto: + duration: 8760h # auto minimum is 8760h (365 days) +``` + +## Providers + +`provider` is the union discriminator: exactly the matching member block +(`auto:` or `certManager:`) may be set, enforced at apply time by CEL. Provider +identifiers are domain-style (like `StorageClass.provisioner`): the built-in +self-signed provider is `auto`, cert-manager is addressed by its API group +`cert-manager.io`. + +- **auto** — the operator mints a self-signed certificate per surface. Zero + external dependencies; certificates are not renewed automatically + (`renewBefore` is accepted but reserved). +- **cert-manager.io** — the operator creates cert-manager `Certificate` objects + signed by the referenced `issuerRef`. To issue from a custom CA, point + `issuerRef` at a cert-manager [CA `Issuer`](https://cert-manager.io/docs/configuration/ca/) + whose Secret holds your CA keypair — the operator deliberately has no + bring-your-own-CA input of its own. + +Both member blocks accept the same curated cert-manager-style fields: +`commonName`, `organizations`, `dnsNames`, `ipAddresses` (literal IP strings), +`duration`, `renewBefore`. + +**Durations use Go units (h/m/s) only.** Day suffixes like `365d` are rejected +at apply time. The CRD floors are 8760h (365 days) for `auto` — etcd's own +self-cert minimum — and 1h for `certManager`. + +**`issuerRef.kind` defaults to `Issuer`**, which is namespaced. If you +previously set `issuerKind: ClusterIssuer` and drop the kind while porting a +spec, the operator will look for a namespaced Issuer and report +`IssuerNotFound`. + +## Choose a dedicated CA per cluster + +With `clientCertAuth: true` (the default) any certificate chaining to the +surface's trusted CA has full access to etcd — etcd's authorization is the CA +boundary. Point the client surface's `issuerRef` at a CA dedicated to this etcd +cluster, not a broad shared intermediate: with a shared CA, every workload that +can obtain a certificate from it can read and write all of etcd. The `auto` +provider's per-surface self-signed CA is the safe zero-config default. + +## Trust bundles (`trustBundleConfigMapRef`) + +Each surface optionally references a ConfigMap (same namespace, fixed key +`ca.crt`, one or more PEM certificates). The bundle is **appended to — never +replaces —** the surface's member-side trusted CAs: the operator composes +` + ` into a per-surface ConfigMap +(`-server-trusted-ca` / `-peer-trusted-ca`) and points +`--trusted-ca-file` / `--peer-trusted-ca-file` at it. This exists for +CA-rotation overlap windows: trust the incoming CA before certificates from it +appear. + +Semantics worth knowing: + +- The bundle broadens which client/peer certificates **etcd members accept**. + It does not change which servers the **operator** trusts when dialing etcd — + the operator keeps pinning only the issuing CA from the client-surface + secret. (Anything else would let ConfigMap write access mint certificates + that impersonate etcd to the operator.) +- On the client surface, a trust bundle widens the mTLS admission boundary + described above — every CA in the bundle can mint credentials etcd accepts. +- **etcd reads trusted-CA files at process start only.** The operator keeps the + composed ConfigMap current (including after issued-CA rotation), but a member + honors trust changes only after its next restart: + `kubectl rollout restart statefulset/`. The operator deliberately + does not auto-roll the StatefulSet on trust changes — rolling a + quorum-sensitive workload on trust bytes is a separate decision. +- A malformed bundle (any non-CERTIFICATE or unparseable PEM block, or zero + certificates) fails the reconcile and the composed ConfigMap is **not** + updated. etcd itself hard-errors on any bad block in its CA file, so shipping + it would crash-loop members at their next restart; keeping the last good + composition is the safer failure mode. + +## TLS is create-time + +TLS configuration flows into the pod template and cert mounts. Toggling it 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. + +## etcd RBAC is out of scope + +The operator secures etcd with mTLS only. etcd's own auth system (`auth +enable`, users/roles, cert-CN-as-username) is **not supported for v1alpha1**: +the operator dials with a client certificate and no user identity, so enabling +etcd RBAC on a managed cluster breaks reconciliation (membership and +maintenance RPCs require root once auth is on). Access control is the CA +boundary described above. A future `spec.auth` block would be purely additive. diff --git a/internal/controller/etcdcluster_controller.go b/internal/controller/etcdcluster_controller.go index 3f1f1767..523a6e8a 100644 --- a/internal/controller/etcdcluster_controller.go +++ b/internal/controller/etcdcluster_controller.go @@ -354,6 +354,13 @@ func (r *EtcdClusterReconciler) reconcileClusterState(ctx context.Context, s *re } if targetReplica == int32(s.cluster.Spec.Size) { + // Steady state is the only path that skips reconcileStatefulSet, so + // re-apply the member certs and composed trust ConfigMaps here: a trust + // bundle edit (or issued-CA rotation) on a size-stable cluster must + // still recompose before the user's rollout restart picks it up. + if err := applyEtcdMemberCerts(ctx, s.cluster, r.Client); err != nil { + return ctrl.Result{}, err + } logger.Info("EtcdCluster is already up-to-date") return ctrl.Result{}, nil } diff --git a/internal/controller/tls_cel_test.go b/internal/controller/tls_cel_test.go index 18d3ec06..c16ce919 100644 --- a/internal/controller/tls_cel_test.go +++ b/internal/controller/tls_cel_test.go @@ -155,6 +155,22 @@ func TestTLSCELValidation(t *testing.T) { }, wantApply: false, }, + { + name: "trust bundle ref accepted", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + TrustBundleConfigMapRef: &ecv1alpha1.TrustBundleConfigMapRef{Name: "extra-cas"}, + }, + wantApply: true, + }, + { + name: "trust bundle ref with empty name rejected", + surface: ecv1alpha1.TLSSurface{ + Provider: ecv1alpha1.TLSProviderAuto, + TrustBundleConfigMapRef: &ecv1alpha1.TrustBundleConfigMapRef{Name: ""}, + }, + wantApply: false, + }, } for i, tt := range tests { diff --git a/internal/controller/tls_trustbundle_test.go b/internal/controller/tls_trustbundle_test.go new file mode 100644 index 00000000..f24d1404 --- /dev/null +++ b/internal/controller/tls_trustbundle_test.go @@ -0,0 +1,211 @@ +package controller + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// bundledSurface returns a cert-manager surface that also requests the trust +// bundle from the "extra-cas" ConfigMap. +func bundledSurface() *ecv1alpha1.TLSSurface { + s := cmSurface(nil) + s.TrustBundleConfigMapRef = &ecv1alpha1.TrustBundleConfigMapRef{Name: "extra-cas"} + return s +} + +func trustFixtures(t *testing.T) (issuedCA, bundleCA []byte) { + t.Helper() + _, _, issuedCA = genClientKeypair(t) + _, _, bundleCA = genClientKeypair(t) + return issuedCA, bundleCA +} + +func TestValidateTrustBundlePEM(t *testing.T) { + _, validCA := trustFixtures(t) + + tests := []struct { + name string + data string + wantErr bool + }{ + {"single CA accepted", string(validCA), false}, + {"two CAs accepted", string(validCA) + string(validCA), false}, + {"empty rejected", "", true}, + {"no certificates rejected", "just text\n", true}, + {"corrupt certificate block rejected", + "-----BEGIN CERTIFICATE-----\nnotbase64!!!\n-----END CERTIFICATE-----\n", true}, + {"non-certificate PEM block rejected", + "-----BEGIN PRIVATE KEY-----\nMIGH\n-----END PRIVATE KEY-----\n", true}, + {"valid cert with trailing garbage rejected", string(validCA) + "trailing", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateTrustBundlePEM([]byte(tt.data)) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestApplyTrustBundlesComposition(t *testing.T) { + issuedCA, bundleCA := trustFixtures(t) + + ec := clusterWithTLS(&ecv1alpha1.EtcdClusterTLS{Client: bundledSurface()}) + certSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getServerCertName(ec.Name), Namespace: ec.Namespace}, + Data: map[string][]byte{tlsCAFile: issuedCA}, + } + userCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "extra-cas", Namespace: ec.Namespace}, + Data: map[string]string{trustBundleKey: string(bundleCA)}, + } + c := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(certSecret, userCM).Build() + + require.NoError(t, applyTrustBundles(t.Context(), ec, c)) + + composed := &corev1.ConfigMap{} + require.NoError(t, c.Get(t.Context(), + client.ObjectKey{Name: getServerTrustName(ec.Name), Namespace: ec.Namespace}, composed)) + got := composed.Data[trustBundleKey] + assert.True(t, strings.HasPrefix(got, strings.TrimRight(string(issuedCA), "\n")+"\n"), + "composed bundle must start with the issued CA") + assert.True(t, strings.HasSuffix(got, string(bundleCA)), + "composed bundle must end with the user bundle") + assert.NotEmpty(t, composed.OwnerReferences, "composed ConfigMap must be owned by the EtcdCluster") +} + +func TestApplyTrustBundlesRecomposeOnRotation(t *testing.T) { + issuedCA, bundleCA := trustFixtures(t) + _, _, rotatedCA := genClientKeypair(t) + + ec := clusterWithTLS(&ecv1alpha1.EtcdClusterTLS{Client: bundledSurface()}) + certSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getServerCertName(ec.Name), Namespace: ec.Namespace}, + Data: map[string][]byte{tlsCAFile: issuedCA}, + } + userCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "extra-cas", Namespace: ec.Namespace}, + Data: map[string]string{trustBundleKey: string(bundleCA)}, + } + c := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(certSecret, userCM).Build() + require.NoError(t, applyTrustBundles(t.Context(), ec, c)) + + // Rotate the issued CA; the next reconcile must recompose. + certSecret.Data[tlsCAFile] = rotatedCA + require.NoError(t, c.Update(t.Context(), certSecret)) + require.NoError(t, applyTrustBundles(t.Context(), ec, c)) + + composed := &corev1.ConfigMap{} + require.NoError(t, c.Get(t.Context(), + client.ObjectKey{Name: getServerTrustName(ec.Name), Namespace: ec.Namespace}, composed)) + assert.Contains(t, composed.Data[trustBundleKey], strings.TrimRight(string(rotatedCA), "\n"), + "composed bundle must pick up the rotated issued CA") + assert.NotContains(t, composed.Data[trustBundleKey], strings.TrimRight(string(issuedCA), "\n"), + "composed bundle must not retain the pre-rotation CA") +} + +func TestApplyTrustBundlesFailureModes(t *testing.T) { + issuedCA, bundleCA := trustFixtures(t) + + newEC := func() *ecv1alpha1.EtcdCluster { + return clusterWithTLS(&ecv1alpha1.EtcdClusterTLS{Client: bundledSurface()}) + } + certSecret := func(ec *ecv1alpha1.EtcdCluster) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: getServerCertName(ec.Name), Namespace: ec.Namespace}, + Data: map[string][]byte{tlsCAFile: issuedCA}, + } + } + + t.Run("missing user ConfigMap is an explicit error", func(t *testing.T) { + ec := newEC() + c := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(certSecret(ec)).Build() + assert.Error(t, applyTrustBundles(t.Context(), ec, c)) + }) + + t.Run("invalid bundle errors and preserves the last good composition", func(t *testing.T) { + ec := newEC() + lastGood := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: getServerTrustName(ec.Name), Namespace: ec.Namespace}, + Data: map[string]string{trustBundleKey: string(issuedCA) + string(bundleCA)}, + } + badUserCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "extra-cas", Namespace: ec.Namespace}, + Data: map[string]string{trustBundleKey: "-----BEGIN CERTIFICATE-----\nbad!\n-----END CERTIFICATE-----\n"}, + } + c := fake.NewClientBuilder().WithScheme(newScheme(t)). + WithObjects(certSecret(ec), lastGood, badUserCM).Build() + + err := applyTrustBundles(t.Context(), ec, c) + require.Error(t, err) + assert.Contains(t, err.Error(), "trust bundle invalid") + + preserved := &corev1.ConfigMap{} + require.NoError(t, c.Get(t.Context(), + client.ObjectKey{Name: getServerTrustName(ec.Name), Namespace: ec.Namespace}, preserved)) + assert.Equal(t, lastGood.Data, preserved.Data, "a bad bundle must not clobber the last good composition") + }) + + t.Run("bundle ConfigMap missing the ca.crt key is rejected", func(t *testing.T) { + ec := newEC() + userCM := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "extra-cas", Namespace: ec.Namespace}, + Data: map[string]string{"wrong-key": string(bundleCA)}, + } + c := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(certSecret(ec), userCM).Build() + assert.Error(t, applyTrustBundles(t.Context(), ec, c)) + }) +} + +// TestTrustBundleArgsAndMounts asserts the trust-bundle wiring flips exactly the +// --*trusted-ca-file flag of the requesting surface and that output without a +// bundle stays byte-identical to the bundle-free path. +func TestTrustBundleArgsAndMounts(t *testing.T) { + t.Run("no bundle output is byte-identical", func(t *testing.T) { + plain := createArgs("ec", nil, tlsArgs{peerEnabled: true, clientEnabled: true, peerCertAuth: true, clientCertAuth: true}) + viaSurfaces := createArgs("ec", nil, tlsArgsFor(clusterWithTLS( + &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: cmSurface(nil)}))) + assert.Equal(t, plain, viaSurfaces) + }) + + t.Run("client bundle flips only --trusted-ca-file", func(t *testing.T) { + args := createArgs("ec", nil, tlsArgsFor(clusterWithTLS( + &ecv1alpha1.EtcdClusterTLS{Peer: cmSurface(nil), Client: bundledSurface()}))) + assert.Contains(t, args, "--trusted-ca-file="+serverTrustMountPath+"/"+tlsCAFile) + assert.Contains(t, args, "--peer-trusted-ca-file="+peerCertMountPath+"/"+tlsCAFile) + assert.Contains(t, args, "--cert-file="+serverCertMountPath+"/"+tlsCertFile, + "the cert/key flags must keep pointing at the secret mount") + }) + + t.Run("peer bundle flips only --peer-trusted-ca-file", func(t *testing.T) { + args := createArgs("ec", nil, tlsArgsFor(clusterWithTLS( + &ecv1alpha1.EtcdClusterTLS{Peer: bundledSurface(), Client: cmSurface(nil)}))) + assert.Contains(t, args, "--peer-trusted-ca-file="+peerTrustMountPath+"/"+tlsCAFile) + assert.Contains(t, args, "--trusted-ca-file="+serverCertMountPath+"/"+tlsCAFile) + }) + + t.Run("trust ConfigMap volume is mounted per requesting surface", func(t *testing.T) { + ec := clusterWithTLS(&ecv1alpha1.EtcdClusterTLS{ + Peer: cmSurface(nil), + Client: bundledSurface(), + }) + container, vols := stsContainer(t, ec) + vNames := volumeNames(vols) + mNames := mountNames(container.VolumeMounts) + assert.Contains(t, vNames, serverTrustVolumeName, "server trust volume") + assert.Contains(t, mNames, serverTrustVolumeName, "server trust mount") + assert.NotContains(t, vNames, peerTrustVolumeName, "no peer trust volume without a peer bundle") + }) +} diff --git a/internal/controller/trustbundle.go b/internal/controller/trustbundle.go new file mode 100644 index 00000000..e37cc3c0 --- /dev/null +++ b/internal/controller/trustbundle.go @@ -0,0 +1,152 @@ +package controller + +import ( + "bytes" + "context" + "crypto/x509" + "encoding/pem" + "fmt" + + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1" +) + +// Trust-bundle mechanics. etcd takes exactly ONE --trusted-ca-file per surface, +// so "append a user bundle to the trusted CAs" is implemented by composing a +// per-surface ConfigMap: issued CA (from the cert secret) + "\n" + user bundle, +// re-composed every reconcile so cert-secret CA rotation is picked up. The +// composed ConfigMap is mounted next to the cert secret and the surface's +// --*trusted-ca-file flag points at it instead of the secret's ca.crt. +// +// NOTE: etcd builds its trust pools once at process start (only the member's +// own keypair hot-reloads), so composition keeps the FILE current but a member +// only honors trust changes after its next restart. Deliberately no automatic +// StatefulSet roll on trust change -- rolling a quorum-sensitive workload on +// trust bytes is its own behavior change. + +const ( + serverTrustVolumeName = "server-trust" + peerTrustVolumeName = "peer-trust" + + serverTrustMountPath = "/etc/etcd/server-trust" + peerTrustMountPath = "/etc/etcd/peer-trust" + + // trustBundleKey is the fixed ConfigMap key for both the user bundle and + // the composed output. + trustBundleKey = "ca.crt" +) + +func getServerTrustName(etcdClusterName string) string { + return fmt.Sprintf("%s-server-trusted-ca", etcdClusterName) +} + +func getPeerTrustName(etcdClusterName string) string { + return fmt.Sprintf("%s-peer-trusted-ca", etcdClusterName) +} + +// trustBundleEnabled reports whether a surface requests an additional trust +// bundle. +func trustBundleEnabled(s *ecv1alpha1.TLSSurface) bool { + return s != nil && s.TrustBundleConfigMapRef != nil +} + +// validateTrustBundlePEM strictly validates a user-supplied trust bundle: every +// PEM block must be a parseable certificate and at least one certificate must +// be present. etcd's own CA-file loader (tlsutil.NewCertPool) hard-errors on +// any bad block, so a lenient check here (e.g. AppendCertsFromPEM, which skips +// bad blocks) would admit a bundle that crash-loops members at their next +// restart. +func validateTrustBundlePEM(data []byte) error { + rest := data + count := 0 + for { + var block *pem.Block + block, rest = pem.Decode(rest) + if block == nil { + break + } + if block.Type != "CERTIFICATE" { + return fmt.Errorf("trust bundle contains a %q PEM block; only CERTIFICATE blocks are allowed", block.Type) + } + if _, err := x509.ParseCertificate(block.Bytes); err != nil { + return fmt.Errorf("trust bundle contains an unparseable certificate: %w", err) + } + count++ + } + if count == 0 { + return fmt.Errorf("trust bundle contains no certificates") + } + if len(bytes.TrimSpace(rest)) != 0 { + return fmt.Errorf("trust bundle contains trailing non-PEM data") + } + return nil +} + +// reconcileTrustConfigMap composes the surface's trusted-CA ConfigMap from the +// issued cert secret's ca.crt and the user bundle. An invalid or missing user +// bundle is an error and the composed ConfigMap is NOT written, preserving the +// last good trust set. +func reconcileTrustConfigMap(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client, + surface *ecv1alpha1.TLSSurface, certSecretName, trustCMName string) error { + userCM := &corev1.ConfigMap{} + userCMKey := client.ObjectKey{Name: surface.TrustBundleConfigMapRef.Name, Namespace: ec.Namespace} + if err := c.Get(ctx, userCMKey, userCM); err != nil { + return fmt.Errorf("trust bundle ConfigMap %s/%s: %w", userCMKey.Namespace, userCMKey.Name, err) + } + bundle, ok := userCM.Data[trustBundleKey] + if !ok || len(bundle) == 0 { + return fmt.Errorf("trust bundle invalid: ConfigMap %s/%s missing key %q", userCMKey.Namespace, userCMKey.Name, trustBundleKey) + } + if err := validateTrustBundlePEM([]byte(bundle)); err != nil { + return fmt.Errorf("trust bundle invalid: ConfigMap %s/%s: %w", userCMKey.Namespace, userCMKey.Name, err) + } + + certSecret := &corev1.Secret{} + if err := c.Get(ctx, client.ObjectKey{Name: certSecretName, Namespace: ec.Namespace}, certSecret); err != nil { + return fmt.Errorf("cert secret %s/%s for trust composition: %w", ec.Namespace, certSecretName, err) + } + issuedCA, ok := certSecret.Data[tlsCAFile] + if !ok || len(issuedCA) == 0 { + return fmt.Errorf("cert secret %s/%s missing %q; cannot compose trust bundle", ec.Namespace, certSecretName, tlsCAFile) + } + + composed := string(bytes.TrimRight(issuedCA, "\n")) + "\n" + bundle + + trustCM := &corev1.ConfigMap{} + trustCM.Name = trustCMName + trustCM.Namespace = ec.Namespace + _, err := controllerutil.CreateOrUpdate(ctx, c, trustCM, func() error { + trustCM.Data = map[string]string{trustBundleKey: composed} + return controllerutil.SetControllerReference(ec, trustCM, c.Scheme()) + }) + if err != nil { + return fmt.Errorf("failed to reconcile trust ConfigMap %s/%s: %w", ec.Namespace, trustCMName, err) + } + return nil +} + +// applyTrustBundles composes the per-surface trust ConfigMaps for every surface +// that requests one. Runs after the member certs are provisioned (the composed +// output embeds the issued CA) and before the StatefulSet references the +// ConfigMaps. +func applyTrustBundles(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c client.Client) error { + if ec.Spec.TLS == nil { + return nil + } + if clientTLSEnabled(ec) && trustBundleEnabled(ec.Spec.TLS.Client) { + if err := reconcileTrustConfigMap(ctx, ec, c, ec.Spec.TLS.Client, + getServerCertName(ec.Name), getServerTrustName(ec.Name)); err != nil { + return err + } + } + if peerTLSEnabled(ec) && trustBundleEnabled(ec.Spec.TLS.Peer) { + if err := reconcileTrustConfigMap(ctx, ec, c, ec.Spec.TLS.Peer, + getPeerCertName(ec.Name), getPeerTrustName(ec.Name)); err != nil { + return err + } + } + return nil +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 521503cd..33403ccb 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -206,10 +206,12 @@ func reconcileStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1alpha // 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 + peerEnabled bool + clientEnabled bool + peerCertAuth bool + clientCertAuth bool + peerTrustBundle bool + clientTrustBundle bool } // tlsArgsFor derives tlsArgs from an EtcdCluster's two TLS surfaces. @@ -220,6 +222,8 @@ func tlsArgsFor(ec *ecv1alpha1.EtcdCluster) tlsArgs { a.clientEnabled = ec.Spec.TLS.Client != nil a.peerCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Peer) a.clientCertAuth = clientCertAuthEnabled(ec.Spec.TLS.Client) + a.peerTrustBundle = trustBundleEnabled(ec.Spec.TLS.Peer) + a.clientTrustBundle = trustBundleEnabled(ec.Spec.TLS.Client) } return a } @@ -242,12 +246,19 @@ func defaultArgs(name string, tls tlsArgs) []string { 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. + // Server (client-surface) TLS flag group: emitted iff the client surface is + // set. With a trust bundle, --trusted-ca-file points at the composed trust + // ConfigMap mount instead of the secret's ca.crt; without one the output is + // byte-identical to the bundle-free path. if tls.clientEnabled { + clientTrustPath := serverCertMountPath + if tls.clientTrustBundle { + clientTrustPath = serverTrustMountPath + } 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), + fmt.Sprintf("--trusted-ca-file=%s/%s", clientTrustPath, tlsCAFile), ) if tls.clientCertAuth { args = append(args, "--client-cert-auth") @@ -256,10 +267,14 @@ func defaultArgs(name string, tls tlsArgs) []string { // Peer TLS flag group: emitted iff the peer surface is set. if tls.peerEnabled { + peerTrustPath := peerCertMountPath + if tls.peerTrustBundle { + peerTrustPath = peerTrustMountPath + } 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), + fmt.Sprintf("--peer-trusted-ca-file=%s/%s", peerTrustPath, tlsCAFile), ) if tls.peerCertAuth { args = append(args, "--peer-client-cert-auth") @@ -390,6 +405,21 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a MountPath: serverCertMountPath, ReadOnly: true, }) + if trustBundleEnabled(ec.Spec.TLS.Client) { + certVolume = append(certVolume, corev1.Volume{ + Name: serverTrustVolumeName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: getServerTrustName(ec.Name)}, + }, + }, + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: serverTrustVolumeName, + MountPath: serverTrustMountPath, + ReadOnly: true, + }) + } } if peerTLSEnabled(ec) { certVolume = append(certVolume, corev1.Volume{ @@ -403,6 +433,21 @@ func createOrPatchStatefulSet(ctx context.Context, logger logr.Logger, ec *ecv1a MountPath: peerCertMountPath, ReadOnly: true, }) + if trustBundleEnabled(ec.Spec.TLS.Peer) { + certVolume = append(certVolume, corev1.Volume{ + Name: peerTrustVolumeName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: getPeerTrustName(ec.Name)}, + }, + }, + }) + certMounts = append(certMounts, corev1.VolumeMount{ + Name: peerTrustVolumeName, + MountPath: peerTrustMountPath, + ReadOnly: true, + }) + } } if len(certVolume) != 0 { podSpec.Volumes = certVolume @@ -996,7 +1041,10 @@ func applyEtcdMemberCerts(ctx context.Context, ec *ecv1alpha1.EtcdCluster, c cli return err } } - return nil + // Compose the per-surface trust ConfigMaps after the certs exist (the + // composed output embeds each issued CA) and before the StatefulSet + // references them. + return applyTrustBundles(ctx, ec, c) } // buildClientTLSConfig builds the operator's etcd-client *tls.Config from the