Skip to content

✨ Add UnsafeDisableDeepCopy to GetOptions #3227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 34 additions & 18 deletions pkg/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
})
})
})
})
Expand Down
9 changes: 6 additions & 3 deletions pkg/cache/internal/cache_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
}
Expand All @@ -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 {
Expand All @@ -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)
}

Expand Down
23 changes: 16 additions & 7 deletions pkg/client/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{}
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
26 changes: 26 additions & 0 deletions pkg/client/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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() {
Expand Down
Loading