fix: validate podspec scheduling syntax on WekaCluster/WekaClient admission (OP-361) - #2734
Conversation
How to use the Graphite Merge QueueAdd the label main-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Reviewing PR
|
|
Claude finished @caspx's task in 6m 4s —— View job Review
Solid, careful PR. The upstream mirroring is accurate where I could check it against k8s semantics (toleration key/operator/value/effect interaction, node-affinity value-count rules per operator, label values validated in required node-affinity terms only, weight 1-100, Two things I verified beyond the diff, both good news:
Findings (4 inline comments)
Smaller notes (no inline comment)
Not verifiedI could not compile or run the tests in this checkout: |
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/12/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
There was a problem hiding this comment.
Pull request overview
Adds new admission-time validators for WekaCluster and WekaClient to reject invalid scheduling-related spec fields (tolerations, selectors, affinity, topology spread constraints, failureDomain) earlier—at CR admission instead of later at Pod creation.
Changes:
- Introduces shared pod-spec syntax validation helpers plus new
cluster_podspec_syntaxandclient_podspec_syntaxvalidators, and registers them in the validation registry. - Wires the new policies into admission defaults and documents the new policy IDs in Helm values + operator docs (and updates
.ainavnavigation). - Updates
package-lock.json(undici version bumps in two dependency locations).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Bumps undici dependency versions. |
| internal/validation/registry.go | Registers the new cluster/client podspec syntax validators. |
| internal/validation/podspec_syntax.go | Adds shared scheduling/podspec syntax validation helpers. |
| internal/validation/podspec_syntax_test.go | Unit tests for the shared podspec syntax helpers. |
| internal/validation/cluster_podspec_syntax.go | Implements cluster_podspec_syntax admission validator. |
| internal/validation/cluster_podspec_syntax_test.go | Tests for cluster podspec syntax validation behavior and role wiring. |
| internal/validation/client_podspec_syntax.go | Implements client_podspec_syntax admission validator (incl. CSI advanced fields). |
| internal/validation/client_podspec_syntax_test.go | Tests for client podspec syntax validation behavior. |
| internal/admission/defaults.go | Adds default severities for the new policies. |
| doc/operator/operations/admission-control.md | Updates admission-control documentation scope/behavior. |
| charts/weka-operator/values.yaml | Documents new policy IDs and their defaults in Helm values comments. |
| .ainav/config/index.md | Updates navigation docs to include new podspec syntax validators. |
Suppressed comments (1)
internal/validation/podspec_syntax.go:269
- Returning the full raw JSON as the "bad value" can bloat admission errors (and potentially exceed response limits). Consider using a short placeholder for the bad value and keep details in the message.
if err := json.Unmarshal(raw.Raw, &constraints); err != nil {
return field.ErrorList{field.Invalid(fldPath, string(raw.Raw), fmt.Sprintf("does not unmarshal into []v1.TopologySpreadConstraint: %v", err))}
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" |
| The operator runs a validating admission webhook for `WekaCluster`, `WekaClient`, and | ||
| `WekaContainer` resources. On every `kubectl apply` (or Helm/GitOps equivalent) it runs a battery | ||
| of policies and either admits the request, attaches a `kubectl Warning:` line, or | ||
| rejects it. The default posture is non-blocking — most policies emit warnings; | ||
| rejects it (`WekaContainer` carries update-only policies, e.g. cores decrease). |
| case corev1.NodeSelectorOpGt, corev1.NodeSelectorOpLt: | ||
| if len(expr.Values) != 1 { | ||
| errs = append(errs, field.Required(ep.Child("values"), "must be a single value when operator is Gt or Lt")) | ||
| } |
| if err := json.Unmarshal(raw.Raw, &aff); err != nil { | ||
| return field.ErrorList{field.Invalid(fldPath, string(raw.Raw), fmt.Sprintf("does not unmarshal into a v1.Affinity: %v", err))} | ||
| } |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| roleNodeSelectors := []struct { | ||
| role string | ||
| sel *map[string]string | ||
| }{ | ||
| {"compute", wc.Spec.RoleNodeSelector.Compute}, | ||
| {"drive", wc.Spec.RoleNodeSelector.Drive}, | ||
| {"s3", wc.Spec.RoleNodeSelector.S3}, | ||
| {"nfs", wc.Spec.RoleNodeSelector.Nfs}, | ||
| {"smbw", wc.Spec.RoleNodeSelector.Smbw}, | ||
| {"dataServices", wc.Spec.RoleNodeSelector.DataServices}, | ||
| } |
There was a problem hiding this comment.
The four role tables here (and the mirrored role lists in cluster_podspec_syntax_test.go:178-179) are hand-maintained. If someone adds an envoy/dataServices field to RoleNodeSelector / RoleAffinity / RoleTopologySpreadConstraints in weka-k8s-api, that field is silently unvalidated and no test fails — the tests enumerate the same hardcoded list, so they agree with the bug.
Cheap guard: a reflection assertion in the test, e.g.
if got := reflect.TypeOf(weka.RoleNodeSelector{}).NumField(); got != len(sixRoles) {
t.Fatalf("RoleNodeSelector has %d fields, table covers %d — update the validator", got, len(sixRoles))
}(one per struct). That turns a silent coverage hole into a compile-time-ish failure at the next API bump.
| } | ||
| var aff corev1.Affinity | ||
| if err := json.Unmarshal(raw.Raw, &aff); err != nil { | ||
| return field.ErrorList{field.Invalid(fldPath, string(raw.Raw), fmt.Sprintf("does not unmarshal into a v1.Affinity: %v", err))} |
There was a problem hiding this comment.
string(raw.Raw) embeds the entire affinity blob into the denial message (same at line 268 for topology spread constraints). A realistic podConfig.affinity is a few KB of JSON, and this ends up in the kubectl apply error the user sees, buried around the actual json: parse error.
Consider truncating the echoed value, e.g. a small helper:
func truncateRaw(b []byte) string {
const max = 256
if len(b) > max {
return string(b[:max]) + "…"
}
return string(b)
}The err from json.Unmarshal already names the offending field/offset, so the full blob adds little.
| if fd := wc.Spec.FailureDomain; fd != nil { | ||
| // mirror getDefaultRoleTopologySpreadConstraints precedence: label | ||
| // wins over compositeLabels; skew is used only with label | ||
| if fd.Label != nil { | ||
| if *fd.Label == "" { | ||
| errs = append(errs, field.Required(spec.Child("failureDomain", "label"), "failureDomain label may not be empty when set")) | ||
| } else { | ||
| errs = append(errs, validateTopologyKey(spec.Child("failureDomain", "label"), *fd.Label)...) | ||
| } | ||
| // skew becomes the generated spread constraint's maxSkew (must be > 0) | ||
| if fd.Skew != nil && *fd.Skew <= 0 { | ||
| errs = append(errs, field.Invalid(spec.Child("failureDomain", "skew"), *fd.Skew, "must be greater than zero")) | ||
| } | ||
| } else { | ||
| for i, l := range fd.CompositeLabels { | ||
| p := spec.Child("failureDomain", "compositeLabels").Index(i) | ||
| if l == "" { | ||
| errs = append(errs, field.Required(p, "failureDomain compositeLabels entries may not be empty")) | ||
| } else { | ||
| errs = append(errs, validateTopologyKey(p, l)...) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The precedence mirrored here matches the code (container_factory.go:220-240: Label != nil wins, CompositeLabels only in the else), which is the right thing to mirror. But the published API doc says the opposite — doc/api_dump/wekacluster.md:185: "If compositeLabels is set, label and skew will be ignored."
Consequence: a user who follows the doc and sets both gets their compositeLabels neither used nor validated, and a typo there stays invisible. Worth fixing the field comment in weka-k8s-api (source of the generated dump) as a follow-up so doc and validator agree — otherwise this validator quietly cements the undocumented precedence.
|
|
||
| wekaClientDefaults = map[string]PolicyDefaults{ | ||
| "client_target_cluster_exists": {Strict: Error, Relaxed: Warn}, | ||
| "client_podspec_syntax": {Strict: Error, Relaxed: Error}, |
There was a problem hiding this comment.
Relaxed: Error here (and line 20) is defensible — a syntactically invalid podspec can never schedule, so warning about it is pointless. But note the interaction with the grandfathering behaviour: ValidateUpdate short-circuits only while spec is byte-identical (wekacluster.go:57), so the first unrelated edit to an existing bad CR (image bump, core count change) gets denied with a toleration/nodeSelector error the user didn't touch in that apply.
For a relaxed-posture fleet that's a rollout surprise on operator upgrade. Two mitigations worth considering: ship Relaxed: Warn for one release and flip to Error after, or make sure the release note explicitly tells relaxed-posture operators to pre-scan existing CRs (the per-policy override in admissionPolicies is the escape hatch either way — maybe mention it in the note).
TL;DR
Adds admission validators that reject WekaCluster/WekaClient specs whose scheduling-related fields would produce pods the API server rejects or that can never schedule — caught at CR apply instead of at pod create.
What changed?
cluster_podspec_syntax/client_podspec_syntaxvalidators (default Error in both strict and relaxed postures), sharinginternal/validation/podspec_syntax.go— syntax rules mirrored from upstream k8s v1.33pkg/apis/core/validationfor tolerations, nodeSelector, label/annotation maps, node/pod affinity, and topologySpreadConstraints; the cluster validator also gatesfailureDomain(label / skew / compositeLabels, mirroring factory precedence).values.yamlanddoc/operator/operations/admission-control.md.How to test?
go test ./internal/validation/... ./internal/admission/...kubectl apply --dry-run=servera WekaClient with the ticket's toleration (scitix.ai/nodecheck:NoScheduleas a raw string) → denied with a rawTolerations hint; valid baseline specs admit.Why make this change?
OP-361: a WekaClient with a malformed toleration was admitted, then every generated WekaContainer pod failed to start — and deleting the containers didn't help, since they were recreated with the same bad toleration. The webhook now rejects the spec up front.