From 72e3ce5db64753695ad2f467e5450d7c21d23338 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 16 Nov 2023 10:23:04 -0500 Subject: [PATCH] Change ForDynamic to proper generic type *Unstructured ...instead of runtime.Object, which I think was used originally to preserve the behavior of allowing any runtime.Object and internally converting to *Unstructured but we can easily adjust users accordingly. Some resource syncer unit tests failed b/c previously ForDynamic had the side effect of actually modifying the resource on Update and the tests did not explicitly retrieve the updated resource after test.UpdateResource. The fake Federator was also modified to internally convert to *Unstructured to simplify users. Signed-off-by: Tom Pantelis --- pkg/federate/create_or_update_federator.go | 6 +-- pkg/federate/fake/federator.go | 22 ++++---- pkg/federate/update_federator.go | 6 +-- pkg/resource/dynamic.go | 60 +++++++--------------- pkg/resource/interface_test.go | 7 +-- pkg/syncer/resource_syncer_test.go | 31 ++++++----- pkg/syncer/test/util.go | 10 ++-- pkg/util/create_or_update_test.go | 22 ++++---- 8 files changed, 72 insertions(+), 92 deletions(-) diff --git a/pkg/federate/create_or_update_federator.go b/pkg/federate/create_or_update_federator.go index 89a67c78..17e3a55b 100644 --- a/pkg/federate/create_or_update_federator.go +++ b/pkg/federate/create_or_update_federator.go @@ -58,9 +58,9 @@ func (f *createOrUpdateFederator) Distribute(ctx context.Context, obj runtime.Ob f.prepareResourceForSync(toDistribute) - result, err := util.CreateOrUpdate[runtime.Object](ctx, resource.ForDynamic(resourceClient), toDistribute, - func(obj runtime.Object) (runtime.Object, error) { - return util.CopyImmutableMetadata(obj.(*unstructured.Unstructured), toDistribute), nil + result, err := util.CreateOrUpdate[*unstructured.Unstructured](ctx, resource.ForDynamic(resourceClient), toDistribute, + func(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + return util.CopyImmutableMetadata(obj, toDistribute), nil }) if f.eventLogName != "" { diff --git a/pkg/federate/fake/federator.go b/pkg/federate/fake/federator.go index 5402952a..e5f5990e 100644 --- a/pkg/federate/fake/federator.go +++ b/pkg/federate/fake/federator.go @@ -24,12 +24,14 @@ import ( "time" . "github.com/onsi/gomega" + "github.com/submariner-io/admiral/pkg/resource" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" ) type Federator struct { - distribute chan runtime.Object - delete chan runtime.Object + distribute chan *unstructured.Unstructured + delete chan *unstructured.Unstructured FailOnDistribute error FailOnDelete error ResetOnFailure bool @@ -37,13 +39,13 @@ type Federator struct { func New() *Federator { return &Federator{ - distribute: make(chan runtime.Object, 100), - delete: make(chan runtime.Object, 100), + distribute: make(chan *unstructured.Unstructured, 100), + delete: make(chan *unstructured.Unstructured, 100), ResetOnFailure: true, } } -func (f *Federator) Distribute(_ context.Context, resource runtime.Object) error { +func (f *Federator) Distribute(_ context.Context, obj runtime.Object) error { err := f.FailOnDistribute if err != nil { if f.ResetOnFailure { @@ -53,12 +55,12 @@ func (f *Federator) Distribute(_ context.Context, resource runtime.Object) error return err } - f.distribute <- resource + f.distribute <- resource.MustToUnstructured(obj) return nil } -func (f *Federator) Delete(_ context.Context, resource runtime.Object) error { +func (f *Federator) Delete(_ context.Context, obj runtime.Object) error { err := f.FailOnDelete if err != nil { if f.ResetOnFailure { @@ -68,13 +70,13 @@ func (f *Federator) Delete(_ context.Context, resource runtime.Object) error { return err } - f.delete <- resource + f.delete <- resource.MustToUnstructured(obj) return nil } func (f *Federator) VerifyDistribute(expected runtime.Object) { - Eventually(f.distribute, 5).Should(Receive(Equal(expected)), "Distribute was not called") + Eventually(f.distribute, 5).Should(Receive(Equal(resource.MustToUnstructured(expected))), "Distribute was not called") } func (f *Federator) VerifyNoDistribute() { @@ -82,7 +84,7 @@ func (f *Federator) VerifyNoDistribute() { } func (f *Federator) VerifyDelete(expected runtime.Object) { - Eventually(f.delete, 5).Should(Receive(Equal(expected)), "Delete was not called") + Eventually(f.delete, 5).Should(Receive(Equal(resource.MustToUnstructured(expected))), "Delete was not called") } func (f *Federator) VerifyNoDelete() { diff --git a/pkg/federate/update_federator.go b/pkg/federate/update_federator.go index f9d893e3..f93a9160 100644 --- a/pkg/federate/update_federator.go +++ b/pkg/federate/update_federator.go @@ -62,8 +62,8 @@ func (f *updateFederator) Distribute(ctx context.Context, obj runtime.Object) er f.prepareResourceForSync(toUpdate) - return util.Update[runtime.Object](ctx, resource.ForDynamic(resourceClient), toUpdate, - func(obj runtime.Object) (runtime.Object, error) { - return f.update(obj.(*unstructured.Unstructured), toUpdate), nil + return util.Update[*unstructured.Unstructured](ctx, resource.ForDynamic(resourceClient), toUpdate, + func(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + return f.update(obj, toUpdate), nil }) } diff --git a/pkg/resource/dynamic.go b/pkg/resource/dynamic.go index f8a0b7cc..103471ab 100644 --- a/pkg/resource/dynamic.go +++ b/pkg/resource/dynamic.go @@ -23,7 +23,7 @@ import ( "context" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/dynamic" ) @@ -31,62 +31,38 @@ type dynamicType struct { client dynamic.ResourceInterface } -func (d *dynamicType) Get(ctx context.Context, name string, options metav1.GetOptions) (runtime.Object, error) { +func (d *dynamicType) Get(ctx context.Context, name string, options metav1.GetOptions) (*unstructured.Unstructured, error) { return d.client.Get(ctx, name, options) } //nolint:gocritic // hugeParam - we're matching K8s API -func (d *dynamicType) Create(ctx context.Context, obj runtime.Object, options metav1.CreateOptions) (runtime.Object, error) { - raw, err := ToUnstructured(obj) - if err != nil { - return nil, err - } - - return d.client.Create(ctx, raw, options) +func (d *dynamicType) Create(ctx context.Context, obj *unstructured.Unstructured, options metav1.CreateOptions, +) (*unstructured.Unstructured, error) { + return d.client.Create(ctx, obj, options) } //nolint:gocritic // hugeParam - we're matching K8s API -func (d *dynamicType) Update(ctx context.Context, obj runtime.Object, options metav1.UpdateOptions) (runtime.Object, error) { - raw, err := ToUnstructured(obj) - if err != nil { - return nil, err - } - - return d.client.Update(ctx, raw, options) +func (d *dynamicType) Update(ctx context.Context, obj *unstructured.Unstructured, options metav1.UpdateOptions, +) (*unstructured.Unstructured, error) { + return d.client.Update(ctx, obj, options) } //nolint:gocritic // hugeParam - we're matching K8s API -func (d *dynamicType) UpdateStatus(ctx context.Context, obj runtime.Object, options metav1.UpdateOptions) (runtime.Object, error) { - raw, err := ToUnstructured(obj) - if err != nil { - return nil, err - } - - return d.client.UpdateStatus(ctx, raw, options) +func (d *dynamicType) UpdateStatus(ctx context.Context, obj *unstructured.Unstructured, options metav1.UpdateOptions, +) (*unstructured.Unstructured, error) { + return d.client.UpdateStatus(ctx, obj, options) } -func (d *dynamicType) Delete(ctx context.Context, name string, - options metav1.DeleteOptions, //nolint:gocritic // hugeParam - we're matching K8s API -) error { +//nolint:gocritic // hugeParam - we're matching K8s API +func (d *dynamicType) Delete(ctx context.Context, name string, options metav1.DeleteOptions) error { return d.client.Delete(ctx, name, options) } -func (d *dynamicType) List(ctx context.Context, - options metav1.ListOptions, //nolint:gocritic // hugeParam - we're matching K8s API -) ([]runtime.Object, error) { - l, err := d.client.List(ctx, options) - return MustExtractList[runtime.Object](l), err +//nolint:gocritic // hugeParam - we're matching K8s API +func (d *dynamicType) List(ctx context.Context, options metav1.ListOptions) (*unstructured.UnstructuredList, error) { + return d.client.List(ctx, options) } -func ForDynamic(client dynamic.ResourceInterface) *InterfaceFuncs[runtime.Object] { - t := &dynamicType{client: client} - - return &InterfaceFuncs[runtime.Object]{ - GetFunc: t.Get, - CreateFunc: t.Create, - UpdateFunc: t.Update, - UpdateStatusFunc: t.UpdateStatus, - DeleteFunc: t.Delete, - ListFunc: t.List, - } +func ForDynamic(client dynamic.ResourceInterface) Interface[*unstructured.Unstructured] { + return kubernetesStatusInterfaceAdapter[*unstructured.Unstructured, *unstructured.UnstructuredList](&dynamicType{client: client}) } diff --git a/pkg/resource/interface_test.go b/pkg/resource/interface_test.go index 13ac3bc3..659ef14c 100644 --- a/pkg/resource/interface_test.go +++ b/pkg/resource/interface_test.go @@ -30,6 +30,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -191,14 +192,14 @@ var _ = Describe("Interface", func() { }) Context("ForDynamic", func() { - testInterfaceFuncs(func() resource.Interface[runtime.Object] { + testInterfaceFuncs(func() resource.Interface[*unstructured.Unstructured] { return resource.ForDynamic(dynamicfake.NewSimpleDynamicClient(scheme.Scheme).Resource( schema.GroupVersionResource{ Group: corev1.SchemeGroupVersion.Group, Version: corev1.SchemeGroupVersion.Version, Resource: "pods", }).Namespace(test.LocalNamespace)) - }, runtime.Object(resource.MustToUnstructured(&corev1.Pod{ + }, resource.MustToUnstructured(&corev1.Pod{ TypeMeta: metav1.TypeMeta{ Kind: "Pod", APIVersion: "v1", @@ -210,7 +211,7 @@ var _ = Describe("Interface", func() { Spec: corev1.PodSpec{ Hostname: "my-host", }, - }))) + })) }) }) diff --git a/pkg/syncer/resource_syncer_test.go b/pkg/syncer/resource_syncer_test.go index b47ea0cb..d211d47d 100644 --- a/pkg/syncer/resource_syncer_test.go +++ b/pkg/syncer/resource_syncer_test.go @@ -29,7 +29,6 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/submariner-io/admiral/pkg/federate/fake" . "github.com/submariner-io/admiral/pkg/gomega" - "github.com/submariner-io/admiral/pkg/resource" "github.com/submariner-io/admiral/pkg/syncer" "github.com/submariner-io/admiral/pkg/syncer/test" "github.com/submariner-io/admiral/pkg/util" @@ -90,7 +89,7 @@ func testReconcileLocalToRemote() { When("the resource to reconcile is local and does not exist", func() { It("should invoke delete", func() { d.resource.Namespace = test.LocalNamespace - d.federator.VerifyDelete(test.SetClusterIDLabel(resource.MustToUnstructured(d.resource), "")) + d.federator.VerifyDelete(test.SetClusterIDLabel(d.resource, "")) }) }) @@ -133,7 +132,7 @@ func testReconcileRemoteToLocal() { When("the resource to reconcile is remote and does not exist", func() { It("should invoke delete", func() { - d.federator.VerifyDelete(resource.MustToUnstructured(d.resource)) + d.federator.VerifyDelete(d.resource) }) }) @@ -177,7 +176,7 @@ func testReconcileNoDirection() { When("the resource to reconcile does not exist", func() { It("should invoke delete", func() { - d.federator.VerifyDelete(resource.MustToUnstructured(d.resource)) + d.federator.VerifyDelete(d.resource) }) }) @@ -355,11 +354,11 @@ func testTransformFunction() { }) verifyDistribute := func() { - d.federator.VerifyDistribute(test.SetClusterIDLabel(test.ToUnstructured(transformed), "remote")) + d.federator.VerifyDistribute(test.SetClusterIDLabel(transformed.DeepCopy(), "remote")) } verifyDelete := func() { - d.federator.VerifyDelete(test.SetClusterIDLabel(test.ToUnstructured(transformed), "remote")) + d.federator.VerifyDelete(test.SetClusterIDLabel(transformed.DeepCopy(), "remote")) } When("a resource is created in the datastore", func() { @@ -399,7 +398,7 @@ func testTransformFunction() { d.resource = test.NewPodWithImage(d.config.SourceNamespace, "updated") test.UpdateResource(d.sourceClient, test.NewPodWithImage(d.config.SourceNamespace, "updated")) - d.federator.VerifyDistribute(test.ToUnstructured(transformed)) + d.federator.VerifyDistribute(transformed) Eventually(expOperation).Should(Receive(Equal(syncer.Update))) }) }) @@ -630,7 +629,7 @@ func testOnSuccessfulSyncFunction() { It("should invoke the OnSuccessfulSync function with the transformed resource", func() { test.CreateResource(d.sourceClient, d.resource) - d.federator.VerifyDistribute(test.ToUnstructured(expResource)) + d.federator.VerifyDistribute(expResource) Eventually(expOperation).Should(Receive(Equal(syncer.Create))) }) }) @@ -883,7 +882,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) @@ -894,7 +893,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) }) @@ -931,7 +930,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) @@ -941,7 +940,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) }) @@ -960,7 +959,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) }) @@ -976,7 +975,7 @@ func testUpdateSuppression() { }) It("should distribute it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(d.resource)) + d.federator.VerifyDistribute(test.GetResource(d.sourceClient, d.resource)) }) }) @@ -1152,11 +1151,11 @@ func testRequeueResource() { }) It("should requeue it", func() { - d.federator.VerifyDistribute(test.ToUnstructured(transformed)) + d.federator.VerifyDistribute(transformed) d.syncer.RequeueResource(d.resource.Name, d.resource.Namespace) - d.federator.VerifyDistribute(test.ToUnstructured(transformed)) + d.federator.VerifyDistribute(transformed) }) }) diff --git a/pkg/syncer/test/util.go b/pkg/syncer/test/util.go index a62cfcb6..0b5dd5e3 100644 --- a/pkg/syncer/test/util.go +++ b/pkg/syncer/test/util.go @@ -46,10 +46,7 @@ const ( ) func GetResourceAndError(resourceInterface dynamic.ResourceInterface, obj runtime.Object) (*unstructured.Unstructured, error) { - meta, err := metaapi.Accessor(obj) - Expect(err).To(Succeed()) - - return resourceInterface.Get(context.TODO(), meta.GetName(), metav1.GetOptions{}) + return resourceInterface.Get(context.TODO(), resource.MustToMeta(obj).GetName(), metav1.GetOptions{}) } func GetResource(resourceInterface dynamic.ResourceInterface, obj runtime.Object) *unstructured.Unstructured { @@ -70,8 +67,9 @@ func CreateResource(resourceInterface dynamic.ResourceInterface, obj runtime.Obj } func UpdateResource(resourceInterface dynamic.ResourceInterface, obj runtime.Object) *unstructured.Unstructured { - err := util.Update[runtime.Object](context.Background(), resource.ForDynamic(resourceInterface), obj, - util.Replace(obj)) + u := resource.MustToUnstructured(obj) + err := util.Update[*unstructured.Unstructured](context.Background(), resource.ForDynamic(resourceInterface), u, + util.Replace(u)) Expect(err).To(Succeed()) return GetResource(resourceInterface, obj) diff --git a/pkg/util/create_or_update_test.go b/pkg/util/create_or_update_test.go index c021ac00..a6d29444 100644 --- a/pkg/util/create_or_update_test.go +++ b/pkg/util/create_or_update_test.go @@ -33,6 +33,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/wait" @@ -44,8 +45,8 @@ var _ = Describe("CreateAnew function", func() { t := newCreateOrUpdateTestDiver() createAnew := func() (runtime.Object, error) { - return util.CreateAnew[runtime.Object](context.TODO(), resource.ForDynamic(t.client), t.pod, metav1.CreateOptions{}, - metav1.DeleteOptions{}) + return util.CreateAnew[*unstructured.Unstructured](context.TODO(), resource.ForDynamic(t.client), + resource.MustToUnstructured(t.pod), metav1.CreateOptions{}, metav1.DeleteOptions{}) } createAnewSuccess := func() *corev1.Pod { @@ -144,7 +145,8 @@ var _ = Describe("CreateOrUpdate function", func() { t := newCreateOrUpdateTestDiver() createOrUpdate := func(expResult util.OperationResult) error { - result, err := util.CreateOrUpdate[runtime.Object](context.TODO(), resource.ForDynamic(t.client), test.ToUnstructured(t.pod), t.mutateFn) + result, err := util.CreateOrUpdate[*unstructured.Unstructured](context.TODO(), resource.ForDynamic(t.client), + resource.MustToUnstructured(t.pod), t.mutateFn) if err != nil && expResult != util.OperationResultNone { return err } @@ -246,7 +248,8 @@ var _ = Describe("Update function", func() { t := newCreateOrUpdateTestDiver() update := func() error { - return util.Update[runtime.Object](context.TODO(), resource.ForDynamic(t.client), test.ToUnstructured(t.pod), t.mutateFn) + return util.Update[*unstructured.Unstructured](context.TODO(), resource.ForDynamic(t.client), resource.MustToUnstructured(t.pod), + t.mutateFn) } When("the resource doesn't exist", func() { @@ -268,7 +271,8 @@ var _ = Describe("MustUpdate function", func() { t := newCreateOrUpdateTestDiver() mustUpdate := func() error { - return util.MustUpdate[runtime.Object](context.TODO(), resource.ForDynamic(t.client), test.ToUnstructured(t.pod), t.mutateFn) + return util.MustUpdate[*unstructured.Unstructured](context.TODO(), resource.ForDynamic(t.client), + resource.MustToUnstructured(t.pod), t.mutateFn) } When("the resource doesn't exist", func() { @@ -292,7 +296,7 @@ type createOrUpdateTestDriver struct { client *fake.DynamicResourceClient origBackoff wait.Backoff expectedErr error - mutateFn util.MutateFn[runtime.Object] + mutateFn util.MutateFn[*unstructured.Unstructured] } func newCreateOrUpdateTestDiver() *createOrUpdateTestDriver { @@ -317,7 +321,7 @@ func newCreateOrUpdateTestDiver() *createOrUpdateTestDriver { Duration: 30 * time.Millisecond, }) - t.mutateFn = func(existing runtime.Object) (runtime.Object, error) { + t.mutateFn = func(existing *unstructured.Unstructured) (*unstructured.Unstructured, error) { obj := test.ToUnstructured(t.pod) obj.SetUID(resource.MustToMeta(existing).GetUID()) return util.Replace(obj)(nil) @@ -481,7 +485,7 @@ func (t *createOrUpdateTestDriver) testUpdate(doUpdate func(util.OperationResult Context("and the resource to update is the same", func() { BeforeEach(func() { - t.mutateFn = func(existing runtime.Object) (runtime.Object, error) { + t.mutateFn = func(existing *unstructured.Unstructured) (*unstructured.Unstructured, error) { return existing, nil } }) @@ -496,7 +500,7 @@ func (t *createOrUpdateTestDriver) testUpdate(doUpdate func(util.OperationResult Context("and the mutate function returns an error", func() { BeforeEach(func() { t.expectedErr = errors.New("mutate failure") - t.mutateFn = func(existing runtime.Object) (runtime.Object, error) { + t.mutateFn = func(existing *unstructured.Unstructured) (*unstructured.Unstructured, error) { return nil, t.expectedErr } })