Skip to content

Commit eb07b0e

Browse files
alvaroalemanalvaroaleman
committed
🐛 Fakeclient: Fix dataraces when writing to the scheme
We have a scheme write lock but plenty of other codeptaths that read from the scheme and that don't do looking, resulting in dataraces if the two happen in parallel. This change introduces a simple RW lock and makes the fakeclient acquire read locking for all its operations except when needing the write lock. This isn't particularly smart, but given that we only have one codepath that writes to the scheme, it seems good enough.
1 parent 9f4e0f7 commit eb07b0e

File tree

2 files changed

+108
-4
lines changed

2 files changed

+108
-4
lines changed

‎pkg/client/fake/client.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ type fakeClient struct {
7575
trackerWriteLock sync.Mutex
7676
tracker versionedTracker
7777

78-
schemeWriteLock sync.Mutex
79-
scheme *runtime.Scheme
78+
schemeLock sync.RWMutex
79+
scheme *runtime.Scheme
8080

8181
restMapper meta.RESTMapper
8282
withStatusSubresource sets.Set[schema.GroupVersionKind]
@@ -512,6 +512,8 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt
512512
}
513513

514514
func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
515+
c.schemeLock.RLock()
516+
defer c.schemeLock.RUnlock()
515517
gvr, err := getGVRFromObject(obj, c.scheme)
516518
if err != nil {
517519
return err
@@ -561,6 +563,8 @@ func (c *fakeClient) Watch(ctx context.Context, list client.ObjectList, opts ...
561563
}
562564

563565
func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...client.ListOption) error {
566+
c.schemeLock.RLock()
567+
defer c.schemeLock.RUnlock()
564568
gvk, err := apiutil.GVKForObject(obj, c.scheme)
565569
if err != nil {
566570
return err
@@ -573,9 +577,11 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
573577
if _, isUnstructuredList := obj.(runtime.Unstructured); isUnstructuredList && !c.scheme.Recognizes(gvk) {
574578
// We need to register the ListKind with UnstructuredList:
575579
// https://github.com/kubernetes/kubernetes/blob/7b2776b89fb1be28d4e9203bdeec079be903c103/staging/src/k8s.io/client-go/dynamic/fake/simple.go#L44-L51
576-
c.schemeWriteLock.Lock()
580+
c.schemeLock.RUnlock()
581+
c.schemeLock.Lock()
577582
c.scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{})
578-
c.schemeWriteLock.Unlock()
583+
c.schemeLock.Unlock()
584+
c.schemeLock.RLock()
579585
}
580586

581587
listOpts := client.ListOptions{}
@@ -726,6 +732,8 @@ func (c *fakeClient) IsObjectNamespaced(obj runtime.Object) (bool, error) {
726732
}
727733

728734
func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
735+
c.schemeLock.RLock()
736+
defer c.schemeLock.RUnlock()
729737
createOptions := &client.CreateOptions{}
730738
createOptions.ApplyOptions(opts)
731739

@@ -762,6 +770,8 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
762770
}
763771

764772
func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
773+
c.schemeLock.RLock()
774+
defer c.schemeLock.RUnlock()
765775
gvr, err := getGVRFromObject(obj, c.scheme)
766776
if err != nil {
767777
return err
@@ -807,6 +817,8 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
807817
}
808818

809819
func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
820+
c.schemeLock.RLock()
821+
defer c.schemeLock.RUnlock()
810822
gvk, err := apiutil.GVKForObject(obj, c.scheme)
811823
if err != nil {
812824
return err
@@ -856,6 +868,8 @@ func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...clie
856868
}
857869

858870
func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.UpdateOption) error {
871+
c.schemeLock.RLock()
872+
defer c.schemeLock.RUnlock()
859873
updateOptions := &client.UpdateOptions{}
860874
updateOptions.ApplyOptions(opts)
861875

@@ -884,6 +898,8 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
884898
}
885899

886900
func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
901+
c.schemeLock.RLock()
902+
defer c.schemeLock.RUnlock()
887903
patchOptions := &client.PatchOptions{}
888904
patchOptions.ApplyOptions(opts)
889905

‎pkg/client/fake/client_test.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2516,6 +2516,93 @@ var _ = Describe("Fake client", func() {
25162516
Expect(cl.SubResource(subResourceScale).Update(context.Background(), obj, client.WithSubResourceBody(scale)).Error()).To(Equal(expectedErr))
25172517
})
25182518

