fix: Copy ClusterClass and Templates only if they do not already exist (#1386)

**What problem does this PR solve?**:
It is possible for some resource to exist, but a copy of it cannot be
created. For example, CAPX validation became more strict without
changing its API version. Templates already on the cluster remain
usable. However, they cannot be copied, because the copy does not pass
validation.

Prior to this change, we always copied the template, and ignored the
"already exists" error. Now we first check if the template exist, and
only create it if it does not.

**Which issue(s) this PR fixes**:
Fixes NCN-110955

**How Has This Been Tested?**:
<!--
Please describe the tests that you ran to verify your changes.
Provide output from the tests and any manual steps needed to replicate
the tests.
-->
New unit tests exercise the changes. 

**Special notes for your reviewer**:
<!--
Use this to provide any additional information to the reviewers.
This may include:
- Best way to review the PR.
- Where the author wants the most review attention on.
- etc.
-->

Author: dlipovetsky

pkg/controllers/namespacesync/copy.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -29,17 +30,10 @@ func copyClusterClassAndTemplates(
 			return fmt.Errorf("failed to get reference: %w", err)
 		}
 
-		// Copy Template to target namespace
+		// Copy Template to target namespace, if it does not exist there.
 		targetTemplate := copyObjectForCreate(sourceTemplate, sourceTemplate.GetName(), namespace)
-
-		//nolint:gocritic // Inline error is checked.
-		if err := w.Create(ctx, targetTemplate); client.IgnoreAlreadyExists(err) != nil {
-			return fmt.Errorf(
-				"failed to create %s %s: %w",
-				targetTemplate.GetKind(),
-				client.ObjectKeyFromObject(targetTemplate),
-				err,
-			)
+		if err = createIfNotExists(ctx, templateReader, w, targetTemplate); err != nil {
+			return fmt.Errorf("failed to create template: %w", err)
 		}
 
 		// Update reference to point to newly created Template
@@ -51,14 +45,32 @@ func copyClusterClassAndTemplates(
 		return fmt.Errorf("error processing references: %w", err)
 	}
 
-	//nolint:gocritic // Inline error is checked.
-	if err := w.Create(ctx, target); client.IgnoreAlreadyExists(err) != nil {
-		return fmt.Errorf(
-			"failed to create %s %s: %w",
-			target.Kind,
-			client.ObjectKeyFromObject(target),
-			err,
-		)
+	// Copy ClusterClass to target namespace, if it does not exist there.
+	if err := createIfNotExists(ctx, templateReader, w, target); err != nil {
+		return fmt.Errorf("failed to create cluster class: %w", err)
+	}
+	return nil
+}
+
+func createIfNotExists(ctx context.Context, r client.Reader, w client.Writer, obj client.Object) error {
+	key := client.ObjectKeyFromObject(obj)
+	gvk := obj.GetObjectKind().GroupVersionKind().String()
+
+	// Check if the resource exists.
+	// We do not need the object itself, just the metadata, so we use PartialObjectMetadata.
+	partial := &metav1.PartialObjectMetadata{}
+	partial.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind())
+	err := r.Get(ctx, key, partial)
+
+	if apierrors.IsNotFound(err) {
+		// The resource does not exist, so create it.
+		if err := w.Create(ctx, obj); err != nil {
+			return fmt.Errorf("failed to create %s %s: %w", gvk, key, err)
+		}
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("failed to check if %s %s exists: %w", gvk, key, err)
 	}
 	return nil
 }

pkg/controllers/namespacesync/copy_test.go

@@ -15,6 +15,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/internal/test/builder"
 )
@@ -24,53 +25,50 @@ const (
 	targetNamespace = "target-ns"
 )
 
