Skip to content

Commit

Permalink
🐛 Cache: Keep selectors when byObject.Namespaces is defaulted
Browse files Browse the repository at this point in the history 
Prior to this patch, configuring for example a labelSelector in
`ByObject` and then inheriting namespaces from `DefaultNamespaces` meant
that the `labelSelector` would be ignored. This is because if namespaces
are configured, we set p a multinamespace cache. If we do that, we
expect each namespace entry to have the appropriate selectors
configured.

Unfortunately we defaulted the configs for`byObject.Namespaces` before
defaulting `byObject.Namespace` itself, causing the above-described
issue.

This change also adds a couple more tests for the cache defaulting.
  • Loading branch information
@alvaroaleman
alvaroaleman committed Mar 31, 2024
1 parent 5615941 commit e8ef21b
Show file tree
Hide file tree
Showing 2 changed files with 183 additions and 15 deletions.
42 changes: 27 additions & 15 deletions pkg/cache/cache.go
View file
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"net/http"
"sort"
"time"

"golang.org/x/exp/maps"
Expand Down Expand Up @@ -421,7 +422,12 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
for namespace, cfg := range opts.DefaultNamespaces {
cfg = defaultConfig(cfg, optionDefaultsToConfig(&opts))
if namespace == metav1.NamespaceAll {
cfg.FieldSelector = fields.AndSelectors(appendIfNotNil(namespaceAllSelector(maps.Keys(opts.DefaultNamespaces)), cfg.FieldSelector)...)
cfg.FieldSelector = fields.AndSelectors(
appendIfNotNil(
namespaceAllSelector(maps.Keys(opts.DefaultNamespaces)),
cfg.FieldSelector,
)...,
)
}
opts.DefaultNamespaces[namespace] = cfg
}
Expand All @@ -435,7 +441,12 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
return opts, fmt.Errorf("type %T is not namespaced, but its ByObject.Namespaces setting is not nil", obj)
}

// Default the namespace-level configs first, because they need to use the undefaulted type-level config.
if isNamespaced && byObject.Namespaces == nil {
byObject.Namespaces = maps.Clone(opts.DefaultNamespaces)
}

// Default the namespace-level configs first, because they need to use the undefaulted type-level config
// to be able to potentially fall through to settings from DefaultNamespaces.
for namespace, config := range byObject.Namespaces {
// 1. Default from the undefaulted type-level config
config = defaultConfig(config, byObjectToConfig(byObject))
Expand All @@ -461,14 +472,14 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
byObject.Namespaces[namespace] = config
}

defaultedConfig := defaultConfig(byObjectToConfig(byObject), optionDefaultsToConfig(&opts))
byObject.Label = defaultedConfig.LabelSelector
byObject.Field = defaultedConfig.FieldSelector
byObject.Transform = defaultedConfig.Transform
byObject.UnsafeDisableDeepCopy = defaultedConfig.UnsafeDisableDeepCopy

