Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow restricted labels in pod affinity/nodeSelector #1608

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions pkg/apis/v1/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 requirement label 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))
}
}
Comment on lines +113 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this change. We're specifically allowing these sub-domains as an exception, understanding that some users use these labels. Are you saying all kops labels shouldn't be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For nodepool's requirements, I didn't see any reason to configure something like this.

Copy link
Contributor Author

@jwcesign jwcesign Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be used as nodepool's labels, not requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check the logic, this function is used to check the nodepool's requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @njtran


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
}
Expand Down
13 changes: 10 additions & 3 deletions pkg/apis/v1/nodeclaim_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,24 @@ 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
}
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))
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/v1/nodepool_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/v1/nodepool_validation_cel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/provisioning/provisioner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}))
}
Expand Down
Loading