Merge pull request #2316 from lleshchi/deletion_timestamp

⚠ Update fake client deletionTimestamp behavior

Author: k8s-ci-robot

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
@@ -31,7 +32,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.6 // indirect
 	github.com/go-openapi/jsonreference v0.20.1 // indirect
 	github.com/go-openapi/swag v0.22.3 // indirect

pkg/client/fake/client.go

@@ -27,7 +27,10 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"time"
 
+	// Using v4 to match upstream
+	jsonpatch "github.com/evanphx/json-patch"
 	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	corev1 "k8s.io/api/core/v1"
@@ -41,8 +44,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"
@@ -285,6 +290,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 create 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.
@@ -368,10 +376,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)
@@ -438,6 +446,11 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob
 	}
 	intResourceVersion++
 	accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10))
+
+	if !deleting && !deletionTimestampEqual(accessor, oldAccessor) {
+		return fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName())
+	}
+
 	if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 {
 		return t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName())
 	}
@@ -667,6 +680,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())
 }
@@ -778,7 +795,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 {
@@ -813,8 +830,39 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
 		return err
 	}
 
+	oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName())
+	if err != nil {
+		return err
+	}
+	oldAccessor, err := meta.Accessor(oldObj)
+	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 !deletionTimestampEqual(newObj, oldAccessor) {
+		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
 	}
@@ -838,6 +886,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 deletionTimestampEqual(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 {
@@ -945,7 +1068,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)
 			}
 		}
 	}