fix: Namespacesync copies resources after partial copy failure (#1228)

**What problem does this PR solve?**:
By design, the namespacesync controller does not update resources in the
target namespace. However, if it only copies some resources before being
interrupted (i.e. the controller restarts), it will never copy the
remaining resources.

This PR adds a (failing) test that simulates this scenario, and then
adds a fix.

**Which issue(s) this PR fixes**:
Fixes https://jira.nutanix.com/browse/NCN-108612

**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.
-->

**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/controller.go

@@ -138,7 +138,7 @@ func (r *Reconciler) Reconcile(
 			scc,
 			namespace,
 		)
-		if client.IgnoreAlreadyExists(err) != nil {
+		if err != nil {
 			// TODO Record an Event.
 			return ctrl.Result{}, fmt.Errorf(
 				"failed to copy source ClusterClass %s or its referenced Templates to namespace %s: %w",

pkg/controllers/namespacesync/controller_test.go

@@ -10,6 +10,7 @@ import (
 
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/storage/names"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -21,7 +22,7 @@ func TestReconcileExistingNamespaceWithUpdatedLabels(t *testing.T) {
 	g := NewWithT(t)
 	timeout := 5 * time.Second
 
-	sourceClusterClassName, cleanup, err := createUniqueClusterClassAndTemplates(
+	sourceClusterClassName, _, cleanup, err := createUniqueClusterClassAndTemplates(
 		sourceClusterClassNamespace,
 	)
 	g.Expect(err).ToNot(HaveOccurred())
@@ -53,7 +54,7 @@ func TestReconcileNewNamespaces(t *testing.T) {
 	g := NewWithT(t)
 	timeout := 5 * time.Second
 
-	sourceClusterClassName, cleanup, err := createUniqueClusterClassAndTemplates(
+	sourceClusterClassName, _, cleanup, err := createUniqueClusterClassAndTemplates(
 		sourceClusterClassNamespace,
 	)
 	g.Expect(err).ToNot(HaveOccurred())
@@ -84,7 +85,7 @@ func TestReconcileNewClusterClass(t *testing.T) {
 	targetNamespaces, err := createTargetNamespaces(3)
 	g.Expect(err).ToNot(HaveOccurred())
 
-	sourceClusterClassName, cleanup, err := createUniqueClusterClassAndTemplates(
+	sourceClusterClassName, _, cleanup, err := createUniqueClusterClassAndTemplates(
 		sourceClusterClassNamespace,
 	)
 	g.Expect(err).ToNot(HaveOccurred())
@@ -108,7 +109,7 @@ func TestReconcileNewClusterClass(t *testing.T) {
 func TestSourceClusterClassNamespaceEmpty(t *testing.T) {
 	g := NewWithT(t)
 
-	_, cleanup, err := createUniqueClusterClassAndTemplates(
+	_, _, cleanup, err := createUniqueClusterClassAndTemplates(
 		sourceClusterClassNamespace,
 	)
 	g.Expect(err).ToNot(HaveOccurred())
@@ -128,6 +129,60 @@ func TestSourceClusterClassNamespaceEmpty(t *testing.T) {
 	g.Expect(ns).To(BeEmpty())
 }
 
+func TestReconcileAfterPartialFailureToCopy(t *testing.T) {
+	g := NewWithT(t)
+	timeout := 5 * time.Second
+	prefix := "partial-failure"
+
+	sourceClusterClassName, sourceTemplates, sourceCleanup, err := createClusterClassAndTemplates(
+		prefix,
+		sourceClusterClassNamespace,
+	)
+	g.Expect(err).ToNot(HaveOccurred())
+	defer func() {
+		g.Expect(sourceCleanup()).To(Succeed())
+	}()
+
+	// Create a namespace with the same ClusterClass and templates.
+	targetNamespace, err := env.CreateNamespace(ctx, "target", map[string]string{})
+	g.Expect(err).ToNot(HaveOccurred())
+	targetClusterClassName, targetTemplates, _, err := createClusterClassAndTemplates(
+		prefix,
+		targetNamespace.Name,
+	)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	// Verify that the target ClusterClass and templates match the source.
+	g.Expect(targetClusterClassName).To(Equal(sourceClusterClassName))
+	for i := range targetTemplates {
+		g.Expect(targetTemplates[i].GetName()).To(Equal(sourceTemplates[i].GetName()))
+	}
+
+	// Simulate a partial copy failure by removing the ClusterClass--the last object copied--from the target namespace.
+	g.Expect(env.CleanupAndWait(ctx, &clusterv1.ClusterClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      targetClusterClassName,
+			Namespace: targetNamespace.Name,
+		},
+	})).To(Succeed())
+
+	// Label the namespace so that it is considered a target namespace to trigger the reconcile loop.
+	targetNamespace.Labels[targetNamespaceLabelKey] = ""
+	err = env.Update(ctx, targetNamespace)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	// Verify that the templates are copied.
+	g.Eventually(func() error {
+		return verifyClusterClassAndTemplates(
+			env.Client,
+			sourceClusterClassName,
+			targetNamespace.Name,
+		)
+	},
+		timeout,
+	).Should(Succeed())
+}
+
 func verifyClusterClassAndTemplates(
 	cli client.Reader,
 	name,
@@ -151,6 +206,7 @@ func verifyClusterClassAndTemplates(
 
 func createUniqueClusterClassAndTemplates(namespace string) (
 	clusterClassName string,
+	templates []client.Object,
 	cleanup func() error,
 	err error,
 ) {
@@ -165,6 +221,7 @@ func createClusterClassAndTemplates(
 	namespace string,
 ) (
 	clusterClassName string,
+	templates []client.Object,
 	cleanup func() error,
 	err error,
 ) {
@@ -206,7 +263,7 @@ func createClusterClassAndTemplates(
 
 	// Create the set of initObjects from the objects above to add to the API server when the test environment starts.
 
-	templates := []client.Object{
+	templates = []client.Object{
 		bootstrapTemplate,
 		infraMachineTemplateWorker,
 		infraMachineTemplateControlPlane,
@@ -216,11 +273,11 @@ func createClusterClassAndTemplates(
 
 	for _, obj := range templates {
 		if err := env.CreateAndWait(ctx, obj); err != nil {
-			return "", nil, err
+			return "", nil, nil, err
 		}
 	}
 	if err := env.CreateAndWait(ctx, clusterClass); err != nil {
-		return "", nil, err
+		return "", nil, nil, err
 	}
 
 	cleanup = func() error {
@@ -232,7 +289,7 @@ func createClusterClassAndTemplates(
 		return env.CleanupAndWait(ctx, clusterClass)
 	}
 
-	return clusterClass.Name, cleanup, nil
+	return clusterClass.Name, templates, cleanup, nil
 }
 
 func createTargetNamespaces(number int) ([]*corev1.Namespace, error) {

pkg/controllers/namespacesync/copy.go

@@ -32,7 +32,8 @@ func copyClusterClassAndTemplates(
 		// Copy Template to target namespace
 		targetTemplate := copyObjectForCreate(sourceTemplate, sourceTemplate.GetName(), namespace)
 
-		if err := w.Create(ctx, targetTemplate); err != nil {
+		//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(),
@@ -50,7 +51,8 @@ func copyClusterClassAndTemplates(
 		return fmt.Errorf("error processing references: %w", err)
 	}
 
-	if err := w.Create(ctx, target); err != nil {
+	//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,

pkg/controllers/namespacesync/copy_test.go

@@ -4,76 +4,153 @@
 package namespacesync
 
 import (
+	"context"
+	"errors"
+	"fmt"
 	"testing"
-	"time"
 
 	. "github.com/onsi/gomega"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/storage/names"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/internal/test/builder"
+)
+
+const (
+	sourceNamespace = "source-ns"
+	targetNamespace = "target-ns"
 )
 
-func TestDoNotUpdateIfTargetExists(t *testing.T) {
+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
+}
+
+func TestCopyClusterClassAndTemplates(t *testing.T) {
 	g := NewWithT(t)
-	timeout := 50 * time.Second
-
-	prefix := names.SimpleNameGenerator.GenerateName("test-")
-	sourceClusterClassName, cleanup, err := createClusterClassAndTemplates(
-		prefix,
-		sourceClusterClassNamespace,
-	)
-	g.Expect(err).ToNot(HaveOccurred())
-
-	targetNamespaces, err := createTargetNamespaces(3)
-	g.Expect(err).ToNot(HaveOccurred())
-
-	for _, targetNamespace := range targetNamespaces {
-		g.Eventually(func() error {
-			return verifyClusterClassAndTemplates(
-				env.Client,
-				sourceClusterClassName,
-				targetNamespace.Name,
-			)
+	ctx := context.Background()
+
+	testCases := []struct {
+		name            string
+		createErrOnKind 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 the clusterclass fails",
+			createErrOnKind: "ClusterClass",
+			expectErr:       errFakeCreate,
+			expectNumCopies: 5, // All 5 templates are created before ClusterClass.
 		},
-			timeout,
-		).Should(Succeed())
 	}
 
-	// Delete source class
-	g.Expect(cleanup()).To(Succeed())
-
-	// Create source class again
-	sourceClusterClassName, cleanup, err = createClusterClassAndTemplates(
-		prefix,
-		sourceClusterClassNamespace,
-	)
-	g.Expect(err).ToNot(HaveOccurred())
-	defer func() {
-		g.Expect(cleanup()).To(Succeed())
-	}()
-
-	source := &clusterv1.ClusterClass{}
-	err = env.Get(
-		ctx,
-		client.ObjectKey{
-			Namespace: sourceClusterClassNamespace,
-			Name:      sourceClusterClassName,
-		},
-		source,
-	)
-	g.Expect(err).ToNot(HaveOccurred())
-
-	// Verify that the copy function returns an error because the target ClusterClass and Templates
-	// already exist.
-	for _, targetNamespace := range targetNamespaces {
-		err = copyClusterClassAndTemplates(
-			ctx,
-			env.Client,
-			env.Client,
-			source,
-			targetNamespace.Name,
-		)
-		g.Expect(apierrors.IsAlreadyExists(err)).To(BeTrue())
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			sourceClusterClass, sourceTemplates := newTestClusterClassAndTemplates(
+				sourceNamespace,
+				names.SimpleNameGenerator.GenerateName("test-cc-"),
+			)
+			initObjs := []runtime.Object{sourceClusterClass}
+			for _, template := range sourceTemplates {
+				initObjs = append(initObjs, template)
+			}
+
+			fakeReader := fake.NewClientBuilder().WithRuntimeObjects(initObjs...).Build()
+			mockWriter := &mockWriter{
+				createErrOnKind: tc.createErrOnKind,
+			}
+
+			err := copyClusterClassAndTemplates(
+				ctx,
+				mockWriter,
+				fakeReader,
+				sourceClusterClass,
+				targetNamespace,
+			)
+
+			if tc.expectErr != nil {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err).To(MatchError(ContainSubstring(tc.expectErr.Error())))
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+
+			g.Expect(len(mockWriter.createdObjects)).To(Equal(tc.expectNumCopies))
+
+			for _, obj := range mockWriter.createdObjects {
+				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())
+			}
+		})
+	}
+}
+
+// newTestClusterClassAndTemplates is a helper to generate a valid ClusterClass with all its referenced templates.
+func newTestClusterClassAndTemplates(
+	namespace,
+	prefix string,
+) (*clusterv1.ClusterClass, []client.Object) {
+	bootstrapTemplate := builder.BootstrapTemplate(namespace, prefix).Build()
+	infraMachineTemplateControlPlane := builder.InfrastructureMachineTemplate(
+		namespace,
+		fmt.Sprintf("%s-control-plane", prefix),
+	).Build()
+	infraMachineTemplateWorker := builder.InfrastructureMachineTemplate(
+		namespace,
+		fmt.Sprintf("%s-worker", prefix),
+	).Build()
+	controlPlaneTemplate := builder.ControlPlaneTemplate(namespace, prefix).Build()
+	infraClusterTemplate := builder.InfrastructureClusterTemplate(namespace, prefix).Build()
+	machineDeploymentClass := builder.MachineDeploymentClass(fmt.Sprintf("%s-worker", prefix)).
+		WithBootstrapTemplate(bootstrapTemplate).
+		WithInfrastructureTemplate(infraMachineTemplateWorker).
+		Build()
+	clusterClass := builder.ClusterClass(namespace, prefix).
+		WithInfrastructureClusterTemplate(infraClusterTemplate).
+		WithControlPlaneTemplate(controlPlaneTemplate).
+		WithControlPlaneInfrastructureMachineTemplate(infraMachineTemplateControlPlane).
+		WithWorkerMachineDeploymentClasses(*machineDeploymentClass).
+		Build()
+
+	templates := []client.Object{
+		bootstrapTemplate,
+		infraMachineTemplateWorker,
+		infraMachineTemplateControlPlane,
+		controlPlaneTemplate,
+		infraClusterTemplate,
 	}
+
+	return clusterClass, templates
 }