Repository: kubernetes-sigs/controller-runtime

Issue:
Fake client scheme locking is incomplete
In the fake client, we have a codepath where we write to the scheme. We have a `SchemeWriteLock` for that, however there are plenty of other paths that read from the scheme and they do not have any locking at all. As a result, hitting that scheme-writing codepath in parallel with anything else almost always produces a datarace.

We need a RW lock and to properly lock around everything that accesses the scheme.

Discussion:

Solution patch:
diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go
index 69bc3d66..99124e2f 100644
--- a/pkg/client/fake/client.go
+++ b/pkg/client/fake/client.go
@@ -29,7 +29,19 @@ import (
 	"sync"
 	"time"
 
-	// Using v4 to match upstream
+	/*
+	  Stick with gopkg.in/evanphx/json-patch.v4 here to match
+	  upstream Kubernetes code and avoid breaking changes introduced in v5.
+	  - Kubernetes itself remains on json-patch v4 to avoid compatibility issues
+	    tied to v5’s stricter RFC6902 compliance.
+	  - The fake client code is adapted from client-go’s testing fixture, which also
+	    relies on json-patch v4.
+	  See:
+	    https://github.com/kubernetes/kubernetes/pull/91622 (discussion of why K8s
+	    stays on v4)
+	    https://github.com/kubernetes/kubernetes/pull/120326 (v5.6.0+incompatible
+	    missing a critical fix)
+	*/
 	jsonpatch "gopkg.in/evanphx/json-patch.v4"
 	appsv1 "k8s.io/api/apps/v1"
 	authenticationv1 "k8s.io/api/authentication/v1"
@@ -75,8 +87,8 @@ type fakeClient struct {
 	trackerWriteLock sync.Mutex
 	tracker          versionedTracker
 
-	schemeWriteLock sync.Mutex
-	scheme          *runtime.Scheme
+	schemeLock sync.RWMutex
+	scheme     *runtime.Scheme
 
 	restMapper            meta.RESTMapper
 	withStatusSubresource sets.Set[schema.GroupVersionKind]
@@ -512,6 +524,8 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt
 }
 
 func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	gvr, err := getGVRFromObject(obj, c.scheme)
 	if err != nil {
 		return err
@@ -561,6 +575,8 @@ func (c *fakeClient) Watch(ctx context.Context, list client.ObjectList, opts ...
 }
 
 func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...client.ListOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	gvk, err := apiutil.GVKForObject(obj, c.scheme)
 	if err != nil {
 		return err
@@ -573,9 +589,11 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
 	if _, isUnstructuredList := obj.(runtime.Unstructured); isUnstructuredList && !c.scheme.Recognizes(gvk) {
 		// We need to register the ListKind with UnstructuredList:
 		// https://github.com/kubernetes/kubernetes/blob/7b2776b89fb1be28d4e9203bdeec079be903c103/staging/src/k8s.io/client-go/dynamic/fake/simple.go#L44-L51
-		c.schemeWriteLock.Lock()
+		c.schemeLock.RUnlock()
+		c.schemeLock.Lock()
 		c.scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{})
-		c.schemeWriteLock.Unlock()
+		c.schemeLock.Unlock()
+		c.schemeLock.RLock()
 	}
 
 	listOpts := client.ListOptions{}
@@ -726,6 +744,8 @@ func (c *fakeClient) IsObjectNamespaced(obj runtime.Object) (bool, error) {
 }
 
 func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	createOptions := &client.CreateOptions{}
 	createOptions.ApplyOptions(opts)
 
@@ -762,6 +782,8 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
 }
 
 func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	gvr, err := getGVRFromObject(obj, c.scheme)
 	if err != nil {
 		return err
@@ -807,6 +829,8 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
 }
 
 func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	gvk, err := apiutil.GVKForObject(obj, c.scheme)
 	if err != nil {
 		return err
@@ -856,6 +880,8 @@ func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...clie
 }
 
 func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.UpdateOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	updateOptions := &client.UpdateOptions{}
 	updateOptions.ApplyOptions(opts)
 
@@ -884,6 +910,8 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
 }
 
 func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+	c.schemeLock.RLock()
