feat(validator): carry redaction-safe CTRF extra outcome data - #1973
Conversation
njhensley
added
the
theme/validation
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
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:
📝 WalkthroughWalkthroughThe PR adds structured Estimated code review effort: 3 (Moderate) | ~25 minutes 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: 9
🤖 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 213-214: Update the inline code spans in the validator
documentation table to remove trailing spaces from the code content: represent
the delimiters as `RESULT:` and `##AICR-EXTRA##`, then describe the following
single space separately in prose. Apply the same MD038 fix to the additional
occurrence at the referenced later location.
In `@pkg/evidence/redact/redact_test.go`:
- Around line 411-427: Refactor TestCTRFAllowlistsExtra into named table-driven
cases that define each input Extra map and its expected output, including
invalid values for otherwise allowed keys. Execute each case independently and
assert preserved allowlisted entries, removed unknown entries, and nil Extra
when all values are discarded, without relying on hard-coded result indexes.
In `@pkg/evidence/redact/redact.go`:
- Around line 301-308: Update the allowlisted Extra-value handling in the
surrounding redaction function to validate values as well as keys: accept only
non-negative decimal counts for numeric fields such as nodesTotal, and only the
defined closed enum values for skipReason; drop invalid entries while preserving
valid ones. Add adversarial tests covering hostname/IP and other invalid values
under allowed keys.
In `@pkg/validator/ctrf/builder.go`:
- Around line 169-171: Update the result-building logic around tr.Extra to
retain a defensive clone of r.Extra rather than the caller-owned map, while
preserving the existing empty-map behavior. Add a regression test that mutates
ValidatorResult.Extra after insertion and verifies the stored CTRF report
remains unchanged.
In `@pkg/validator/ctrf/types.go`:
- Around line 155-166: Enforce Extra value validation at the publication
boundary in the pkg/evidence/redact flow, not only at emission sites. For
allowlisted keys such as nodesTotal and skipReason, retain only canonical
non-negative count strings and values from the finite skip-reason enum; drop
invalid entries and omit the Extra object when no valid entries remain. Ensure
this also protects data parsed from raw prefixed stdout.
In `@pkg/validator/job/result.go`:
- Around line 130-152: The sentinel loop should retain the last valid non-empty
payload instead of storing only the final sentinel before parsing. Update the
logic around sawSentinel and parsed to unmarshal each sentinel as encountered,
warn for malformed entries, and replace the accumulated extra map only after a
valid non-empty object; preserve the earlier valid map when a later sentinel is
malformed, including the valid-then-malformed case.
In `@validators/deployment/nvidia_smi.go`:
- Line 121: Replace the success message’s fmt.Printf call with structured
slog.Info logging, preserving the verification message and passing len(gpuNodes)
as a named node-count attribute. Remove the production logging use of fmt.Printf
in this success path.
- Around line 110-119: Update the NVIDIA SMI validation flow around
nvidiaSMICoverageExtra and the failed-node return to determine the GPU-node
denominator before verification and emit coverage for both success and failure
outcomes. Track FindAllGpuNodes discovery failure separately so failed discovery
does not fall back to len(gpuNodes) or emit nodesTotal; preserve coverage
emission on validation failures. Add check-level tests covering successful and
failed verification paths, including denominator-discovery failure.
In `@validators/helper/gpu.go`:
- Around line 80-83: Update the node-filtering loop in the GPU validation flow
to check the request context via ctx.Err() or ctx.Done() on each iteration. When
cancellation is detected, stop processing and return the established wrapped
structured cancellation error; otherwise preserve the existing allocatable-GPU
filtering 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: 97d8a0be-b858-4a76-bc31-c6e1ecca78f6
📒 Files selected for processing (15)
docs/contributor/validator.mdpkg/evidence/attestation/emit_test.gopkg/evidence/redact/redact.gopkg/evidence/redact/redact_test.gopkg/validator/ctrf/builder.gopkg/validator/ctrf/ctrf_test.gopkg/validator/ctrf/types.gopkg/validator/job/result.gopkg/validator/job/result_test.govalidators/deployment/nvidia_smi.govalidators/deployment/nvidia_smi_test.govalidators/extras.govalidators/extras_test.govalidators/helper/gpu.govalidators/helper/gpu_test.go
| // Extra carries structured, low-cardinality outcome data that survives the | ||
| // default "minimal" redaction policy (unlike Stdout and Message, which are | ||
| // free-form log text stripped by default). It mirrors the CTRF spec's | ||
| // `extra` object. | ||
| // | ||
| // CONTRACT: values MUST be low-cardinality counts or enum codes only — | ||
| // e.g. "1", "2", "all-cordoned", "no-gpu-nodes". NEVER node names, IPs, | ||
| // hostnames, or any operator-identifying free text; those belong in Stdout, | ||
| // which is redacted by default. Only keys on the redact package's explicit | ||
| // allowlist survive publication; unknown keys are dropped fail-closed | ||
| // (see pkg/evidence/redact). | ||
| Extra map[string]string `json:"extra,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce allowed value formats at the publication boundary.
The documented contract is not enforced: downstream redaction allowlists keys, but preserves their values. A validator can emit {"skipReason":"node-a.example"} or {"nodesTotal":"10.0.0.5"} and that identifier survives minimal redaction under an allowed key. Validate canonical non-negative counts and a finite skip-reason enum in pkg/evidence/redact; drop invalid values and empty maps. Emission-side validation alone is bypassable through raw prefixed stdout.
🤖 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/validator/ctrf/types.go` around lines 155 - 166, Enforce Extra value
validation at the publication boundary in the pkg/evidence/redact flow, not only
at emission sites. For allowlisted keys such as nodesTotal and skipReason,
retain only canonical non-negative count strings and values from the finite
skip-reason enum; drop invalid entries and omit the Extra object when no valid
entries remain. Ensure this also protects data parsed from raw prefixed stdout.
| } | ||
| emitExtraOrWarn(nvidiaSMICoverageExtra(len(gpuNodes), total)) | ||
|
|
||
| fmt.Printf("Successfully verified GPU on all %d nodes\n", len(gpuNodes)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use structured logging for the added success message.
Route this through slog.Info with the node count as an attribute rather than adding a fmt.Printf production log.
As per coding guidelines, “Use slog for production logging; do not use fmt.Printf for logging.”
🤖 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 `@validators/deployment/nvidia_smi.go` at line 121, Replace the success
message’s fmt.Printf call with structured slog.Info logging, preserving the
verification message and passing len(gpuNodes) as a named node-count attribute.
Remove the production logging use of fmt.Printf in this success path.
Source: Coding guidelines
cdb9f21 to
3a846ba
Compare
|
Addressed CodeRabbit's review in the amended commit. Disposition: Applied (7):
Not applied (2), with reasoning:
|
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 `@docs/contributor/validator.md`:
- Around line 239-244: Update the documented EmitExtra examples to capture and
log returned errors using the warning pattern from
validators/deployment/nvidia_smi.go, while preserving the validator verdict and
existing success/skip payloads.
🪄 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: 8528dd0f-e16d-4b5c-8d05-a2118b05b9aa
📒 Files selected for processing (15)
docs/contributor/validator.mdpkg/evidence/attestation/emit_test.gopkg/evidence/redact/redact.gopkg/evidence/redact/redact_test.gopkg/validator/ctrf/builder.gopkg/validator/ctrf/ctrf_test.gopkg/validator/ctrf/types.gopkg/validator/job/result.gopkg/validator/job/result_test.govalidators/deployment/nvidia_smi.govalidators/deployment/nvidia_smi_test.govalidators/extras.govalidators/extras_test.govalidators/helper/gpu.govalidators/helper/gpu_test.go
3a846ba to
f958530
Compare
|
Follow-up: fixed the doc |
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 `@docs/contributor/validator.md`:
- Around line 195-197: Update the stdout row in the contributor validator
documentation to identify pod logs as CTRF stdout only; remove the message
mapping, while leaving the stderr and /dev/termination-log mappings unchanged.
In `@pkg/evidence/redact/redact.go`:
- Around line 177-199: Replace the regex-only validator used by the skipReason
entry in ctrfExtraAllowlist with an explicit allowlist of supported skip codes,
while retaining the existing validation for other enum values. In
pkg/evidence/redact/redact_test.go lines 411-472, add a lowercase kebab-case
value that is not a supported skip code and assert that minimal redaction drops
it.
In `@validators/deployment/nvidia_smi.go`:
- Line 53: Update validators/deployment/nvidia_smi.go at lines 53-53 so
FindSchedulableGpuNodes returning zero emits a distinct “no schedulable GPU
nodes” code rather than skipReasonNoGPUNodes, unless actual GPU-node absence has
been established. At lines 77-78, handle busy-status lookup failures with a
separate status-unavailable reason or propagate the lookup error; do not emit
nodes-busy for an unknown status.
In `@validators/helper/gpu_test.go`:
- Around line 54-65: Update the test assertions for FindAllGpuNodes and
FindSchedulableGpuNodes to verify node identities, not just counts: require the
expected GPU node names, ensure gpu-cordoned is included only in the all-nodes
result as appropriate, and explicitly exclude the cpu-only node from both
results.
🪄 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: 798b092a-f29f-4804-8860-d035224be5ff
📒 Files selected for processing (15)
docs/contributor/validator.mdpkg/evidence/attestation/emit_test.gopkg/evidence/redact/redact.gopkg/evidence/redact/redact_test.gopkg/validator/ctrf/builder.gopkg/validator/ctrf/ctrf_test.gopkg/validator/ctrf/types.gopkg/validator/job/result.gopkg/validator/job/result_test.govalidators/deployment/nvidia_smi.govalidators/deployment/nvidia_smi_test.govalidators/extras.govalidators/extras_test.govalidators/helper/gpu.govalidators/helper/gpu_test.go
f958530 to
dc3acec
Compare
|
Second review round addressed (all 4 applied):
|
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/ctrf/types.go`:
- Around line 154-168: Update the Extra contract documentation near the Extra
field to remove the stale "all-cordoned" example and use only skip-reason values
accepted by ctrfSkipReasons, such as "no-gpu-nodes", "no-schedulable-gpu-nodes",
and "nodes-busy". Do not change the Extra field or redaction implementation.
🪄 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: 7fe33d31-4a26-484a-a15d-525173e960b8
📒 Files selected for processing (15)
docs/contributor/validator.mdpkg/evidence/attestation/emit_test.gopkg/evidence/redact/redact.gopkg/evidence/redact/redact_test.gopkg/validator/ctrf/builder.gopkg/validator/ctrf/ctrf_test.gopkg/validator/ctrf/types.gopkg/validator/job/result.gopkg/validator/job/result_test.govalidators/deployment/nvidia_smi.govalidators/deployment/nvidia_smi_test.govalidators/extras.govalidators/extras_test.govalidators/helper/gpu.govalidators/helper/gpu_test.go
294ea4f to
f398ea7
Compare
|
Applied the multi-persona review. Disposition of all 9 findings: Applied (8):
Not applied (1):
|
f398ea7 to
00c6f65
Compare
|
Thanks @yuanchen8911 — all three addressed in the latest push ( #1 — TOCTOU between two Node LISTs (blocking). Fixed as you prescribed: added #2 — probe errors masked as #3 — inaccurate "no GPU nodes found" message. Eliminated by #1: with a single snapshot there is no second-LIST-failure branch, so the misleading message can no longer be produced. The cordoned-only case accurately reads Side effects: |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
00c6f65 verified — both blockers fixed: confirmedBusy gates the nodes-busy evidence signing so probe errors no longer masquerade as occupancy, and the single-snapshot GpuNodePartition drives node selection, zero-schedule classification, and the coverage denominator, eliminating the TOCTOU window. Finding 3 dissolved with the second LIST as anticipated, and TestNvidiaSMIZeroScheduleOutcome asserts both accurate outcomes. Approving.
00c6f65 to
b371989
Compare
|
Rebased onto
Net: |
|
@njhensley this PR now has merge conflicts with |
…#1951) Validator coverage disclosure (e.g. check-nvidia-smi's nodesValidated/nodesTotal and skip reasons from NVIDIA#1936) only reached TestResult.Stdout/.Message, which the default "minimal" redaction policy strips from a signed evidence bundle. A downstream consumer verifying the default artifact could not distinguish a 1-of-2-node pass from a 2-of-2 pass, and an all-cordoned skip lost its reason. Carry that signal in a structured, low-cardinality CTRF field instead of overloading free-form log text: - ctrf.TestResult gains an Extra map[string]string mirroring the CTRF spec's `extra` object, plus an ExtraLinePrefix contract shared between the in-pod producer and the orchestrator consumer. - validators.EmitExtra marshals a check's Extra map to a single prefixed stdout line; job.ExtractResult parses each sentinel line and keeps the last VALID non-empty payload (a later malformed line no longer discards an earlier valid one), stripping all prefixed lines from human-readable Stdout. - redact.CTRF rebuilds Extra against a fail-closed key AND value allowlist (ctrfExtraAllowlist): a surviving key's value must also match its canonical shape (decimal count / kebab-case enum code), so an identifier smuggled under an allowed key is dropped at the publication boundary — emission-side validation alone is bypassable. Unknown keys and empty results ship nothing; PolicyVersion bumps to v2. - checkNvidiaSMI computes the coverage denominator before verification and emits coverage on BOTH pass and fail paths, counting only nodes that passed and omitting nodesTotal when node discovery failed rather than fabricating a full-coverage claim; helper adds FindAllGpuNodes. - builder.AddResult defensively copies Extra so later caller mutation cannot alter the built report. - docs/contributor/validator.md documents the field, the contract, the value-shape enforcement, and the allowlist. The contract is enforced by godoc and the redact allowlist: values must be counts or enum codes only, never node names, IPs, or hostnames. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
b371989 to
88ee46f
Compare
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Re-approving after the rebase onto merged #1936 (the earlier approval was auto-dismissed by the force-push). Verified at 88ee46f:
- The plumbing files (ctrf types/builder, redact, extras) are byte-identical to the previously approved head, and the nvidia_smi adoption is rebuilt cleanly on #1936's partition with all three earlier fixes preserved (single-snapshot classification, confirmedBusy gating, accurate skip messages).
- The only new delta is the result.go sentinel transport, which is sound: Extra is parsed from the full logs before tail-truncation, malformed sentinel lines are skipped without discarding an earlier valid payload, sentinels are stripped from human stdout, and both extraction paths are covered.
On the two open CodeRabbit threads: the value-format enforcement one is already implemented — redact.go's ctrfExtraAllowlist is fail-closed on key AND value (bounded counts, closed skip-code enum) at the publication boundary, so the thread is stale against current code. The fmt.Printf one misapplies the logging rule: validator-check stdout is the CTRF evidence transport; slog would remove the line from evidence. Neither blocks.
2 participants
Summary
Carry validator coverage disclosure (per-check node counts, skip-reason codes) in a structured CTRF
extrafield that survives the default "minimal" redaction policy, instead of overloadingStdout/Message(which are correctly stripped from signed evidence bundles by default).Motivation / Context
#1936addednodesValidated: <schedulable>/<total>(plus per-node cordoned-skip disclosure) tocheck-nvidia-smistdout, so a pass on a reduced node set is visible rather than silent (#1668). But that disclosure only reachesTestResult.Stdout/.Message, whichpkg/evidence/redact's default "minimal" policy omits from a published, signed bundle. So in the default signed-bundle path a 1-of-2-node pass was indistinguishable from a 2-of-2 pass, and an all-cordoned skip lost its reason — for the very artifact a downstream consumer verifies by default.Fixes: #1951
Related: #1936, #1668
Type of Change
Component(s) Affected
pkg/validator)docs/,examples/)pkg/evidence/redact,validators/Implementation Notes
ctrf.TestResult.Extra(map[string]string) mirrors the CTRF spec'sextraobject.ctrf.ExtraLinePrefix(##AICR-EXTRA##) is the shared transport contract between the in-pod producer and the orchestrator consumer; it lives inpkg/validator/ctrfbecause both sides import it.validators.EmitExtra(map[string]string)marshals a check's outcome data to one prefixed stdout line. Consumer —pkg/validator/job.ExtractResultparses the last such line intoTestResult.Extraand strips all prefixed lines from human-readableStdout.redact.CTRFrebuildsExtraagainst a fail-closed allowlist (ctrfExtraAllowlist:nodesValidated,nodesTotal,skipReason). Unknown keys (including any a future check adds) are dropped; an empty result ships noextra: {}.PolicyVersionbumpsv1→v2.checkNvidiaSMInow populatesExtrawith its coverage counts / skip code;validators/helper.FindAllGpuNodescomputes the candidate-node total.Extravalues must be low-cardinality counts or enum codes only — never node names, IPs, or hostnames. The default treatment of free-formStdout/Messageis unchanged (a stated non-goal of the issue).Testing
golangci-linton all changed packages — 0 issuesgo test -race ./pkg/validator/... ./pkg/evidence/... ./validators/...— PASSmake test-coverage— 80.8% (threshold 80%) PASSmake qualify— PASS end-to-end (chainsaw e2e 23/0/0; grype: only pre-existing Low/Unknown transitive advisories, non-gating)New exported funcs covered:
EmitExtra75.0%,FindAllGpuNodes87.5%.Risk Assessment
Rollout notes: Backwards compatible.
Extraisomitempty; existing checks that don't emit it are unaffected. Redaction fails closed, so no new data leaks into signed bundles.PolicyVersionbump (v2) lets verifiers tell which rules ran.Checklist
make testwith-race)make lint)git commit -S)