From 73d416b2f515a536a93a1ad72e7dd6ef55005283 Mon Sep 17 00:00:00 2001 From: jwcesign Date: Thu, 29 Aug 2024 13:36:03 +0800 Subject: [PATCH] fix: allow restricted labels in pod affinity/nodeSelector Signed-off-by: jwcesign --- pkg/apis/v1/labels.go | 15 ++++++++++++--- pkg/apis/v1/nodeclaim_validation.go | 13 ++++++++++--- pkg/apis/v1/nodepool_validation.go | 4 ++-- pkg/apis/v1/nodepool_validation_cel_test.go | 8 ++++---- pkg/controllers/provisioning/provisioner.go | 2 +- 5 files changed, 29 insertions(+), 13 deletions(-) diff --git a/pkg/apis/v1/labels.go b/pkg/apis/v1/labels.go index 722508c9ee..23ebacf94e 100644 --- a/pkg/apis/v1/labels.go +++ b/pkg/apis/v1/labels.go @@ -103,13 +103,22 @@ var ( } ) -// IsRestrictedLabel returns an error if the label is restricted. -func IsRestrictedLabel(key string) error { +// IsRestrictedRequirements returns an error if the label requirement is restricted. +func IsRestrictedRequirements(key string) error { if WellKnownLabels.Has(key) { return nil } + + // In the node pool requirements configuration, labels like kops.k8s.io/x should not be allowed, as they do not make sense. + labelDomain := GetLabelDomain(key) + for exceptionLabelDomain := range LabelDomainExceptions { + if strings.HasSuffix(labelDomain, exceptionLabelDomain) { + return fmt.Errorf("requirement label key %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains)) + } + } + if IsRestrictedNodeLabel(key) { - return fmt.Errorf("label %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains)) + return fmt.Errorf("requirement label key %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains)) } return nil } diff --git a/pkg/apis/v1/nodeclaim_validation.go b/pkg/apis/v1/nodeclaim_validation.go index a185fcbdb2..5fc425124a 100644 --- a/pkg/apis/v1/nodeclaim_validation.go +++ b/pkg/apis/v1/nodeclaim_validation.go @@ -110,7 +110,17 @@ func (in *NodeClaimTemplateSpec) validateRequirements() (errs error) { return errs } +// ValidateRequirement checks if the requirements are valid, including restricted labels, and the format of the requirements. func ValidateRequirement(requirement NodeSelectorRequirementWithMinValues) error { //nolint:gocyclo + errs := ValidateBasicRequirements(requirement) + if e := IsRestrictedRequirements(requirement.Key); e != nil { + errs = multierr.Append(errs, e) + } + return errs +} + +// ValidateBasicRequirements checks if the requirements are valid, including the format of the requirements. +func ValidateBasicRequirements(requirement NodeSelectorRequirementWithMinValues) error { //nolint:gocyclo var errs error if normalized, ok := NormalizedLabels[requirement.Key]; ok { requirement.Key = normalized @@ -118,9 +128,6 @@ func ValidateRequirement(requirement NodeSelectorRequirementWithMinValues) error if !SupportedNodeSelectorOps.Has(string(requirement.Operator)) { errs = multierr.Append(errs, fmt.Errorf("key %s has an unsupported operator %s not in %s", requirement.Key, requirement.Operator, SupportedNodeSelectorOps.UnsortedList())) } - if e := IsRestrictedLabel(requirement.Key); e != nil { - errs = multierr.Append(errs, e) - } for _, err := range validation.IsQualifiedName(requirement.Key) { errs = multierr.Append(errs, fmt.Errorf("key %s is not a qualified name, %s", requirement.Key, err)) } diff --git a/pkg/apis/v1/nodepool_validation.go b/pkg/apis/v1/nodepool_validation.go index 8ee3dc56ff..167f3c833f 100644 --- a/pkg/apis/v1/nodepool_validation.go +++ b/pkg/apis/v1/nodepool_validation.go @@ -41,8 +41,8 @@ func (in *NodeClaimTemplate) validateLabels() (errs error) { for _, err := range validation.IsValidLabelValue(value) { errs = multierr.Append(errs, fmt.Errorf("invalid value: %s for label[%s], %s", value, key, err)) } - if err := IsRestrictedLabel(key); err != nil { - errs = multierr.Append(errs, fmt.Errorf("invalid key name %q in labels, %s", key, err.Error())) + if IsRestrictedNodeLabel(key) { + errs = multierr.Append(errs, fmt.Errorf("restricted key name %q in labels", key)) } } return errs diff --git a/pkg/apis/v1/nodepool_validation_cel_test.go b/pkg/apis/v1/nodepool_validation_cel_test.go index e5e62cb2ef..44f07bcb70 100644 --- a/pkg/apis/v1/nodepool_validation_cel_test.go +++ b/pkg/apis/v1/nodepool_validation_cel_test.go @@ -411,26 +411,26 @@ var _ = Describe("CEL/Validation", func() { Expect(nodePool.RuntimeValidate()).ToNot(Succeed()) } }) - It("should allow restricted domains exceptions", func() { + It("should fail for the allow restricted domains exceptions", func() { oldNodePool := nodePool.DeepCopy() for label := range LabelDomainExceptions { nodePool.Spec.Template.Spec.Requirements = []NodeSelectorRequirementWithMinValues{ {NodeSelectorRequirement: v1.NodeSelectorRequirement{Key: label + "/test", Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}}, } Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) - Expect(nodePool.RuntimeValidate()).To(Succeed()) + Expect(nodePool.RuntimeValidate()).ToNot(Succeed()) Expect(env.Client.Delete(ctx, nodePool)).To(Succeed()) nodePool = oldNodePool.DeepCopy() } }) - It("should allow restricted subdomains exceptions", func() { + It("should fail for allow restricted subdomains exceptions", func() { oldNodePool := nodePool.DeepCopy() for label := range LabelDomainExceptions { nodePool.Spec.Template.Spec.Requirements = []NodeSelectorRequirementWithMinValues{ {NodeSelectorRequirement: v1.NodeSelectorRequirement{Key: "subdomain." + label + "/test", Operator: v1.NodeSelectorOpIn, Values: []string{"test"}}}, } Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) - Expect(nodePool.RuntimeValidate()).To(Succeed()) + Expect(nodePool.RuntimeValidate()).ToNot(Succeed()) Expect(env.Client.Delete(ctx, nodePool)).To(Succeed()) nodePool = oldNodePool.DeepCopy() } diff --git a/pkg/controllers/provisioning/provisioner.go b/pkg/controllers/provisioning/provisioner.go index 59e2fe0b97..9a40e577dc 100644 --- a/pkg/controllers/provisioning/provisioner.go +++ b/pkg/controllers/provisioning/provisioner.go @@ -512,7 +512,7 @@ func validateNodeSelectorTerm(term corev1.NodeSelectorTerm) (errs error) { } if term.MatchExpressions != nil { for _, requirement := range term.MatchExpressions { - errs = multierr.Append(errs, v1.ValidateRequirement(v1.NodeSelectorRequirementWithMinValues{ + errs = multierr.Append(errs, v1.ValidateBasicRequirements(v1.NodeSelectorRequirementWithMinValues{ NodeSelectorRequirement: requirement, })) }