feat(docker): add UBI10 container image for FIPS compliance#1499
Conversation
CI Test Summary1 failed · 12 passed · 2 skipped
|
|
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:
📝 WalkthroughWalkthroughThis change adds a UBI10/RHEL10 (FIPS 140-3) container image variant for cuOpt alongside the existing Ubuntu image. It introduces a new Dockerfile, build/push CI workflow steps, multi-arch manifest generation, test workflow/script updates, README documentation, and install-selector UI support for the new variant. ChangesUBI10 image support
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (3)
ci/docker/Dockerfile.ubi10 (1)
1-79: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing
USERdirective — image runs as root by default.Trivy flags this (DS-0002). The
chmod 777 /opt/cuoptandchmod -R 755on the package/bin directories (lines 59-64) suggest an intent to support running as an arbitrary non-root UID, but without aUSERinstruction the default runtime UID is still root. For a FIPS-hardened image this is the exact kind of gap worth closing.Note:
test_image.shcurrently relies on the container running as root touseradda test user viasu -, so switching the defaultUSERwould need to account for that test flow (e.g., only affects defaultdocker run, tests could still run as root in CI, or add--user 0for test invocation).🔒 Example fix
WORKDIR /opt/cuopt COPY ./LICENSE ./VERSION ./THIRD_PARTY_LICENSES ./COMMIT_SHA ./COMMIT_TIME /opt/cuopt/ @@ COPY ./entrypoint.sh /opt/cuopt/entrypoint.sh +USER 1001 ENTRYPOINT ["/opt/cuopt/entrypoint.sh"] CMD ["python", "-m", "cuopt_server.cuopt_service"]🤖 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 `@ci/docker/Dockerfile.ubi10` around lines 1 - 79, The image currently defaults to root because ci/docker/Dockerfile.ubi10 has no USER directive, so the runtime security intent in the chmod setup is not actually enforced. Add an explicit non-root USER in the final cuopt-final stage after permissions are set, using a dedicated runtime account or an arbitrary UID-compatible user, and make sure entrypoint.sh and CMD still work under that user. Keep test_image.sh in mind: if it depends on root for useradd/su - flows, preserve CI compatibility by overriding the user for tests or adjusting the test invocation rather than leaving the production image as root.Source: Linters/SAST tools
ci/docker/test_image.sh (2)
58-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
useradd ... || truemasks real failures.If
useraddfails for a reason other than "already exists" (e.g., UID collision with a reserved system account on UBI10), the script silently proceeds andsu - tempuser1001fails downstream with a "no such user" error, obscuring the actual root cause during CI triage.Consider narrowing the suppression to the expected case, e.g. checking
id -u tempuser1001 &>/dev/null || useradd -m -u 1001 -s /bin/bash tempuser1001, so unexpecteduseraddfailures aren't swallowed.🤖 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 `@ci/docker/test_image.sh` around lines 58 - 62, The temp user setup in `test_image.sh` is swallowing real `useradd` failures with `|| true`, which hides UID collision or permission issues. Update the `useradd`/`su - tempuser1001` flow so the script only skips creation when `tempuser1001` already exists, using the existing user check before `useradd`, and let unexpected failures surface instead of being masked.
8-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
python3.12/cu13inEXTRA_LD_PATHis version-coupled to the UBI10 image build.This path assumes the UBI10 image is always built with Python 3.12 and CUDA 13 pip wheels (confirmed: CUDA Toolkit 13.0+ wheels do consolidate under
site-packages/nvidia/cu13/, per NVIDIA's blog post noting "site-packages/nvidia/cu13/include will contain headers for both cuBLAS and cuFFT (if installed)"). If the UBI10 Dockerfile's Python version or CUDA major version changes in the future, this path silently becomes wrong (glob won't exist), andLD_LIBRARY_PATHrestoration will quietly do nothing rather than fail loudly — aset -e-invisible correctness gap.Consider deriving this dynamically instead, e.g. via
python3 -c "import nvidia, os; print(os.path.dirname(nvidia.__file__))"combined with a directory glob forcu*/lib, so the test script doesn't need updating whenever the Dockerfile's Python/CUDA pins change.🤖 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 `@ci/docker/test_image.sh` around lines 8 - 19, The UBI/RHEL branch in test_image.sh hardcodes a Python/CUDA-specific EXTRA_LD_PATH that is tied to one build configuration. Update the logic around the distro detection block to derive the NVIDIA site-packages location dynamically instead of embedding python3.12/cu13, using the existing shell setup to discover the installed nvidia package path and a cu*/lib directory. Keep the same EXTRA_LD_PATH variable assignment behavior, but make it resilient to future Python or CUDA version changes so the LD_LIBRARY_PATH restoration in the script remains correct.
🤖 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 `@ci/docker/Dockerfile.ubi10`:
- Around line 1-79: The image currently defaults to root because
ci/docker/Dockerfile.ubi10 has no USER directive, so the runtime security intent
in the chmod setup is not actually enforced. Add an explicit non-root USER in
the final cuopt-final stage after permissions are set, using a dedicated runtime
account or an arbitrary UID-compatible user, and make sure entrypoint.sh and CMD
still work under that user. Keep test_image.sh in mind: if it depends on root
for useradd/su - flows, preserve CI compatibility by overriding the user for
tests or adjusting the test invocation rather than leaving the production image
as root.
In `@ci/docker/test_image.sh`:
- Around line 58-62: The temp user setup in `test_image.sh` is swallowing real
`useradd` failures with `|| true`, which hides UID collision or permission
issues. Update the `useradd`/`su - tempuser1001` flow so the script only skips
creation when `tempuser1001` already exists, using the existing user check
before `useradd`, and let unexpected failures surface instead of being masked.
- Around line 8-19: The UBI/RHEL branch in test_image.sh hardcodes a
Python/CUDA-specific EXTRA_LD_PATH that is tied to one build configuration.
Update the logic around the distro detection block to derive the NVIDIA
site-packages location dynamically instead of embedding python3.12/cu13, using
the existing shell setup to discover the installed nvidia package path and a
cu*/lib directory. Keep the same EXTRA_LD_PATH variable assignment behavior, but
make it resilient to future Python or CUDA version changes so the
LD_LIBRARY_PATH restoration in the script remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82068228-5202-4866-abd9-9a8b5eb00f29
📒 Files selected for processing (5)
.github/workflows/build_images.yaml.github/workflows/test_images.yamlci/docker/Dockerfile.ubi10ci/docker/create_multiarch_manifest.shci/docker/test_image.sh
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/cuopt/source/cuopt-server/quick-start.rst`:
- Around line 47-51: The quick-start run example only shows the Ubuntu-based
cuopt image, so update the example in the quick-start docs to also include the
UBI10 variant or use a tag placeholder. Keep the change aligned with the
existing container launch instructions around the docker run example so both
image variants are discoverable and the documentation stays consistent.
🪄 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: CHILL
Plan: Enterprise
Run ID: 64fad6c0-6292-4abe-bcbe-d0f18464611a
📒 Files selected for processing (3)
README.mdci/docker/README.mddocs/cuopt/source/cuopt-server/quick-start.rst
✅ Files skipped from review due to trivial changes (1)
- README.md
| To run the container: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| docker run --gpus all -it --rm -p 8000:8000 -e CUOPT_SERVER_PORT=8000 nvidia/cuopt:latest-cu13 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the UBI10 docker run example here.
The section documents both image variants, but the run example still hardcodes the Ubuntu tag. Showing the equivalent UBI10 invocation (or making the tag a placeholder) would keep the new variant discoverable. As per path instructions, documentation changes should focus on completeness and consistency.
🤖 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 `@docs/cuopt/source/cuopt-server/quick-start.rst` around lines 47 - 51, The
quick-start run example only shows the Ubuntu-based cuopt image, so update the
example in the quick-start docs to also include the UBI10 variant or use a tag
placeholder. Keep the change aligned with the existing container launch
instructions around the docker run example so both image variants are
discoverable and the documentation stays consistent.
Source: Path instructions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cuopt/source/_static/install-selector.js (1)
296-302: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSilent fallback could serve a non-FIPS image when a UBI10 key is missing.
If
data[release][cudaKey](e.g."cu12-ubi10") isn't present for some interface/release combination, this silently falls back todata[release].cu12(the Ubuntu image) with no indication to the user. Since the whole point of the Variant selector is to guarantee a FIPS 140-3 compliant image, silently substituting a non-FIPS image on a lookup miss is a meaningful correctness/compliance risk if any interface or release is later added without full UBI10 coverage.Consider falling back only within the same variant family, or surfacing an explicit "unsupported" state instead of quietly downgrading to Ubuntu:
🛡️ Proposed fix to avoid a silent cross-variant fallback
var variant = getSelectedValue("cuopt-variant") || "ubuntu"; var cudaKey = (cuda || "cu12") + (variant === "ubi10" ? "-ubi10" : ""); - var c = data[release][cudaKey] || data[release].cu12; + var c = data[release][cudaKey]; + if (!c) { + // Avoid silently downgrading a UBI10/FIPS selection to Ubuntu. + c = data[release][(cuda || "cu12")] || data[release].cu12; + }🤖 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 `@docs/cuopt/source/_static/install-selector.js` around lines 296 - 302, The container image selection in install-selector.js silently falls back from the UBI10 key lookup to the Ubuntu cu12 image, which can hide missing FIPS coverage. Update the selection logic in the container branch of the release command builder so that a missing variant-specific key (for example via getSelectedValue("cuopt-variant") and the derived cudaKey) does not cross-fallback to a non-UBI image. Instead, keep the fallback within the same variant family or return an explicit unsupported/missing-state result from the data[release][cudaKey] lookup path.
🧹 Nitpick comments (1)
docs/cuopt/source/_static/install-selector.js (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGrowing duplication across the four ubi10 command blocks.
The
cu12-ubi10/cu13-ubi10entries are hand-duplicated across shared and server, stable and nightly maps. This mirrors existing structure, but the four-way duplication increases the chance one combination is missed in a future edit — which would trigger the silent fallback noted above. Consider a small helper that builds the{default, run}pair for a given tag suffix to reduce repetition.Also applies to: 68-75, 233-240, 251-258
🤖 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 `@docs/cuopt/source/_static/install-selector.js` around lines 50 - 57, The ubi10 command objects in install-selector.js are duplicated across the shared/server and stable/nightly maps, which makes future edits easy to miss. Refactor the repeated cu12-ubi10 and cu13-ubi10 `{default, run}` entries into a small helper that builds the pair from a tag suffix, then reuse it in each map entry so all four blocks stay consistent. Use the existing map keys and the duplicated command strings as the call sites to update.
🤖 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.
Outside diff comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 296-302: The container image selection in install-selector.js
silently falls back from the UBI10 key lookup to the Ubuntu cu12 image, which
can hide missing FIPS coverage. Update the selection logic in the container
branch of the release command builder so that a missing variant-specific key
(for example via getSelectedValue("cuopt-variant") and the derived cudaKey) does
not cross-fallback to a non-UBI image. Instead, keep the fallback within the
same variant family or return an explicit unsupported/missing-state result from
the data[release][cudaKey] lookup path.
---
Nitpick comments:
In `@docs/cuopt/source/_static/install-selector.js`:
- Around line 50-57: The ubi10 command objects in install-selector.js are
duplicated across the shared/server and stable/nightly maps, which makes future
edits easy to miss. Refactor the repeated cu12-ubi10 and cu13-ubi10 `{default,
run}` entries into a small helper that builds the pair from a tag suffix, then
reuse it in each map entry so all four blocks stay consistent. Use the existing
map keys and the duplicated command strings as the call sites to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f9a1ba80-bb48-4f5d-88fa-eccd12c6bc71
📒 Files selected for processing (1)
docs/cuopt/source/_static/install-selector.js
ramakrishnap-nv
added
non-breaking
…nments - New Dockerfile.ubi using the same 3-stage pip-based build as the Ubuntu image - Unified test_image.sh with OS detection (dnf vs apt-get) covering both images - UBI image tags use -ubi10 suffix (e.g. latest-cu13-ubi10) - CI workflows updated for UBI build, test, and multiarch manifest - Docs: README and install selector updated with UBI variant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ramakrishnap-nv
force-pushed
the
feat/ubi10-container-image
branch
from
295435e to
6360452
Compare
2 participants
Add a UBI (Red Hat Universal Base Image) container variant for FIPS 140-3 compliant environments.
Dockerfile.ubiusing the same 3-stage pip-based build as the Ubuntu imagetest_image.shcovering both Ubuntu and UBI images-ubi10suffix (e.g.latest-cu13-ubi10)