fix(aiperf-bench): move runtime to NVIDIA distroless python - #1999
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Coverage Report ✅
Coverage BadgeCoverage unchanged by this PR. |
a78e574 to
4517c5d
Compare
|
All four review items addressed in 4517c5d (force-push;
The force-push also corrected a mistake of mine: an over-eager squash had absorbed upstream #1987 (the codeql-action bump) into this branch. The branch is now a single commit parented directly on Re-verified after the changes: wrapper exit paths in the rebuilt distroless image (missing binary → 127, happy path → 0 with correct sentinel framing, aiperf failure → 7), |
4517c5d to
84cbe6c
Compare
84cbe6c to
1612e03
Compare
njhensley
left a comment
There was a problem hiding this comment.
📋 Multi-Persona Review — ✅ Approve with comments
Method: 4 independent persona reviewers (Correctness · Security/Supply-chain · Domain & Architecture · Test-coverage) → an adversarial senior meta-reviewer that re-derived every finding from the resolved code at 1612e034.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
This is a well-executed, honestly-documented base-image swap. The meta-reviewer independently reproduced and upheld the load-bearing claims:
- Injection surface is genuinely eliminated —
subprocess.run([bin, "profile", *argv], check=False),shell=False, noos.system; the model is a discrete argv element handed to execvp, so a metacharacter-bearing name is inert with no quoting. - Exit-code parity with the removed
/bin/sh -cholds — 127 exec-not-found, 128+N signal (OOMKilled still reports 137), verbatim non-zero, fail-closed on a missing result file. Benchmark-flag equivalence is exact. - The VEX work is rigorous, not hand-wavy — the 13 libexpat CVEs fixed only in 2.8.2 explicitly state the statically-bundled 2.8.1 is not itself patched and fall back to the independent unreachability argument, rather than a false "already patched." No suppression coverage is actually lost by the 11 dropped
GO-statements — each has a retained GHSA twin for the same CVE. - Non-root posture preserved (uid 1000
nvs), digest pin correct,RELEASING.mdupdated.
No blocker or major finding survived adjudication. Two 🟡 items are worth addressing (a py_compile/test gate for the Python wrapper, and a factual correction to the .openvex.json tooling note); the rest is polish. Inline comments follow.
Confirmed non-issues (examined, cleared)
- Injection surface — genuinely eliminated.
- libexpat suppressions — not over-claimed; the 2.8.2-only CVEs correctly disclaim "already patched."
- Dropped
GO-statements — no coverage lost; retained GHSA twins cover the same CVEs. - Exit-code parity + fail-closed on missing result file — holds.
- Non-root (uid 1000 nvs), digest pin, benchmark-flag equivalence — all sound.
Tier summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick | Recommendation |
|---|---|---|---|---|
| 0 | 0 | 3 | 6 | Approve with comments |
Reviewed with a multi-persona + adversarial meta-reviewer workflow. Findings are anchored to the diff at 1612e034.
Swap the aiperf-bench runtime stage from python:3.13-slim to nvcr.io/nvidia/distroless/python:3.13-v4.0.8, pinned by digest. The builder stage stays on python:3.13-slim because the distroless image ships no shell, apt, or pip and cannot host RUN steps. The distroless base has no /bin/sh, which broke two things that the image relied on: - buildAIPerfJob ran the benchmark via `/bin/sh -c` to chain aiperf, echo, and cat for the sentinel framing parseAIPerfOutput consumes. That framing now lives in aiperf_entrypoint.py, invoked in exec form. All benchmark flags stay in Go so this file remains the single source of truth; the wrapper only frames output. The model becomes a discrete argv element rather than a "$AICR_MODEL" shell expansion, which is strictly safer because no shell can re-scan it. A missing aiperf exits 127, matching what the shell predecessor returned. - `RUN useradd` cannot run, so the image adopts the base's built-in non-root nvs user (uid 1000) instead of minting uid 10001. Nothing else in the repo runs Python, so the wrapper is gated on both sides. The builder stage byte-compiles it with py_compile before the runtime stage copies it forward, and aiperf_entrypoint_test.go executes the real file against a stub aiperf to lock the exit-code contract (7 verbatim, 137 on SIGKILL, 127 exec-not-found, 1 missing result file, 2 unset env) and feeds its actual stdout to parseAIPerfOutput. A companion test reads the Dockerfile so the Go path constants cannot drift from the COPY destination or the venv layout. The base ships two Debian packages older than python:3.13-slim. The 21 libexpat1 2.7.1-2 findings (4 High, 16 Medium, 1 Low) are suppressed in .openvex.json: nothing in the image links libexpat (verified by reading ELF DT_NEEDED across every binary), and CPython routes xml.parsers.expat through a pyexpat that statically bundles expat 2.8.1. Suppression is scoped to that unused system library; where the bundled 2.8.1 is not itself patched the impact statement says so rather than overclaiming. liblzma5 5.8.1-1 (CVE-2026-34743, Medium) is deliberately left visible: _lzma.so does link it, so the evidence is weaker than for expat, and the finding is a useful signal that the base image is stale. Also drops 11 redundant aicr-gate statements and bumps the document to version 10. The current scan reports those CVEs under their GHSA identifiers rather than the go.dev ones, and the GHSA-aliased statement for each is retained, so no suppression coverage is lost. Chainsaw is unchanged at v0.2.15 and the vulnerable golang.org/x/crypto/ssh code still ships in the embedded binary, so the retained statements must stay. Net posture on the built image is unchanged at High=2 (the two pre-existing CPython CVEs, which the swap does not fix since both bases ship 3.13.14) with Medium 14 -> 15 from liblzma5. Signed-off-by: Mark Chmarny <mark@chmarny.com>
1612e03 to
86fc21f
Compare
|
All 9 review items addressed in
Every new assertion was mutation-checked: removing the newline guard, the 128+N signal mapping, the Thanks for the multi-persona pass — the VEX audit-trail catch in particular was the one that would have quietly cost us the retained GHSA suppressions later. |
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 `@validators/performance/inference_perf_test.go`:
- Around line 981-983: Correct the t.Errorf message in the envAIPerfModel
assertion to state that the variable is required for pod visibility or kubectl
describe output, not for the wrapper to pass --model. Keep the validation
condition unchanged.
🪄 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: fc13a6b1-8022-4983-9d76-ea490b844036
📒 Files selected for processing (8)
.grype.yaml.openvex.jsonRELEASING.mdvalidators/performance/aiperf-bench.Dockerfilevalidators/performance/aiperf_entrypoint.pyvalidators/performance/aiperf_entrypoint_test.govalidators/performance/inference_perf_constraint.govalidators/performance/inference_perf_test.go
| if env[envAIPerfModel] == "" { | ||
| t.Errorf("%s must be set so the wrapper can pass --model", envAIPerfModel) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the assertion message: the wrapper does not read AICR_MODEL.
The message states the env var exists "so the wrapper can pass --model". aiperf_entrypoint.py never reads it. inference_perf_constraint.go lines 149-152 and the wrapper docstring lines 29-31 both state the model reaches aiperf as a discrete --model argv element, and that the env var exists only for kubectl describe pod visibility. The wrong message invites a future change that makes the wrapper depend on the env var.
📝 Proposed fix
if env[envAIPerfModel] == "" {
- t.Errorf("%s must be set so the wrapper can pass --model", envAIPerfModel)
+ t.Errorf("%s must be set for operator visibility via `kubectl describe pod`", envAIPerfModel)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if env[envAIPerfModel] == "" { | |
| t.Errorf("%s must be set so the wrapper can pass --model", envAIPerfModel) | |
| } | |
| if env[envAIPerfModel] == "" { | |
| t.Errorf("%s must be set for operator visibility via `kubectl describe pod`", envAIPerfModel) | |
| } |
🤖 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/performance/inference_perf_test.go` around lines 981 - 983,
Correct the t.Errorf message in the envAIPerfModel assertion to state that the
variable is required for pod visibility or kubectl describe output, not for the
wrapper to pass --model. Keep the validation condition unchanged.
njhensley
left a comment
There was a problem hiding this comment.
🔁 Re-Review — ✅ Approve
Delta reviewed: 1612e034 → 86fc21f6 (fix commit). Disposition of all 9 prior findings against the resolved code + an adversarial pass on the fix surface. Delta is test + docs only (4 files). Validated at the fix head: go test -race PASS, golangci-lint 0 issues, py_compile OK, .openvex.json valid.
Thanks for the thorough turnaround — the fixes aren't cosmetic: the new +416-line aiperf_entrypoint_test.go executes the real Python wrapper end-to-end with a stub aiperf, and the Dockerfile now byte-compiles the wrapper in the builder and ships the exact file it accepted.
Prior-feedback status
| Finding | Tier | Disposition |
|---|---|---|
| F1 — Python wrapper untested / no compile gate | 🟡 | ✔️ Addressed — py_compile gate in builder + test asserts exit 7/137/127/1/2 on the real wrapper |
| F2 — openvex note "chainsaw bump" factually wrong | 🟡 | ✔️ Addressed — reframed as GHSA-alias dedup; states chainsaw unchanged at v0.2.15 |
| F5 — Dockerfile↔Go path test tautological | 🟡 | ✔️ Addressed — TestAIPerfEntrypointPathsMatchDockerfile reads the real Dockerfile (residual precision gap noted inline as B1) |
| F3 — comment claims newline guard needed to parse | 🔵 | ✔️ Addressed — comment corrected + new glued-sentinel parse case |
| F4 — parse test hand-encodes wrapper output | 🔵 | ✔️ Addressed — TestAIPerfEntrypointFramingFeedsParser feeds real wrapper stdout to parseAIPerfOutput |
| F6 — artifact dir double-sourced, no guard | 🔵 | ✔️ Addressed — argv --output-artifact-dir == env assertion added |
| F7 — "confirmed absent from scan" un-reproducible | 🔵 | ◐ Mitigated — reworded note is now concrete (GHSA-alias mapping); inherent VEX-repro difficulty remains |
F8 — AICR_MODEL dead-but-set naming |
🔵 | ✖️ Not addressed — out of delta (optional nit) |
| F9 — builder floating tag vs digest-pinned runtime | 🔵 | ✖️ Not addressed — optional nit |
Adversarially cleared (no new issue)
- CI gate correct —
requirePython3fatals whenCIis set; GitHub Actions always exportsCI=trueandubuntu-latestships python3, so the Python contract tests genuinely execute in CI and never silent-skip. - No
__pycache__/.pyc shipped — runtimeCOPY --from=buildernames the exact.py; shipped bytes byte-identical. - SIGKILL→137 deterministic; no goroutine/race/timeout fragility; no vacuous assertions.
One new low-severity follow-up nit (B1, inline): the Dockerfile-parser test is stage-blind. Not a blocker.
Verdict
| 🔴 Blocker | 🟠 Major | 🟡 Minor (open) | 🔵 Nitpick (new) | Recommendation |
|---|---|---|---|---|
| 0 | 0 | 0 | 1 (B1) | Approve |
Re-reviewed with a multi-persona + adversarial meta-reviewer workflow, dispositioned against the fix commit at 86fc21f6.
| copiesVenv = true | ||
| case src == "validators/performance/"+aiperfEntrypointSource && dst == aiperfEntrypointScript: | ||
| copiesSource = true | ||
| case src == aiperfEntrypointScript && dst == aiperfEntrypointScript: |
There was a problem hiding this comment.
🔵 Nitpick — Dockerfile-parser test is stage-blind; runtime COPYs aren't required to carry --from=builder
TestAIPerfEntrypointPathsMatchDockerfile flattens all Dockerfile lines ignoring FROM stage boundaries, and the copiesVenv/copiesToRuntime matchers check only src == dst, not the --from=builder flag. A refactor that dropped --from=builder (copying from the build context instead of the builder stage) or misplaced a COPY across stages would still satisfy the guard.
Blast radius: Narrow — the common breakages this test targets (renamed dest, deleted COPY, removed py_compile, moved venv path) are all still caught, and dropping --from=builder would usually break docker build outright, so this is a low-exploitability precision gap rather than a real hole.
Fix: Optional: assert the two runtime COPY lines contain --from=builder, and/or track the current FROM stage while scanning. Follow-up nit, not a merge blocker.
Summary
Move the
aiperf-benchruntime stage frompython:3.13-slimtonvcr.io/nvidia/distroless/python:3.13-v4.0.8(pinned by digest), and replace the/bin/sh -cbenchmark invocation with a shell-free Python entrypoint since the distroless base ships no shell.Motivation / Context
Requested move to the NVIDIA distroless Python base for the AIPerf benchmark image. The swap is not drop-in: the base has no shell, no apt, no pip, and no
useradd, which breaks both the builder stage and the benchmark Job's runtime contract. This PR does the work those constraints require and re-validates the vulnerability posture end to end.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
pkg/validator) —validators/performanceonlyaiperf-benchimage,.openvex.json,.grype.yaml,RELEASING.mdImplementation Notes
Why only the runtime stage moved. The distroless image has no shell, apt, or pip, so it cannot host
RUNsteps. The builder stage stays onpython:3.13-slimand still compiles the venv; only the final stage changed. Both bases put CPython 3.13 at/usr/local/bin, so the venv's interpreter symlinks and compiled extensions (pyzmq, uvloop, crick) resolve unchanged.Replacing
/bin/sh -c.buildAIPerfJobchainedaiperf,echo, andcatin a shell to produce the sentinel framingparseAIPerfOutputconsumes. That framing moved tovalidators/performance/aiperf_entrypoint.py(stdlib only), invoked in exec form. Division of responsibility:aiperf profileand appends the framed result. This keeps flag correctness — including the--modelflag that aiperf 0.11.0 requires — asserted in Go tests rather than in an untested Python file."$AICR_MODEL"shell expansion. This is strictly safer: with no shell there is nothing to re-scan a metacharacter-bearing value. Verified by running the image with model$(touch /tmp/pwned)and confirming the payload arrives verbatim and does not execute.AICR_MODELis still set on the container, now informational, sokubectl describe podshows what a run benchmarked.Non-root user.
RUN useraddcannot run, so the image adopts the base's built-innvsuser (uid 1000) instead of minting uid 10001. Still non-root; verifieduid 1000at runtime.Vulnerability posture. The distroless base ships two Debian packages older than
python:3.13-slim:python:3.13-slim(before)CVE-2026-11940/CVE-2026-11972. The swap does not fix them — both bases ship CPython 3.13.14..openvex.jsonon two independently verified grounds: (1) nothing in the image links it — reading ELFDT_NEEDEDfor every binary under/usr/local/lib,/usr/lib,/lib,/opt/venv/lib,/usr/local/bin,/usr/binreturns zero references to libexpat; (2) CPython routesxml.parsers.expatthrough apyexpatwhoseDT_NEEDEDis['libc.so.6']only, with expat statically bundled at 2.8.1. aiperf itself has zero XML imports across its 682.pyfiles. Suppression is scoped to the unused system library; for the 13 CVEs fixed only in 2.8.2 the impact statement explicitly states the bundled 2.8.1 is not itself patched rather than overclaiming.CVE-2026-34743, Medium) is deliberately left unsuppressed._lzma.sogenuinely links it, so the evidence is weaker than for expat, andpython:3.13-slimalready ships the fixed5.8.1-1+deb13u1. Leaving it visible preserves the signal that the base image is stale. This is the one net-new finding in this PR.Recommended follow-up: file upstream with the NVIDIA distroless team to refresh the Debian layer —
3.13-v4.0.8was built 2026-06-11 and both stale packages have fixes available in Debian trixie.VEX housekeeping. Dropped 11
aicr-gatestatements (GO-2026-5005/5006/5013/5014/5015/5017/5018/5019/5020/5021/5023) confirmed entirely absent from the current scan after a chainsaw bump — verified individually, not inferred from a count delta. Document bumped to version 10 with a refreshed timestamp and tooling note. The remainingaicr-gatestatements still apply (count-neutral).Note for reviewers: two unsuppressed Highs remain on
aicr-gate—GO-2026-5970(x/text) andGO-2026-5942(x/net). Those want a dependency bump, not a VEX entry, and are out of scope here.Testing
make qualify # exit 0make qualifypasses clean on the final tree.golangci-lint run -c .golangci.yaml ./validators/performance/...→ 0 issues.go test -race ./validators/performance/passes; coverage 56.4% (unchanged —validators/is excluded from the project floor per [Bug]: Validator unit tests are excluded from standard CI #1752).linux/amd64andlinux/arm64;aiperf --version→0.11.0in the distroless runtime.aiperf:sentinel / JSON / sentineland parses correctly;parseAIPerfOutput.--only-fixed --vex .openvex.json -c .grype.yamlflags, against an image tagged so the PURL resolves topkg:oci/aicr-aiperf-bench; all 4 new High suppressions confirmed landing underappliedIgnoreRuleswithnamespace = "vex".Risk Assessment
Rollout notes: The benchmark Job's
Command/Argsshape changes, so a releasedaicrbinary must run against a matchingaiperf-benchimage. The validator resolves the inner image from the same tag as the outer validator (resolveAiperfImage/AICR_VALIDATOR_IMAGE_TAG), so they move together in normal use. A pinned olderaiperf-benchimage combined with a new binary would fail at pod start on the missing/opt/aicr/aiperf_entrypoint.py. Revert is a straight rollback of the Dockerfile and Go change.Checklist
make testwith-race)make lint)git commit -S)