-var errFakeCreate = errors.New("fake create error")
-
-type mockWriter struct {
-	client.Writer
-	createErrOnKind string
-	createdObjects  []client.Object
-}
-
-func (m *mockWriter) Create(
-	ctx context.Context,
-	obj client.Object,
-	opts ...client.CreateOption,
-) error {
-	if m.createErrOnKind != "" && obj.GetObjectKind().GroupVersionKind().Kind == m.createErrOnKind {
-		return errFakeCreate
-	}
-	m.createdObjects = append(m.createdObjects, obj)
-	// Fake setting of UID to simulate a real API server create.
-	obj.SetUID("fake-uid")
-	return nil
-}
+var (
+	errFakeCreate = errors.New("fake create error")
+	errFakeGet    = errors.New("fake get error")
+)
 
 func TestCopyClusterClassAndTemplates(t *testing.T) {
 	g := NewWithT(t)
 	ctx := context.Background()
 
 	testCases := []struct {
-		name            string
-		createErrOnKind string
-		expectErr       error
-		expectNumCopies int
+		name                string
+		interceptorFuncs    interceptor.Funcs
+		kindThatFailsCreate string
+		kindThatFailsGet    string
+		expectErr           error
+		expectNumCopies     int
 	}{
 		{
 			name:            "should succeed if all objects are created",
 			expectNumCopies: 6, // 1 ClusterClass + 5 templates
 		},
 		{
-			name:            "should fail if creating a template fails",
-			createErrOnKind: "GenericInfrastructureClusterTemplate",
-			expectErr:       errFakeCreate,
-			expectNumCopies: 0, // The first template create will fail.
+			name:                "should fail if creating a template fails",
+			kindThatFailsCreate: "GenericInfrastructureClusterTemplate",
+			expectErr:           errFakeCreate,
+			expectNumCopies:     0, // The first template create will fail.
+		},
+		{
+			name:                "should fail if creating the clusterclass fails",
+			kindThatFailsCreate: "ClusterClass",
+			expectErr:           errFakeCreate,
+			expectNumCopies:     5, // All 5 templates are created before ClusterClass.
+		},
+		{
+			name:             "should fail if getting a template fails",
+			kindThatFailsGet: "GenericInfrastructureClusterTemplate",
+			expectErr:        errFakeGet,
+			expectNumCopies:  0, // The first template get will fail.
 		},
 		{
-			name:            "should fail if creating the clusterclass fails",
-			createErrOnKind: "ClusterClass",
-			expectErr:       errFakeCreate,
-			expectNumCopies: 5, // All 5 templates are created before ClusterClass.
+			name:             "should fail if getting the clusterclass fails",
+			kindThatFailsGet: "ClusterClass",
+			expectErr:        errFakeGet,
+			expectNumCopies:  5, // All 5 templates are created before ClusterClass.
 		},
 	}
 
@@ -85,15 +83,23 @@ func TestCopyClusterClassAndTemplates(t *testing.T) {
 				initObjs = append(initObjs, template)
 			}
 
-			fakeReader := fake.NewClientBuilder().WithRuntimeObjects(initObjs...).Build()
-			mockWriter := &mockWriter{
-				createErrOnKind: tc.createErrOnKind,
-			}
+			createdObjs := []client.Object{}
+			fakeClient := fake.
+				NewClientBuilder().
+				WithInterceptorFuncs(
+					interceptors(
+						&createdObjs,
+						tc.kindThatFailsGet,
+						tc.kindThatFailsCreate,
+					),
+				).
+				WithRuntimeObjects(initObjs...).
+				Build()
 
 			err := copyClusterClassAndTemplates(
 				ctx,
-				mockWriter,
-				fakeReader,
+				fakeClient,
+				fakeClient,
 				sourceClusterClass,
 				targetNamespace,
 			)
@@ -105,18 +111,39 @@ func TestCopyClusterClassAndTemplates(t *testing.T) {
 				g.Expect(err).ToNot(HaveOccurred())
 			}
 
-			g.Expect(len(mockWriter.createdObjects)).To(Equal(tc.expectNumCopies))
+			g.Expect(len(createdObjs)).To(Equal(tc.expectNumCopies))
 
-			for _, obj := range mockWriter.createdObjects {
+			for _, obj := range createdObjs {
 				g.Expect(obj.GetNamespace()).To(Equal(targetNamespace))
 				g.Expect(obj.GetOwnerReferences()).To(BeEmpty())
 				g.Expect(obj.GetUID()).ToNot(BeEmpty())
-				g.Expect(obj.GetResourceVersion()).To(BeEmpty())
 			}
 		})
 	}
 }
 
+func interceptors(createdObjs *[]client.Object, kindThatFailsGet, kindThatFailsCreate string) interceptor.Funcs {
+	return interceptor.Funcs{
+		Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+			if kindThatFailsGet != "" && obj.GetObjectKind().GroupVersionKind().Kind == kindThatFailsGet {
+				return errFakeGet
+			}
+			return c.Get(ctx, key, obj, opts...)
+		},
+		Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+			if kindThatFailsCreate != "" && obj.GetObjectKind().GroupVersionKind().Kind == kindThatFailsCreate {
+				return errFakeCreate
+			}
+
+			// Fake setting of UID to simulate a real API server create.
+			obj.SetUID("fake-uid")
+
+			*createdObjs = append(*createdObjs, obj)
+			return c.Create(ctx, obj, opts...)
+		},
+	}
+}
+
 // newTestClusterClassAndTemplates is a helper to generate a valid ClusterClass with all its referenced templates.
 func newTestClusterClassAndTemplates(
 	namespace,