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..8517e3a --- /dev/null +++ b/internal/webhook/selector_overlap.go @@ -0,0 +1,181 @@ +/* +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() + // A non-selectable selector is labels.Nothing(), which matches no labels + // and therefore cannot overlap with anything. + 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.Set[string] + forbidden sets.Set[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.New(req.ValuesUnsorted()...) + if allowed == nil { + allowed = values + } else { + allowed = allowed.Intersection(values) + } + case selection.NotIn, selection.NotEquals: + if forbidden == nil { + forbidden = sets.Set[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 true + } + value, err := strconv.ParseInt(values[0], 10, 64) + if err != nil { + return true + } + 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: + // Unknown operator: assume overlap defensively, matching the + // webhook wrapper's "if we can't analyze, assume overlap" stance. + return true + } + } + + 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.Set[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 +} + +// 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 + } + if hasUpper && upper <= 0 { + return false + } + + candidate := int64(0) + if hasLower { + 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++ + } +}