Skip to content

Commit 9fcd561

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#3143 from alvaroaleman/fix-threadsafety
🐛 Fakeclient: Fix dataraces when writing to the scheme
2 parents de4367f + eb07b0e commit 9fcd561

2 files changed

Lines changed: 108 additions & 4 deletions

File tree

‎pkg/client/fake/client.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,8 @@ type fakeClient struct {
8787
trackerWriteLock sync.Mutex
8888
tracker versionedTracker
8989

90-
schemeWriteLock sync.Mutex
91-
scheme *runtime.Scheme
90+
schemeLock sync.RWMutex
91+
scheme *runtime.Scheme
9292

9393
restMapper meta.RESTMapper
9494
withStatusSubresource sets.Set[schema.GroupVersionKind]
@@ -524,6 +524,8 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt
524524
}
525525

526526
func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
527+
c.schemeLock.RLock()
528+
defer c.schemeLock.RUnlock()
527529
gvr, err := getGVRFromObject(obj, c.scheme)
528530
if err != nil {
529531
return err
@@ -573,6 +575,8 @@ func (c *fakeClient) Watch(ctx context.Context, list client.ObjectList, opts ...
573575
}
574576

575577
func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...client.ListOption) error {
578+
c.schemeLock.RLock()
579+
defer c.schemeLock.RUnlock()
576580
gvk, err := apiutil.GVKForObject(obj, c.scheme)
577581
if err != nil {
578582
return err
@@ -585,9 +589,11 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
585589
if _, isUnstructuredList := obj.(runtime.Unstructured); isUnstructuredList && !c.scheme.Recognizes(gvk) {
586590
// We need to register the ListKind with UnstructuredList:
587591
// https://github.com/kubernetes/kubernetes/blob/7b2776b89fb1be28d4e9203bdeec079be903c103/staging/src/k8s.io/client-go/dynamic/fake/simple.go#L44-L51
588-
c.schemeWriteLock.Lock()
592+
c.schemeLock.RUnlock()
593+
c.schemeLock.Lock()
589594
c.scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{})
590-
c.schemeWriteLock.Unlock()
595+
c.schemeLock.Unlock()
596+
c.schemeLock.RLock()
591597
}
592598

593599
listOpts := client.ListOptions{}
@@ -738,6 +744,8 @@ func (c *fakeClient) IsObjectNamespaced(obj runtime.Object) (bool, error) {
738744
}
739745

740746
func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
747+
c.schemeLock.RLock()
748+
defer c.schemeLock.RUnlock()
741749
createOptions := &client.CreateOptions{}
742750
createOptions.ApplyOptions(opts)
743751

@@ -774,6 +782,8 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
774782
}
775783

776784
func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
785+
c.schemeLock.RLock()
786+
defer c.schemeLock.RUnlock()
777787
gvr, err := getGVRFromObject(obj, c.scheme)
778788
if err != nil {
779789
return err
@@ -819,6 +829,8 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
819829
}
820830

821831
func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
832+
c.schemeLock.RLock()
833+
defer c.schemeLock.RUnlock()
822834
gvk, err := apiutil.GVKForObject(obj, c.scheme)
823835
if err != nil {
824836
return err
@@ -868,6 +880,8 @@ func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...clie
868880
}
869881

870882
func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.UpdateOption) error {
883+
c.schemeLock.RLock()
884+
defer c.schemeLock.RUnlock()
871885
updateOptions := &client.UpdateOptions{}
872886
updateOptions.ApplyOptions(opts)
873887

@@ -896,6 +910,8 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
896910
}
897911

898912
func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
913+
c.schemeLock.RLock()
914+
defer c.schemeLock.RUnlock()
899915
patchOptions := &client.PatchOptions{}
900916
patchOptions.ApplyOptions(opts)
901917

‎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)