kubernetes-sigs / kubernetes-sigs/node-readiness-controller
Migrate the non-empty nodeSelector validation from the admission webhook to CEL
- Dominant language
- Go
- Stars
- 163
- Forks
- 74
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 9
Description
### Is your feature request related to a problem or existing issue? Please describe.
Following the direction in #449, moving validation out of the Go admission webhook and onto the CRD as native CEL.
One constraint sits outside that issue's scope. `validateSpec` rejects an empty `spec.nodeSelector`:
```go
if selector != nil && selector.Empty() {
allErrs = append(allErrs, field.Required(field.NewPath("spec", "nodeSelector"), "nodeSelector must not be empty"))
}
```
`metav1.LabelSelectorAsSelector` turns an empty selector into `labels.Everything()`, so a rule carrying one applies its taint to every Node in the cluster. With `NoExecute` that evicts pods everywhere that lack a toleration.
The webhook is optional and off by default (`--enable-webhook=false`, `webhook.enabled: false` in the chart), so on a default install nothing stops it. #403 hit this from the other side: the chart's own error message suggested `nodeSelector: {}` for matching all Nodes, which the webhook forbids and the CRD allows.
### Describe the solution you'd like
Express it as CEL on the field, so it applies on every cluster whether or not the webhook is deployed:
```go
// +kubebuilder:validation:XValidation:rule="(has(self.matchLabels) && size(self.matchLabels) > 0) || (has(self.matchExpressions) && size(self.matchExpressions) > 0)",message="nodeSelector must not be empty"
NodeSelector metav1.LabelSelector `json:"nodeSelector,omitempty,omitzero"`
```
Then drop the empty check from `validateSpec`. CRD validation runs before validating webhooks, so the branch becomes unreachable once the rule is in place.
Two details worth stating, since both surfaced while testing this against envtest:
The `LabelSelectorAsSelector` error check has to stay in the webhook. CEL can see that `matchExpressions` is non-empty but not whether an operator is one the selector parser accepts, so an entry like `{key: k, operator: NotARealOperator}` still needs Go.
An absent `nodeSelector` is caught by the `+required` marker, not by this rule. The field is `omitempty,omitzero`, so an empty struct serialises away entirely and the CEL rule never evaluates. The API server reports `spec.nodeSelector: Required value` for that case and `nodeSelector must not be empty` for a selector that is present but empty. Both are rejected, they just report differently, and it is worth covering both paths in tests.
After this, the only validation left in the webhook is cross-object taint conflict detection, which genuinely cannot move to CEL since it has to list other rules.
### Describe alternatives you've considered
Leaving it in the webhook and documenting that the guard requires the webhook. That is what happens today and it is what made #403 possible, so it does not seem worth keeping.
Adding it to the CRD while leaving the webhook copy in place. Harmless but dead, since CRD validation runs first.
### Additional context
Checked for overlap before filing. #451 covers the two bootstrap-only constraints from #449 and does not touch `nodeSelector`. No other open issue or PR references the empty selector check.
PR to follow.
Contributor guide
Research direction
Start at the NodeSelector type declaration and validateSpec entry point. Add the shown CEL validation, remove only the empty-selector check, and keep LabelSelectorAsSelector error handling and cross-object taint conflict detection in the webhook. Use envtest coverage to verify both absent and present-but-empty selectors are rejected with their expected messages.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, backend, testing
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 74/100