Skip to content

Commit

Permalink
Remove workarounds for missing zeroing in json decoder
Browse files Browse the repository at this point in the history
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
  • Loading branch information
timebertt committed Oct 12, 2021
1 parent dea9ca1 commit ee64b1a
Show file tree
Hide file tree
Showing 5 changed files with 2 additions and 107 deletions.
17 changes: 0 additions & 17 deletions pkg/controllerutils/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
2 changes: 0 additions & 2 deletions pkg/controllerutils/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}),
Expand Down
31 changes: 1 addition & 30 deletions pkg/extensions/customresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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))
})
}

Expand Down
29 changes: 1 addition & 28 deletions pkg/utils/kubernetes/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,17 @@ 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"
"k8s.io/apimachinery/pkg/runtime"
"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"
)
Expand Down Expand Up @@ -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`
Expand Down
30 changes: 0 additions & 30 deletions pkg/utils/kubernetes/object_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit ee64b1a

Please sign in to comment.