Skip to content

Migrate the non-empty nodeSelector validation from the admission webhook to CEL #454

Description

@tejassinghbhati

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:

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:

// +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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions