kubernetes-sigs / kubernetes-sigs/controller-tools

crd: nondeterministic x-kubernetes-validations order when +k8s: markers are mixed with +kubebuilder:validation:XValidation on one field

Open
#1,429 5 comments 1 reaction 0 assignees View on GitHub
Dominant language
Go
Stars
868
Forks
482
Avg merge
1d 11h
Merged PRs (30d)
8

Description

## What happened

When a field mixes a `+k8s:` declarative validation marker with `+kubebuilder:validation:XValidation` markers, the relative order of the emitted `x-kubernetes-validations` rules is nondeterministic across runs of `controller-gen crd`. The rules contributed by each marker family keep their declared order internally, but the *relative* order of the two groups flips randomly between runs.

This makes committed CRD manifests flap: repos that commit generated CRDs (and verify them in CI with a `git diff --exit-code` style check) see spurious diffs/failures depending on which ordering a given run produces.

## Minimal reproduction

```go
// WidgetSpec defines the desired state of Widget.
type WidgetSpec struct {
// +k8s:immutable
// +kubebuilder:validation:XValidation:rule="!self.startsWith('-')",message="branch must not start with '-'"
// +kubebuilder:validation:XValidation:rule="!self.contains(':')",message="branch must not contain ':'"
// +kubebuilder:validation:Required
Branch string `json:"branch"`
}
```

```sh
for i in $(seq 1 20); do
controller-gen crd paths="./api/v1alpha1/..." output:crd:artifacts:config=out-$i
done
md5sum out-*/*.yaml | awk '{print $1}' | sort | uniq -c
```

Observed with v0.21.0 (20 runs produced 2 distinct outputs, 19/1 split):

```diff
x-kubernetes-validations:
+ - message: field is immutable
+ rule: self == oldSelf
- message: branch must not start with '-'
rule: '!self.startsWith(''-'')'
- message: branch must not contain ':'
rule: '!self.contains('':'')'
- - message: field is immutable
- rule: self == oldSelf
```

## Root cause

In `applyMarkers` (`pkg/crd/schema.go`), markers are collected by iterating the `markers.MarkerValues` map (keyed by marker name), then ordered with `slices.SortStableFunc` using only the apply-priority:

```go
for markerName, markerValues := range markerSet {
...
}
slices.SortStableFunc(markers, func(i, j schemaMarkerWithName) int { return cmpPriority(i, j) })
```

`Immutable` (from `+k8s:immutable`) doesn't implement `ApplyPriorityMarker` and `XValidation.ApplyPriority()` returns `ApplyPriorityDefault`, so both families sort equal — and the *stable* sort then preserves whatever order the randomized map iteration happened to produce. Both markers append to `schema.XValidations`, so the appended rule order is nondeterministic.

This only bites when two *different* marker names contribute rules to the same field's `x-kubernetes-validations`; repeated values under a single marker name keep their declared order, which is why this wasn't visible before the `+k8s:` validation markers landed. The code is unchanged on `main` as of today.

Related: #1324 attempted to fix this by sorting the emitted rules, and was (correctly, I think) rejected — CEL rules execute in order, so reordering them changes semantics. But the underlying nondeterminism it observed is real; it was closed as "fixed by the stable sort update for markers", which doesn't actually cover the cross-marker-name case since the stable sort has no deterministic key for equal priorities.

## Proposed fix

Give equal-priority markers a deterministic secondary sort key — e.g. sort by `(applyPriority, markerName)` while keeping the stable sort so repeated values of the same marker preserve declared order:

```go
slices.SortStableFunc(markers, func(i, j schemaMarkerWithName) int {
if c := cmpPriority(i, j); c != 0 {
return c
}
return strings.Compare(i.Name, j.Name)
})
```

This doesn't reorder rules within a marker family (so existing single-family ordering guarantees are untouched), it just makes the cross-family relative order deterministic. The one caveat is that any output generated since the `+k8s:` markers landed may churn once when this lands, since the current cross-family order is arbitrary. An alternative that avoids even that churn for most repos would be sorting by source position (file/line of the marker comment) instead of name, which would also match user intuition that rules apply in the order written.

Happy to send a PR for either approach if there's agreement on the desired key.

## Environment

- controller-gen v0.21.0 (also inspected `pkg/crd/schema.go` on `main` — same code)
- go1.26, darwin/arm64 (map iteration randomization makes it reproducible everywhere)

## Workaround

Don't mix marker families on a field: replace `+k8s:immutable` with an explicit `+kubebuilder:validation:XValidation:rule="self == oldSelf",message="field is immutable"` so all rules come from one marker name and are emitted in declared order.

Contributor guide

Open the contributing guide

Research direction

Read pkg/crd/schema.go, especially applyMarkers and the marker ordering logic. Run the 20-iteration controller-gen crd reproduction to observe the differing outputs, then add regression coverage for mixed marker families; done means x-kubernetes-validations ordering is deterministic while repeated values within one marker family retain their declared order.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
65/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.