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..729d130d --- /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) + + ipStrings := make([]string, 0, len(cert.IPAddresses)) + 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) +}