From f27befe711d75c54a6cfb24c5292f2dd7a49ff27 Mon Sep 17 00:00:00 2001
From: Alex Kalenyuk <akalenyu@redhat.com>
Date: Mon, 11 Sep 2023 13:48:32 +0300
Subject: [PATCH] bug: inherited defaults not respected in cache
 BuilderWithOptions

When using BuilderWithOptions the inherited (manager/cluster) options
are lost. If they were provided, they are stronger than the cache package defaults.
(Overrides default dynamic mapper in my case)

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
---
 pkg/cache/cache.go           | 44 ++++++++++++++++++++++++++++--------
 pkg/cache/cache_unit_test.go | 15 ++++++++++++
 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go
index bcb1141a50..5788a274b0 100644
--- a/pkg/cache/cache.go
+++ b/pkg/cache/cache.go
@@ -194,16 +194,7 @@ func New(config *rest.Config, opts Options) (Cache, error) {
 // returned from cache get/list before mutating it.
 func BuilderWithOptions(options Options) NewCacheFunc {
 	return func(config *rest.Config, inherited Options) (Cache, error) {
-		var err error
-		inherited, err = defaultOpts(config, inherited)
-		if err != nil {
-			return nil, err
-		}
-		options, err = defaultOpts(config, options)
-		if err != nil {
-			return nil, err
-		}
-		combined, err := options.inheritFrom(inherited)
+		combined, err := options.combinedOpts(config, inherited)
 		if err != nil {
 			return nil, err
 		}
@@ -211,6 +202,24 @@ func BuilderWithOptions(options Options) NewCacheFunc {
 	}
 }
 
+func (options Options) combinedOpts(config *rest.Config, inherited Options) (*Options, error) {
+	var err error
+	inherited, err = defaultOpts(config, inherited)
+	if err != nil {
+		return nil, err
+	}
+	options = defaultToInheritedOpts(options, inherited)
+	options, err = defaultOpts(config, options)
+	if err != nil {
+		return nil, err
+	}
+	combined, err := options.inheritFrom(inherited)
+	if err != nil {
+		return nil, err
+	}
+	return combined, nil
+}
+
 func (options Options) inheritFrom(inherited Options) (*Options, error) {
 	var (
 		combined Options
@@ -424,6 +433,21 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
 	return opts, nil
 }
 
+func defaultToInheritedOpts(opts, inherited Options) Options {
+	if opts.Scheme == nil {
+		opts.Scheme = inherited.Scheme
+	}
+
+	if opts.Mapper == nil {
+		opts.Mapper = inherited.Mapper
+	}
+
+	if opts.Resync == nil {
+		opts.Resync = inherited.Resync
+	}
+	return opts
+}
+
 func convertToByGVK[T any](byObject map[client.Object]T, def T, scheme *runtime.Scheme) (map[schema.GroupVersionKind]T, error) {
 	byGVK := map[schema.GroupVersionKind]T{}
 	for object, value := range byObject {
diff --git a/pkg/cache/cache_unit_test.go b/pkg/cache/cache_unit_test.go
index 059bce97cb..9076cc3cb5 100644
--- a/pkg/cache/cache_unit_test.go
+++ b/pkg/cache/cache_unit_test.go
@@ -73,6 +73,21 @@ var _ = Describe("cache.inheritFrom", func() {
 			Expect(checkError(specified.inheritFrom(inherited)).Scheme.AllKnownTypes()).To(HaveLen(2))
 		})
 	})
+	Context("Post defaulting of specified", func() {
+		It("inherited not lost", func() {
+			inherited.Scheme = runtime.NewScheme()
+			inherited.Scheme.AddKnownTypes(gv, &unstructured.Unstructured{})
+			Expect(inherited.Scheme.KnownTypes(gv)).To(HaveLen(1))
+			inherited.Mapper = meta.NewDefaultRESTMapper([]schema.GroupVersion{gv})
+			inherited.Resync = pointer.Duration(time.Minute)
+
+			combined := checkError(specified.combinedOpts(nil, inherited))
+			Expect(combined.Mapper).To(Equal(inherited.Mapper))
+			Expect(combined.Resync).To(Equal(inherited.Resync))
+			Expect(combined.Scheme).NotTo(BeNil())
+			Expect(combined.Scheme.KnownTypes(gv)).To(HaveLen(1))
+		})
+	})
 	Context("Mapper", func() {
 		It("is nil when specified and inherited are unset", func() {
 			Expect(checkError(specified.inheritFrom(inherited)).Mapper).To(BeNil())