⚠️ Fakeclient: Fix a number of bugs when updating through apply (#3319)

* Move the versioned tracked into its own file

* Versioned tracker: Implement object tracker

* Fakeclient Apply Update: strip status and other issues

Author: alvaroaleman

pkg/client/fake/client.go

@@ -17,13 +17,10 @@ limitations under the License.
 package fake
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"reflect"
-	"runtime/debug"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -65,7 +62,6 @@ import (
 	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"
 	clientgoapplyconfigurations "k8s.io/client-go/applyconfigurations"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -79,13 +75,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/internal/objectutil"
 )
 
-type versionedTracker struct {
-	testing.ObjectTracker
-	scheme                        *runtime.Scheme
-	withStatusSubresource         sets.Set[schema.GroupVersionKind]
-	usesFieldManagedObjectTracker bool
-}
-
 type fakeClient struct {
 	// trackerWriteLock must be acquired before writing to
 	// the tracker or performing reads that affect a following
@@ -313,7 +302,7 @@ func (f *ClientBuilder) Build() client.WithWatch {
 		usesFieldManagedObjectTracker = true
 	}
 	tracker := versionedTracker{
-		ObjectTracker:                 f.objectTracker,
+		upstream:                      f.objectTracker,
 		scheme:                        f.scheme,
 		withStatusSubresource:         withStatusSubResource,
 		usesFieldManagedObjectTracker: usesFieldManagedObjectTracker,
@@ -354,83 +343,6 @@ func (f *ClientBuilder) Build() client.WithWatch {
 
 const trackerAddResourceVersion = "999"
 
-func (t versionedTracker) Add(obj runtime.Object) error {
-	var objects []runtime.Object
-	if meta.IsListType(obj) {
-		var err error
-		objects, err = meta.ExtractList(obj)
-		if err != nil {
-			return err
-		}
-	} else {
-		objects = []runtime.Object{obj}
-	}
-	for _, obj := range objects {
-		accessor, err := meta.Accessor(obj)
-		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.
-			// As we can't go lower, go very high instead so this can
-			// be recognized
-			accessor.SetResourceVersion(trackerAddResourceVersion)
-		}
-
-		obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj)
-		if err != nil {
-			return err
-		}
-
-		// If the fieldManager can not decode fields, it will just silently clear them. This is pretty
-		// much guaranteed not to be what someone that initializes a fake client with objects that
-		// have them set wants, so validate them here.
-		// Ref https://github.com/kubernetes/kubernetes/blob/a956ef4862993b825bcd524a19260192ff1da72d/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go#L105
-		if t.usesFieldManagedObjectTracker {
-			if err := managedfields.ValidateManagedFields(accessor.GetManagedFields()); err != nil {
-				return fmt.Errorf("invalid managedFields on %T: %w", obj, err)
-			}
-		}
-		if err := t.ObjectTracker.Add(obj); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-func (t versionedTracker) Create(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.CreateOptions) error {
-	accessor, err := meta.Accessor(obj)
-	if err != nil {
-		return fmt.Errorf("failed to get accessor for object: %w", err)
-	}
-	if accessor.GetName() == "" {
-		gvk, _ := apiutil.GVKForObject(obj, t.scheme)
-		return apierrors.NewInvalid(
-			gvk.GroupKind(),
-			accessor.GetName(),
-			field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")})
-	}
-	if accessor.GetResourceVersion() != "" {
-		return apierrors.NewBadRequest("resourceVersion can not be set for Create requests")
-	}
-	accessor.SetResourceVersion("1")
-	obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj)
-	if err != nil {
-		return err
-	}
-	if err := t.ObjectTracker.Create(gvr, obj, ns, opts...); err != nil {
-		accessor.SetResourceVersion("")
-		return err
-	}
-
-	return nil
-}
-
 // convertFromUnstructuredIfNecessary will convert runtime.Unstructured for a GVK that is recognized
 // by the schema into the whatever the schema produces with New() for said GVK.
 // This is required because the tracker unconditionally saves on manipulations, but its List() implementation
@@ -465,151 +377,6 @@ func convertFromUnstructuredIfNecessary(s *runtime.Scheme, o runtime.Object) (ru
 	return typed, nil
 }
 
-func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.UpdateOptions) error {
-	updateOpts, err := getSingleOrZeroOptions(opts)
-	if err != nil {
-		return err
-	}
-
-	return t.update(gvr, obj, ns, false, false, updateOpts)
-}
-
-func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, opts metav1.UpdateOptions) error {
-	gvk, err := apiutil.GVKForObject(obj, t.scheme)
-	if err != nil {
-		return err
-	}
-	obj, err = t.updateObject(gvr, obj, ns, isStatus, deleting, opts.DryRun)
-	if err != nil {
-		return err
-	}
-	if obj == nil {
-		return nil
-	}
-
-	if u, unstructured := obj.(*unstructured.Unstructured); unstructured {
-		u.SetGroupVersionKind(gvk)
-	}
-
-	return t.ObjectTracker.Update(gvr, obj, ns, opts)
-}
-
-func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.PatchOptions) error {
-	patchOptions, err := getSingleOrZeroOptions(opts)
-	if err != nil {
-		return err
-	}
-
-	// We apply patches using a client-go reaction that ends up calling the trackers Patch. As we can't change
-	// that reaction, we use the callstack to figure out if this originated from the status client.
-	isStatus := bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).statusPatch"))
-
-	obj, err = t.updateObject(gvr, obj, ns, isStatus, false, patchOptions.DryRun)
-	if err != nil {
-		return err
-	}
-	if obj == nil {
-		return nil
-	}
-
-	return t.ObjectTracker.Patch(gvr, obj, ns, patchOptions)
-}
-
-func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, dryRun []string) (runtime.Object, error) {
-	accessor, err := meta.Accessor(obj)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get accessor for object: %w", err)
-	}
-
-	if accessor.GetName() == "" {
-		gvk, _ := apiutil.GVKForObject(obj, t.scheme)
-		return nil, apierrors.NewInvalid(
-			gvk.GroupKind(),
-			accessor.GetName(),
-			field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")})
-	}
-
-	gvk, err := apiutil.GVKForObject(obj, t.scheme)
-	if err != nil {
-		return nil, err
-	}
-
-	oldObject, err := t.ObjectTracker.Get(gvr, ns, accessor.GetName())
-	if err != nil {
-		// If the resource is not found and the resource allows create on update, issue a
-		// create instead.
-		if apierrors.IsNotFound(err) && allowsCreateOnUpdate(gvk) {
-			return nil, t.Create(gvr, obj, ns)
-		}
-		return nil, err
-	}
-
-	if t.withStatusSubresource.Has(gvk) {
-		if isStatus { // copy everything but status and metadata.ResourceVersion from original object
-			if err := copyStatusFrom(obj, oldObject); err != nil {
-				return nil, fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err)
-			}
-			passedRV := accessor.GetResourceVersion()
-			if err := copyFrom(oldObject, obj); err != nil {
-				return nil, fmt.Errorf("failed to restore non-status fields: %w", err)
-			}
-			accessor.SetResourceVersion(passedRV)
-		} else { // copy status from original object
-			if err := copyStatusFrom(oldObject, obj); err != nil {
-				return nil, fmt.Errorf("failed to copy the status for object with status subresource: %w", err)
-			}
-		}
-	} else if isStatus {
-		return nil, apierrors.NewNotFound(gvr.GroupResource(), accessor.GetName())
-	}
-
-	oldAccessor, err := meta.Accessor(oldObject)
-	if err != nil {
-		return nil, err
-	}
-
-	// If the new object does not have the resource version set and it allows unconditional update,
-	// default it to the resource version of the existing resource
-	if accessor.GetResourceVersion() == "" {
-		switch {
-		case allowsUnconditionalUpdate(gvk):
-			accessor.SetResourceVersion(oldAccessor.GetResourceVersion())
-			// This is needed because if the patch explicitly sets the RV to null, the client-go reaction we use
-			// to apply it and whose output we process here will have it unset. It is not clear why the Kubernetes
-			// apiserver accepts such a patch, but it does so we just copy that behavior.
-			// Kubernetes apiserver behavior can be checked like this:
-			// `kubectl patch configmap foo --patch '{"metadata":{"annotations":{"foo":"bar"},"resourceVersion":null}}' -v=9`
-		case bytes.
-			Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Patch")):
-			// We apply patches using a client-go reaction that ends up calling the trackers Update. As we can't change
-			// that reaction, we use the callstack to figure out if this originated from the "fakeClient.Patch" func.
-			accessor.SetResourceVersion(oldAccessor.GetResourceVersion())
-		}
-	}
-
-	if accessor.GetResourceVersion() != oldAccessor.GetResourceVersion() {
-		return nil, apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), errors.New("object was modified"))
-	}
-	if oldAccessor.GetResourceVersion() == "" {
-		oldAccessor.SetResourceVersion("0")
-	}
-	intResourceVersion, err := strconv.ParseUint(oldAccessor.GetResourceVersion(), 10, 64)
-	if err != nil {
-		return nil, fmt.Errorf("can not convert resourceVersion %q to int: %w", oldAccessor.GetResourceVersion(), err)
-	}
-	intResourceVersion++
-	accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10))
-
-	if !deleting && !deletionTimestampEqual(accessor, oldAccessor) {
-		return nil, fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName())
-	}
-
-	if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 {
-		return nil, t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName(), metav1.DeleteOptions{DryRun: dryRun})
-	}
-	return convertFromUnstructuredIfNecessary(t.scheme, obj)
-}
-
 func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
 	if err := c.addToSchemeIfUnknownAndUnstructuredOrPartial(obj); err != nil {
 		return err
@@ -1233,6 +1000,15 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
 	reaction := testing.ObjectReaction(c.tracker)
 	handled, o, err := reaction(action)
 	if err != nil {
+		// The reaction calls tracker.Get after tracker.Apply to return the object,
+		// but we may have deleted it in tracker.Apply if there was no finalizer
+		// left.
+		if apierrors.IsNotFound(err) &&
+			patch.Type() == types.ApplyPatchType &&
+			oldAccessor.GetDeletionTimestamp() != nil &&
+			len(obj.GetFinalizers()) == 0 {
+			return nil
+		}
 		return err
 	}
 	if !handled {