Merge pull request #253 from ArkaSaha30/fix-cert-duration-check

Handle invalid ValidityDuration user input

Author: ahrtr

internal/controller/utils.go

@@ -566,11 +566,27 @@ func getPeerCertName(etcdClusterName string) string {
 	return peerCertName
 }
 
-func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Config {
+// 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
+}
+
+func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) {
 	cmConfig := ec.Spec.TLS.ProviderCfg.CertManagerCfg
-	duration, err := time.ParseDuration(cmConfig.ValidityDuration)
+
+	// Set default duration to 90 days for cert-manager if not provided
+	duration, err := parseValidityDuration(cmConfig.ValidityDuration, certInterface.DefaultCertManagerValidity)
 	if err != nil {
-		log.Printf("Failed to parse ValidityDuration: %s", err)
+		return nil, err
 	}
 
 	var getAltNames certInterface.AltNames
@@ -580,7 +596,12 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Config
 			IPs:      make([]net.IP, len(cmConfig.AltNames.DNSNames)),
 		}
 	} else {
-		defaultDNSNames := []string{fmt.Sprintf("%s.svc.cluster.local", cmConfig.CommonName)}
+		// 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.svc.cluster.local", ec.Name, ec.Namespace),
+			fmt.Sprintf("%s.%s.svc.cluster.local", ec.Name, ec.Namespace),
+		}
 		getAltNames = certInterface.AltNames{
 			DNSNames: defaultDNSNames,
 		}
@@ -596,14 +617,16 @@ func createCMCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Config
 			"issuerKind": cmConfig.IssuerKind,
 		},
 	}
-	return config
+	return config, nil
 }
 
-func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Config {
+func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) (*certInterface.Config, error) {
 	autoConfig := ec.Spec.TLS.ProviderCfg.AutoCfg
-	duration, err := time.ParseDuration(autoConfig.ValidityDuration)
+
+	// Set default duration to 365 days for auto provider if not provided
+	duration, err := parseValidityDuration(autoConfig.ValidityDuration, certInterface.DefaultAutoValidity)
 	if err != nil {
-		log.Printf("Failed to parse ValidityDuration: %s", err)
+		return nil, err
 	}
 
 	var altNames certInterface.AltNames
@@ -613,7 +636,12 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Conf
 			IPs:      make([]net.IP, len(autoConfig.AltNames.DNSNames)),
 		}
 	} else {
-		defaultDNSNames := []string{fmt.Sprintf("%s.svc.cluster.local", autoConfig.CommonName)}
+		// 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.svc.cluster.local", ec.Name, ec.Namespace),
+			fmt.Sprintf("%s.%s.svc.cluster.local", ec.Name, ec.Namespace),
+		}
 		altNames = certInterface.AltNames{
 			DNSNames: defaultDNSNames,
 		}
@@ -625,7 +653,7 @@ func createAutoCertificateConfig(ec *ecv1alpha1.EtcdCluster) *certInterface.Conf
 		ValidityDuration: duration,
 		AltNames:         altNames,
 	}
