From 121afaafc260d8d9e86f1cddb9edcb12a90658d5 Mon Sep 17 00:00:00 2001 From: Jonathan Innis Date: Tue, 20 May 2025 10:31:51 -0700 Subject: [PATCH] Add UnsafeDisableDeepCopy to GetOptions --- pkg/cache/cache_test.go | 52 +++++++++++++++++++----------- pkg/cache/internal/cache_reader.go | 9 ++++-- pkg/client/options.go | 23 +++++++++---- pkg/client/options_test.go | 26 +++++++++++++++ 4 files changed, 82 insertions(+), 28 deletions(-) diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 955c3cc020..33a176c2dc 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -2462,27 +2462,43 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca }) }) }) - Describe("use UnsafeDisableDeepCopy list options", func() { - It("should be able to change object in informer cache", func() { - By("listing pods") - out := corev1.PodList{} - Expect(informerCache.List(context.Background(), &out, client.UnsafeDisableDeepCopy)).To(Succeed()) - for _, item := range out.Items { - if strings.Compare(item.Name, "test-pod-3") == 0 { // test-pod-3 has labels - item.Labels["UnsafeDisableDeepCopy"] = "true" - break + Context("using UnsafeDisableDeepCopy", func() { + Describe("with ListOptions", func() { + It("should be able to change object in informer cache", func() { + By("listing pods") + out := corev1.PodList{} + Expect(informerCache.List(context.Background(), &out, client.UnsafeDisableDeepCopy)).To(Succeed()) + for _, item := range out.Items { + if strings.Compare(item.Name, "test-pod-3") == 0 { // test-pod-3 has labels + item.Labels["UnsafeDisableDeepCopy"] = "true" + break + } } - } - By("verifying that the returned pods were changed") - out2 := corev1.PodList{} - Expect(informerCache.List(context.Background(), &out, client.UnsafeDisableDeepCopy)).To(Succeed()) - for _, item := range out2.Items { - if strings.Compare(item.Name, "test-pod-3") == 0 { - Expect(item.Labels["UnsafeDisableDeepCopy"]).To(Equal("true")) - break + By("verifying that the returned pods were changed") + out2 := corev1.PodList{} + Expect(informerCache.List(context.Background(), &out, client.UnsafeDisableDeepCopy)).To(Succeed()) + for _, item := range out2.Items { + if strings.Compare(item.Name, "test-pod-3") == 0 { + Expect(item.Labels["UnsafeDisableDeepCopy"]).To(Equal("true")) + break + } } - } + }) + }) + Describe("with GetOptions", func() { + It("should be able to change object in informer cache", func() { + out := corev1.Pod{} + podKey := client.ObjectKey{Name: "test-pod-2", Namespace: testNamespaceTwo} + Expect(informerCache.Get(context.Background(), podKey, &out, client.UnsafeDisableDeepCopy)).To(Succeed()) + + out.Labels["UnsafeDisableDeepCopy"] = "true" + + By("verifying that the returned pod was changed") + out2 := corev1.Pod{} + Expect(informerCache.Get(context.Background(), podKey, &out2, client.UnsafeDisableDeepCopy)).To(Succeed()) + Expect(out2.Labels["UnsafeDisableDeepCopy"]).To(Equal("true")) + }) }) }) }) diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go index 33ce8a830a..eb6b544855 100644 --- a/pkg/cache/internal/cache_reader.go +++ b/pkg/cache/internal/cache_reader.go @@ -54,7 +54,10 @@ type CacheReader struct { } // Get checks the indexer for the object and writes a copy of it if found. -func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object, _ ...client.GetOption) error { +func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object, opts ...client.GetOption) error { + getOpts := client.GetOptions{} + getOpts.ApplyOptions(opts) + if c.scopeName == apimeta.RESTScopeNameRoot { key.Namespace = "" } @@ -81,7 +84,7 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Ob return fmt.Errorf("cache contained %T, which is not an Object", obj) } - if c.disableDeepCopy { + if c.disableDeepCopy || (getOpts.UnsafeDisableDeepCopy != nil && *getOpts.UnsafeDisableDeepCopy) { // skip deep copy which might be unsafe // you must DeepCopy any object before mutating it outside } else { @@ -97,7 +100,7 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Ob return fmt.Errorf("cache had type %s, but %s was asked for", objVal.Type(), outVal.Type()) } reflect.Indirect(outVal).Set(reflect.Indirect(objVal)) - if !c.disableDeepCopy { + if !c.disableDeepCopy && (getOpts.UnsafeDisableDeepCopy == nil || !*getOpts.UnsafeDisableDeepCopy) { out.GetObjectKind().SetGroupVersionKind(c.groupVersionKind) } diff --git a/pkg/client/options.go b/pkg/client/options.go index db50ed8feb..ad4b8182ac 100644 --- a/pkg/client/options.go +++ b/pkg/client/options.go @@ -21,6 +21,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + "k8s.io/utils/ptr" ) // {{{ "Functional" Option Interfaces @@ -431,6 +432,12 @@ type GetOptions struct { // Raw represents raw GetOptions, as passed to the API server. Note // that these may not be respected by all implementations of interface. Raw *metav1.GetOptions + + // UnsafeDisableDeepCopy indicates not to deep copy objects during get object. + // Be very careful with this, when enabled you must DeepCopy any object before mutating it, + // otherwise you will mutate the object in the cache. + // +optional + UnsafeDisableDeepCopy *bool } var _ GetOption = &GetOptions{} @@ -440,6 +447,9 @@ func (o *GetOptions) ApplyToGet(lo *GetOptions) { if o.Raw != nil { lo.Raw = o.Raw } + if o.UnsafeDisableDeepCopy != nil { + lo.UnsafeDisableDeepCopy = o.UnsafeDisableDeepCopy + } } // AsGetOptions returns these options as a flattened metav1.GetOptions. @@ -692,15 +702,14 @@ func (l Limit) ApplyToList(opts *ListOptions) { // otherwise you will mutate the object in the cache. type UnsafeDisableDeepCopyOption bool +// ApplyToGet applies this configuration to the given an Get options. +func (d UnsafeDisableDeepCopyOption) ApplyToGet(opts *GetOptions) { + opts.UnsafeDisableDeepCopy = ptr.To(bool(d)) +} + // ApplyToList applies this configuration to the given an List options. func (d UnsafeDisableDeepCopyOption) ApplyToList(opts *ListOptions) { - definitelyTrue := true - definitelyFalse := false - if d { - opts.UnsafeDisableDeepCopy = &definitelyTrue - } else { - opts.UnsafeDisableDeepCopy = &definitelyFalse - } + opts.UnsafeDisableDeepCopy = ptr.To(bool(d)) } // UnsafeDisableDeepCopy indicates not to deep copy objects during list objects. diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go index c6dc09b676..9ad092a288 100644 --- a/pkg/client/options_test.go +++ b/pkg/client/options_test.go @@ -66,6 +66,19 @@ var _ = Describe("ListOptions", func() { o.ApplyToList(newListOpts) Expect(newListOpts).To(Equal(o)) }) + It("Should set UnsafeDisableDeepCopy", func() { + definitelyTrue := true + o := &client.ListOptions{UnsafeDisableDeepCopy: &definitelyTrue} + newListOpts := &client.ListOptions{} + o.ApplyToList(newListOpts) + Expect(newListOpts).To(Equal(o)) + }) + It("Should set UnsafeDisableDeepCopy through option", func() { + listOpts := &client.ListOptions{} + client.UnsafeDisableDeepCopy.ApplyToList(listOpts) + Expect(listOpts.UnsafeDisableDeepCopy).ToNot(BeNil()) + Expect(*listOpts.UnsafeDisableDeepCopy).To(BeTrue()) + }) It("Should not set anything", func() { o := &client.ListOptions{} newListOpts := &client.ListOptions{} @@ -81,6 +94,19 @@ var _ = Describe("GetOptions", func() { o.ApplyToGet(newGetOpts) Expect(newGetOpts).To(Equal(o)) }) + It("Should set UnsafeDisableDeepCopy", func() { + definitelyTrue := true + o := &client.GetOptions{UnsafeDisableDeepCopy: &definitelyTrue} + newGetOpts := &client.GetOptions{} + o.ApplyToGet(newGetOpts) + Expect(newGetOpts).To(Equal(o)) + }) + It("Should set UnsafeDisableDeepCopy through option", func() { + getOpts := &client.GetOptions{} + client.UnsafeDisableDeepCopy.ApplyToGet(getOpts) + Expect(getOpts.UnsafeDisableDeepCopy).ToNot(BeNil()) + Expect(*getOpts.UnsafeDisableDeepCopy).To(BeTrue()) + }) }) var _ = Describe("CreateOptions", func() {