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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done -- doesn't look like there were test in place for the ListOptions so I added those as well

// 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
51 changes: 51 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1976,6 +1976,30 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC

})

It("should not deep copy results when UnsafeDisableDeepCopy is set", func() {
By("first creating the Deployment")
dep, err := clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
Expect(err).NotTo(HaveOccurred())

cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expect(cl).NotTo(BeNil())

By("fetching the created Deployment")
actual := &appsv1.Deployment{}
key := client.ObjectKey{Namespace: ns, Name: dep.Name}
err = cl.Get(context.TODO(), key, actual, client.UnsafeDisableDeepCopy)
Expect(err).NotTo(HaveOccurred())
Expect(actual).NotTo(BeNil())

newActual := &appsv1.Deployment{}
err = cl.Get(context.TODO(), key, newActual, client.UnsafeDisableDeepCopy)
Expect(err).NotTo(HaveOccurred())
Expect(newActual).NotTo(BeNil())

Expect(newActual).To(BeIdenticalTo(actual))
})

// Test this with an integrated type and a CRD to make sure it covers both proto
// and json deserialization.
for idx, object := range []client.Object{&corev1.ConfigMap{}, &pkg.ChaosPod{}} {
Expand Down Expand Up @@ -2647,6 +2671,33 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
Expect(deps.Items[1].Name).To(Equal(dep4.Name))
})

It("should not deep copy results when UnsafeDisableDeepCopy is set", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already tested, no need to add another:

Describe("use UnsafeDisableDeepCopy list options", func() {

By("creating an initial object")
_, err := clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
Expect(err).NotTo(HaveOccurred())

cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())

By("listing all objects of that type in the cluster")
deps := &appsv1.DeploymentList{}
Expect(cl.List(context.Background(), deps), client.UnsafeDisableDeepCopy).NotTo(HaveOccurred())

Expect(deps.Items).NotTo(BeEmpty())

By("listing all objects of that type again in the cluster")
newDeps := &appsv1.DeploymentList{}
Expect(cl.List(context.Background(), newDeps), client.UnsafeDisableDeepCopy).NotTo(HaveOccurred())

Expect(newDeps.Items).NotTo(BeEmpty())

Expect(newDeps).To(HaveLen(len(deps.Items)))
// Expect the elements to be the same since we didn't DeepCopy them
for i := range deps.Items {
Expect(deps.Items[i]).To(BeIdenticalTo(newDeps.Items[i]))
}
})

PIt("should fail if the object doesn't have meta", func() {

})
Expand Down
20 changes: 20 additions & 0 deletions pkg/client/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,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 +446,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,6 +701,17 @@ 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) {
definitelyTrue := true
definitelyFalse := false
if d {
opts.UnsafeDisableDeepCopy = &definitelyTrue
} else {
opts.UnsafeDisableDeepCopy = &definitelyFalse
}
}

// ApplyToList applies this configuration to the given an List options.
func (d UnsafeDisableDeepCopyOption) ApplyToList(opts *ListOptions) {
definitelyTrue := true
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