bug: Fakeclient: Fix TOCTOU races

The fake client currently has a number of time of check time of use
races, where it fetches an object to determine what to do in a mutating
operation. The problem is that the object might change in between
fetching it and doing the mutating operation. Most notably, this
happens when:
* Patching is done in parallel. Only one of the patches will succeed,
  the other ones will fail with a conflict
* Updates of objects that allow unconditional updates: All updates will
  succeed, but not all of them will increment the resource version (i.E
  dirty writes for the RV)
* An update for an object that allows createOnUpdate races with a create
  or delete
* A DeleteAllOf call races with Delete calls
* A scale update races with a normal update

This change:
* Adds tests for all of these cases
* Fixes them by adding a lock around the write operations, including
  their read part, if any

Author: alvaroaleman

pkg/client/fake/client.go

@@ -68,16 +68,21 @@ type versionedTracker struct {
 }
 
 type fakeClient struct {
-	tracker               versionedTracker
-	scheme                *runtime.Scheme
+	// trackerWriteLock must be acquired before writing to
+	// the tracker or performing reads that affect a following
+	// write.
+	trackerWriteLock sync.Mutex
+	tracker          versionedTracker
+
+	schemeWriteLock sync.Mutex
+	scheme          *runtime.Scheme
+
 	restMapper            meta.RESTMapper
 	withStatusSubresource sets.Set[schema.GroupVersionKind]
 
 	// indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK.
 	// The inner map maps from index name to IndexerFunc.
 	indexes map[schema.GroupVersionKind]map[string]client.IndexerFunc
-
-	schemeWriteLock sync.Mutex
 }
 
 var _ client.WithWatch = &fakeClient{}
@@ -467,6 +472,11 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt
 		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
@@ -732,6 +742,8 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
 		accessor.SetDeletionTimestamp(nil)
 	}
 
+	c.trackerWriteLock.Lock()
+	defer c.trackerWriteLock.Unlock()
 	return c.tracker.Create(gvr, obj, accessor.GetNamespace())
 }
 
@@ -753,6 +765,8 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
 		}
 	}
 
+	c.trackerWriteLock.Lock()
+	defer c.trackerWriteLock.Unlock()
 	// Check the ResourceVersion if that Precondition was specified.
 	if delOptions.Preconditions != nil && delOptions.Preconditions.ResourceVersion != nil {
 		name := accessor.GetName()
@@ -775,7 +789,7 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
 		}
 	}
 
-	return c.deleteObject(gvr, accessor)
+	return c.deleteObjectLocked(gvr, accessor)
 }
 
 func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
@@ -793,6 +807,9 @@ func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ..
 		}
 	}
 
+	c.trackerWriteLock.Lock()
+	defer c.trackerWriteLock.Unlock()
+
 	gvr, _ := meta.UnsafeGuessKindToResource(gvk)
 	o, err := c.tracker.List(gvr, gvk, dcOptions.Namespace)
 	if err != nil {
@@ -812,7 +829,7 @@ func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ..
 		if err != nil {
 			return err
 		}
-		err = c.deleteObject(gvr, accessor)
+		err = c.deleteObjectLocked(gvr, accessor)
 		if err != nil {
 			return err
 		}
@@ -842,6 +859,9 @@ func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.Upd
 	if err != nil {
 		return err
 	}
+
+	c.trackerWriteLock.Lock()
+	defer c.trackerWriteLock.Unlock()
 	return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus, false, *updateOptions.AsUpdateOptions())
 }
 
@@ -877,6 +897,8 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
 		return err
 	}
 
+	c.trackerWriteLock.Lock()
+	defer c.trackerWriteLock.Unlock()
 	oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName())
 	if err != nil {
 		return err
@@ -1085,7 +1107,7 @@ func (c *fakeClient) SubResource(subResource string) client.SubResourceClient {
 	return &fakeSubResourceClient{client: c, subResource: subResource}
 }
 
-func (c *fakeClient) deleteObject(gvr schema.GroupVersionResource, accessor metav1.Object) error {
+func (c *fakeClient) deleteObjectLocked(gvr schema.GroupVersionResource, accessor metav1.Object) error {
 	old, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName())
 	if err == nil {
 		oldAccessor, err := meta.Accessor(old)
@@ -1167,7 +1189,7 @@ func (sw *fakeSubResourceClient) Update(ctx context.Context, obj client.Object,
 
 	switch sw.subResource {
 	case subResourceScale:
-		if err := sw.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
+		if err := sw.client.Get(ctx, client.ObjectKeyFromObject(obj), obj.DeepCopyObject().(client.Object)); err != nil {
 			return err
 		}
 		if updateOptions.SubResourceBody == nil {