Skip to content

Commit 64dd305

Browse files
alvaroalemank8s-infra-cherrypick-robot
alvaroaleman
authored and
k8s-infra-cherrypick-robot
committedApr 2, 2024·
bug: Cache: Keep selectors when byObject.Namespaces is defaulted
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.
·
v0.17.6v0.17.3
1 parent bd9ea79 commit 64dd305

File tree

2 files changed

+182
-15
lines changed

2 files changed

+182
-15
lines changed
 

‎pkg/cache/cache.go

+27-15
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"fmt"
2222
"net/http"
23+
"sort"
2324
"time"
2425

2526
"golang.org/x/exp/maps"
@@ -421,7 +422,12 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
421422
for namespace, cfg := range opts.DefaultNamespaces {
422423
cfg = defaultConfig(cfg, optionDefaultsToConfig(&opts))
423424
if namespace == metav1.NamespaceAll {
424-
cfg.FieldSelector = fields.AndSelectors(appendIfNotNil(namespaceAllSelector(maps.Keys(opts.DefaultNamespaces)), cfg.FieldSelector)...)
425+
cfg.FieldSelector = fields.AndSelectors(
426+
appendIfNotNil(
427+
namespaceAllSelector(maps.Keys(opts.DefaultNamespaces)),
428+
cfg.FieldSelector,
429+
)...,
430+
)
425431
}
426432
opts.DefaultNamespaces[namespace] = cfg
427433
}
@@ -435,7 +441,12 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
435441
return opts, fmt.Errorf("type %T is not namespaced, but its ByObject.Namespaces setting is not nil", obj)
436442
}
437443

438-
// Default the namespace-level configs first, because they need to use the undefaulted type-level config.
444+
if isNamespaced && byObject.Namespaces == nil {
445+
byObject.Namespaces = maps.Clone(opts.DefaultNamespaces)
446+
}
447+
448+
// Default the namespace-level configs first, because they need to use the undefaulted type-level config
449+
// to be able to potentially fall through to settings from DefaultNamespaces.
439450
for namespace, config := range byObject.Namespaces {
440451
// 1. Default from the undefaulted type-level config
441452
config = defaultConfig(config, byObjectToConfig(byObject))
@@ -461,14 +472,14 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
461472
byObject.Namespaces[namespace] = config
462473
}
463474

