Update fake client deletionTimestamp behavior

The current fake client methods diverge from the behavior of real k8s.
This results in scenarios where an object can be created or modified to
have a deletionTimestamp and no finalizers, with a delay in garbage
collection, causing inaccurate or ambiguous behavior in  test cases that
use the fake client.

Bring fake client behavior closer in line with real k8s behavior by
modifying the following:
- Update() and Patch() will reject changes that set a previously-unset deletionTimestamp
- Create() will ignore a deletionTimestamp on an object
- Build() will error if initialized with an object that has a deletionTimestamp but no finalizers;
but will accept a deletionTimestamp if it has finalizers.

Signed-off-by: Leah Leshchinsky <[email protected]>

Author: lleshchi

go.mod

@@ -3,6 +3,7 @@ module sigs.k8s.io/controller-runtime
 go 1.20
 
 require (
+	github.com/evanphx/json-patch v4.12.0+incompatible
 	github.com/evanphx/json-patch/v5 v5.6.0
 	github.com/fsnotify/fsnotify v1.6.0
 	github.com/go-logr/logr v1.2.4
@@ -32,7 +33,6 @@ require (
 	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/emicklei/go-restful/v3 v3.9.0 // indirect
-	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
 	github.com/go-openapi/jsonpointer v0.19.5 // indirect
 	github.com/go-openapi/jsonreference v0.20.0 // indirect
 	github.com/go-openapi/swag v0.19.14 // indirect

pkg/client/fake/client.go

@@ -27,7 +27,9 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"time"
 
+	jsonpatch "github.com/evanphx/json-patch"
 	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -38,8 +40,10 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	utilrand "k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -282,6 +286,9 @@ func (t versionedTracker) Add(obj runtime.Object) error {
 		if err != nil {
 			return fmt.Errorf("failed to get accessor for object: %w", err)
 		}
+		if accessor.GetDeletionTimestamp() != nil && len(accessor.GetFinalizers()) == 0 {
+			return fmt.Errorf("refusing to init obj %s with metadata.deletionTimestamp but no finalizers", accessor.GetName())
+		}
 		if accessor.GetResourceVersion() == "" {
 			// We use a "magic" value of 999 here because this field
 			// is parsed as uint and and 0 is already used in Update.
@@ -365,10 +372,10 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob
 	if bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Patch")) {
 		isStatus = true
 	}
-	return t.update(gvr, obj, ns, isStatus)
+	return t.update(gvr, obj, ns, isStatus, false)
 }
 
-func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool) error {
+func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool, deleting bool) error {
 	accessor, err := meta.Accessor(obj)
 	if err != nil {
 		return fmt.Errorf("failed to get accessor for object: %w", err)
@@ -435,7 +442,12 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob
 	}
 	intResourceVersion++
 	accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10))
-	if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 {
+
+	if !deleting && !validateTimestampDifference(accessor, oldAccessor) {
+		return fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName())
+	}
+
+	if !oldAccessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 {
 		return t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName())
 	}
 	obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj)
@@ -664,6 +676,10 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
 		}
 		accessor.SetName(fmt.Sprintf("%s%s", base, utilrand.String(randomLength)))
 	}
+	// Ignore attempts to set deletion timestamp
+	if !accessor.GetDeletionTimestamp().IsZero() {
+		accessor.SetDeletionTimestamp(nil)
+	}
 
 	return c.tracker.Create(gvr, obj, accessor.GetNamespace())
 }
@@ -775,7 +791,7 @@ func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.Upd
 	if err != nil {
 		return err
 	}
-	return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus)
+	return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus, false)
 }
 
 func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
@@ -810,8 +826,39 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
 		return err
 	}
 
+	o, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName())
+	if err != nil {
+		return err
+	}
+	oldObj, err := meta.Accessor(o)
+	if err != nil {
+		return err
+	}
+
+	// Apply patch without updating object.
+	// To remain in accordance with the behavior of k8s api behavior,
+	// a patch must not allow for changes to the deletionTimestamp of an object.
+	// The reaction() function applies the patch to the object and calls Update(),
+	// whereas dryPatch() replicates this behavior but skips the call to Update().
+	// This ensures that the patch may be rejected if a deletionTimestamp is modified, prior
+	// to updating the object.
+	action := testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data)
+	o, err = dryPatch(action, c.tracker)
+	if err != nil {
+		return err
+	}
+	newObj, err := meta.Accessor(o)
+	if err != nil {
+		return err
+	}
+
+	// Validate that deletionTimestamp has not been changed
+	if !validateTimestampDifference(newObj, oldObj) {
+		return fmt.Errorf("rejected patch, metadata.deletionTimestamp immutable")
+	}
+
 	reaction := testing.ObjectReaction(c.tracker)
-	handled, o, err := reaction(testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data))
+	handled, o, err := reaction(action)
 	if err != nil {
 		return err
 	}
@@ -835,6 +882,81 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
 	return err
 }
 
+// Applying a patch results in a deletionTimestamp that is truncated to the nearest second.
+// Check that the diff between a new and old deletion timestamp is within a reasonable threshold
+// to be considered unchanged.
+func validateTimestampDifference(newObj metav1.Object, obj metav1.Object) bool {
+	newTime := newObj.GetDeletionTimestamp()
+	oldTime := obj.GetDeletionTimestamp()
+
+	if newTime == nil || oldTime == nil {
+		return newTime == oldTime
+	}
+	return newTime.Time.Sub(oldTime.Time).Abs() < time.Second
+}
+
+// The behavior of applying the patch is pulled out into dryPatch(),
+// which applies the patch and returns an object, but does not Update() the object.
+// This function returns a patched runtime object that may then be validated before a call to Update() is executed.
+// This results in some code duplication, but was found to be a cleaner alternative than unmarshalling and introspecting the patch data
+// and easier than refactoring the k8s client-go method upstream.
+// Duplicate of upstream: https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194
+func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (runtime.Object, error) {
+	ns := action.GetNamespace()
+	gvr := action.GetResource()
+
+	obj, err := tracker.Get(gvr, ns, action.GetName())
+	if err != nil {
+		return nil, err
+	}
+
+	old, err := json.Marshal(obj)
+	if err != nil {
+		return nil, err
+	}
+
+	// reset the object in preparation to unmarshal, since unmarshal does not guarantee that fields
+	// in obj that are removed by patch are cleared
+	value := reflect.ValueOf(obj)
+	value.Elem().Set(reflect.New(value.Type().Elem()).Elem())
+
+	switch action.GetPatchType() {
+	case types.JSONPatchType:
+		patch, err := jsonpatch.DecodePatch(action.GetPatch())
+		if err != nil {
+			return nil, err
+		}
+		modified, err := patch.Apply(old)
+		if err != nil {
+			return nil, err
+		}
+
+		if err = json.Unmarshal(modified, obj); err != nil {
+			return nil, err
+		}
+	case types.MergePatchType:
+		modified, err := jsonpatch.MergePatch(old, action.GetPatch())
+		if err != nil {
+			return nil, err
+		}
+
+		if err := json.Unmarshal(modified, obj); err != nil {
+			return nil, err
+		}
+	case types.StrategicMergePatchType, types.ApplyPatchType:
+		mergedByte, err := strategicpatch.StrategicMergePatch(old, action.GetPatch(), obj)
+		if err != nil {
+			return nil, err
+		}
+		if err = json.Unmarshal(mergedByte, obj); err != nil {
+			return nil, err
+		}
+	default:
+		return nil, fmt.Errorf("PatchType is not supported")
+	}
+	return obj, nil
+}
+
 func copyNonStatusFrom(old, new runtime.Object) error {
 	newClientObject, ok := new.(client.Object)
 	if !ok {
@@ -942,7 +1064,9 @@ func (c *fakeClient) deleteObject(gvr schema.GroupVersionResource, accessor meta
 			if len(oldAccessor.GetFinalizers()) > 0 {
 				now := metav1.Now()
 				oldAccessor.SetDeletionTimestamp(&now)
-				return c.tracker.Update(gvr, old, accessor.GetNamespace())
+				// Call update directly with mutability parameter set to true to allow
+				// changes to deletionTimestamp
+				return c.tracker.update(gvr, old, accessor.GetNamespace(), false, true)
 			}
 		}
 	}