feat: enforce non-empty nodeSelector with CEL instead of the webhook

[tejassinghbhati]

Description

Continues the webhook to CEL migration from #449. This one is outside #451's scope, which covers the two bootstrap-only constraints.

An empty spec.nodeSelector becomes labels.Everything(), so a rule carrying one taints every Node in the cluster. The only guard was validateSpec in the validating webhook, which is off by default, so a default install had nothing stopping it. #403 came at the same gap from the chart side, where the chart's own error text suggested nodeSelector: {} while the webhook forbade it.

The constraint now lives on the CRD:

// +kubebuilder:validation:XValidation:rule="(has(self.matchLabels) && size(self.matchLabels) > 0) || (has(self.matchExpressions) && size(self.matchExpressions) > 0)",message="nodeSelector must not be empty"

and the empty check is removed from the webhook rather than left beside it, since CRD validation runs before validating webhooks and the branch is unreachable once the rule is in place.

Two things I deliberately did not change:

The LabelSelectorAsSelector error check stays. CEL can tell that matchExpressions is non-empty but not whether the operator is one the parser accepts, so {key: k, operator: NotARealOperator} still needs Go. The webhook specs that used an empty selector as their invalid input now use an unparseable one instead, so they exercise what the webhook still owns.

An absent nodeSelector is caught by +required, not by this rule, because the field is omitempty,omitzero and serialises away so CEL never evaluates. I expected my rule to catch it and it does not, which is why there is a separate spec asserting spec.nodeSelector: Required value for that path.

After this the only thing left in the webhook is cross-object taint conflict detection, which cannot move to CEL because it has to list other rules.

Related Issue

Fixes #454

Type of Change

/kind feature
/kind api-change

Testing

New nodeSelector CEL validation specs drive a real API server through envtest rather than calling the validator directly, since the point is that the API server enforces it now:

• absent selector, rejected with spec.nodeSelector: Required value
• selector present but empty, rejected with nodeSelector must not be empty
• matchLabels: {} present but empty, rejected
• matchExpressions: [] present but empty, rejected
• matchLabels set, accepted and persisted
• matchExpressions only, accepted

Full suite: 83 of 83 specs, up from 78. internal/webhook 37 specs green after the three migrated ones were reworked. internal/metrics and the reporter pass. golangci-lint v2.12.1 reports 0 issues and gofmt is clean.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

NodeReadinessRule now rejects an empty spec.nodeSelector through CRD validation rather than only through the optional admission webhook, so a rule can no longer be created that silently taints every node on clusters where the webhook is not deployed.

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

Name	Link
🔨 Latest commit	1b15ef7
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a8d1613b99d4f000983a512

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tejassinghbhati
Once this PR has been reviewed and has the lgtm label, please assign dchen1107 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @tejassinghbhati. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[tejassinghbhati]

Flagging an overlap with #451 before either of these merges, so nobody lands a broken combination.

Four of the five files here are also touched by #451. Two spots actually conflict rather than just sitting near each other.

#451 deletes the should accumulate errors across nodeSelector, defaultStatus, and conditionPolicy violations spec. This PR edits that same spec instead, since nodeSelector stops contributing an error once the CEL rule lands. Whichever merges second will hit a conflict there.

The one that matters more: in should perform comprehensive validation, #451 leaves this untouched as context.

allErrs = webhook.validateNodeReadinessRule(ctx, invalidRule, false)
Expect(allErrs).To(HaveLen(1)) // Empty nodeSelector validation
Expect(allErrs[0].Field).To(Equal("spec.nodeSelector"))

That assertion only holds while the webhook still rejects an empty selector, which this PR removes. So if #451 goes in first and this is rebased naively, that spec either fails or passes for the wrong reason. This PR repoints it at an unparseable selector, which is what the webhook still owns.

No preference on ordering, happy to rebase on #451 if you would rather take that one first. Just did not want it discovered at merge time.

/cc @vishnukothakapu

[kubernetes-prow]

@tejassinghbhati: GitHub didn't allow me to request PR reviews from the following users: vishnukothakapu.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Flagging an overlap with #451 before either of these merges, so nobody lands a broken combination.

Four of the five files here are also touched by #451. Two spots actually conflict rather than just sitting near each other.

#451 deletes the should accumulate errors across nodeSelector, defaultStatus, and conditionPolicy violations spec. This PR edits that same spec instead, since nodeSelector stops contributing an error once the CEL rule lands. Whichever merges second will hit a conflict there.

The one that matters more: in should perform comprehensive validation, #451 leaves this untouched as context.

allErrs = webhook.validateNodeReadinessRule(ctx, invalidRule, false)
Expect(allErrs).To(HaveLen(1)) // Empty nodeSelector validation
Expect(allErrs[0].Field).To(Equal("spec.nodeSelector"))

That assertion only holds while the webhook still rejects an empty selector, which this PR removes. So if #451 goes in first and this is rebased naively, that spec either fails or passes for the wrong reason. This PR repoints it at an unparseable selector, which is what the webhook still owns.

No preference on ordering, happy to rebase on #451 if you would rather take that one first. Just did not want it discovered at merge time.

/cc @vishnukothakapu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.