fix: Make Cluster the owner of image registry credential secret

Author: dlipovetsky

pkg/handlers/generic/mutation/imageregistries/credentials/inject.go

@@ -17,6 +17,7 @@ import (
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/handlers/mutation"
@@ -69,7 +70,7 @@ func (h *imageRegistriesPatchHandler) Mutate(
 	vars map[string]apiextensionsv1.JSON,
 	holderRef runtimehooksv1.HolderReference,
 	clusterKey ctrlclient.ObjectKey,
-	_ mutation.ClusterGetter,
+	clusterGetter mutation.ClusterGetter,
 ) error {
 	log := ctrl.LoggerFrom(ctx).WithValues(
 		"holderRef", holderRef,
@@ -151,6 +152,33 @@ func (h *imageRegistriesPatchHandler) Mutate(
 		return generateErr
 	}
 
+	credentialsSecret, generateErr := generateCredentialsSecret(
+		registriesWithOptionalCredentials,
+		clusterKey.Name,
+		clusterKey.Namespace,
+	)
+	if generateErr != nil {
+		return fmt.Errorf(
+			"error generating credentials Secret for Image Registry Credentials variable: %w",
+			err,
+		)
+	}
+
+	cluster, err := clusterGetter(ctx)
+	if err != nil {
+		log.Error(
+			err,
+			"failed to get cluster from Image Registry Credentials mutation handler",
+		)
+		return err
+	}
+	if err = controllerutil.SetOwnerReference(cluster, credentialsSecret, h.client.Scheme()); err != nil {
+		return fmt.Errorf(
+			"failed to set owner reference on Image Registry Credentials Secret: %w",
+			err,
+		)
+	}
+
 	if err := patches.MutateIfApplicable(
 		obj, vars, &holderRef, selectors.ControlPlane(), log,
 		func(obj *controlplanev1.KubeadmControlPlaneTemplate) error {
@@ -172,9 +200,8 @@ func (h *imageRegistriesPatchHandler) Mutate(
 				commands...,
 			)
 
-			generateErr = createSecretIfNeeded(ctx, h.client, registriesWithOptionalCredentials, clusterKey)
-			if generateErr != nil {
-				return generateErr
+			if err := client.ServerSideApply(ctx, h.client, credentialsSecret, client.ForceOwnership); err != nil {
+				return fmt.Errorf("failed to apply Image Registry Credentials Secret: %w", err)
 			}
 
 			initConfiguration := obj.Spec.Template.Spec.KubeadmConfigSpec.InitConfiguration
@@ -216,9 +243,8 @@ func (h *imageRegistriesPatchHandler) Mutate(
 			).Info("adding PreKubeadmCommands to worker node kubeadm config template")
 			obj.Spec.Template.Spec.PreKubeadmCommands = append(obj.Spec.Template.Spec.PreKubeadmCommands, commands...)
 
-			generateErr := createSecretIfNeeded(ctx, h.client, registriesWithOptionalCredentials, clusterKey)
-			if generateErr != nil {
-				return generateErr
+			if err := client.ServerSideApply(ctx, h.client, credentialsSecret, client.ForceOwnership); err != nil {
+				return fmt.Errorf("failed to apply Image Registry Credentials Secret: %w", err)
 			}
 
 			joinConfiguration := obj.Spec.Template.Spec.JoinConfiguration
@@ -331,32 +357,6 @@ func generateFilesAndCommands(
 	return files, commands, err
 }
 
-func createSecretIfNeeded(
-	ctx context.Context,
-	c ctrlclient.Client,
-	registriesWithOptionalCredentials []providerConfig,
-	clusterKey ctrlclient.ObjectKey,
-) error {
-	credentialsSecret, err := generateCredentialsSecret(
-		registriesWithOptionalCredentials,
-		clusterKey.Name,
-		clusterKey.Namespace,
-	)
-	if err != nil {
-		return fmt.Errorf(
-			"error generating credentials Secret for Image Registry Credentials variable: %w",
-			err,
-		)
-	}
-	if credentialsSecret != nil {
-		if err := client.ServerSideApply(ctx, c, credentialsSecret, client.ForceOwnership); err != nil {
-			return fmt.Errorf("failed to apply Image Registry Credentials Secret: %w", err)
-		}
-	}
-
-	return nil
-}
-
 // secretForImageRegistryCredentials returns the Secret for the given ImageRegistryCredentials.
 // Returns nil if the secret field is empty.
 func secretForImageRegistryCredentials(

pkg/handlers/generic/mutation/imageregistries/credentials/inject_test.go

@@ -11,7 +11,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apiserver/pkg/storage/names"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
@@ -126,15 +130,13 @@ func TestImageRegistriesPatch(t *testing.T) {
 }
 
 var _ = Describe("Generate Image registry patches", func() {
+	clientScheme := runtime.NewScheme()
+	utilruntime.Must(clientgoscheme.AddToScheme(clientScheme))
+	utilruntime.Must(clusterv1.AddToScheme(clientScheme))
+
 	patchGenerator := func() mutation.GeneratePatches {
-		// Always initialize the testEnv variable in the closure.
-		// This will allow ginkgo to initialize testEnv variable during test execution time.
-		testEnv := helpers.TestEnv
-		// use direct client instead of controller client. This will allow the patch handler to read k8s object
-		// that are written by the tests.
-		// Test cases writes credentials secret that the mutator handler reads.
-		// Using direct client will enable reading it immediately.
-		client, err := testEnv.GetK8sClient()
+		// Use direct client to allow patch handler to read objects created by tests.
+		client, err := helpers.TestEnv.GetK8sClientWithScheme(clientScheme)
 		gomega.Expect(err).To(gomega.BeNil())
 		return mutation.NewMetaGeneratePatchesHandler("", client, NewPatch(client)).(mutation.GeneratePatches)
 	}
@@ -392,22 +394,44 @@ var _ = Describe("Generate Image registry patches", func() {
 
 	// Create credentials secret before each test
 	BeforeEach(func(ctx SpecContext) {
-		client, err := helpers.TestEnv.GetK8sClient()
+		client, err := helpers.TestEnv.GetK8sClientWithScheme(clientScheme)
 		gomega.Expect(err).To(gomega.BeNil())
+
 		gomega.Expect(client.Create(
 			ctx,
 			newRegistryCredentialsSecret(validSecretName, request.Namespace),
 		)).To(gomega.BeNil())
+
+		gomega.Expect(client.Create(
+			ctx,
+			&clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      request.ClusterName,
+					Namespace: metav1.NamespaceDefault,
+				},
+			},
+		)).To(gomega.BeNil())
 	})
 
 	// Delete credentials secret after each test
 	AfterEach(func(ctx SpecContext) {
-		client, err := helpers.TestEnv.GetK8sClient()
+		client, err := helpers.TestEnv.GetK8sClientWithScheme(clientScheme)
 		gomega.Expect(err).To(gomega.BeNil())
+
 		gomega.Expect(client.Delete(
 			ctx,
 			newRegistryCredentialsSecret(validSecretName, request.Namespace),
 		)).To(gomega.BeNil())
+
+		gomega.Expect(client.Delete(
+			ctx,
+			&clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      request.ClusterName,
+					Namespace: metav1.NamespaceDefault,
+				},
+			},
+		)).To(gomega.BeNil())
 	})
 	// create test node for each case
 	for testIdx := range testDefs {