464-
defaultedConfig := defaultConfig(byObjectToConfig(byObject), optionDefaultsToConfig(&opts))
465-
byObject.Label = defaultedConfig.LabelSelector
466-
byObject.Field = defaultedConfig.FieldSelector
467-
byObject.Transform = defaultedConfig.Transform
468-
byObject.UnsafeDisableDeepCopy = defaultedConfig.UnsafeDisableDeepCopy
469-
470-
if isNamespaced && byObject.Namespaces == nil {
471-
byObject.Namespaces = opts.DefaultNamespaces
475+
// Only default ByObject iself if it isn't namespaced or has no namespaces configured, as only
476+
// then any of this will be honored.
477+
if !isNamespaced || len(byObject.Namespaces) == 0 {
478+
defaultedConfig := defaultConfig(byObjectToConfig(byObject), optionDefaultsToConfig(&opts))
479+
byObject.Label = defaultedConfig.LabelSelector
480+
byObject.Field = defaultedConfig.FieldSelector
481+
byObject.Transform = defaultedConfig.Transform
482+
byObject.UnsafeDisableDeepCopy = defaultedConfig.UnsafeDisableDeepCopy
472483
}
473484

474485
opts.ByObject[obj] = byObject
@@ -498,20 +509,21 @@ func defaultConfig(toDefault, defaultFrom Config) Config {
498509
return toDefault
499510
}
500511

501-
func namespaceAllSelector(namespaces []string) fields.Selector {
512+
func namespaceAllSelector(namespaces []string) []fields.Selector {
502513
selectors := make([]fields.Selector, 0, len(namespaces)-1)
514+
sort.Strings(namespaces)
503515
for _, namespace := range namespaces {
504516
if namespace != metav1.NamespaceAll {
505517
selectors = append(selectors, fields.OneTermNotEqualSelector("metadata.namespace", namespace))
506518
}
507519
}
508520

509-
return fields.AndSelectors(selectors...)
521+
return selectors
510522
}
511523

512-
func appendIfNotNil[T comparable](a, b T) []T {
524+
func appendIfNotNil[T comparable](a []T, b T) []T {
513525
if b != *new(T) {
514-
return []T{a, b}
526+
return append(a, b)
515527
}
516-
return []T{a}
528+
return a
517529
}

‎pkg/cache/defaulting_test.go

+155
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"time"
2323

2424
"github.com/google/go-cmp/cmp"
25+
"github.com/google/go-cmp/cmp/cmpopts"
2526
fuzz "github.com/google/gofuzz"
2627
corev1 "k8s.io/api/core/v1"
2728
"k8s.io/apimachinery/pkg/api/meta"
@@ -38,6 +39,22 @@ func TestDefaultOpts(t *testing.T) {
3839
t.Parallel()
3940

4041
pod := &corev1.Pod{}
42+
43+
compare := func(a, b any) string {
44+
return cmp.Diff(a, b,
45+
cmpopts.IgnoreUnexported(Options{}),
46+
cmpopts.IgnoreFields(Options{}, "HTTPClient", "Scheme", "Mapper", "SyncPeriod"),
47+
cmp.Comparer(func(a, b fields.Selector) bool {
48+
if (a != nil) != (b != nil) {
49+
return false
50+
}
51+
if a == nil {
52+
return true
53+
}
54+
return a.String() == b.String()
55+
}),
56+
)
57+
}
4158
testCases := []struct {
4259
name string
4360
in Options
@@ -221,6 +238,120 @@ func TestDefaultOpts(t *testing.T) {
221238
return cmp.Diff(expected, o.DefaultNamespaces)
222239
},
223240
},
241+
{
242+
name: "ByObject.Namespaces get selector from DefaultNamespaces before DefaultSelector",
243+
in: Options{
244+
ByObject: map[client.Object]ByObject{
245+
pod: {Namespaces: map[string]Config{"default": {}}},
246+
},
247+
DefaultNamespaces: map[string]Config{"default": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"})}},
248+
DefaultLabelSelector: labels.SelectorFromSet(map[string]string{"from": "default"}),
249+
},
250+
251+
verification: func(o Options) string {
252+
expected := Options{
253+
ByObject: map[client.Object]ByObject{
254+
pod: {Namespaces: map[string]Config{"default": {
255+
LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"}),
256+
}}},
257+
},
258+
DefaultNamespaces: map[string]Config{"default": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "namespace"})}},
259+
DefaultLabelSelector: labels.SelectorFromSet(map[string]string{"from": "default"}),
260+
}
261+
262+
return compare(expected, o)
263+
},
264+
},
265+
{
266+
name: "Two namespaces in DefaultNamespaces with custom selection logic",
267+
in: Options{DefaultNamespaces: map[string]Config{
268+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
269+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
270+
"": {},
271+
}},
272+
273+
verification: func(o Options) string {
274+
expected := Options{
275+
DefaultNamespaces: map[string]Config{
276+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
277+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
278+
"": {FieldSelector: fields.ParseSelectorOrDie("metadata.namespace!=kube-public,metadata.namespace!=kube-system")},
279+
},
280+
}
281+
282+
return compare(expected, o)
283+
},
284+
},
285+
{
286+
name: "Two namespaces in DefaultNamespaces with custom selection logic and namespace default has its own field selector",
287+
in: Options{DefaultNamespaces: map[string]Config{
288+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
289+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
290+
"": {FieldSelector: fields.ParseSelectorOrDie("spec.nodeName=foo")},
291+
}},
292+
293+
verification: func(o Options) string {
294+
expected := Options{
295+
DefaultNamespaces: map[string]Config{
296+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
297+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
298+
"": {FieldSelector: fields.ParseSelectorOrDie(
299+
"metadata.namespace!=kube-public,metadata.namespace!=kube-system,spec.nodeName=foo",
300+
)},
301+
},
302+
}
303+
304+
return compare(expected, o)
305+
},
306+
},
307+
{
308+
name: "Two namespaces in ByObject.Namespaces with custom selection logic",
309+
in: Options{ByObject: map[client.Object]ByObject{pod: {
310+
Namespaces: map[string]Config{
311+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
312+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
313+
"": {},
314+
},
315+
}}},
316+
317+
verification: func(o Options) string {
318+
expected := Options{ByObject: map[client.Object]ByObject{pod: {
319+
Namespaces: map[string]Config{
320+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
321+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
322+
"": {FieldSelector: fields.ParseSelectorOrDie(
323+
"metadata.namespace!=kube-public,metadata.namespace!=kube-system",
324+
)},
325+
},
326+
}}}
327+
328+
return compare(expected, o)
329+
},
330+
},
331+
{
332+
name: "Two namespaces in ByObject.Namespaces with custom selection logic and namespace default has its own field selector",
333+
in: Options{ByObject: map[client.Object]ByObject{pod: {
334+
Namespaces: map[string]Config{
335+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
336+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
337+
"": {FieldSelector: fields.ParseSelectorOrDie("spec.nodeName=foo")},
338+
},
339+
}}},
340+
341+
verification: func(o Options) string {
342+
expected := Options{ByObject: map[client.Object]ByObject{pod: {
343+
Namespaces: map[string]Config{
344+
"kube-public": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-public"})},
345+
"kube-system": {LabelSelector: labels.SelectorFromSet(map[string]string{"from": "kube-system"})},
346+
"": {FieldSelector: fields.ParseSelectorOrDie(
347+
"metadata.namespace!=kube-public,metadata.namespace!=kube-system,spec.nodeName=foo",
348+
)},
349+
},
350+
}}}
351+
352+
return compare(expected, o)
353+
},
354+
},
224355
{
225356
name: "DefaultNamespace label selector doesn't get defaulted when set",
226357
in: Options{
@@ -235,6 +366,30 @@ func TestDefaultOpts(t *testing.T) {
235366
return cmp.Diff(expected, o.DefaultNamespaces)
236367
},
237368
},
369+
{
370+
name: "Defaulted namespaces in ByObject contain ByObject's selector",
371+
in: Options{
372+
ByObject: map[client.Object]ByObject{
373+
pod: {Label: labels.SelectorFromSet(map[string]string{"from": "pod"})},
374+
},
375+
DefaultNamespaces: map[string]Config{"default": {}},
376+
},
377+
verification: func(o Options) string {
378+
expected := Options{
379+
ByObject: map[client.Object]ByObject{
380+
pod: {
381+
Label: labels.SelectorFromSet(map[string]string{"from": "pod"}),
382+
Namespaces: map[string]Config{"default": {
383+
LabelSelector: labels.SelectorFromSet(map[string]string{"from": "pod"}),
384+
}},
385+
},
386+
},
387+
388+
DefaultNamespaces: map[string]Config{"default": {}},
389+
}
390+
return compare(expected, o)
391+
},
392+
},
238393
}
239394

240395
for _, tc := range testCases {

0 commit comments

Comments
 (0)
Please sign in to comment.