DeepCopy by default from the cache

Not DeepCopying can save some resources in some situations, but is also
a common way to introduce hard-to-find bugs, since items in the cache
can then mutate unexpectedly. This library now follows the pattern from
controller-runtime, which does a DeepCopy by default, which can be
disabled with an option when creating the client.

Signed-off-by: Justin Kulikauskas <[email protected]>

client/cache.go

@@ -75,6 +75,9 @@ type ObjectCacheOptions struct {
 	// The time for a failed GVK to GVR conversion to not be retried. The default behavior is to not cache failures.
 	// Setting this can be useful if you don't want to continuously query the Kubernetes API if a CRD is missing.
 	MissingAPIResourceCacheTTL time.Duration
+	// Whether to *skip* the DeepCopy when retrieving an object from the cache.
+	// Be careful when disabling deep copy: it makes it possible to mutate objects inside the cache.
+	UnsafeDisableDeepCopy bool
 }
 
 // NewObjectCache will create an object cache with the input discovery client.
@@ -164,7 +167,16 @@ func (o *objectCache) FromObjectIdentifier(objID ObjectIdentifier) ([]unstructur
 	}
 
 	// Type assertion checks aren't needed since this is the only method that stores data.
-	result := loadedResult.([]unstructured.Unstructured)
+	uncopiedResult := loadedResult.([]unstructured.Unstructured)
+
+	if o.options.UnsafeDisableDeepCopy {
+		return uncopiedResult, nil
+	}
+
+	result := make([]unstructured.Unstructured, 0, len(uncopiedResult))
+	for _, obj := range uncopiedResult {
+		result = append(result, *obj.DeepCopy())
+	}
 
 	return result, nil
 }

client/cache_test.go

@@ -143,3 +143,55 @@ var _ = Describe("Test the cache", Ordered, func() {
 		Expect(cachedObject).To(BeNil())
 	})
 })
+
+var _ = Describe("Test the UnsafeDisableDeepCopy option of the cache", func() {
+	var discoveryClient *discovery.DiscoveryClient
+	configMapGVK := schema.GroupVersionKind{
+		Version: "v1",
+		Kind:    "ConfigMap",
+	}
+
+	BeforeEach(func() {
+		var err error
+		discoveryClient, err = discovery.NewDiscoveryClientForConfig(k8sConfig)
+		Expect(err).ToNot(HaveOccurred())
+	})
+
+	It("Defaults to DeepCopy, preventing mutations", func() {
+		cache := NewObjectCache(discoveryClient, ObjectCacheOptions{})
+
+		object := unstructured.Unstructured{}
+		object.SetName("cached-object1")
+		object.SetLabels(map[string]string{"mylabel": "one"})
+		cache.CacheObject(configMapGVK, "default", "cached-object1", &object)
+
+		cachedObject1, err := cache.Get(configMapGVK, "default", "cached-object1")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cachedObject1.GetLabels()).To(HaveKeyWithValue("mylabel", "one"))
+
+		cachedObject1.SetLabels(map[string]string{"mylabel": "two"})
+
+		cachedObject2, err := cache.Get(configMapGVK, "default", "cached-object1")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cachedObject2.GetLabels()).To(HaveKeyWithValue("mylabel", "one"))
+	})
+
+	It("Can be set to disable the DeepCopy, allowing mutations", func() {
+		cache := NewObjectCache(discoveryClient, ObjectCacheOptions{UnsafeDisableDeepCopy: true})
+
+		object := &unstructured.Unstructured{}
+		object.SetName("cached-object1")
+		object.SetLabels(map[string]string{"mylabel": "one"})
+		cache.CacheObject(configMapGVK, "default", "cached-object1", object)
+
+		cachedObject1, err := cache.Get(configMapGVK, "default", "cached-object1")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cachedObject1.GetLabels()).To(HaveKeyWithValue("mylabel", "one"))
+
+		cachedObject1.SetLabels(map[string]string{"mylabel": "two"})
+
+		cachedObject2, err := cache.Get(configMapGVK, "default", "cached-object1")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cachedObject2.GetLabels()).To(HaveKeyWithValue("mylabel", "two"))
+	})
+})

client/client.go

@@ -57,7 +57,8 @@ type Options struct {
 	// DisableInitialReconcile causes the initial reconcile from the list request before the watch to not cause a
 	// reconcile. This is useful if you are exclusively using the caching query API.
 	DisableInitialReconcile bool
-	// Options for how long to cache GVK to GVR conversions.
+	// Options for how long to cache GVK to GVR conversions, and whether to disable the DeepCopy when retrieving items
+	// from the cache.
 	ObjectCacheOptions ObjectCacheOptions
 }