validate: fail-closed GKE device-plugin ownership check - #2000
validate: fail-closed GKE device-plugin ownership check#2000yuanchen8911 wants to merge 1 commit into
Conversation
yuanchen8911
added
the
theme/validation
github-actions
Bot
added
area/recipes
area/tests
area/validator
area/docs
size/XL
labels
yuanchen8911
changed the title
Recipe evidence checkProtected recipesRecipes with committed evidence (
Other affected recipes without evidence yet: 10These recipes are affected by this PR but carry no committed evidence pointer, so there is
How to refresh evidenceRun on a cluster matching the recipe's aicr snapshot -o snapshot.yaml
# Profiled families (AKS gpuStack): capture the pool projection and
# hydrate the recipe with the pointer's recorded 'profile:' selection
# first — validating the raw overlay resolves only the declaration
# default, and 'aicr validate' has no --profile flag:
# az aks nodepool list -g <rg> --cluster-name <cluster> -o json > pools.json
# aicr snapshot --aks-gpu-pools pools.json -o snapshot.yaml
# aicr recipe -s snapshot.yaml --intent <intent> [--platform <platform>] \
# --profile <name>=<value> -o recipe.yaml
# State the target leaf's intent/platform explicitly (the snapshot
# fingerprint supplies service/accelerator/OS but intent and platform
# default to 'any') and pass -r recipe.yaml below instead of the raw
# overlay.
aicr validate \
-r recipes/overlays/<slug>.yaml \
-s snapshot.yaml \
--emit-attestation ./out \
--push ghcr.io/<your-fork>/aicr-evidence
# Copy to the per-source path printed in the emit 'copyTo' hint:
# recipes/evidence/<slug>/<source>/<bundle-digest>.yamlThis gate is warning-only and never blocks merge. See ADR-007 for the trust model. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validator/validator_test.go`:
- Around line 355-410: Extend
TestCheckReadinessEvaluatesReadinessPhaseConstraints with a recipe containing
both a top-level Validation.Constraints entry and a
Validation.Readiness.Constraints entry; make one or both fail to verify both are
evaluated, confirm the resulting error includes the relevant remediation, and
snapshot validationInput.Constraints before checkReadiness to assert it remains
unchanged afterward, preserving the combined ordering and non-aliasing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 508c292a-df5b-4790-8e79-48253edb795d
📒 Files selected for processing (10)
docs/integrator/recipe-development.mddocs/user/validation.mdpkg/constraints/doc.gopkg/constraints/evaluate.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/validator/validator.gopkg/validator/validator_test.gorecipes/overlays/gke-cos.yamltests/uat/gcp/cluster-config.yaml
7bac947 to
f6cab51
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/collector/topology/topology_test.go`:
- Around line 465-481: Refactor TestIsTruncatedNodeListRoundTrip into a
table-driven test containing the truncated, full, and exact-limit cases with
their expected results. Iterate over the cases using subtests, while preserving
the existing formatNodeList inputs and IsTruncatedNodeList assertions.
In `@pkg/constraints/gpu_nodes.go`:
- Line 22: Replace the deprecated validation import and validator calls in the
GPU node constraint logic with content.IsQualifiedName and content.IsLabelValue.
Remove the k8s.io/apimachinery/pkg/util/validation dependency while preserving
the existing validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7863614d-d4f6-425b-b95e-81ca870595f2
📒 Files selected for processing (14)
docs/contributor/validator.mddocs/integrator/recipe-development.mddocs/user/validation.mdpkg/collector/topology/topology.gopkg/collector/topology/topology_test.gopkg/constraints/doc.gopkg/constraints/evaluate.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/validator/doc.gopkg/validator/validator.gopkg/validator/validator_test.gorecipes/overlays/gke-cos.yamltests/uat/gcp/cluster-config.yaml
| "sort" | ||
| "strings" | ||
|
|
||
| "k8s.io/apimachinery/pkg/util/validation" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does k8s.io/apimachinery v0.36.3 mark pkg/util/validation.IsQualifiedName and IsValidLabelValue as deprecated?
💡 Result:
Yes, in k8s.io/apimachinery v0.36.3, the functions IsQualifiedName and IsValidLabelValue located in the pkg/util/validation package are marked as deprecated [1][2]. They now include deprecation notices in their source code, which advise users to migrate to the following alternatives found in the k8s.io/apimachinery/pkg/api/validate/content package [1][3]: - For IsQualifiedName: Use k8s.io/apimachinery/pkg/api/validate/content.IsQualifiedName instead [1][4]. - For IsValidLabelValue: Use k8s.io/apimachinery/pkg/api/validate/content.IsLabelValue instead [1][2]. These changes are part of ongoing efforts to consolidate validation logic within the Kubernetes apimachinery library [5][6].
Citations:
- 1: https://github.com/kubernetes/kubernetes/blob/HEAD/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 2: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 3: https://raw.githubusercontent.com/kubernetes/apimachinery/master/pkg/util/validation/validation.go
- 4: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/content/kube.go
- 5: https://pkg.go.dev/k8s.io/apimachinery/pkg/api/validate/content
- 6: kubernetes/apimachinery@da7b67e
🏁 Script executed:
set -eu
printf '%s\n' '--- module version ---'
rg -n 'k8s.io/apimachinery' go.mod go.sum 2>/dev/null | head -20
printf '%s\n' '--- validator usage ---'
rg -n -C 2 'validation\.(IsQualifiedName|IsValidLabelValue)' pkg/constraints/gpu_nodes.go
printf '%s\n' '--- v0.36.3 declarations and deprecations ---'
curl -fsSL https://raw.githubusercontent.com/kubernetes/apimachinery/v0.36.3/pkg/util/validation/validation.go |
rg -n -C 4 'IsQualifiedName|IsValidLabelValue|Deprecated'
printf '%s\n' '--- replacement declarations ---'
curl -fsSL https://raw.githubusercontent.com/kubernetes/apimachinery/v0.36.3/pkg/api/validate/content/kube.go |
rg -n -C 3 'IsQualifiedName|IsLabelKey|IsLabelValue'Repository: NVIDIA/aicr
Length of output: 4280
Replace deprecated Kubernetes validators.
Use content.IsQualifiedName and content.IsLabelValue instead of the deprecated validation aliases to avoid SA1019 lint failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/constraints/gpu_nodes.go` at line 22, Replace the deprecated validation
import and validator calls in the GPU node constraint logic with
content.IsQualifiedName and content.IsLabelValue. Remove the
k8s.io/apimachinery/pkg/util/validation dependency while preserving the existing
validation behavior.
f6cab51 to
a663fdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/constraints/gpu_nodes_test.go`:
- Around line 397-409: Refactor TestSummarizeNodesCapsList into a table-driven
test containing both the capped seven-node case and the uncapped single-node
case, then iterate through the table with named subtests while preserving the
existing expected outputs.
In `@pkg/constraints/gpu_nodes.go`:
- Around line 204-208: Update the topology label decoding around
cutLabelEncoding so readings missing the separator, with an empty node list, or
containing empty node members are rejected as malformed validation data.
Propagate the decoding error through evaluateNoGPUNodeHasKey so both positive
and negated checks fail closed instead of producing an empty labelNodeSet. Add
regression tests covering positive and negated evaluations for each malformed
form.
- Around line 134-140: Update the node-set constraint parsing validation around
strings.Cut to remove the want == "" rejection, allowing key= values to be
validated by validation.IsValidLabelValue. Add a test case covering a valid
empty label value and preserve rejection of malformed inputs through the
existing validation flow.
In `@pkg/validator/validator.go`:
- Around line 80-82: Update the remediation handling in the validator
error-message construction to trim c.Remediation before checking whether it is
non-empty, then append the trimmed value only when it contains content. Preserve
the existing newline formatting for valid remediation text, and add a test
covering whitespace-only Remediation to ensure it does not append an extra
newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: de16349f-e220-4240-ab15-89216983aa1d
📒 Files selected for processing (15)
docs/contributor/validator.mddocs/integrator/recipe-development.mddocs/user/validation.mdpkg/collector/topology/topology.gopkg/collector/topology/topology_test.gopkg/constraints/doc.gopkg/constraints/evaluate.gopkg/constraints/extractor.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/validator/doc.gopkg/validator/validator.gopkg/validator/validator_test.gorecipes/overlays/gke-cos.yamltests/uat/gcp/cluster-config.yaml
| key, want, ok := strings.Cut(v, "=") | ||
| if !ok || want == "" { | ||
| return "", "", false, errors.NewWithContext(errors.ErrCodeInvalidRequest, | ||
| "invalid node-set constraint value: expected \"<label-key>=<value>\" or \"!<label-key>\"", | ||
| map[string]any{ctxValue: raw}) | ||
| } | ||
| if errs := append(validation.IsQualifiedName(key), validation.IsValidLabelValue(want)...); len(errs) > 0 { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' pkg/constraints/gpu_nodes.go
printf '\n--- related tests and validation helpers ---\n'
rg -n --glob '*.go' 'gpu_nodes|node-set|IsValidLabelValue|key=' pkgRepository: NVIDIA/aicr
Length of output: 18332
🏁 Script executed:
#!/bin/sh
sed -n '1,220p' pkg/constraints/gpu_nodes.go
printf '\n--- related tests and validation helpers ---\n'
rg -n --glob '*.go' 'gpu_nodes|node-set|IsValidLabelValue|key=' pkgRepository: NVIDIA/aicr
Length of output: 18332
🏁 Script executed:
#!/bin/sh
printf '%s\n' '--- constraint tests ---'
sed -n '1,180p' pkg/constraints/gpu_nodes_test.go
printf '%s\n' '--- remaining constraint tests ---'
sed -n '180,380p' pkg/constraints/gpu_nodes_test.go
printf '%s\n' '--- validator implementation/tests in repository or module metadata ---'
rg -n --hidden --glob '!vendor' --glob '!node_modules' 'func IsValidLabelValue|IsValidLabelValue\\(' .
rg -n 'k8s.io/apimachinery' go.mod go.sumRepository: NVIDIA/aicr
Length of output: 13903
🌐 Web query:
k8s.io/apimachinery v0.36.3 validation IsValidLabelValue empty string kube.go
💡 Result:
In k8s.io/apimachinery v0.36.3, the function IsValidLabelValue (and its modern equivalent k8s.io/apimachinery/pkg/api/validate/content.IsLabelValue) explicitly allows an empty string as a valid label value [1][2][3]. The validation logic is defined such that a label value can be an empty string, or it must consist of alphanumeric characters, '-', '', or '.', while starting and ending with an alphanumeric character [1][3]. Specifically, the implementation uses a regular expression pattern equivalent to ([-A-Za-z0-9.]*)? (in the context of the underlying validation package), which makes the entire string optional, thereby permitting an empty string [1][4][3]. Technical details: - Status: The function IsValidLabelValue in k8s.io/apimachinery/pkg/util/validation is deprecated in favor of k8s.io/apimachinery/pkg/api/validate/content.IsLabelValue [2][5][6]. - Validation Logic: Both versions treat the empty string as a success case [7][3]. - Source Reference: You can verify this in the official repository under pkg/api/validate/content/kube.go and the corresponding test file pkg/api/validate/content/kube_test.go, which explicitly include "" (empty string) in their success test cases [1][3].
Citations:
- 1: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/content/kube.go
- 2: kubernetes/apimachinery@da7b67e
- 3: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/content/kube_test.go
- 4: https://github.com/kubernetes/kubernetes/blob/1ffc17e329f9ba3dbe840a7f37513eb8ef074740/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 5: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 6: https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go
- 7: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/util/validation/validation_test.go
Accept valid empty label values.
Remove want == "" so key= reaches IsValidLabelValue; Kubernetes permits empty label values. Add a key= test case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/constraints/gpu_nodes.go` around lines 134 - 140, Update the node-set
constraint parsing validation around strings.Cut to remove the want == ""
rejection, allowing key= values to be validated by validation.IsValidLabelValue.
Add a test case covering a valid empty label value and preserve rejection of
malformed inputs through the existing validation flow.
a663fdd to
26e0748
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributor/validator.md`:
- Around line 47-53: Update the later pre-flight section describing
checkReadiness to state that it evaluates both top-level validation.constraints
and validation.readiness.constraints, matching the contract documented near the
phase checks. Remove any wording that limits pre-flight evaluation to top-level
constraints only.
In `@pkg/validator/validator_test.go`:
- Around line 416-439: Update the aliasing coverage in the checkReadiness test
around the topLevel setup and post-check assertions: construct topLevel with
spare capacity and initialize its unused tail with a sentinel value, then verify
after checkReadiness that both its length and unused tail remain unchanged. Keep
the existing error assertions and ensure the test would fail if checkReadiness
appends readiness constraints into the aliased backing array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a168baec-79c7-4640-a371-4a6cb526e2a1
📒 Files selected for processing (15)
docs/contributor/validator.mddocs/integrator/recipe-development.mddocs/user/validation.mdpkg/collector/topology/topology.gopkg/collector/topology/topology_test.gopkg/constraints/doc.gopkg/constraints/evaluate.gopkg/constraints/extractor.gopkg/constraints/gpu_nodes.gopkg/constraints/gpu_nodes_test.gopkg/validator/doc.gopkg/validator/validator.gopkg/validator/validator_test.gorecipes/overlays/gke-cos.yamltests/uat/gcp/cluster-config.yaml
2724380 to
91c0d71
Compare
Add the node-set constraint form NodeTopology.gpu-nodes.label (issue NVIDIA#1755): a name-dispatched evaluator in pkg/constraints that quantifies a label predicate over the snapshot's GPU-node set instead of comparing a scalar reading. The value grammar is "key=value" (every GPU node carries the label with exactly that value) or "!key" (no GPU node carries the key) -- the shape ADR-015's GKE gpuStack profile consumes unchanged when profile recipes land. The GPU-node universe is synthesized from the snapshot's existing NodeTopology.label readings: nodes carrying GKE's native cloud.google.com/gke-accelerator label, which exists from pool creation -- before the GPU Operator or NFD run. Both predicate directions fail closed on a truncated node list (snapshots captured with --max-nodes-per-entry append a "(+N more)" tail that makes set membership unprovable), on an empty GPU-node universe (a vacuous pass is the dangerous direction), and on values that parse as neither grammar form. Disambiguated label keys (key.value, emitted when a key carries multiple values across the cluster) are handled in both shapes, and mixed values fail the positive predicate naming the offending nodes. The readiness gate runs in both validator entry points: ValidatePhases and the per-phase ValidatePhase (SDK callers running a single phase must not bypass the recipe's readiness constraints), and declared readiness constraints with no snapshot to evaluate them against fail closed rather than silently skipping. Mark ADR-015 Deferred Decision 2 resolved in the design doc, note the node-set form's name-dispatched bypass of the scalar operator grammar in the contributor guide, and caution in the remediation text that --node-labels replaces the pool's full label set. Wire the check into the GKE-COS base overlay's validation.readiness.constraints -- not spec.constraints, which would exclude the GKE overlays during snapshot-based generation on exactly the unlabeled cluster the diagnostic exists to fix. checkReadiness now evaluates readiness-phase constraints alongside the top-level set (they were declared and merged but never consumed) and carries the constraint's remediation text into the failure message, so an unlabeled GKE cluster fails `aicr validate` closed (exit 2) with the device-plugin ownership diagnostic before any phase runs. Also label the UAT GKE GPU pool with gke-no-default-nvidia-gpu-device-plugin=true: the pool predated the documented prerequisite and would fail the new gate. Review rounds hardened two fail-open paths: label keys and values are validated with Kubernetes's own validators (a key no node can legally carry would make the negated predicate pass vacuously), and disambiguated-shape decoding now enforces encodeLabels' invariants (plain and disambiguated forms never coexist; genuine disambiguation yields at least two entries), so a distinct dotted label whose value equals its own suffix can no longer satisfy the predicate. The truncation detector moved next to the collector's encoder (topology.IsTruncatedNodeList) so format and detector cannot drift apart. A further round closed a hybrid-collision fail-open: the encoder's "<key>.<value>" disambiguation can collide with a real label of that literal name (silently overwriting one reading — NVIDIA#2003), so an accepted disambiguated set must now partition its nodes; overlapping node sets fail closed as an ambiguous reading. Two further negated- predicate fail-opens are closed: a single disambiguated-shape entry without the plain key (possibly a collision remnant) and structurally malformed readings are rejected instead of decoding to sets the negated form passes vacuously: missing or extra separators, empty node lists, and node tokens that are not canonical Kubernetes node names (RFC 1123 subdomains) all fail closed. This resolves ADR-015 Deferred Decision 2 in place: the node-set constraint form lands under NVIDIA#1755, and the follow-up GKE profile recipes consume it. Fixes NVIDIA#1755 Refs NVIDIA#1761 Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
91c0d71 to
1aede4d
Compare
yuanchen8911
changed the title
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
🔎 Multi-Persona Review — fail-closed GKE device-plugin ownership check
Method: 4 independent persona reviewers (Correctness · Security/fail-closed · Domain & Architecture · Test-coverage) fanned out against the diff, then every finding was re-derived from the resolved code at 1aede4d5 by an adversarial senior meta-reviewer (confirm / refute / re-tier). Personas post nothing directly; only surviving findings appear below.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
This is an unusually careful, defense-in-depth PR, and the two highest-stakes lenses came back clean:
- Correctness traced
encodeLabels↔decodeLabelEntriesand confirmed the disambiguation/collision invariants hold — every ambiguous / truncated / empty / malformed shape returns an error rather than vacuously satisfying a predicate. Thecs[:len(cs):len(cs)]three-index slice correctly avoids mutating the caller's constraints; no double-evaluation (Readiness is excluded fromPhaseOrder); regex, nil-safety andgo vetclean. - Security / fail-closed confirmed both entry points (
ValidatePhases+ValidatePhase) gate before the--no-clustershort-circuit, nil snapshot fails closed, the truncation regex is not defeatable by crafted node names / label values, and value-grammar validation closes the vacuous-negated-pass hole in both directions.
No blockers. The single 🟠 is a test-coverage gap, not a code defect — the code is correct today but nothing pins the default aicr validate path's fail-closed behavior. The 🟡s are a merge-order doc dependency, an architecture-coupling smell, and a missing round-trip guard.
Recommendation: ✅ Approve with comments.
Confirmed non-issues (examined and refuted)
- No fail-open path in either predicate direction — empty universe, truncated universe/target, missing readings, malformed encodings, non-canonical node tokens, single-dotted ambiguous shape, and collision-remnant all return errors.
- Truncation regex
\(\+\d+ more\)$is not forgeable — RFC-1123 node names and label values cannot contain the pattern or|. - No constraint-slice aliasing —
cs[:len(cs):len(cs)]is the correct full three-index expression. - No double-evaluation of readiness constraints — Readiness is excluded from
PhaseOrder; phase execution consumes different constraint sets. - Both entry points gated,
--no-clusterstill evaluates the gate, nil snapshot fails closed, no SDK bypass viapkg/client/v1.
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 1 |
| 🟡 Minor | 3 |
| 🔵 Nitpick | 2 |
Inline comments follow.
Reviewed with a multi-persona + adversarial meta-review workflow. Findings were re-derived from the resolved code before posting; the reviewed commit's code was not executed beyond go test/go vet.
| // Pre-flight: evaluate the top-level and readiness-phase constraints | ||
| // against the snapshot. Fails fast before deploying any Jobs if | ||
| // prerequisites aren't met. | ||
| if err := checkReadiness(validationInput, snap); err != nil { |
There was a problem hiding this comment.
🟠 Major — ValidatePhases readiness-failure branch is untested — the default aicr validate path can regress to fail-open
Coverage confirms this return nil, err (when checkReadiness fails inside ValidatePhases) has hit count 0. Only the singular ValidatePhase path is tested (TestValidatePhaseRunsReadinessPreflight); both ValidatePhases tests pass a nil snapshot so the gate returns early. The default client validate path routes through ValidatePhases (pkg/client/v1/aicr.go). A regression that reorders checkReadiness below the NoCluster short-circuit here would pass every existing test while silently fail-opening the primary path — the exact #1755 failure this PR exists to prevent. The code is correct today; the gap is that nothing pins it.
Blast radius: Default aicr validate (full-phase) could regress to fail-open on the readiness gate with no test catching it; the singular path is guarded but the primary plural path is not.
Fix: Add a ValidatePhases analog of TestValidatePhaseRunsReadinessPreflight — no-cluster Validator + a recipe with a failing readiness constraint + non-nil snapshot — asserting ValidatePhases returns ErrCodeInvalidRequest and no PhaseResults.
|
|
||
| "k8s.io/apimachinery/pkg/util/validation" | ||
|
|
||
| "github.com/NVIDIA/aicr/pkg/collector/topology" |
There was a problem hiding this comment.
🟡 Minor — I/O-free constraints eval package now transitively imports client-go for a 3-line regexp helper
pkg/constraints imports pkg/collector/topology solely for the pure IsTruncatedNodeList regexp, but topology.go imports k8s.io/client-go/kubernetes + pkg/k8s/client, so this foundational shared eval package (used by recipe/validator/client) now transitively drags in the k8s client graph. Verified this is coupling, not a defect: topology does not import constraints (no import cycle), and doc.go's runtime claim ("never performs network or cluster I/O") still holds — the helper is pure. Compiles, tests pass.
Fix: Optional/directional: hoist the truncation marker + IsTruncatedNodeList (ideally the whole value|nodes codec) into a client-go-free leaf that both the collector and pkg/constraints depend on. Reasonable but heavy for one helper — fine to fold into the #2003 lossless-encoding work.
| // universe member, letting the negated predicate pass vacuously. Truncated | ||
| // readings skip token validation — their "(+N more)" tail is not a node | ||
| // name by design — and keep their distinct truncation diagnostic. | ||
| func decodeLabelEntries(data map[string]measurement.Reading, key string) ([]labelNodeSet, error) { |
There was a problem hiding this comment.
🟡 Minor — Cross-package wire-format contract has no encode→decode round-trip test
decodeLabelEntries/cutLabelEncoding/splitNodes independently re-implement topology.encodeLabels' wire format — the | separator, comma-joined node list, and the <key>.<value> disambiguation rule. Only IsTruncatedNodeList has a shared round-trip guard; the decoder tests hand-build encoded strings rather than feeding real encodeLabels() output through decodeLabelEntries. A future collector-side format change (separator swap, different disambiguation trigger) could silently break decode — or make it fail-closed on healthy snapshots — with zero failing tests.
Fix: Add a round-trip test that runs encodeLabels output (plain, disambiguated, truncated shapes) through decodeLabelEntries, so the format and its parser cannot diverge. The #2003 lossless-encoding fix is the durable resolution.
| (devicePlugin.enabled: true). Add the label to the GPU node pool | ||
| (gcloud container node-pools update ... --node-labels=...); | ||
| note --node-labels REPLACES the pool's full label set, so pass | ||
| every existing label plus the new one (see the Component |
There was a problem hiding this comment.
🟡 Minor — Failure remediation cites a Component Catalog section that does not exist on this tree
The readiness remediation tells operators to "see the Component Catalog's GKE Device-Plugin Ownership section for the discovery-then-update procedure," but docs/user/component-catalog.md has no such section, and the gcloud --node-labels discovery-then-update procedure appears nowhere in docs/. A user who hits exit 2 today follows a dead pointer. The PR body notes this is gated on #1998 (merge-first) adding the paragraph, refreshed here on rebase — which mitigates but leaves a fragile merge-order dependency.
Fix: Ensure #1998 lands first (or add the section here) and re-verify the reference resolves before merge. If timing slips, point the remediation at docs/user/validation.md, which does cover the behavior.
| for _, c := range cs { | ||
| result := constraints.Evaluate(c, snap) | ||
| if result.Error != nil { | ||
| return errors.WrapWithContext(errors.ErrCodeInvalidRequest, |
There was a problem hiding this comment.
🔵 Nitpick — checkReadiness flattens the evaluator's deliberate ErrCodeNotFound to ErrCodeInvalidRequest
This unconditional re-wrap collapses the ErrCodeNotFound that gpu_nodes.go returns for empty-universe / missing-readings, unlike PropagateOrWrap used elsewhere in the file. Defensible on the readiness path — the documented contract is a uniform fail-closed exit 2, and "constraint can't be evaluated" reads as invalid-request — so no change is required. Flagging only for awareness; a one-line comment noting the deliberate flattening would preempt the next reader's double-take.
| "invalid node-set constraint value: expected \"<label-key>=<value>\" or \"!<label-key>\"", | ||
| map[string]any{ctxValue: raw}) | ||
| } | ||
| if errs := append(validation.IsQualifiedName(key), validation.IsValidLabelValue(want)...); len(errs) > 0 { |
There was a problem hiding this comment.
🔵 Nitpick — IsValidLabelValue(want) is never the sole rejecting predicate in tests
The positive-form validation append(IsQualifiedName(key), IsValidLabelValue(want)…) is only exercised with an invalid key (where IsQualifiedName already fails). No case supplies a valid key with an invalid label value (e.g. foo=bad value with a space), so IsValidLabelValue(want)'s independent contribution is unpinned. Working guard, no current defect.
Fix: Add a table row with a valid key and an invalid label value expecting ErrCodeInvalidRequest. (Companion nit: no case supplies key= with an empty value — the load-bearing want == "" guard at line 138 is similarly unpinned, though its misparse direction fails closed.)
2 participants
Summary
Adds the fail-closed GKE device-plugin ownership check from #1755: a new node-set constraint form (
NodeTopology.gpu-nodes.label) evaluated ataicr validatereadiness, wired into the GKE-COS overlays so an unlabeled GPU node pool fails validation closed (exit 2) with an actionable diagnostic before any phase runs.Motivation / Context
AICR's GKE recipes ship the GPU Operator with
devicePlugin.enabled: true, while a default-provisioned GKE GPU node pool also runs GKE's managed device plugin — two advertisers ofnvidia.com/gpuper node, with nondeterministic ownership. Until now nothing detected the conflict:aicr bundleis offline, and the generic constraint syntax compares scalar readings, so "every GPU node carriesgke-no-default-nvidia-gpu-device-plugin=true" was inexpressible. PR #1998 documented the prerequisite; this PR enforces it.Scope position (ADR-015 Deferred Decision 2 — marked resolved in the ADR by this PR). The reusable node-set constraint form — including the negated direction — lands here under #1755, exactly as the ADR reads it. The follow-up GKE
gpuStackprofile recipes (#1761 adoption step) will consume the form unchanged when they land:operatoruses the positive predicate,csp-managedthe negated one. Nothing in this PR touches the profile mechanism, theConstraintschema, the OpenAPIpropertyNamesallowlists, or the v1alpha2/v1alpha3 version gate.Merge order / dependency: #1998 (the documentation half of #1755) should merge first; this PR is then rebased, and its one-line refresh of the component-catalog paragraph ("AICR has no deterministic check for a violation today" → point at this readiness gate) lands with the rebase. The two trees otherwise merge without conflict.
Fixes: #1755
Related: #1761, #1998 (merge first — see above)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter) — exported truncation predicate onlypkg/validator)pkg/errors,pkg/k8s)docs/,examples/)recipes/overlays/gke-cos.yaml,tests/uat/gcp/cluster-config.yaml,pkg/constraintsImplementation Notes
The form is name-dispatched, not schema-extended.
constraints.Evaluatedispatches on the exact nameNodeTopology.gpu-nodes.labelbefore the scalar path (precedent: thenccl-benchmark-*sentinel names). The constraint staysname+value, so both existing call sites (readiness, overlay/profile evaluation) get the form with no API or artifact-version change.Value grammar is
<label-key>=<value>(every GPU node carries it) or!<label-key>(no GPU node carries the key), validated with the vendored Kubernetes label validators (IsQualifiedName/IsValidLabelValue) — so the scalar operator grammar (>= x,!= x) cannot be misread as a key/value predicate, and a key no node can legally carry (e.g. a double slash) is rejected instead of making the negated predicate pass vacuously.GPU-node universe = nodes under the snapshot's
cloud.google.com/gke-accelerator*label readings — GKE's native signal, present from pool creation before the Operator or NFD run (NFD'snvidia.com/gpu.*labels don't exist on the pre-deployment cluster this check validates). Per the issue's acceptance requirements, both predicate directions fail closed on: truncated node lists (--max-nodes-per-entrysnapshots), an empty universe (no vacuous pass), missingNodeTopology.labelreadings, and mixed/disambiguated label values (key.valueencoding). Disambiguated-shape decoding enforcesencodeLabels' invariants — plain and disambiguated forms never coexist, and genuine disambiguation always yields at least two entries — so a distinct dotted label whose value equals its own suffix (e.g. a literal<key>.true=truelabel) cannot satisfy the predicate; an accepted disambiguated set must additionally partition its nodes — overlapping node sets prove the encoder's<key>.<value>map key collided with a real label of that literal name (one reading silently overwritten, #2003) and fail closed as an ambiguous reading. A single disambiguated-shape entry without the plain key (a possible collision remnant) and structurally malformed readings (missing or extra|separators, empty node lists, node tokens that are not canonical RFC-1123 node names) also fail closed — both would otherwise let the negated predicate pass vacuously. The residual ambiguity (collisions with identical node sets, or every disambiguated entry overwritten) is what the lossy encoding cannot express; the lossless-encoding root-cause fix is tracked in #2003, structured truncation metadata in #2002. The truncation detector is owned by the collector (topology.IsTruncatedNodeList, next toformatNodeList, with a round-trip test) so the format and its detector cannot drift apart.Both validator entry points are gated.
checkReadinessruns inValidatePhasesand in the per-phaseValidatePhase(the exported SDK pathClient.ValidateStatedocumentation directs per-phase callers to) — a single-phase caller cannot bypass the recipe's readiness constraints, and declared readiness constraints with a nil snapshot fail closed instead of silently skipping. Both pinned by tests.Wiring: readiness, not
spec.constraints. A top-level constraint would exclude the GKE overlays during snapshot-based generation on exactly the unlabeled cluster the diagnostic exists to fix. The check lives in the GKE-COS base overlay'svalidation.readiness.constraints;checkReadinessnow evaluates readiness-phase constraints alongside the top-level set (they were declared, merged, and carried intoValidationInputbut never consumed) and appends the constraint'sremediationto the failure message — that's where the issue's required device-plugin diagnostic lives.UAT cluster config. The GCP UAT GPU pool (
tests/uat/gcp/cluster-config.yaml) predates the documented prerequisite and does not carry the label; without fixing it this PR would fail the GKE UAT lane. The label is added to the pool'snodeConfig.labels. Note the config actuates at cluster provision time — the nightly lifecycle (provision→CUJ→teardown) picks it up on the next run; a held daytime cluster would need re-provisioning or a manualgcloud container node-pools update.Sequencing with #1998. #1998 merges first; this PR rebases on it and refreshes the component-catalog paragraph (added there) that states "AICR has no deterministic check for a violation today" — kept out of this PR until the rebase to avoid cross-PR conflicts.
Testing
Coverage (per-package, current vs origin/main baseline):
pkg/constraints: 97.9% → 97.9% (flat)pkg/validator: 48.5% → 50.9% (+2.4%)pkg/collector/topology: 84.2% → 84.4% (+0.2%)CLI end-to-end (branch-built binary, no cluster): generated the GKE H100 training recipe with
aicr recipe— the hydrated artifact carries the readiness constraint with its remediation — then ranaicr validate --no-clusteragainst four crafted snapshots: labeled pool → exit 0 ("all 2 GPU node(s) carry …"); unlabeled pool → exit 2 naming the offending nodes plus the full device-plugin remediation; truncated snapshot → exit 2 with the regenerate-without---max-nodes-per-entryinstruction; no GPU nodes → exit 2 with the empty-universe diagnostic. Live-cluster validation deliberately skipped: the gate is inline (no cluster I/O), and the first post-merge nightly UAT provisions from this PR's cluster-config and runsaicr validatethrough the gate, providing the live confirmation automatically.New tests: table-driven coverage of both predicate directions × {pass, fail, mixed values via disambiguated keys, multiple accelerator types, non-GPU nodes carrying the label, truncated universe reading, truncated target reading (both directions), empty universe (both directions), missing label subtype/measurement/nil snapshot, prefix-collision non-misattribution, scalar-grammar rejection, malformed values}; validator-level test that readiness-phase constraints are evaluated and carry remediation into the failure.
New exported symbols: the
GPUNodesLabelConstraintNameconst andtopology.IsTruncatedNodeList(covered by the round-trip test).make qualifywas run unsandboxed after verifying the sandbox-only failures (cleanup_test.shstubs,httptestport binds) reproduce identically on a cleanorigin/maincheckout.Risk Assessment
Rollout notes: A pre-change
aicrbinary evaluates only top-level constraints and silently ignoresvalidation.readiness.constraintsin a recipe generated at this commit (version skew is advisory-only) — validate recipes with a matching or newer binary.aicr validateagainst a GKE cluster whose GPU pools lackgke-no-default-nvidia-gpu-device-plugin=truenow fails readiness closed (exit 2) — intended and documented, but visible to existing users of unlabeled clusters; the failure message carries the exact remediation. Snapshots captured with--max-nodes-per-entryalso fail this constraint closed with a regenerate instruction. Recipe generation and bundling are unaffected. The UAT GKE pool config is updated in this PR; held daytime UAT clusters need one re-provision (or a manual pool update) before their next validate run.Checklist
make testwith-race)make lint)git commit -S) — GPG signing info