2519+
It("is threadsafe", func() {
2520+
cl := NewClientBuilder().Build()
2521+
2522+
u := func() *unstructured.Unstructured {
2523+
u := &unstructured.Unstructured{}
2524+
u.SetAPIVersion("custom/v1")
2525+
u.SetKind("Version")
2526+
u.SetName("foo")
2527+
return u
2528+
}
2529+
2530+
uList := func() *unstructured.UnstructuredList {
2531+
u := &unstructured.UnstructuredList{}
2532+
u.SetAPIVersion("custom/v1")
2533+
u.SetKind("Version")
2534+
2535+
return u
2536+
}
2537+
2538+
meta := func() *metav1.PartialObjectMetadata {
2539+
return &metav1.PartialObjectMetadata{
2540+
ObjectMeta: metav1.ObjectMeta{
2541+
Name: "foo",
2542+
Namespace: "default",
2543+
},
2544+
TypeMeta: metav1.TypeMeta{
2545+
APIVersion: "custom/v1",
2546+
Kind: "Version",
2547+
},
2548+
}
2549+
}
2550+
metaList := func() *metav1.PartialObjectMetadataList {
2551+
return &metav1.PartialObjectMetadataList{
2552+
TypeMeta: metav1.TypeMeta{
2553+
2554+
APIVersion: "custom/v1",
2555+
Kind: "Version",
2556+
},
2557+
}
2558+
}
2559+
2560+
pod := func() *corev1.Pod {
2561+
return &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
2562+
Name: "foo",
2563+
Namespace: "default",
2564+
}}
2565+
}
2566+
2567+
ctx := context.Background()
2568+
ops := []func(){
2569+
func() { _ = cl.Create(ctx, u()) },
2570+
func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(u()), u()) },
2571+
func() { _ = cl.Update(ctx, u()) },
2572+
func() { _ = cl.Patch(ctx, u(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
2573+
func() { _ = cl.Delete(ctx, u()) },
2574+
func() { _ = cl.DeleteAllOf(ctx, u(), client.HasLabels{"foo"}) },
2575+
func() { _ = cl.List(ctx, uList()) },
2576+
2577+
func() { _ = cl.Create(ctx, meta()) },
2578+
func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(meta()), meta()) },
2579+
func() { _ = cl.Update(ctx, meta()) },
2580+
func() { _ = cl.Patch(ctx, meta(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
2581+
func() { _ = cl.Delete(ctx, meta()) },
2582+
func() { _ = cl.DeleteAllOf(ctx, meta(), client.HasLabels{"foo"}) },
2583+
func() { _ = cl.List(ctx, metaList()) },
2584+
2585+
func() { _ = cl.Create(ctx, pod()) },
2586+
func() { _ = cl.Get(ctx, client.ObjectKeyFromObject(pod()), pod()) },
2587+
func() { _ = cl.Update(ctx, pod()) },
2588+
func() { _ = cl.Patch(ctx, pod(), client.RawPatch(types.StrategicMergePatchType, []byte("foo"))) },
2589+
func() { _ = cl.Delete(ctx, pod()) },
2590+
func() { _ = cl.DeleteAllOf(ctx, pod(), client.HasLabels{"foo"}) },
2591+
func() { _ = cl.List(ctx, &corev1.PodList{}) },
2592+
}
2593+
2594+
wg := sync.WaitGroup{}
2595+
wg.Add(len(ops))
2596+
for _, op := range ops {
2597+
go func() {
2598+
defer wg.Done()
2599+
op()
2600+
}()
2601+
}
2602+
2603+
wg.Wait()
2604+
})
2605+
25192606
scalableObjs := []client.Object{
25202607
&appsv1.Deployment{
25212608
ObjectMeta: metav1.ObjectMeta{
@@ -2604,6 +2691,7 @@ var _ = Describe("Fake client", func() {
26042691
scaleExpected.ResourceVersion = scaleActual.ResourceVersion
26052692
Expect(cmp.Diff(scaleExpected, scaleActual)).To(BeEmpty())
26062693
})
2694+
26072695
}
26082696
})
26092697

0 commit comments

Comments
 (0)