fix(docker): reduce runtime CVE exposure, fix e2e tests - #1210
Merged
Conversation
mckornfield
commented
Aug 10, 2026
Contributor
|
mckornfield
force-pushed
the
cve-pass-08-10/mck
branch
from
August 10, 2026 21:23
a8f083b to
58c0f52
Compare
mckornfield
commented
Aug 10, 2026
mckornfield
marked this pull request as ready for review
August 11, 2026 04:07
Contributor
|
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 Python 3.13 distroless runtime support, updates ChangesRuntime and E2E container updates
Sequence Diagram(s)sequenceDiagram
participant DockerE2EBackend
participant ClickHouseContainer
participant APIContainer
DockerE2EBackend->>ClickHouseContainer: create and start sidecar
DockerE2EBackend->>ClickHouseContainer: poll health endpoint
DockerE2EBackend->>APIContainer: configure intake with ClickHouse network URL
DockerE2EBackend->>APIContainer: start API container
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nmp_testing/tests/unit/test_e2e_docker_backend.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace quoted
FakeDockerContainerannotations.Add
from __future__ import annotationsand remove the quotes from all listed annotations.🤖 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 `@packages/nmp_testing/tests/unit/test_e2e_docker_backend.py` at line 14, Add from __future__ import annotations at the top of the module, then update the FakeDockerContainer annotations, including the instances declaration, to use unquoted type references throughout the file.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/nmp_testing/tests/unit/test_e2e_docker_backend.py`:
- Line 14: Add from __future__ import annotations at the top of the module, then
update the FakeDockerContainer annotations, including the instances declaration,
to use unquoted type references throughout the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e819b07-43c9-409c-8af7-f1dd63ea819e
📒 Files selected for processing (8)
docker-bake.hcldocker/Dockerfile.auditor-tasksdocker/Dockerfile.nmp-apidocker/Dockerfile.nmp-unsloth-trainingdocker/Dockerfile.safe-synthesizer-taskse2e/k8s/values/kind.yamlpackages/nmp_testing/src/nmp/testing/e2e/docker.pypackages/nmp_testing/tests/unit/test_e2e_docker_backend.py
ironcommit
approved these changes
Aug 11, 2026
Contributor
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 `@docker-bake.hcl`:
- Around line 39-41: Update the NMP_API_RUNTIME_BASE variable default from
root-distroless-base-3-13 to nmp-python-base so nmp-api-docker uses the
shell-compatible base by default, while preserving support for explicitly
selecting the distroless base.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 6370d5a8-3c31-4e79-b13d-faf3ff79c1a0
📒 Files selected for processing (4)
docker-bake.hcldocker/Dockerfile.bakedocker/Dockerfile.nmp-apidocker/Dockerfile.nmp-core
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/Dockerfile.nmp-api
mckornfield
changed the title
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
mckornfield
force-pushed
the
cve-pass-08-10/mck
branch
from
August 11, 2026 22:46
5e0d9b7 to
8284c0f
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
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 `@packages/nmp_testing/src/nmp/testing/e2e/docker.py`:
- Around line 331-337: Update the existing ClickHouse cleanup block around
self.clickhouse_container.stop() to invoke the container’s force-removal
fallback when graceful stopping raises, using the same removal behavior as
stop(). Only clear self.clickhouse_container after the fallback attempt,
preserving cleanup of the attached sidecar before subsequent network removal.
- Around line 277-303: Update ClickHouse startup around
clickhouse_container.with_exposed_ports and _wait_for_clickhouse_healthy so the
exposed host port is bound only to loopback, or replace the host-based health
probe with an internal-network probe using the ClickHouse network alias.
Preserve the existing health-check behavior while preventing unauthenticated
ClickHouse access from all host interfaces.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 408d01a4-5b20-429b-bc8c-94ab3a331bec
📒 Files selected for processing (10)
docker-bake.hcldocker/Dockerfile.auditor-tasksdocker/Dockerfile.bakedocker/Dockerfile.nmp-apidocker/Dockerfile.nmp-coredocker/Dockerfile.nmp-unsloth-trainingdocker/Dockerfile.safe-synthesizer-taskse2e/k8s/values/kind.yamlpackages/nmp_testing/src/nmp/testing/e2e/docker.pypackages/nmp_testing/tests/unit/test_e2e_docker_backend.py
🚧 Files skipped from review as they are similar to previous changes (9)
- docker/Dockerfile.safe-synthesizer-tasks
- e2e/k8s/values/kind.yaml
- docker/Dockerfile.nmp-api
- docker/Dockerfile.auditor-tasks
- docker/Dockerfile.nmp-unsloth-training
- docker/Dockerfile.nmp-core
- docker-bake.hcl
- docker/Dockerfile.bake
- packages/nmp_testing/tests/unit/test_e2e_docker_backend.py
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Contributor
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 `@packages/nmp_testing/src/nmp/testing/utils.py`:
- Around line 42-49: Update the local CLI resolution in
packages/nmp_testing/src/nmp/testing/utils.py:42-49 to select a candidate only
when it is a regular executable file, while preserving the repository-boundary
validation. In packages/nmp_testing/tests/unit/test_utils.py:52-72, mark the
valid fixture executable and add coverage verifying that a non-executable
candidate is skipped in favor of the fallback.
In `@packages/nmp_testing/tests/unit/test_utils.py`:
- Around line 74-90: Update
test_falls_back_to_uv_when_not_running_from_repo_virtualenv to set VIRTUAL_ENV
to an external virtualenv path containing the nemo executable instead of
deleting the variable, so run_nemo_local exercises the relative_to rejection
branch. Keep the existing uv command assertion unchanged.
🪄 Autofix
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: CHILL
Plan: Enterprise
Run ID: 2f16739c-6081-4d47-ad3c-dd15cf615014
📒 Files selected for processing (2)
packages/nmp_testing/src/nmp/testing/utils.pypackages/nmp_testing/tests/unit/test_utils.py
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This reduces non-Automodel CVE exposure in Docker runtime images while keeping
nmp-apion the shell-compatible Python base by default. Platform-Deploy E2E showed the distroless API runtime breaks the local subprocess job backend because commands such asechoandshare not present;NMP_API_RUNTIME_BASEremains available for explicit distroless experiments.Changes
nmp-apifinal runtime base asnmp-python-baseso Docker/local subprocess jobs retain shell/coreutils compatibility.NMP_API_RUNTIME_BASEas a bake override for testing alternate API runtime bases, including NVIDIA distroless Python.nmp-api; Platform-Deploy E2E showed non-root UID 1000 cannot access existing root-owned/data/files_storagemounts.aiohttpruntime remediation floors to>=3.14.3,<4for Auditor, Safe Synthesizer, and Unsloth images.GitPythonandjupyterlabcopies from the Unsloth image.CVE_SUMMARY.mdfile.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run --frozen pytest packages/nmp_testing/tests/unit/test_e2e_docker_backend.py -v- passed.docker buildx bake --print nmp-api-docker | rg -n 'NMP_API_RUNTIME_BASE|nmp-python-base|distroless|dockerfile|target'- passed; confirmedNMP_API_RUNTIME_BASE=nmp-python-base.git diff --check- passed.git diff --check origin/main...HEAD && git diff --check && git diff --cached --check- passed earlier in this PR.docker buildx bake --print nmp-api-docker- passed bake parse after the runtime UID compatibility fix.docker buildx bake --print nmp-api-docker auditor-tasks-docker safe-synthesizer-tasks-docker nmp-unsloth-training- passed bake parse before the follow-up UID/runtime-default fixes.make docker-print TARGET=docker-cpu- passed bake parse.make docker-print TARGET=docker-gpu- passed bake parse.make docker-print TARGET=docker-auditor- passed bake parse.make docker-print TARGET=docker-python-base- passed bake parse.make docker-print TARGET=nmp-unsloth- passed bake parse.make docker-print TARGET=nmp-automodel- passed bake parse.docker run --rm --platform linux/amd64 --entrypoint python nvcr.io/nvidia/distroless/python:3.13-v4.0.9 -c '...'- confirmed Python 3.13.14, UID/GID 1000 by default,/home/nvs, and writable/tmp.docker run --rm --user 1000:1000 -v <root-owned-dir>:/data --entrypoint python nvcr.io/nvidia/distroless/python:3.13-v4.0.9 -c 'pathlib.Path("/data/files_storage").stat()'- reproduced the Platform-DeployPermissionError; the same command with--user 0:0passed.31440874724against58c0f52eshowednmp-apidistroless failures in Docker E2E: subprocess jobs could not startechoorsh.uv run pre-commit run -a- blocked before hooks ran because fetchinghttps://github.com/norwoodj/helm-docs/returned HTTP 503 on repeated attempts.BUILD_ARCH=linux/amd64 docker buildx bake --load nmp-api-docker- attempted before this push and blocked before project layers because this environment cannot pullghcr.io/astral-sh/uv:0.9.14(failed to fetch oauth token: denied).Follow-up validation in progress:
scheduled-docker-e2e-alerts/mckagainst this PR head SHAb8342921051193ef1c15da504f697e851d7b942f: https://github.com/NVIDIA-NeMo/Platform-Deploy/actions/runs/31444563128Summary by CodeRabbit
New Features
Bug Fixes
aiohttprequirements to supported 3.14.3-or-newer versions across services.Tests