From 368f1b6d75382188b69457794c8e6b6f2eb62b88 Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Wed, 1 Jul 2026 22:40:32 -0400 Subject: [PATCH 1/2] fix: propagate user-supplied altNames.ipAddresses to certificates createCMCertificateConfig and createAutoCertificateConfig filled AltNames.IPs with nil placeholders sized by the DNS-name count and never read the user-supplied field, so spec.tls.*.providerCfg.*.altNames.ipAddresses was silently dropped from issued certificates. The cert-manager provider also stringified IPs via fmt.Sprint (rendering nil entries as literal ""), and the auto provider never passed IPs to transport.SelfCert at all. Pass the user-supplied IPs through on both the custom and default DNS-name config paths, reject nil/empty IP entries with an explicit error, build Certificate.Spec.IPAddresses from ip.String() in the cert-manager provider, and append the IPs to the transport.SelfCert hosts list in the auto provider so they land in the issued certificate's IP SANs. Tests assert the emitted Certificate spec (cert-manager) and the parsed x509 IP SANs (auto). Signed-off-by: Xavier Lange --- internal/controller/utils.go | 25 +++- internal/controller/utils_test.go | 110 +++++++++++++++++- pkg/certificate/auto/provider.go | 9 ++ pkg/certificate/auto/provider_test.go | 53 +++++++++ pkg/certificate/cert_manager/provider.go | 13 ++- pkg/certificate/cert_manager/provider_test.go | 45 +++++++ 6 files changed, 249 insertions(+), 6 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 3092c379..fc63fb52 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -590,6 +590,17 @@ func parseValidityDuration(customizedDuration string, defaultDuration time.Durat return duration, nil } +// validateAltNameIPs rejects nil/empty entries so that invalid IP addresses +// never reach certificate providers. +func validateAltNameIPs(ips []net.IP) error { + for i, ip := range ips { + if len(ip) == 0 { + return fmt.Errorf("altNames.ipAddresses[%d] is not a valid IP address", i) + } + } + return nil +} + func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) { cmConfig := ec.Spec.TLS.ProviderCfg.CertManagerCfg if cmConfig == nil { @@ -602,11 +613,15 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Confi return nil, err } + if err := validateAltNameIPs(cmConfig.AltNames.IPs); err != nil { + 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)), + IPs: cmConfig.AltNames.IPs, } } else { // Use wildcard DNS for the cluster's headless service to cover all pods @@ -617,6 +632,7 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Confi } getAltNames = certInterface.AltNames{ DNSNames: defaultDNSNames, + IPs: cmConfig.AltNames.IPs, } } @@ -651,11 +667,15 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Con return nil, err } + if err := validateAltNameIPs(autoConfig.AltNames.IPs); err != nil { + 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)), + IPs: autoConfig.AltNames.IPs, } } else { // Use wildcard DNS for the cluster's headless service to cover all pods @@ -666,6 +686,7 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Con } altNames = certInterface.AltNames{ DNSNames: defaultDNSNames, + IPs: autoConfig.AltNames.IPs, } } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 67592316..dd96eee0 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -931,6 +931,7 @@ func TestCreateAutoCertificateConfig(t *testing.T) { ValidityDuration: "720h", // 30 days AltNames: ecv1alpha1.AltNames{ DNSNames: []string{"custom1.example.com", "custom2.example.com"}, + IPs: []net.IP{net.ParseIP("10.96.99.99")}, }, }, }, @@ -944,11 +945,73 @@ func TestCreateAutoCertificateConfig(t *testing.T) { ValidityDuration: 720 * time.Hour, // 30 days AltNames: certInterface.AltNames{ DNSNames: []string{"custom1.example.com", "custom2.example.com"}, - IPs: make([]net.IP, 2), + IPs: []net.IP{net.ParseIP("10.96.99.99")}, }, }, wantErr: false, }, + { + name: "auto config with only ipAddresses - IPs kept alongside default DNS names", + ec: &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + 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", + AltNames: ecv1alpha1.AltNames{ + IPs: []net.IP{net.ParseIP("10.96.99.99")}, + }, + }, + }, + }, + }, + }, + }, + expected: &certInterface.Config{ + CommonName: "custom.example.com", + ValidityDuration: certInterface.DefaultAutoValidity, + AltNames: certInterface.AltNames{ + DNSNames: []string{ + "*.test-cluster.test-namespace.svc.cluster.local", + "test-cluster.test-namespace.svc.cluster.local", + }, + IPs: []net.IP{net.ParseIP("10.96.99.99")}, + }, + }, + wantErr: false, + }, + { + name: "auto config with invalid IP entry", + ec: &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + 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", + AltNames: ecv1alpha1.AltNames{ + IPs: []net.IP{nil}, + }, + }, + }, + }, + }, + }, + }, + expected: nil, + wantErr: true, + }, { name: "auto config with nil AutoCfg - should use defaults", ec: &ecv1alpha1.EtcdCluster{ @@ -1025,6 +1088,7 @@ func TestCreateCMCertificateConfig(t *testing.T) { ValidityDuration: "1440h", // 60 days AltNames: ecv1alpha1.AltNames{ DNSNames: []string{"cm1.example.com", "cm2.example.com"}, + IPs: []net.IP{net.ParseIP("10.96.99.99")}, }, }, IssuerName: "test-issuer", @@ -1040,7 +1104,49 @@ func TestCreateCMCertificateConfig(t *testing.T) { ValidityDuration: 1440 * time.Hour, // 60 days AltNames: certInterface.AltNames{ DNSNames: []string{"cm1.example.com", "cm2.example.com"}, - IPs: make([]net.IP, 2), + IPs: []net.IP{net.ParseIP("10.96.99.99")}, + }, + ExtraConfig: map[string]any{ + "issuerName": "test-issuer", + "issuerKind": "ClusterIssuer", + }, + }, + wantErr: false, + }, + { + name: "cert-manager config with only ipAddresses - IPs kept alongside default DNS names", + ec: &ecv1alpha1.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + 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", + AltNames: ecv1alpha1.AltNames{ + IPs: []net.IP{net.ParseIP("10.96.99.99")}, + }, + }, + IssuerName: "test-issuer", + IssuerKind: "ClusterIssuer", + }, + }, + }, + }, + }, + expected: &certInterface.Config{ + CommonName: "cm.example.com", + ValidityDuration: certInterface.DefaultCertManagerValidity, + AltNames: certInterface.AltNames{ + DNSNames: []string{ + "*.test-cluster.test-namespace.svc.cluster.local", + "test-cluster.test-namespace.svc.cluster.local", + }, + IPs: []net.IP{net.ParseIP("10.96.99.99")}, }, ExtraConfig: map[string]any{ "issuerName": "test-issuer", diff --git a/pkg/certificate/auto/provider.go b/pkg/certificate/auto/provider.go index cf7c7526..de332beb 100644 --- a/pkg/certificate/auto/provider.go +++ b/pkg/certificate/auto/provider.go @@ -282,6 +282,15 @@ func (ac *Provider) createNewSecret(ctx context.Context, secretKey client.Object hosts = append(hosts, hostPort) } } + // transport.SelfCert classifies IP-shaped hosts into the certificate's + // IPAddresses. Nil entries are rejected by the controller before + // reaching providers; skip them defensively here. + for _, ip := range cfg.AltNames.IPs { + if ip == nil { + continue + } + hosts = append(hosts, net.JoinHostPort(ip.String(), "5500")) + } } log.Printf("calling SelfCert with hosts: %v", hosts) diff --git a/pkg/certificate/auto/provider_test.go b/pkg/certificate/auto/provider_test.go new file mode 100644 index 00000000..eed66bf6 --- /dev/null +++ b/pkg/certificate/auto/provider_test.go @@ -0,0 +1,53 @@ +package auto + +import ( + "context" + "crypto/x509" + "encoding/pem" + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + 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" +) + +func TestCreateNewSecretIncludesIPSANs(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + ac := &Provider{Client: c} + + secretKey := client.ObjectKey{Name: "test-cert", Namespace: "test-ns"} + cfg := &interfaces.Config{ + CommonName: "test.example.com", + ValidityDuration: 365 * 24 * time.Hour, + AltNames: interfaces.AltNames{ + DNSNames: []string{"test.example.com"}, + IPs: []net.IP{net.ParseIP("10.96.99.99")}, + }, + } + + require.NoError(t, ac.createNewSecret(context.Background(), secretKey, cfg)) + + var secret corev1.Secret + require.NoError(t, c.Get(context.Background(), secretKey, &secret)) + + block, _ := pem.Decode(secret.Data[corev1.TLSCertKey]) + require.NotNil(t, block) + cert, err := x509.ParseCertificate(block.Bytes) + require.NoError(t, err) + + var ipStrings []string + for _, ip := range cert.IPAddresses { + ipStrings = append(ipStrings, ip.String()) + } + assert.Contains(t, ipStrings, "10.96.99.99") + assert.Contains(t, cert.DNSNames, "test.example.com") +} diff --git a/pkg/certificate/cert_manager/provider.go b/pkg/certificate/cert_manager/provider.go index 84bf2c27..ebf16840 100644 --- a/pkg/certificate/cert_manager/provider.go +++ b/pkg/certificate/cert_manager/provider.go @@ -13,7 +13,6 @@ import ( "fmt" "log" "net" - "strings" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -321,6 +320,16 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey return fmt.Errorf("value for %s not correctly provided, try again", IssuerKindKey) } + var ipAddresses []string + for _, ip := range cfg.AltNames.IPs { + // Nil entries are rejected by the controller before reaching + // providers; skip them defensively here. + if ip == nil { + continue + } + ipAddresses = append(ipAddresses, ip.String()) + } + certificateResource := &certmanagerv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ Name: secretKey.Name, @@ -333,7 +342,7 @@ func (cm *CertManagerProvider) createCertificate(ctx context.Context, secretKey }, SecretName: secretKey.Name, DNSNames: cfg.AltNames.DNSNames, - IPAddresses: strings.Fields(strings.Trim(fmt.Sprint(cfg.AltNames.IPs), "[]")), + IPAddresses: ipAddresses, IssuerRef: cmmeta.IssuerReference{ Name: issuerName, Kind: issuerKind, diff --git a/pkg/certificate/cert_manager/provider_test.go b/pkg/certificate/cert_manager/provider_test.go new file mode 100644 index 00000000..a56ccdda --- /dev/null +++ b/pkg/certificate/cert_manager/provider_test.go @@ -0,0 +1,45 @@ +package cert_manager + +import ( + "context" + "net" + "testing" + "time" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "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 TestCreateCertificatePropagatesIPAddresses(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, certmanagerv1.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + cm := &CertManagerProvider{c} + + secretKey := client.ObjectKey{Name: "test-cert", Namespace: "test-ns"} + cfg := &interfaces.Config{ + CommonName: "test.example.com", + ValidityDuration: 90 * 24 * time.Hour, + AltNames: interfaces.AltNames{ + DNSNames: []string{"test.example.com"}, + IPs: []net.IP{net.ParseIP("10.96.99.99"), net.ParseIP("192.168.1.10")}, + }, + ExtraConfig: map[string]any{ + IssuerNameKey: "test-issuer", + IssuerKindKey: "ClusterIssuer", + }, + } + + require.NoError(t, cm.createCertificate(context.Background(), secretKey, cfg)) + + cert := &certmanagerv1.Certificate{} + require.NoError(t, c.Get(context.Background(), secretKey, cert)) + assert.Equal(t, []string{"10.96.99.99", "192.168.1.10"}, cert.Spec.IPAddresses) + assert.Equal(t, []string{"test.example.com"}, cert.Spec.DNSNames) +} From 0f5b174294d66726080eb9cfa3bd7419989001ab Mon Sep 17 00:00:00 2001 From: Xavier Lange Date: Thu, 2 Jul 2026 21:29:08 -0400 Subject: [PATCH 2/2] test: preallocate ipStrings to satisfy prealloc lint Signed-off-by: Xavier Lange --- pkg/certificate/auto/provider_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/certificate/auto/provider_test.go b/pkg/certificate/auto/provider_test.go index eed66bf6..729d130d 100644 --- a/pkg/certificate/auto/provider_test.go +++ b/pkg/certificate/auto/provider_test.go @@ -44,7 +44,7 @@ func TestCreateNewSecretIncludesIPSANs(t *testing.T) { cert, err := x509.ParseCertificate(block.Bytes) require.NoError(t, err) - var ipStrings []string + ipStrings := make([]string, 0, len(cert.IPAddresses)) for _, ip := range cert.IPAddresses { ipStrings = append(ipStrings, ip.String()) }