fix: cache.BuilderWithOptions inherit options from caller

using cache.BuilderWithOptions does not properly inherit all
options passed in from the caller:
- scheme was overridden instead of merged
- selectors were not inherited at all, even if specified options
  selectors remained unset
- transforms were not inherited at all, even if specified options
  transforms remained unset
- disable deep copy settings were not inherited at all, even if
  specified options for disabling deep copies remained unset

This commit resolves this issues by implementing merge logic for all
fields in the cache.Options struct:
- Schemes are merged
- RESTMapper is chosen by precedence only falling back to inherited
  options if left unset in specified options
- Resync is chosen by precedence only falling back to inherited
  options if left unset in specified options
- Namespace is chosen by precedence only falling back to inherited
  options if left unset in specified options
- Selectors are merged. If both inherited and specified Options defined
  selectors for a given type, those selectors are merged via logical
  AND.
- DisableDeepCopy is combined via precedence. Only if a value for a
  particular GVK is unset in the specified options will a value from the
  inherited options be used.
- Transform functions are combined via chaining. If both inherited and
  specified options define a transform function, the transform function
  from the inherited options will be called first, and the transform
  function from the specified options will be called second.

Signed-off-by: Joe Lanford <[email protected]>

Author: joelanford

go.mod

@@ -1,6 +1,6 @@
 module sigs.k8s.io/controller-runtime
 
