Merge pull request kubernetes-sigs#8926 from sbueringer/pr-kcp-cache-secrets-ownerref

🌱 KCP: cache secrets between LookupOrGenerate and ensureCertificatesOwnerRef

Author: k8s-ci-robot

controlplane/kubeadm/internal/cluster_test.go

@@ -93,6 +93,9 @@ func TestGetWorkloadCluster(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "my-cluster-etcd",
 			Namespace: ns.Name,
+			Labels: map[string]string{
+				clusterv1.ClusterNameLabel: "my-cluster",
+			},
 		},
 		Data: map[string][]byte{
 			secret.TLSCrtDataName: certs.EncodeCertPEM(cert),
@@ -120,6 +123,9 @@ func TestGetWorkloadCluster(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "my-cluster-kubeconfig",
 			Namespace: ns.Name,
+			Labels: map[string]string{
+				clusterv1.ClusterNameLabel: "my-cluster",
+			},
 		},
 		Data: map[string][]byte{
 			secret.KubeconfigDataName: testEnvKubeconfig,
@@ -179,7 +185,7 @@ func TestGetWorkloadCluster(t *testing.T) {
 			g := NewWithT(t)
 
 			for _, o := range tt.objs {
-				g.Expect(env.Client.Create(ctx, o)).To(Succeed())
+				g.Expect(env.CreateAndWait(ctx, o)).To(Succeed())
 				defer func(do client.Object) {
 					g.Expect(env.CleanupAndWait(ctx, do)).To(Succeed())
 				}(o)
@@ -195,10 +201,8 @@ func TestGetWorkloadCluster(t *testing.T) {
 			)
 			g.Expect(err).ToNot(HaveOccurred())
 
-			// Note: The API reader is intentionally used instead of the regular (cached) client
-			// to avoid test failures when the local cache isn't able to catch up in time.
 			m := Management{
-				Client:  env.GetAPIReader(),
+				Client:  env.GetClient(),
 				Tracker: tracker,
 			}
 

controlplane/kubeadm/internal/controllers/controller.go

@@ -488,7 +488,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileClusterCertificates(ctx context
 		return ctrl.Result{}, err
 	}
 
-	if err := r.ensureCertificatesOwnerRef(ctx, util.ObjectKey(controlPlane.Cluster), certificates, *controllerRef); err != nil {
+	if err := r.ensureCertificatesOwnerRef(ctx, certificates, *controllerRef); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -928,38 +928,36 @@ func (r *KubeadmControlPlaneReconciler) adoptOwnedSecrets(ctx context.Context, k
 }
 
 // ensureCertificatesOwnerRef ensures an ownerReference to the owner is added on the Secrets holding certificates.
-func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.Context, clusterKey client.ObjectKey, certificates secret.Certificates, owner metav1.OwnerReference) error {
+func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.Context, certificates secret.Certificates, owner metav1.OwnerReference) error {
 	for _, c := range certificates {
-		s := &corev1.Secret{}
-		secretKey := client.ObjectKey{Namespace: clusterKey.Namespace, Name: secret.Name(clusterKey.Name, c.Purpose)}
-		if err := r.Client.Get(ctx, secretKey, s); err != nil {
-			return errors.Wrapf(err, "failed to get Secret %s", secretKey)
+		if c.Secret == nil {
+			continue
 		}
 
-		patchHelper, err := patch.NewHelper(s, r.Client)
+		patchHelper, err := patch.NewHelper(c.Secret, r.Client)
 		if err != nil {
-			return errors.Wrapf(err, "failed to create patchHelper for Secret %s", secretKey)
+			return errors.Wrapf(err, "failed to create patchHelper for Secret %s", klog.KObj(c.Secret))
 		}
 
-		controller := metav1.GetControllerOf(s)
+		controller := metav1.GetControllerOf(c.Secret)
 		// If the current controller is KCP, ensure the owner reference is up to date.
 		// Note: This ensures secrets created prior to v1alpha4 are updated to have the correct owner reference apiVersion.
 		if controller != nil && controller.Kind == kubeadmControlPlaneKind {
-			s.SetOwnerReferences(util.EnsureOwnerRef(s.GetOwnerReferences(), owner))
+			c.Secret.SetOwnerReferences(util.EnsureOwnerRef(c.Secret.GetOwnerReferences(), owner))
 		}
 
 		// If the Type doesn't match the type used for secrets created by core components continue without altering the owner reference further.
 		// Note: This ensures that control plane related secrets created by KubeadmConfig are eventually owned by KCP.
 		// TODO: Remove this logic once standalone control plane machines are no longer allowed.
-		if s.Type == clusterv1.ClusterSecretType {
+		if c.Secret.Type == clusterv1.ClusterSecretType {
 			// Remove the current controller if one exists.
 			if controller != nil {
-				s.SetOwnerReferences(util.RemoveOwnerRef(s.GetOwnerReferences(), *controller))
+				c.Secret.SetOwnerReferences(util.RemoveOwnerRef(c.Secret.GetOwnerReferences(), *controller))
 			}
-			s.SetOwnerReferences(util.EnsureOwnerRef(s.GetOwnerReferences(), owner))
+			c.Secret.SetOwnerReferences(util.EnsureOwnerRef(c.Secret.GetOwnerReferences(), owner))
 		}
-		if err := patchHelper.Patch(ctx, s); err != nil {
-			return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", secretKey, owner.String())
+		if err := patchHelper.Patch(ctx, c.Secret); err != nil {
+			return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", klog.KObj(c.Secret), owner.String())
 		}
 	}
 	return nil

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -815,13 +815,16 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
 			// Set the Secret Type to clusterv1.ClusterSecretType which signals this Secret was generated by CAPI.
 			s.Type = clusterv1.ClusterSecretType
 
+			// Store the secret in the certificate.
+			c.Secret = s
+
 			objs = append(objs, s)
 		}
 
 		fakeClient := newFakeClient(objs...)
 
 		r := KubeadmControlPlaneReconciler{Client: fakeClient}
-		err = r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
+		err = r.ensureCertificatesOwnerRef(ctx, certificates, kcpOwner)
 		g.Expect(err).To(BeNil())
 
 		secrets := &corev1.SecretList{}
@@ -857,13 +860,17 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
 					Controller: pointer.Bool(true),
 				},
 			})
+
+			// Store the secret in the certificate.
+			c.Secret = s
+
 			objs = append(objs, s)
 		}
 
 		fakeClient := newFakeClient(objs...)
 
 		r := KubeadmControlPlaneReconciler{Client: fakeClient}
-		err := r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
+		err := r.ensureCertificatesOwnerRef(ctx, certificates, kcpOwner)
 		g.Expect(err).To(BeNil())
 
 		secrets := &corev1.SecretList{}
@@ -902,13 +909,16 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
 				},
 			))
 