-	return config
+	return config, nil
 }
 
 func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client.Client, certName string) error {
@@ -647,17 +675,23 @@ func createCertificate(ec *ecv1alpha1.EtcdCluster, ctx context.Context, c client
 			secretKey := client.ObjectKey{Name: certName, Namespace: ec.Namespace}
 			switch {
 			case ec.Spec.TLS.ProviderCfg.AutoCfg != nil:
-				autoConfig := createAutoCertificateConfig(ec)
+				autoConfig, err := createAutoCertificateConfig(ec)
+				if err != nil {
+					return fmt.Errorf("error creating auto certificate config: %w", err)
+				}
 				createCertErr := cert.EnsureCertificateSecret(ctx, secretKey, autoConfig)
 				if createCertErr != nil {
-					log.Printf("Error creating certificate: %s", createCertErr)
+					return fmt.Errorf("error creating auto certificate: %w", createCertErr)
 				}
 				return nil
 			case ec.Spec.TLS.ProviderCfg.CertManagerCfg != nil:
-				cmConfig := createCMCertificateConfig(ec)
+				cmConfig, err := createCMCertificateConfig(ec)
+				if err != nil {
+					return fmt.Errorf("error creating cert-manager certificate config: %w", err)
+				}
 				createCertErr := cert.EnsureCertificateSecret(ctx, secretKey, cmConfig)
 				if createCertErr != nil {
-					log.Printf("Error creating certificate: %s", createCertErr)
+					return fmt.Errorf("error creating cert-manager certificate: %w", createCertErr)
 				}
 				return nil
 			default:

pkg/certificate/auto/provider.go

@@ -28,7 +28,9 @@ import (
 )
 
 const (
-	DefaultValidity = 365 * 24 * time.Hour
+	// oneYearInHours is used to convert validity duration to years for transport.SelfCert.
+	// transport.SelfCert expects the validity parameter to be in years (uint).
+	oneYearInHours = 365 * 24 * time.Hour
 )
 
 type Provider struct {
@@ -218,9 +220,14 @@ func checkKeyPair(cert *x509.Certificate, privateKey crypto.PrivateKey) error {
 // https://github.com/etcd-io/etcd/blob/b87bc1c3a275d7d4904f4d201b963a2de2264f0d/client/pkg/transport/listener.go#L275
 func (ac *Provider) createNewSecret(ctx context.Context, secretKey client.ObjectKey,
 	cfg *interfaces.Config) error {
-	validity := DefaultValidity
+	validity := interfaces.DefaultAutoValidity
 	if cfg.ValidityDuration != 0 {
-		validity = cfg.ValidityDuration * time.Hour
+		validity = cfg.ValidityDuration
+	}
+
+	// Validate validity duration: minimum is 1 year as required by etcd
+	if validity < oneYearInHours {
+		return fmt.Errorf("validity duration must be at least 1 year (365 days), got: %v", validity)
 	}
 
 	tmpDir, err := os.MkdirTemp("", fmt.Sprintf("etcd-auto-cert-%s-*", secretKey.Name))
@@ -252,7 +259,15 @@ func (ac *Provider) createNewSecret(ctx context.Context, secretKey client.Object
 
 	log.Printf("calling SelfCert with hosts: %v", hosts)
 
-	tlsInfo, selfCertErr := transport.SelfCert(zap.NewNop(), tmpDir, hosts, uint(validity/DefaultValidity))
+	// Convert validity duration to years for transport.SelfCert
+	// transport.SelfCert expects the validity parameter to be in years (uint)
+	validityYears := uint(validity / oneYearInHours)
+	if validityYears == 0 {
+		// This should not happen due to the earlier check, but adding as a safeguard
+		return fmt.Errorf("validity duration converts to 0 years, must be at least 1 year")
+	}
+
+	tlsInfo, selfCertErr := transport.SelfCert(zap.NewNop(), tmpDir, hosts, validityYears)
 	if selfCertErr != nil {
 		return fmt.Errorf("certificate creation via transport.SelfCert failed: %w", selfCertErr)
 	}

pkg/certificate/interfaces/interface.go

@@ -46,6 +46,12 @@ const (
 	// with a delay of RetryInterval between consecutive retries
 	MaxRetries    = 36
 	RetryInterval = 5 * time.Second
+
+	// DefaultAutoValidity is the default validity duration for auto-generated certificates (365 days)
+	DefaultAutoValidity = 365 * 24 * time.Hour
+
+	// DefaultCertManagerValidity is the default validity duration for cert-manager certificates (90 days)
+	DefaultCertManagerValidity = 90 * 24 * time.Hour
 )
 
 // AltNames contains the domain names and IP addresses that will be added

test/e2e/auto_provider_test.go

@@ -29,7 +29,7 @@ import (
 const (
 	autoCertificateName      = "sample-cert"
 	autoCertificateNamespace = "default"
-	autoCertificateValidity  = auto.DefaultValidity
+	autoCertificateValidity  = interfaces.DefaultAutoValidity
 )
 
 func TestAutoProvider(t *testing.T) {

test/e2e/cert_manager_test.go

@@ -30,7 +30,7 @@ const (
 	cmIssuerType           = "ClusterIssuer"
 	cmCertificateName      = "sample-cert"
 	cmCertificateNamespace = "default"
-	cmCertificateValidity  = 24 * time.Hour
+	cmCertificateValidity  = interfaces.DefaultCertManagerValidity
 )
 
 var cmIssuer = &certv1.ClusterIssuer{