fixup! runtime/secrets: add package for consolidated secret handling

TLSConfigFromSecret: support flexible certificate configurations

Implement review feedback from stefanprodan and matheuscscp:

- Support CA-only configuration for server verification
- Support cert+key-only for mTLS client authentication
- Support both CA and cert+key for full mTLS
- Add validation to error when cert exists without key or vice versa
- Simplify tlsCertificateData with method-based validation
- Update tests to handle different certificate scenarios dynamically

Signed-off-by: cappyzawa <[email protected]>

Author: cappyzawa

runtime/secrets/reader.go

@@ -36,6 +36,33 @@ type tlsCertificateData struct {
 	caCert []byte
 }
 
+func (t *tlsCertificateData) validate() error {
+	hasCert := len(t.cert) > 0
+	hasKey := len(t.key) > 0
+	hasCA := len(t.caCert) > 0
+
+	if hasCert != hasKey {
+		if hasCert {
+			return fmt.Errorf("found certificate but missing private key")
+		}
+		return fmt.Errorf("found private key but missing certificate")
+	}
+
+	if !hasCert && !hasCA {
+		return fmt.Errorf("no CA certificate or client certificate pair found")
+	}
+
+	return nil
+}
+
+func (t *tlsCertificateData) hasCertPair() bool {
+	return len(t.cert) > 0 && len(t.key) > 0
+}
+
+func (t *tlsCertificateData) hasCA() bool {
+	return len(t.caCert) > 0
+}
+
 // TLSConfigFromSecret creates a TLS configuration from a Kubernetes secret.
 //
 // The function looks for TLS certificate data in the secret using standard
@@ -165,37 +192,31 @@ func getSecret(ctx context.Context, c client.Client, name, namespace string) (*c
 }
 
 func getTLSCertificateData(secret *corev1.Secret, supportDeprecated bool) (*tlsCertificateData, error) {
-	tlsCert, err := getSecretData(secret, TLSCertKey, TLSCertFileKey, supportDeprecated)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get TLS certificate: %w", err)
+	data := &tlsCertificateData{
+		cert:   getSecretData(secret, TLSCertKey, TLSCertFileKey, supportDeprecated),
+		key:    getSecretData(secret, TLSKeyKey, TLSKeyFileKey, supportDeprecated),
+		caCert: getSecretData(secret, CACertKey, CACertFileKey, supportDeprecated),
 	}
 
-	tlsKey, err := getSecretData(secret, TLSKeyKey, TLSKeyFileKey, supportDeprecated)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get TLS private key: %w", err)
+	if err := data.validate(); err != nil {
+		return nil, err
 	}
 
-	// CA certificate is optional, ignore error if not found
-	caCert, _ := getSecretData(secret, CACertKey, CACertFileKey, supportDeprecated)
-
-	return &tlsCertificateData{
-		cert:   tlsCert,
-		key:    tlsKey,
-		caCert: caCert,
-	}, nil
+	return data, nil
 }
 
 func buildTLSConfig(certData *tlsCertificateData) (*tls.Config, error) {
-	cert, err := tls.X509KeyPair(certData.cert, certData.key)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse TLS certificate and key: %w", err)
-	}
+	tlsConfig := &tls.Config{}
 
-	tlsConfig := &tls.Config{
-		Certificates: []tls.Certificate{cert},
+	if certData.hasCertPair() {
+		cert, err := tls.X509KeyPair(certData.cert, certData.key)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse TLS certificate and key: %w", err)
+		}
+		tlsConfig.Certificates = []tls.Certificate{cert}
 	}
 