+			// Store the secret in the certificate.
+			c.Secret = s
+
 			objs = append(objs, s)
 		}
 
 		fakeClient := newFakeClient(objs...)
 
 		r := KubeadmControlPlaneReconciler{Client: fakeClient}
-		err := r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
+		err := r.ensureCertificatesOwnerRef(ctx, certificates, kcpOwner)
 		g.Expect(err).To(BeNil())
 
 		secrets := &corev1.SecretList{}

controlplane/kubeadm/internal/suite_test.go

@@ -20,7 +20,9 @@ import (
 	"os"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"sigs.k8s.io/cluster-api/internal/test/envtest"
 )
@@ -32,7 +34,11 @@ var (
 
 func TestMain(m *testing.M) {
 	os.Exit(envtest.Run(ctx, envtest.RunInput{
-		M:        m,
+		M: m,
+		ManagerUncachedObjs: []client.Object{
+			&corev1.ConfigMap{},
+			&corev1.Secret{},
+		},
 		SetupEnv: func(e *envtest.Environment) { env = e },
 	}))
 }

util/secret/certificates.go

@@ -212,6 +212,7 @@ func (c Certificates) Lookup(ctx context.Context, ctrlclient client.Client, clus
 			return err
 		}
 		certificate.KeyPair = kp
+		certificate.Secret = s
 	}
 	return nil
 }
@@ -257,6 +258,7 @@ func (c Certificates) SaveGenerated(ctx context.Context, ctrlclient client.Clien
 		if err := ctrlclient.Create(ctx, s); err != nil {
 			return errors.WithStack(err)
 		}
+		certificate.Secret = s
 	}
 	return nil
 }
@@ -284,6 +286,7 @@ type Certificate struct {
 	Purpose           Purpose
 	KeyPair           *certs.KeyPair
 	CertFile, KeyFile string
+	Secret            *corev1.Secret
 }
 
 // Hashes hashes all the certificates stored in a CA certificate.