-go 1.17
+go 1.18
 
 require (
 	github.com/evanphx/json-patch/v5 v5.6.0

go.sum

@@ -19,14 +19,18 @@ package cache
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"time"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	toolscache "k8s.io/client-go/tools/cache"
+
 	"sigs.k8s.io/controller-runtime/pkg/cache/internal"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -150,49 +154,244 @@ func New(config *rest.Config, opts Options) (Cache, error) {
 	if err != nil {
 		return nil, err
 	}
-	selectorsByGVK, err := convertToSelectorsByGVK(opts.SelectorsByObject, opts.DefaultSelector, opts.Scheme)
+	selectorsByGVK, err := convertToByGVK(opts.SelectorsByObject, opts.DefaultSelector, opts.Scheme)
 	if err != nil {
 		return nil, err
 	}
 	disableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(opts.UnsafeDisableDeepCopyByObject, opts.Scheme)
 	if err != nil {
 		return nil, err
 	}
-	transformByGVK, err := convertToTransformByKindAndGVK(opts.TransformByObject, opts.DefaultTransform, opts.Scheme)
+	transformByGVK, err := convertToByGVK(opts.TransformByObject, opts.DefaultTransform, opts.Scheme)
 	if err != nil {
 		return nil, err
 	}
+	transformByObj := internal.TransformFuncByObjectFromMap(transformByGVK)
+
+	internalSelectorsByGVK := internal.SelectorsByGVK{}
+	for gvk, selector := range selectorsByGVK {
+		internalSelectorsByGVK[gvk] = internal.Selector(selector)
+	}
 
-	im := internal.NewInformersMap(config, opts.Scheme, opts.Mapper, *opts.Resync, opts.Namespace, selectorsByGVK, disableDeepCopyByGVK, transformByGVK)
+	im := internal.NewInformersMap(config, opts.Scheme, opts.Mapper, *opts.Resync, opts.Namespace, internalSelectorsByGVK, disableDeepCopyByGVK, transformByObj)
 	return &informerCache{InformersMap: im}, nil
 }
 
-// BuilderWithOptions returns a Cache constructor that will build the a cache
+// BuilderWithOptions returns a Cache constructor that will build a cache
 // honoring the options argument, this is useful to specify options like
 // SelectorsByObject
 // WARNING: if SelectorsByObject is specified. filtered out resources are not
 //          returned.
 // WARNING: if UnsafeDisableDeepCopy is enabled, you must DeepCopy any object
 //          returned from cache get/list before mutating it.
 func BuilderWithOptions(options Options) NewCacheFunc {
-	return func(config *rest.Config, opts Options) (Cache, error) {
-		if options.Scheme == nil {
-			options.Scheme = opts.Scheme
+	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)
+		if err != nil {
+			return nil, err
+		}
+		return New(config, *combined)
+	}
+}
+
+func (options Options) inheritFrom(inherited Options) (*Options, error) {
+	var (
+		combined Options
+		err      error
+	)
+	combined.Scheme = combineScheme(inherited.Scheme, options.Scheme)
+	combined.Mapper = selectMapper(inherited.Mapper, options.Mapper)
+	combined.Resync = selectResync(inherited.Resync, options.Resync)
+	combined.Namespace = selectNamespace(inherited.Namespace, options.Namespace)
+	combined.SelectorsByObject, combined.DefaultSelector, err = combineSelectors(inherited, options, combined.Scheme)
+	if err != nil {
+		return nil, fmt.Errorf("combine selectors: %v", err)
+	}
+	combined.UnsafeDisableDeepCopyByObject, err = combineUnsafeDeepCopy(inherited, options, combined.Scheme)
+	if err != nil {
+		return nil, fmt.Errorf("combine unsafe disable deep copy by object: %v", err)
+	}
+
+	combined.TransformByObject, combined.DefaultTransform, err = combineTransforms(inherited, options, combined.Scheme)
+	if err != nil {
+		return nil, fmt.Errorf("combine transforms: %v", err)
+	}
+
+	return &combined, nil
+}
+
+func combineScheme(schemes ...*runtime.Scheme) *runtime.Scheme {
+	var out *runtime.Scheme
+	for _, sch := range schemes {
+		if sch == nil {
+			continue
+		}
+		for gvk, t := range sch.AllKnownTypes() {
+			if out == nil {
+				out = runtime.NewScheme()
+			}
+			out.AddKnownTypeWithName(gvk, reflect.New(t).Interface().(runtime.Object))
+		}
+	}
+	return out
+}
+
+func selectMapper(def, override meta.RESTMapper) meta.RESTMapper {
+	if override != nil {
+		return override
+	}
+	return def
+}
+
+func selectResync(def, override *time.Duration) *time.Duration {
+	if override != nil {
+		return override
+	}
+	return def
+}
+
+func selectNamespace(def, override string) string {
+	if override != "" {
+		return override
+	}
+	return def
+}
+
+func combineSelectors(inherited, options Options, scheme *runtime.Scheme) (SelectorsByObject, ObjectSelector, error) {
+	// Selectors are combined via logical AND.
+	//  - Combined label selector is a union of the selectors requirements from both sets of options.
+	//  - Combined field selector uses fields.AndSelectors with the combined list of non-nil field selectors
+	//    defined in both sets of options.
+	//
+	// There is a bunch of complexity here because we need to convert to SelectorsByGVK
+	// to be able to match keys between options and inherited and then convert back to SelectorsByObject
+	optionsSelectorsByGVK, err := convertToByGVK(options.SelectorsByObject, options.DefaultSelector, options.Scheme)
+	if err != nil {
+		return nil, ObjectSelector{}, err
+	}
+	inheritedSelectorsByGVK, err := convertToByGVK(inherited.SelectorsByObject, inherited.DefaultSelector, inherited.Scheme)
+	if err != nil {
+		return nil, ObjectSelector{}, err
+	}
+
+	for gvk, selector := range inheritedSelectorsByGVK {
+		optionsSelectorsByGVK[gvk] = combineSelector(optionsSelectorsByGVK[gvk], selector)
+	}
+	return convertToByObject(optionsSelectorsByGVK, scheme)
+}
+
+func combineSelector(selectors ...ObjectSelector) ObjectSelector {
+	ls := make([]labels.Selector, 0, len(selectors))
+	fs := make([]fields.Selector, 0, len(selectors))
+	for _, s := range selectors {
+		ls = append(ls, s.Label)
+		fs = append(fs, s.Field)
+	}
+	return ObjectSelector{
+		Label: combineLabelSelectors(ls...),
+		Field: combineFieldSelectors(fs...),
+	}
+}
+
+func combineLabelSelectors(ls ...labels.Selector) labels.Selector {
+	var combined labels.Selector
+	for _, l := range ls {
+		if l == nil {
+			continue
 		}
-		if options.Mapper == nil {
-			options.Mapper = opts.Mapper
+		if combined == nil {
+			combined = labels.NewSelector()
 		}
-		if options.Resync == nil {
-			options.Resync = opts.Resync
+		reqs, _ := l.Requirements()
+		combined = combined.Add(reqs...)
+	}
+	return combined
+}
+
+func combineFieldSelectors(fs ...fields.Selector) fields.Selector {
+	nonNil := fs[:0]
+	for _, f := range fs {
+		if f == nil {
+			continue
 		}
-		if options.Namespace == "" {
-			options.Namespace = opts.Namespace
+		nonNil = append(nonNil, f)
+	}
+	if len(nonNil) == 0 {
+		return nil
+	}
+	if len(nonNil) == 1 {
+		return nonNil[0]
+	}
+	return fields.AndSelectors(nonNil...)
+}
+
+func combineUnsafeDeepCopy(inherited, options Options, scheme *runtime.Scheme) (DisableDeepCopyByObject, error) {
+	// UnsafeDisableDeepCopyByObject is combined via precedence. Only if a value for a particular GVK is unset
+	// in options will a value from inherited be used.
+	optionsDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(options.UnsafeDisableDeepCopyByObject, options.Scheme)
+	if err != nil {
+		return nil, err
+	}
+	inheritedDisableDeepCopyByGVK, err := convertToDisableDeepCopyByGVK(inherited.UnsafeDisableDeepCopyByObject, inherited.Scheme)
+	if err != nil {
+		return nil, err
+	}
+
+	for gvk, inheritedDeepCopy := range inheritedDisableDeepCopyByGVK {
+		if _, ok := optionsDisableDeepCopyByGVK[gvk]; !ok {
+			if optionsDisableDeepCopyByGVK == nil {
+				optionsDisableDeepCopyByGVK = map[schema.GroupVersionKind]bool{}
+			}
+			optionsDisableDeepCopyByGVK[gvk] = inheritedDeepCopy
 		}
-		if opts.Resync == nil {
-			opts.Resync = options.Resync
+	}
+	return convertToDisableDeepCopyByObject(optionsDisableDeepCopyByGVK, scheme)
+}
+
+func combineTransforms(inherited, options Options, scheme *runtime.Scheme) (TransformByObject, toolscache.TransformFunc, error) {
+	// Transform functions are combined via chaining. If both inherited and options define a transform
+	// function, the transform function from inherited will be called first, and the transform function from
+	// options will be called second.
+	optionsTransformByGVK, err := convertToByGVK(options.TransformByObject, options.DefaultTransform, options.Scheme)
+	if err != nil {
+		return nil, nil, err
+	}
+	inheritedTransformByGVK, err := convertToByGVK(inherited.TransformByObject, inherited.DefaultTransform, inherited.Scheme)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	for gvk, inheritedTransform := range inheritedTransformByGVK {
+		if optionsTransformByGVK == nil {
+			optionsTransformByGVK = map[schema.GroupVersionKind]toolscache.TransformFunc{}
 		}
+		optionsTransformByGVK[gvk] = combineTransform(inheritedTransform, optionsTransformByGVK[gvk])
+	}
+	return convertToByObject(optionsTransformByGVK, scheme)
+}
 
-		return New(config, options)
+func combineTransform(inherited, current toolscache.TransformFunc) toolscache.TransformFunc {
+	if inherited == nil {
+		return current
+	}
+	if current == nil {
+		return inherited
+	}
+	return func(in interface{}) (interface{}, error) {
+		mid, err := inherited(in)
+		if err != nil {
+			return nil, err
+		}
+		return current(mid)
 	}
 }
 
@@ -219,17 +418,40 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
 	return opts, nil
 }
 
-func convertToSelectorsByGVK(selectorsByObject SelectorsByObject, defaultSelector ObjectSelector, scheme *runtime.Scheme) (internal.SelectorsByGVK, error) {
-	selectorsByGVK := internal.SelectorsByGVK{}
-	for object, selector := range selectorsByObject {
+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 {
 		gvk, err := apiutil.GVKForObject(object, scheme)
 		if err != nil {
 			return nil, err
 		}
-		selectorsByGVK[gvk] = internal.Selector(selector)
+		byGVK[gvk] = value
+	}
+	byGVK[schema.GroupVersionKind{}] = def
+	return byGVK, nil
+}
+
+func convertToByObject[T any](byGVK map[schema.GroupVersionKind]T, scheme *runtime.Scheme) (map[client.Object]T, T, error) {
+	var byObject map[client.Object]T
+	def := byGVK[schema.GroupVersionKind{}]
+	for gvk, value := range byGVK {
+		if gvk == (schema.GroupVersionKind{}) {
+			continue
+		}
+		obj, err := scheme.New(gvk)
+		if err != nil {
+			return nil, def, err
+		}
+		cObj, ok := obj.(client.Object)
+		if !ok {
+			return nil, def, fmt.Errorf("object %T for GVK %q does not implement client.Object", obj, gvk)
+		}
+		if byObject == nil {
+			byObject = map[client.Object]T{}
+		}
+		byObject[cObj] = value
 	}
-	selectorsByGVK[schema.GroupVersionKind{}] = internal.Selector(defaultSelector)
-	return selectorsByGVK, nil
+	return byObject, def, nil
 }
 
 // DisableDeepCopyByObject associate a client.Object's GVK to disable DeepCopy during get or list from cache.
@@ -259,17 +481,30 @@ func convertToDisableDeepCopyByGVK(disableDeepCopyByObject DisableDeepCopyByObje
 	return disableDeepCopyByGVK, nil
 }
 
-// TransformByObject associate a client.Object's GVK to a transformer function
-// to be applied when storing the object into the cache.
-type TransformByObject map[client.Object]toolscache.TransformFunc
-
-func convertToTransformByKindAndGVK(t TransformByObject, defaultTransform toolscache.TransformFunc, scheme *runtime.Scheme) (internal.TransformFuncByObject, error) {
-	result := internal.NewTransformFuncByObject()
-	for obj, transformation := range t {
-		if err := result.Set(obj, scheme, transformation); err != nil {
+func convertToDisableDeepCopyByObject(byGVK internal.DisableDeepCopyByGVK, scheme *runtime.Scheme) (DisableDeepCopyByObject, error) {
+	var byObject DisableDeepCopyByObject
+	for gvk, value := range byGVK {
+		if byObject == nil {
+			byObject = DisableDeepCopyByObject{}
+		}
+		if gvk == (schema.GroupVersionKind{}) {
+			byObject[ObjectAll{}] = value
+			continue
+		}
+		obj, err := scheme.New(gvk)
+		if err != nil {
 			return nil, err
 		}
+		cObj, ok := obj.(client.Object)
+		if !ok {
+			return nil, fmt.Errorf("object %T for GVK %q does not implement client.Object", obj, gvk)
+		}
+
+		byObject[cObj] = value
 	}
-	result.SetDefault(defaultTransform)
-	return result, nil
+	return byObject, nil
 }
+
+// TransformByObject associate a client.Object's GVK to a transformer function
+// to be applied when storing the object into the cache.
+type TransformByObject map[client.Object]toolscache.TransformFunc

pkg/cache/cache.go

@@ -26,7 +26,6 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
-
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"