From 873603c106a92984ad1fb46e365253a741245ec2 Mon Sep 17 00:00:00 2001 From: Sahitya Chandra Date: Wed, 13 May 2026 23:41:01 +0530 Subject: [PATCH 1/2] webhook: detect matchExpression selector overlaps Signed-off-by: Sahitya Chandra --- .../webhook/nodereadinessgaterule_webhook.go | 10 +- .../nodereadinessgaterule_webhook_test.go | 119 +++++++++++- internal/webhook/selector_overlap.go | 174 ++++++++++++++++++ 3 files changed, 291 insertions(+), 12 deletions(-) create mode 100644 internal/webhook/selector_overlap.go diff --git a/internal/webhook/nodereadinessgaterule_webhook.go b/internal/webhook/nodereadinessgaterule_webhook.go index c3ef955..0d799c0 100644 --- a/internal/webhook/nodereadinessgaterule_webhook.go +++ b/internal/webhook/nodereadinessgaterule_webhook.go @@ -22,7 +22,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" @@ -123,14 +122,7 @@ func (w *NodeReadinessRuleWebhook) nodeSelectorsOverlap(selector1, selector2 met return true } - // Check overlap: if one selector's matchLabels is a subset of the other, - // a node could match both selectors, causing a conflict. - // Note: this only covers matchLabels-based overlap; selectors using - // matchExpressions may still overlap without being detected here. - if sel1.Matches(labels.Set(selector2.MatchLabels)) { - return true - } - return sel2.Matches(labels.Set(selector1.MatchLabels)) + return selectorsOverlap(sel1, sel2) } // generateNoExecuteWarnings generates admission warnings for NoExecute taint usage. diff --git a/internal/webhook/nodereadinessgaterule_webhook_test.go b/internal/webhook/nodereadinessgaterule_webhook_test.go index 757f955..85f8d53 100644 --- a/internal/webhook/nodereadinessgaterule_webhook_test.go +++ b/internal/webhook/nodereadinessgaterule_webhook_test.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -317,19 +318,20 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { It("should not detect different selectors as overlapping", func() { selector1 := metav1.LabelSelector{ MatchLabels: map[string]string{ - "node-role.kubernetes.io/worker": "", + "env": "prod", }, } selector2 := metav1.LabelSelector{ MatchLabels: map[string]string{ - "node-role.kubernetes.io/control-plane": "", + "env": "dev", }, } overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) - Expect(overlaps).To(BeFalse()) // Different selectors don't overlap (simple heuristic) + Expect(overlaps).To(BeFalse()) }) + It("should detect subset selectors as overlapping", func() { // selector1 (env=prod) is subset of selector2 (env=prod,region=us) // Both match nodes labeled env=prod,region=us => they overlap @@ -342,6 +344,117 @@ var _ = Describe("NodeReadinessRule Validation Webhook", func() { overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) Expect(overlaps).To(BeTrue()) // subset selectors overlap }) + + It("should detect non-subset matchLabels selectors as overlapping", func() { + selector1 := metav1.LabelSelector{ + MatchLabels: map[string]string{ + "env": "prod", + "region": "us", + }, + } + selector2 := metav1.LabelSelector{ + MatchLabels: map[string]string{ + "env": "prod", + "disk": "ssd", + }, + } + + overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) + Expect(overlaps).To(BeTrue()) + }) + + It("should detect intersecting matchExpressions as overlapping", func() { + selector1 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"prod", "stage"}}, + }, + } + selector2 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"stage", "dev"}}, + }, + } + + overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) + Expect(overlaps).To(BeTrue()) + }) + + It("should reject disjoint matchExpressions as non-overlapping", func() { + selector1 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"prod", "stage"}}, + }, + } + selector2 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"dev", "test"}}, + }, + } + + overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) + Expect(overlaps).To(BeFalse()) + }) + + It("should reject exists and does-not-exist matchExpressions as non-overlapping", func() { + selector1 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpExists}, + }, + } + selector2 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + } + + overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) + Expect(overlaps).To(BeFalse()) + }) + + It("should allow a missing label to satisfy notIn and does-not-exist expressions", func() { + selector1 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + } + selector2 := metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"prod"}}, + }, + } + + overlaps := webhook.nodeSelectorsOverlap(selector1, selector2) + Expect(overlaps).To(BeTrue()) + }) + }) + + Context("Selector Overlap Helper", func() { + It("should detect numeric selector overlap", func() { + selector1, err := labels.Parse("version>1") + Expect(err).NotTo(HaveOccurred()) + selector2, err := labels.Parse("version<3") + Expect(err).NotTo(HaveOccurred()) + + Expect(selectorsOverlap(selector1, selector2)).To(BeTrue()) + }) + + It("should reject disjoint numeric selectors", func() { + selector1, err := labels.Parse("version>3") + Expect(err).NotTo(HaveOccurred()) + selector2, err := labels.Parse("version<3") + Expect(err).NotTo(HaveOccurred()) + + Expect(selectorsOverlap(selector1, selector2)).To(BeFalse()) + }) + + It("should keep searching an upper-bounded numeric range when zero is forbidden", func() { + selector1, err := labels.Parse("version<3") + Expect(err).NotTo(HaveOccurred()) + selector2, err := labels.Parse("version!=0") + Expect(err).NotTo(HaveOccurred()) + + Expect(selectorsOverlap(selector1, selector2)).To(BeTrue()) + }) }) Context("CustomValidator Interface", func() { diff --git a/internal/webhook/selector_overlap.go b/internal/webhook/selector_overlap.go new file mode 100644 index 0000000..0de1622 --- /dev/null +++ b/internal/webhook/selector_overlap.go @@ -0,0 +1,174 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhook + +import ( + "math" + "strconv" + + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + "k8s.io/apimachinery/pkg/util/sets" +) + +// selectorsOverlap returns true if any label set could match both selectors. +func selectorsOverlap(selector1, selector2 labels.Selector) bool { + reqs1, selectable1 := selector1.Requirements() + reqs2, selectable2 := selector2.Requirements() + if !selectable1 || !selectable2 { + return false + } + + requirementsByKey := map[string][]labels.Requirement{} + for _, req := range reqs1 { + requirementsByKey[req.Key()] = append(requirementsByKey[req.Key()], req) + } + for _, req := range reqs2 { + requirementsByKey[req.Key()] = append(requirementsByKey[req.Key()], req) + } + + for _, reqs := range requirementsByKey { + if !requirementsOverlap(reqs) { + return false + } + } + return true +} + +func requirementsOverlap(reqs []labels.Requirement) bool { + var ( + mustExist bool + mustNotExist bool + allowed sets.String + forbidden sets.String + hasLower bool + lower int64 + hasUpper bool + upper int64 + ) + + for _, req := range reqs { + switch req.Operator() { + case selection.In, selection.Equals, selection.DoubleEquals: + mustExist = true + values := sets.NewString(req.ValuesUnsorted()...) + if allowed == nil { + allowed = values + } else { + allowed = allowed.Intersection(values) + } + case selection.NotIn, selection.NotEquals: + if forbidden == nil { + forbidden = sets.String{} + } + forbidden.Insert(req.ValuesUnsorted()...) + case selection.Exists: + mustExist = true + case selection.DoesNotExist: + mustNotExist = true + case selection.GreaterThan, selection.LessThan: + mustExist = true + values := req.ValuesUnsorted() + if len(values) != 1 { + return false + } + value, err := strconv.ParseInt(values[0], 10, 64) + if err != nil { + return false + } + switch req.Operator() { + case selection.GreaterThan: + if !hasLower || value > lower { + hasLower = true + lower = value + } + case selection.LessThan: + if !hasUpper || value < upper { + hasUpper = true + upper = value + } + } + default: + return false + } + } + + if mustNotExist { + return !mustExist + } + if allowed != nil { + for value := range allowed { + if valueSatisfies(value, forbidden, hasLower, lower, hasUpper, upper) { + return true + } + } + return false + } + if hasLower || hasUpper { + return numericValueExists(forbidden, hasLower, lower, hasUpper, upper) + } + return true +} + +func valueSatisfies(value string, forbidden sets.String, hasLower bool, lower int64, hasUpper bool, upper int64) bool { + if forbidden.Has(value) { + return false + } + if hasLower || hasUpper { + intValue, err := strconv.ParseInt(value, 10, 64) + if err != nil { + return false + } + if hasLower && intValue <= lower { + return false + } + if hasUpper && intValue >= upper { + return false + } + } + return true +} + +func numericValueExists(forbidden sets.String, hasLower bool, lower int64, hasUpper bool, upper int64) bool { + if hasLower && lower == math.MaxInt64 { + return false + } + if hasUpper && upper <= 0 { + return false + } + + candidate := int64(0) + if hasLower && candidate <= lower { + candidate = lower + 1 + } + if candidate < 0 { + candidate = 0 + } + + for { + if hasUpper && candidate >= upper { + return false + } + if !forbidden.Has(strconv.FormatInt(candidate, 10)) { + return true + } + if candidate == math.MaxInt64 { + return false + } + candidate++ + } +} From 95cb376ab977e353c7769caca3b1330a86bf421d Mon Sep 17 00:00:00 2001 From: Sahitya Chandra Date: Thu, 14 May 2026 12:48:38 +0530 Subject: [PATCH 2/2] fix: update selector overlap logic and improve type usage --- internal/webhook/selector_overlap.go | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/internal/webhook/selector_overlap.go b/internal/webhook/selector_overlap.go index 0de1622..8517e3a 100644 --- a/internal/webhook/selector_overlap.go +++ b/internal/webhook/selector_overlap.go @@ -29,6 +29,8 @@ import ( func selectorsOverlap(selector1, selector2 labels.Selector) bool { reqs1, selectable1 := selector1.Requirements() reqs2, selectable2 := selector2.Requirements() + // A non-selectable selector is labels.Nothing(), which matches no labels + // and therefore cannot overlap with anything. if !selectable1 || !selectable2 { return false } @@ -53,8 +55,8 @@ func requirementsOverlap(reqs []labels.Requirement) bool { var ( mustExist bool mustNotExist bool - allowed sets.String - forbidden sets.String + allowed sets.Set[string] + forbidden sets.Set[string] hasLower bool lower int64 hasUpper bool @@ -65,7 +67,7 @@ func requirementsOverlap(reqs []labels.Requirement) bool { switch req.Operator() { case selection.In, selection.Equals, selection.DoubleEquals: mustExist = true - values := sets.NewString(req.ValuesUnsorted()...) + values := sets.New(req.ValuesUnsorted()...) if allowed == nil { allowed = values } else { @@ -73,7 +75,7 @@ func requirementsOverlap(reqs []labels.Requirement) bool { } case selection.NotIn, selection.NotEquals: if forbidden == nil { - forbidden = sets.String{} + forbidden = sets.Set[string]{} } forbidden.Insert(req.ValuesUnsorted()...) case selection.Exists: @@ -84,11 +86,11 @@ func requirementsOverlap(reqs []labels.Requirement) bool { mustExist = true values := req.ValuesUnsorted() if len(values) != 1 { - return false + return true } value, err := strconv.ParseInt(values[0], 10, 64) if err != nil { - return false + return true } switch req.Operator() { case selection.GreaterThan: @@ -103,7 +105,9 @@ func requirementsOverlap(reqs []labels.Requirement) bool { } } default: - return false + // Unknown operator: assume overlap defensively, matching the + // webhook wrapper's "if we can't analyze, assume overlap" stance. + return true } } @@ -124,7 +128,7 @@ func requirementsOverlap(reqs []labels.Requirement) bool { return true } -func valueSatisfies(value string, forbidden sets.String, hasLower bool, lower int64, hasUpper bool, upper int64) bool { +func valueSatisfies(value string, forbidden sets.Set[string], hasLower bool, lower int64, hasUpper bool, upper int64) bool { if forbidden.Has(value) { return false } @@ -143,7 +147,10 @@ func valueSatisfies(value string, forbidden sets.String, hasLower bool, lower in return true } -func numericValueExists(forbidden sets.String, hasLower bool, lower int64, hasUpper bool, upper int64) bool { +// k8s label values must match the label-value regex, which disallows a leading +// '-', so numeric label values are always non-negative; the search space is +// [0, math.MaxInt64]. +func numericValueExists(forbidden sets.Set[string], hasLower bool, lower int64, hasUpper bool, upper int64) bool { if hasLower && lower == math.MaxInt64 { return false } @@ -152,7 +159,7 @@ func numericValueExists(forbidden sets.String, hasLower bool, lower int64, hasUp } candidate := int64(0) - if hasLower && candidate <= lower { + if hasLower { candidate = lower + 1 } if candidate < 0 {