if isNamespaced && byObject.Namespaces == nil {
byObject.Namespaces = opts.DefaultNamespaces
// Only default ByObject iself if it isn't namespaced or has no namespaces configured, as only
// then any of this will be honored.
if !isNamespaced || len(byObject.Namespaces) == 0 {
defaultedConfig := defaultConfig(byObjectToConfig(byObject), optionDefaultsToConfig(&opts))
byObject.Label = defaultedConfig.LabelSelector
byObject.Field = defaultedConfig.FieldSelector
byObject.Transform = defaultedConfig.Transform
byObject.UnsafeDisableDeepCopy = defaultedConfig.UnsafeDisableDeepCopy
}

opts.ByObject[obj] = byObject
Expand Down Expand Up @@ -498,20 +509,21 @@ func defaultConfig(toDefault, defaultFrom Config) Config {
return toDefault
}

func namespaceAllSelector(namespaces []string) fields.Selector {
func namespaceAllSelector(namespaces []string) []fields.Selector {
selectors := make([]fields.Selector, 0, len(namespaces)-1)
sort.Strings(namespaces)
for _, namespace := range namespaces {
if namespace != metav1.NamespaceAll {
selectors = append(selectors, fields.OneTermNotEqualSelector("metadata.namespace", namespace))
}
}

return fields.AndSelectors(selectors...)
return selectors
}

func appendIfNotNil[T comparable](a, b T) []T {
func appendIfNotNil[T comparable](a []T, b T) []T {
if b != *new(T) {
return []T{a, b}
return append(a, b)
}
return []T{a}
return a
}
156 changes: 156 additions & 0 deletions pkg/cache/defaulting_test.go
View file
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
fuzz "github.com/google/gofuzz"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -38,6 +39,23 @@ func TestDefaultOpts(t *testing.T) {
t.Parallel()

pod := &corev1.Pod{}

compare := func(a, b any) string {
return cmp.Diff(a, b,
cmpopts.IgnoreUnexported(Options{}),
cmpopts.IgnoreFields(Options{}, "HTTPClient", "Scheme", "Mapper", "SyncPeriod"),
cmp.Comparer(func(a, b fields.Selector) bool {
if (a != nil) != (b != nil) {
return false
}
if a == nil {
return true
} else {

Check failure on line 53 in pkg/cache/defaulting_test.go

View workflow job for this annotation

GitHub Actions / lint 

indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)

Check warning on line 53 in pkg/cache/defaulting_test.go

View workflow job for this annotation

GitHub Actions / lint 

indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
return a.String() == b.String()
}
}),
)
}
testCases := []struct {
name string
in Options
Expand Down Expand Up @@ -221,6 +239,120 @@ func TestDefaultOpts(t *testing.T) {
return cmp.Diff(expected, o.DefaultNamespaces)
},
},
{
name: "ByObject.Namespaces get selector from DefaultNamespaces before DefaultSelector",
in: Options{
ByObject: map[client.Object]ByObject{
pod: {Namespaces: map[string]Config{"default": {}}},
},
DefaultNamespaces: map[string]Config{"default": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"})}},
DefaultLabelSelector: labels.SelectorFromSet(map[string]string{"from": "default"}),
},

verification: func(o Options) string {
expected := Options{
ByObject: map[client.Object]ByObject{
pod: {Namespaces: map[string]Config{"default": {
LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"}),
}}},
},
DefaultNamespaces: map[string]Config{"default": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"})}},
DefaultLabelSelector: labels.SelectorFromSet(map[string]string{"from": "default"}),
}

return compare(expected, o)
},
},
{
name: "Two namespaces in DefaultNamespaces with custom selection logic",
in: Options{DefaultNamespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {},
}},

verification: func(o Options) string {
expected := Options{
DefaultNamespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie("metadata.namespace!=kube-public,metadata.namespace!=kube-system")},
},
}

return compare(expected, o)
},
},
{
name: "Two namespaces in DefaultNamespaces with custom selection logic and namespace default has its own field selector",
in: Options{DefaultNamespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie("spec.nodeName=foo")},
}},

verification: func(o Options) string {
expected := Options{
DefaultNamespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie(
"metadata.namespace!=kube-public,metadata.namespace!=kube-system,spec.nodeName=foo",
)},
},
}

return compare(expected, o)
},
},
{
name: "Two namespaces in ByObject.Namespaces with custom selection logic",
in: Options{ByObject: map[client.Object]ByObject{pod: {
Namespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {},
},
}}},

verification: func(o Options) string {
expected := Options{ByObject: map[client.Object]ByObject{pod: {
Namespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie(
"metadata.namespace!=kube-public,metadata.namespace!=kube-system",
)},
},
}}}

return compare(expected, o)
},
},
{
name: "Two namespaces in ByObject.Namespaces with custom selection logic and namespace default has its own field selector",
in: Options{ByObject: map[client.Object]ByObject{pod: {
Namespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie("spec.nodeName=foo")},
},
}}},

verification: func(o Options) string {
expected := Options{ByObject: map[client.Object]ByObject{pod: {
Namespaces: map[string]Config{
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
"": {FieldSelector: fields.ParseSelectorOrDie(
"metadata.namespace!=kube-public,metadata.namespace!=kube-system,spec.nodeName=foo",
)},
},
}}}

return compare(expected, o)
},
},
{
name: "DefaultNamespace label selector doesn't get defaulted when set",
in: Options{
Expand All @@ -235,6 +367,30 @@ func TestDefaultOpts(t *testing.T) {
return cmp.Diff(expected, o.DefaultNamespaces)
},
},
{
name: "Defaulted namespaces in ByObject contain ByObject's selector",
in: Options{
ByObject: map[client.Object]ByObject{
pod: {Label: labels.SelectorFromSet(map[string]string{"from": "pod"})},
},
DefaultNamespaces: map[string]Config{"default": {}},
},
verification: func(o Options) string {
expected := Options{
ByObject: map[client.Object]ByObject{
pod: {
Label: labels.SelectorFromSet(map[string]string{"from": "pod"}),
Namespaces: map[string]Config{"default": {
LabelSelector: labels.SelectorFromSet(map[string]string{"from": "pod"}),
}},
},
},

DefaultNamespaces: map[string]Config{"default": {}},
}
return compare(expected, o)
},
},
}

for _, tc := range testCases {
Expand Down

0 comments on commit e8ef21b

Please sign in to comment.