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 <[email protected]>
kubernetes-sigs · Sep 13, 2023 · f27befe · f27befe
1 parent 942d53b
commit f27befe
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 10 deletions.
diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go
@@ -194,23 +194,32 @@ 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
 		}
 		return New(config, *combined)
 	}
 }
 
+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
@@ -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())