fix: stop the chart suggesting an empty nodeSelector

[tejassinghbhati]

Description

The chart's error message for nodeReadinessRules told users to set nodeSelector: {} when a rule should match all nodes:

spec.nodeSelector is required and immutable. Set it explicitly; use 'nodeSelector: {}' only if the rule is intended to match ALL nodes.

The validating webhook rejects exactly that, in validateSpec:

if selector != nil && selector.Empty() {
    allErrs = append(allErrs, field.Required(field.NewPath("spec", "nodeSelector"), "nodeSelector must not be empty"))
}

So following the chart's own guidance with the webhook enabled fails on create. With the webhook off, which is the default, nothing blocks it and the rule matches every node in the cluster instead.

This reworks the message so it stops recommending it and says what actually happens either way. It is message only, no behaviour change. The chart's existing renders full rule spec with explicit all-nodes selector test still renders nodeSelector: {}, and the fails rendering when nodeSelector is omitted test matches on spec.nodeSelector is required, which is unchanged.

I left the larger question in #403 rather than deciding it here: the only guard against a rule that taints the whole cluster currently lives in a component that is off by default, and moving it into the CRD as a CEL rule would be an API change that rejects existing rules. That needs a maintainer call, not a drive-by.

Related Issue

Fixes #403

Type of Change

/kind bug

Testing

helm unittest charts/nrr-controller --strict passes, 19 tests across 5 suites, unchanged from before. helm lint clean.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

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

[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.

Regular contributors should join the org to skip this step.

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.

[ajaysundark]

/lgtm

Thanks. I suggested rewording this a bit, could you ptal!

[tejassinghbhati]

Thanks! I can't see the suggestion on my end, nothing showing up as a review comment on the diff. Might still be pending on your side, could you submit it?
The verify-all failure looks unrelated. It is lychee getting a 403 on the Slack links in README.md and introduction.md, neither of which this PR touches. #379 passed verify-all on the same base commit, so it looks intermittent rather than broken.
/retest

[kubernetes-prow]

@tejassinghbhati: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

Thanks! I can't see the suggestion on my end, nothing showing up as a review comment on the diff. Might still be pending on your side, could you submit it?
The verify-all failure looks unrelated. It is lychee getting a 403 on the Slack links in README.md and introduction.md, neither of which this PR touches. #379 passed verify-all on the same base commit, so it looks intermittent rather than broken.
/retest

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.

[ajaysundark]

cc @ajaysundark

[ajaysundark]

Suggested change

	{{- fail (printf "nodeReadinessRules[%s]: spec.nodeSelector is required and immutable. Set it explicitly. An empty selector matches every node in the cluster, and is rejected by the validating webhook when that is enabled." ($rule.name | default "?")) }}
	{{- fail (printf "nodeReadinessRules[%s]: spec.nodeSelector is required and immutable. An empty selector to match every node in the cluster is rejected by the validating webhook to avoid cluster wide misconfiguration risks." ($rule.name | default "?")) }}

[tejassinghbhati]

Taken, thanks.

One thing so you have it: the webhook is off by default, so the empty selector only actually gets rejected when someone turns it on. Default install it just goes through and matches everything. Your wording reads like it is always blocked. Fine by me either way, that gap is really #403's problem not this PR's.

The verify-all failure is unrelated by the way, lychee 403s on the Slack links in README.md and introduction.md, neither of which this PR touches. #379 passed on the same base so it looks like a flake.

/retest

[kubernetes-prow]

@tejassinghbhati: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

Taken, thanks.

One thing so you have it: the webhook is off by default, so the empty selector only actually gets rejected when someone turns it on. Default install it just goes through and matches everything. Your wording reads like it is always blocked. Fine by me either way, that gap is really #403's problem not this PR's.

The verify-all failure is unrelated by the way, lychee 403s on the Slack links in README.md and introduction.md, neither of which this PR touches. #379 passed on the same base so it looks like a flake.

/retest

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]

Wording is in as fe723bb, so this just needs an /ok-to-test and the lgtm back when you get a minute. My /retest bounced since I am not in the org yet.

Same for #372, #388 and #396, none of them have had CI run at all.

[ajaysundark]

/lgtm
/approve

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajaysundark, tejassinghbhati

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ajaysundark]

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