From 2359a8cd410b97d4dd34a81a6749f3a71c6483b1 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 13 Nov 2025 14:46:03 -0800 Subject: [PATCH] fix: Copy ClusterClass and Templates only if they do not already exist 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. --- pkg/controllers/namespacesync/copy.go | 48 +++++---- pkg/controllers/namespacesync/copy_test.go | 111 +++++++++++++-------- 2 files changed, 99 insertions(+), 60 deletions(-) diff --git a/pkg/controllers/namespacesync/copy.go b/pkg/controllers/namespacesync/copy.go index f13bd21e5..47be33af5 100644 --- a/pkg/controllers/namespacesync/copy.go +++ b/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 } diff --git a/pkg/controllers/namespacesync/copy_test.go b/pkg/controllers/namespacesync/copy_test.go index b21b55174..c0b562bfd 100644 --- a/pkg/controllers/namespacesync/copy_test.go +++ b/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,