Merge pull request #597 from fluxcd/kube-tls-secret

Adopt Kubernetes style TLS Secret

Author: stefanprodan

api/v1beta2/provider_types.go

@@ -103,8 +103,11 @@ type ProviderSpec struct {
 	SecretRef *meta.LocalObjectReference `json:"secretRef,omitempty"`
 
 	// CertSecretRef specifies the Secret containing
-	// a PEM-encoded CA certificate (`caFile`).
+	// a PEM-encoded CA certificate (in the `ca.crt` key).
 	// +optional
+	//
+	// Note: Support for the `caFile` key has
+	// been deprecated.
 	CertSecretRef *meta.LocalObjectReference `json:"certSecretRef,omitempty"`
 
 	// Suspend tells the controller to suspend subsequent

config/crd/bases/notification.toolkit.fluxcd.io_providers.yaml

@@ -236,8 +236,9 @@ spec:
                 maxLength: 2048
                 type: string
               certSecretRef:
-                description: CertSecretRef specifies the Secret containing a PEM-encoded
-                  CA certificate (`caFile`).
+                description: "CertSecretRef specifies the Secret containing a PEM-encoded
+                  CA certificate (in the `ca.crt` key). \n Note: Support for the `caFile`
+                  key has been deprecated."
                 properties:
                   name:
                     description: Name of the referent.

docs/api/v1beta2/notification.md

@@ -372,7 +372,9 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference
 <td>
 <em>(Optional)</em>
 <p>CertSecretRef specifies the Secret containing
-a PEM-encoded CA certificate (<code>caFile</code>).</p>
+a PEM-encoded CA certificate (in the <code>ca.crt</code> key).</p>
+<p>Note: Support for the <code>caFile</code> key has
+been deprecated.</p>
 </td>
 </tr>
 <tr>
@@ -964,7 +966,9 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference
 <td>
 <em>(Optional)</em>
 <p>CertSecretRef specifies the Secret containing
-a PEM-encoded CA certificate (<code>caFile</code>).</p>
+a PEM-encoded CA certificate (in the <code>ca.crt</code> key).</p>
+<p>Note: Support for the <code>caFile</code> key has
+been deprecated.</p>
 </td>
 </tr>
 <tr>

docs/spec/v1beta2/providers.md

@@ -1095,11 +1095,12 @@ stringData:
 
 `.spec.certSecretRef` is an optional field to specify a name reference to a
 Secret in the same namespace as the Provider, containing the TLS CA certificate.
+The secret must be of type `kubernetes.io/tls` or `Opaque`.
 
 #### Example
 
 To enable notification-controller to communicate with a provider API over HTTPS
-using a self-signed TLS certificate, set the `caFile` like so:
+using a self-signed TLS certificate, set the `ca.crt` like so:
 
 ```yaml
 ---
@@ -1119,11 +1120,16 @@ kind: Secret
 metadata:
   name: my-ca-crt
   namespace: default
+type: kubernetes.io/tls # or Opaque
 stringData:
-  caFile: |
+  ca.crt: |
     <--- CA Key --->
 ```
 
+**Warning:** Support for the `caFile` key has been
+deprecated. If you have any Secrets using this key,
+the controller will log a deprecation warning.
+
 ### HTTP/S proxy
 
 `.spec.proxy` is an optional field to specify an HTTP/S proxy address.

internal/controller/alert_controller.go

@@ -178,7 +178,7 @@ func (r *AlertReconciler) isProviderReady(ctx context.Context, alert *apiv1beta2
 	providerName := types.NamespacedName{Namespace: alert.Namespace, Name: alert.Spec.ProviderRef.Name}
 	if err := r.Get(ctx, providerName, provider); err != nil {
 		// log not found errors since they get filtered out
-		ctrl.LoggerFrom(ctx).Error(err, "failed to get provider %s", providerName.String())
+		ctrl.LoggerFrom(ctx).Error(err, "failed to get provider", "provider", providerName.String())
 		return fmt.Errorf("failed to get provider '%s': %w", providerName.String(), err)
 	}
 

internal/controller/provider_controller.go

@@ -190,6 +190,8 @@ func (r *ProviderReconciler) validateURLs(provider *apiv1beta2.Provider) error {
 }
 
 func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *apiv1beta2.Provider) error {
+	log := ctrl.LoggerFrom(ctx)
+
 	address := provider.Spec.Address
 	proxy := provider.Spec.Proxy
 	username := provider.Spec.Username
@@ -245,9 +247,19 @@ func (r *ProviderReconciler) validateCredentials(ctx context.Context, provider *
 			return fmt.Errorf("failed to read secret, error: %w", err)
 		}
 
-		caFile, ok := secret.Data["caFile"]
+		switch secret.Type {
+		case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "":
+		default:
+			return fmt.Errorf("cannot use secret '%s' to get TLS certificate: invalid secret type: '%s'", secret.Name, secret.Type)
+		}
+
+		caFile, ok := secret.Data["ca.crt"]
 		if !ok {
-			return fmt.Errorf("no caFile found in secret %s", provider.Spec.CertSecretRef.Name)
+			caFile, ok = secret.Data["caFile"]
+			if !ok {
+				return fmt.Errorf("no 'ca.crt' key found in secret '%s'", provider.Spec.CertSecretRef.Name)
+			}
+			log.Info("warning: specifying CA cert via 'caFile' is deprecated, please use 'ca.crt' instead")
 		}
 
 		certPool = x509.NewCertPool()

internal/controller/provider_controller_test.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"os"
 	"testing"
 	"time"
 
@@ -237,3 +238,99 @@ func TestProviderReconciler_Reconcile(t *testing.T) {
 		}, timeout, time.Second).Should(BeTrue())
 	})
 }