-	if len(certData.caCert) > 0 {
+	if certData.hasCA() {
 		caCertPool := x509.NewCertPool()
 		if !caCertPool.AppendCertsFromPEM(certData.caCert) {
 			return nil, fmt.Errorf("failed to parse CA certificate")
@@ -206,16 +227,16 @@ func buildTLSConfig(certData *tlsCertificateData) (*tls.Config, error) {
 	return tlsConfig, nil
 }
 
-func getSecretData(secret *corev1.Secret, key, fallbackKey string, supportDeprecated bool) ([]byte, error) {
+func getSecretData(secret *corev1.Secret, key, fallbackKey string, supportDeprecated bool) []byte {
 	if data, exists := secret.Data[key]; exists {
-		return data, nil
+		return data
 	}
 
 	if supportDeprecated {
 		if data, exists := secret.Data[fallbackKey]; exists {
-			return data, nil
+			return data
 		}
 	}
 
-	return nil, &KeyNotFoundError{Key: key}
+	return nil
 }

runtime/secrets/reader_test.go

@@ -217,73 +217,96 @@ func TestTLSConfigFromSecret(t *testing.T) {
 			errMsg: "secret default/tls-secret not found",
 		},
 		{
-			name: "missing certificate",
+			name: "deprecated fields without option",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-secret",
 					Namespace: testNS,
 				},
 				Data: map[string][]byte{
-					secrets.TLSKeyKey: tlsKey,
+					secrets.TLSCertFileKey: tlsCert,
+					secrets.TLSKeyFileKey:  tlsKey,
 				},
 			},
-			errMsg: "failed to get TLS certificate",
+			errMsg: "no CA certificate or client certificate pair found",
 		},
 		{
-			name: "missing private key",
+			name: "invalid certificate data",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-secret",
 					Namespace: testNS,
 				},
 				Data: map[string][]byte{
-					secrets.TLSCertKey: tlsCert,
+					secrets.TLSCertKey: []byte("invalid-cert-data"),
+					secrets.TLSKeyKey:  []byte("invalid-key-data"),
 				},
 			},
-			errMsg: "failed to get TLS private key",
+			errMsg: "failed to parse TLS certificate and key",
 		},
 		{
-			name: "deprecated fields without option",
+			name: "invalid CA certificate",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-secret",
 					Namespace: testNS,
 				},
 				Data: map[string][]byte{
-					secrets.TLSCertFileKey: tlsCert,
-					secrets.TLSKeyFileKey:  tlsKey,
+					secrets.TLSCertKey: tlsCert,
+					secrets.TLSKeyKey:  tlsKey,
+					secrets.CACertKey:  []byte("invalid-ca-data"),
 				},
 			},
-			errMsg: `key 'tls.crt' not found in secret`,
+			errMsg: "failed to parse CA certificate",
 		},
 		{
-			name: "invalid certificate data",
+			name: "CA certificate only",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-secret",
 					Namespace: testNS,
 				},
 				Data: map[string][]byte{
-					secrets.TLSCertKey: []byte("invalid-cert-data"),
-					secrets.TLSKeyKey:  []byte("invalid-key-data"),
+					secrets.CACertKey: caCert,
 				},
 			},
-			errMsg: "failed to parse TLS certificate and key",
 		},
 		{
-			name: "invalid CA certificate",
+			name: "certificate without key",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "tls-secret",
 					Namespace: testNS,
 				},
 				Data: map[string][]byte{
 					secrets.TLSCertKey: tlsCert,
-					secrets.TLSKeyKey:  tlsKey,
-					secrets.CACertKey:  []byte("invalid-ca-data"),
 				},
 			},
-			errMsg: "failed to parse CA certificate",
+			errMsg: "found certificate but missing private key",
+		},
+		{
+			name: "key without certificate",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "tls-secret",
+					Namespace: testNS,
+				},
+				Data: map[string][]byte{
+					secrets.TLSKeyKey: tlsKey,
+				},
+			},
+			errMsg: "found private key but missing certificate",
+		},
+		{
+			name: "no certificates at all",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "tls-secret",
+					Namespace: testNS,
+				},
+				Data: map[string][]byte{},
+			},
+			errMsg: "no CA certificate or client certificate pair found",
 		},
 	}
 
@@ -303,11 +326,24 @@ func TestTLSConfigFromSecret(t *testing.T) {
 			} else {
 				g.Expect(err).ToNot(HaveOccurred())
 				g.Expect(tlsConfig).ToNot(BeNil())
-				g.Expect(tlsConfig.Certificates).To(HaveLen(1))
 
-				expectedCert, err := tls.X509KeyPair(tlsCert, tlsKey)
-				g.Expect(err).ToNot(HaveOccurred())
-				g.Expect(tlsConfig.Certificates[0]).To(Equal(expectedCert))
+				hasCert := len(tt.secret.Data[secrets.TLSCertKey]) > 0 || len(tt.secret.Data[secrets.TLSCertFileKey]) > 0
+				hasKey := len(tt.secret.Data[secrets.TLSKeyKey]) > 0 || len(tt.secret.Data[secrets.TLSKeyFileKey]) > 0
+				hasCertPair := hasCert && hasKey
+
+				if hasCertPair {
+					g.Expect(tlsConfig.Certificates).To(HaveLen(1))
+					expectedCert, err := tls.X509KeyPair(tlsCert, tlsKey)
+					g.Expect(err).ToNot(HaveOccurred())
+					g.Expect(tlsConfig.Certificates[0]).To(Equal(expectedCert))
+				} else {
+					g.Expect(tlsConfig.Certificates).To(BeEmpty())
+				}
+
+				hasCA := len(tt.secret.Data[secrets.CACertKey]) > 0 || len(tt.secret.Data[secrets.CACertFileKey]) > 0
+				if hasCA {
+					g.Expect(tlsConfig.RootCAs).ToNot(BeNil())
+				}
 			}
 		})
 	}