Merge pull request #3146 from k8s-infra-cherrypick-robot/cherry-pick-3143-to-release-0.19

[release-0.19] 🐛 Fakeclient: Fix dataraces when writing to the scheme

Author: k8s-ci-robot

Diff for: pkg/client/fake/client.go

@@ -74,8 +74,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]
@@ -509,6 +509,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
@@ -558,6 +560,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
@@ -570,9 +574,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{}
@@ -712,6 +718,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)
 
@@ -748,6 +756,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
@@ -793,6 +803,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
@@ -842,6 +854,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)
 
@@ -870,6 +884,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)
 

Diff for: pkg/client/fake/client_test.go

@@ -2409,6 +2409,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{
@@ -2497,6 +2584,7 @@ var _ = Describe("Fake client", func() {
 			scaleExpected.ResourceVersion = scaleActual.ResourceVersion
 			Expect(cmp.Diff(scaleExpected, scaleActual)).To(BeEmpty())
 		})
+
 	}
 })