+	defer c.schemeLock.RUnlock()
 	patchOptions := &client.PatchOptions{}
 	patchOptions.ApplyOptions(opts)
 
@@ -1138,7 +1166,7 @@ func (c *fakeClient) deleteObjectLocked(gvr schema.GroupVersionResource, accesso
 		}
 	}
 
-	//TODO: implement propagation
+	// TODO: implement propagation
 	return c.tracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName())
 }
 



Test patch:
diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go
index db768cca..f6a493f5 100644
--- a/pkg/client/fake/client_test.go
+++ b/pkg/client/fake/client_test.go
@@ -2516,6 +2516,93 @@ var _ = Describe("Fake client", func() {
 		Expect(cl.SubResource(subResourceScale).Update(context.Background(), obj, client.WithSubResourceBody(scale)).Error()).To(Equal(expectedErr))
 	})
 
+	It("is threadsafe", func() {
+		cl := NewClientBuilder().Build()
+
+		u := func() *unstructured.Unstructured {
+			u := &unstructured.Unstructured{}
+			u.SetAPIVersion("custom/v1")
+			u.SetKind("Version")
+			u.SetName("foo")
+			return u
+		}
+
+		uList := func() *unstructured.UnstructuredList {
+			u := &unstructured.UnstructuredList{}
+			u.SetAPIVersion("custom/v1")
+			u.SetKind("Version")
+
+			return u
+		}
+
+		meta := func() *metav1.PartialObjectMetadata {
+			return &metav1.PartialObjectMetadata{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "default",
+				},
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "custom/v1",
+					Kind:       "Version",
+				},
+			}
+		}
+		metaList := func() *metav1.PartialObjectMetadataList {
+			return &metav1.PartialObjectMetadataList{
+				TypeMeta: metav1.TypeMeta{
+
+					APIVersion: "custom/v1",
+					Kind:       "Version",
+				},
+			}
+		}
+
+		pod := func() *corev1.Pod {
+			return &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
+				Name:      "foo",
+				Namespace: "default",
+			}}
+		}
+
+		ctx := context.Background()
+		ops := []func(){
+			func() { _ = cl.Create(ctx, u()) },
+			func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(u()), u()) },
+			func() { _ = cl.Update(ctx, u()) },
+			func() { _ = cl.Patch(ctx, u(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
+			func() { _ = cl.Delete(ctx, u()) },
+			func() { _ = cl.DeleteAllOf(ctx, u(), client.HasLabels{"foo"}) },
+			func() { _ = cl.List(ctx, uList()) },
+
+			func() { _ = cl.Create(ctx, meta()) },
+			func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(meta()), meta()) },
+			func() { _ = cl.Update(ctx, meta()) },
+			func() { _ = cl.Patch(ctx, meta(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
+			func() { _ = cl.Delete(ctx, meta()) },
+			func() { _ = cl.DeleteAllOf(ctx, meta(), client.HasLabels{"foo"}) },
+			func() { _ = cl.List(ctx, metaList()) },
+
+			func() { _ = cl.Create(ctx, pod()) },
+			func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(pod()), pod()) },
+			func() { _ = cl.Update(ctx, pod()) },
+			func() { _ = cl.Patch(ctx, pod(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
+			func() { _ = cl.Delete(ctx, pod()) },
+			func() { _ = cl.DeleteAllOf(ctx, pod(), client.HasLabels{"foo"}) },
+			func() { _ = cl.List(ctx, &corev1.PodList{}) },
+		}
+
+		wg := sync.WaitGroup{}
+		wg.Add(len(ops))
+		for _, op := range ops {
+			go func() {
+				defer wg.Done()
+				op()
+			}()
+		}
+
+		wg.Wait()
+	})
+
 	scalableObjs := []client.Object{
 		&appsv1.Deployment{
 			ObjectMeta: metav1.ObjectMeta{
@@ -2604,6 +2691,7 @@ var _ = Describe("Fake client", func() {
 			scaleExpected.ResourceVersion = scaleActual.ResourceVersion
 			Expect(cmp.Diff(scaleExpected, scaleActual)).To(BeEmpty())
 		})
+
 	}
 })