+
+func TestProviderReconciler_Reconcile_cacert(t *testing.T) {
+	g := NewWithT(t)
+	namespaceName := "provider-" + randStringRunes(5)
+	secretName := "ca-secret-" + randStringRunes(5)
+
+	caCrt, err := os.ReadFile("./testdata/certs/ca.pem")
+	g.Expect(err).To(Not(HaveOccurred()))
+
+	g.Expect(createNamespace(namespaceName)).NotTo(HaveOccurred(), "failed to create test namespace")
+
+	providerKey := types.NamespacedName{
+		Name:      fmt.Sprintf("provider-%s", randStringRunes(5)),
+		Namespace: namespaceName,
+	}
+
+	provider := &apiv1beta2.Provider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      providerKey.Name,
+			Namespace: providerKey.Namespace,
+		},
+		Spec: apiv1beta2.ProviderSpec{
+			Type:          "generic",
+			Address:       "https://webhook.internal",
+			CertSecretRef: &meta.LocalObjectReference{Name: secretName},
+		},
+	}
+	g.Expect(k8sClient.Create(context.Background(), provider)).To(Succeed())
+
+	certSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: providerKey.Namespace,
+		},
+		Data: map[string][]byte{
+			"caFile": []byte("invalid byte"),
+			"ca.crt": caCrt,
+		},
+	}
+	g.Expect(k8sClient.Create(context.Background(), certSecret)).To(Succeed())
+
+	r := &ProviderReconciler{
+		Client:        k8sClient,
+		EventRecorder: record.NewFakeRecorder(32),
+	}
+
+	t.Run("uses `ca.crt` instead of deprecated `caFile`", func(t *testing.T) {
+		g := NewWithT(t)
+		_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)})
+		g.Expect(err).NotTo(HaveOccurred())
+	})
+
+	t.Run("works if only deprecated `caFile` is specified", func(t *testing.T) {
+		g := NewWithT(t)
+
+		clusterCertSecret := &corev1.Secret{}
+		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(certSecret), clusterCertSecret)).To(Succeed())
+
+		patchHelper, err := patch.NewHelper(clusterCertSecret, k8sClient)
+		g.Expect(err).ToNot(HaveOccurred())
+		clusterCertSecret.Data = map[string][]byte{
+			"caFile": caCrt,
+		}
+		g.Expect(patchHelper.Patch(context.Background(), clusterCertSecret)).ToNot(HaveOccurred())
+
+		_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)})
+		g.Expect(err).NotTo(HaveOccurred())
+	})
+
+	t.Run("returns error with certSecretRef of the wrong type", func(t *testing.T) {
+		g := NewWithT(t)
+
+		dockerSecret := &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "docker-secret",
+				Namespace: providerKey.Namespace,
+			},
+			Type: corev1.DockerConfigJsonKey,
+		}
+		g.Expect(k8sClient.Create(context.Background(), dockerSecret)).To(Succeed())
+
+		clusterProvider := &apiv1beta2.Provider{}
+		g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(provider), clusterProvider)).To(Succeed())
+
+		patchHelper, err := patch.NewHelper(clusterProvider, k8sClient)
+		g.Expect(err).ToNot(HaveOccurred())
+		clusterProvider.Spec.CertSecretRef = &meta.LocalObjectReference{
+			Name: dockerSecret.Name,
+		}
+		g.Expect(patchHelper.Patch(context.Background(), clusterProvider)).ToNot(HaveOccurred())
+
+		_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)})
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("invalid secret type"))
+	})
+}

internal/controller/testdata/certs/Makefile

@@ -0,0 +1,20 @@
+# Copyright 2023 The Flux authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+all: ca-key.pem
+
+ca-key.pem: ca-csr.json
+	cfssl gencert -initca ca-csr.json | cfssljson -bare ca –
+ca.pem: ca-key.pem
+ca.csr: ca-key.pem

internal/controller/testdata/certs/ca-config.json

@@ -0,0 +1,13 @@
+{
+  "signing": {
+    "default": {
+      "expiry": "8760h"
+    },
+    "profiles": {
+      "kubernetes": {
+        "usages": ["signing", "key encipherment", "server auth", "client auth"],
+        "expiry": "8760h"
+      }
+    }
+  }
+}

internal/controller/testdata/certs/ca-csr.json

@@ -0,0 +1,9 @@
+{
+  "CN": "example.com CA",
+  "hosts": [
+    "127.0.0.1",
+    "localhost",
+    "example.com",
+    "www.example.com"
+  ]
+}