From 5d7d61bae60ac35e176de34210fa4604abac8a80 Mon Sep 17 00:00:00 2001 From: Tim Ebert Date: Tue, 12 Oct 2021 11:00:41 +0200 Subject: [PATCH] Remove workarounds for missing zeroing in json decoder Now that the c-r client zeroes fields before decoding into the object, we can drop our workarounds for this, so basically drop kutil.CreateResetObjectFunc and its usages. ref kubernetes-sigs/controller-runtime#1640 --- pkg/controllerutils/patch.go | 17 ---------------- pkg/controllerutils/patch_test.go | 2 -- pkg/extensions/customresources.go | 31 +---------------------------- pkg/utils/kubernetes/object.go | 29 +-------------------------- pkg/utils/kubernetes/object_test.go | 30 ---------------------------- 5 files changed, 2 insertions(+), 107 deletions(-) diff --git a/pkg/controllerutils/patch.go b/pkg/controllerutils/patch.go index 11d9fee1b96..edadbbf529e 100644 --- a/pkg/controllerutils/patch.go +++ b/pkg/controllerutils/patch.go @@ -18,8 +18,6 @@ import ( "context" "reflect" - kutil "github.com/gardener/gardener/pkg/utils/kubernetes" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" @@ -108,16 +106,6 @@ func CreateOrGetAndStrategicMergePatch(ctx context.Context, c client.Client, obj } func createOrGetAndPatch(ctx context.Context, c client.Client, obj client.Object, patchFunc patchFn, f controllerutil.MutateFn) (controllerutil.OperationResult, error) { - var ( - namespace = obj.GetNamespace() - name = obj.GetName() - ) - - resetObj, err := kutil.CreateResetObjectFunc(obj, c.Scheme()) - if err != nil { - return controllerutil.OperationResultNone, err - } - if err := f(); err != nil { return controllerutil.OperationResultNone, err } @@ -127,16 +115,11 @@ func createOrGetAndPatch(ctx context.Context, c client.Client, obj client.Object return controllerutil.OperationResultNone, err } - resetObj() - obj.SetNamespace(namespace) - obj.SetName(name) - if err2 := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err2 != nil { return controllerutil.OperationResultNone, err2 } patch := patchFunc(obj.DeepCopyObject().(client.Object)) - if err2 := f(); err2 != nil { return controllerutil.OperationResultNone, err2 } diff --git a/pkg/controllerutils/patch_test.go b/pkg/controllerutils/patch_test.go index 53aef462cf2..08d175d54a9 100644 --- a/pkg/controllerutils/patch_test.go +++ b/pkg/controllerutils/patch_test.go @@ -214,8 +214,6 @@ var _ = Describe("Patch", func() { gomock.InOrder( c.EXPECT().Create(ctx, obj).Return(apierrors.NewAlreadyExists(schema.GroupResource{}, "")), c.EXPECT().Get(ctx, client.ObjectKeyFromObject(obj), obj).DoAndReturn(func(_ context.Context, _ client.ObjectKey, objToReturn *corev1.ServiceAccount) error { - Expect(obj.GetLabels()).To(BeEmpty(), "object should be reset before getting it again") - obj.DeepCopyInto(objToReturn) return nil }), diff --git a/pkg/extensions/customresources.go b/pkg/extensions/customresources.go index e5366125826..412f4fd205b 100644 --- a/pkg/extensions/customresources.go +++ b/pkg/extensions/customresources.go @@ -93,17 +93,9 @@ func WaitUntilObjectReadyWithHealthFunction( healthFunc = health.And(health.ObjectHasAnnotationWithValue(v1beta1constants.GardenerTimestamp, expectedTimestamp), healthFunc) } - resetObj, err := kutil.CreateResetObjectFunc(obj, c.Scheme()) - if err != nil { - return err - } - if err := retry.UntilTimeout(ctx, interval, timeout, func(ctx context.Context) (bool, error) { retryCountUntilSevere++ - resetObj() - obj.SetName(name) - obj.SetNamespace(namespace) if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { if apierrors.IsNotFound(err) { return retry.MinorError(err) @@ -221,15 +213,7 @@ func WaitUntilExtensionObjectDeleted( namespace = obj.GetNamespace() ) - resetObj, err := kutil.CreateResetObjectFunc(obj, c.Scheme()) - if err != nil { - return err - } - if err := retry.UntilTimeout(ctx, interval, timeout, func(ctx context.Context) (bool, error) { - resetObj() - obj.SetName(name) - obj.SetNamespace(namespace) if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { if apierrors.IsNotFound(err) { return retry.Ok() @@ -363,20 +347,7 @@ func WaitUntilExtensionObjectMigrated( interval time.Duration, timeout time.Duration, ) error { - var ( - name = obj.GetName() - namespace = obj.GetNamespace() - ) - - resetObj, err := kutil.CreateResetObjectFunc(obj, c.Scheme()) - if err != nil { - return err - } - return retry.UntilTimeout(ctx, interval, timeout, func(ctx context.Context) (done bool, err error) { - resetObj() - obj.SetName(name) - obj.SetNamespace(namespace) if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { if client.IgnoreNotFound(err) == nil { return retry.Ok() @@ -396,7 +367,7 @@ func WaitUntilExtensionObjectMigrated( if extensionSpec := obj.GetExtensionSpec(); extensionSpec != nil { extensionType = extensionSpec.GetExtensionType() } - return retry.MinorError(fmt.Errorf("lastOperation for %s with name %s and type %s is not Migrate=Succeeded", obj.GetObjectKind().GroupVersionKind().Kind, name, extensionType)) + return retry.MinorError(fmt.Errorf("lastOperation for %s with name %s and type %s is not Migrate=Succeeded", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), extensionType)) }) } diff --git a/pkg/utils/kubernetes/object.go b/pkg/utils/kubernetes/object.go index 114e2e5ca90..c12ac11b805 100644 --- a/pkg/utils/kubernetes/object.go +++ b/pkg/utils/kubernetes/object.go @@ -17,10 +17,8 @@ package kubernetes import ( "context" "fmt" - "reflect" "strings" - "github.com/gardener/gardener/pkg/resourcemanager/controller/garbagecollector/references" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,8 +26,8 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "github.com/gardener/gardener/pkg/resourcemanager/controller/garbagecollector/references" "github.com/gardener/gardener/pkg/utils" "github.com/gardener/gardener/pkg/utils/flow" ) @@ -91,31 +89,6 @@ func IsNamespaceInUse(ctx context.Context, reader client.Reader, namespace strin return len(objects.Items) > 0, nil } -// CreateResetObjectFunc creates a func that will reset the given object to a new empty object every time the func is called. -// This is useful for resetting an in-memory object before re-getting it from the API server / cache -// to avoid executing checks on stale/removed object data e.g. annotations/lastError -// (json decoder does not unset fields in the in-memory object that are unset in the API server's response) -func CreateResetObjectFunc(obj runtime.Object, scheme *runtime.Scheme) (func(), error) { - gvk, err := apiutil.GVKForObject(obj, scheme) - if err != nil { - return nil, err - } - emptyObj, err := scheme.New(gvk) - if err != nil { - return nil, err - } - return func() { - deepCopyIntoObject(obj, emptyObj) - }, nil -} - -// deepCopyIntoObject deep copies src into dest. -// This is a workaround for runtime.Object's lack of a DeepCopyInto method, similar to what the c-r cache does: -// https://github.com/kubernetes-sigs/controller-runtime/blob/55a329c15d6b4f91a9ff072fed6f6f05ff3339e7/pkg/cache/internal/cache_reader.go#L85-L90 -func deepCopyIntoObject(dest, src runtime.Object) { - reflect.ValueOf(dest).Elem().Set(reflect.ValueOf(src.DeepCopyObject()).Elem()) -} - // MakeUnique takes either a *corev1.ConfigMap or a *corev1.Secret object and makes it immutable, i.e., it sets // .immutable=true, computes a checksum based on .data, and appends the first 8 characters of the computed checksum // to the name of the object. Additionally, it injects the `resources.gardener.cloud/garbage-collectable-reference=true` diff --git a/pkg/utils/kubernetes/object_test.go b/pkg/utils/kubernetes/object_test.go index ba221bae8d9..87467dc42b9 100644 --- a/pkg/utils/kubernetes/object_test.go +++ b/pkg/utils/kubernetes/object_test.go @@ -179,36 +179,6 @@ var _ = Describe("Object", func() { }) }) - Describe("#CreateResetObjectFunc", func() { - var ( - obj = &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: "non-empty-sa", - }, - } - objCopy = obj.DeepCopy() - ) - - It("should fail because the GVK for the object cannot be determined", func() { - f, err := CreateResetObjectFunc(obj, runtime.NewScheme()) - Expect(f).To(BeNil()) - Expect(runtime.IsNotRegisteredError(err)).To(BeTrue()) - Expect(obj).To(Equal(objCopy)) - }) - - It("should return a function that allows resets an object", func() { - scheme := runtime.NewScheme() - Expect(corev1.AddToScheme(scheme)).To(Succeed()) - - f, err := CreateResetObjectFunc(obj, scheme) - Expect(f).NotTo(BeNil()) - Expect(err).NotTo(HaveOccurred()) - - f() - Expect(obj).To(Equal(&corev1.ServiceAccount{})) - }) - }) - Describe("#MakeUnique", func() { var ( name = "some-name"