fix(preflight): validate pull access to rendered NICo image refs#3681
Open
shayan1995 wants to merge 1 commit into
Open
fix(preflight): validate pull access to rendered NICo image refs#3681shayan1995 wants to merge 1 commit into
shayan1995 wants to merge 1 commit into
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughPreflight check ChangesRegistry Image Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Preflight as preflight.sh
participant Curl as curl
participant Registry
participant TokenEndpoint
Preflight->>Curl: HEAD manifest request
Curl->>Registry: Probe rendered image manifest
Registry-->>Curl: Status or WWW-Authenticate challenge
Preflight->>TokenEndpoint: Request Bearer token
TokenEndpoint-->>Preflight: Return token
Preflight->>Curl: Retry manifest request with token
Curl->>Registry: Authenticated manifest probe
Registry-->>Preflight: Final status
🚥 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}
Contributor
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 `@helm-prereqs/preflight.sh`:
- Around line 286-304: Update the token-response handling in the registry
preflight flow before the `_curl_registry_manifest` retry: explicitly classify
successful 2xx responses with a usable token, 401/403 credential responses, 5xx
transport/service failures, other non-2xx statuses, malformed JSON, and
empty-token responses. Only retry the manifest with `_token` when the token
response is valid; otherwise record the appropriate token-endpoint issue instead
of reusing the manifest’s original 401.
- Around line 322-324: Update the HTTP 404 handling in the preflight image-check
logic to remain non-blocking when REGISTRY_PULL_SECRET is not configured,
emitting the existing warning behavior instead of appending to ERRORS. Preserve
the hard-error behavior for 404 responses when a pull secret is configured.
- Around line 899-902: Update the curl assignment in the registry probe to avoid
appending a second fallback value when curl already outputs HTTP status 000 on
transport failure. Preserve a single 000 result so the existing conditional
warning path is reached.
- Around line 163-175: Update the registry probe flow around
_record_registry_transport_issue and its callers to apply REGISTRY_PULL_SECRET
only when the image reference’s registry host matches the configured target
registry; otherwise probe without that secret or require explicit credentials
for the foreign host. Ensure auth failures and warning/error messages remain
scoped to the specific registry host, including all referenced probe paths.
🪄 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: cfa84eb5-12f0-4c18-9639-225c79a9395b
📒 Files selected for processing (1)
helm-prereqs/preflight.sh
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 `@helm-prereqs/README.md`:
- Around line 200-201: Update the secret-handling documentation near the
NICO_IMAGE_REGISTRY probing behavior to state that REGISTRY_PULL_SECRET is sent
only to the configured registry and its trusted WWW-Authenticate Bearer token
service, which may be hosted separately. Remove the stronger host-only wording,
without changing implementation behavior.
- Around line 201-203: Update the image-access behavior description in the
README to limit warning-only findings without a secret to authentication, 404,
server, and transport failures. Explicitly preserve hard errors for malformed
image references, unexpected HTTP responses, and missing curl when validation is
required, while retaining the unreachable-registry 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: CHILL
Plan: Enterprise
Run ID: 0f49bd49-a806-4004-a87b-afb1a27b7c72
📒 Files selected for processing (2)
helm-prereqs/README.mdhelm-prereqs/preflight.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- helm-prereqs/preflight.sh
Contributor
Author
|
/ok to test 079c679 |
preflight.sh only probed https://<registry>/v2/ for reachability, so it passed even when the supplied credentials could not pull the exact NICo Core image setup.sh renders. Setup then installed the full prereq stack before failing an hour in with nico-api-migrate stuck in ImagePullBackOff (NVIDIA#3367). Preflight now checks manifest access for the exact refs setup.sh will deploy, using the Docker Registry v2 API with the same conventions as setup.sh (${NICO_IMAGE_REGISTRY}/nvmetal-carbide:${NICO_CORE_IMAGE_TAG}, $oauthtoken + REGISTRY_PULL_SECRET), plus any literal registry-qualified image refs found in the core values file: - HEAD /v2/<repo>/manifests/<tag-or-digest>, following the WWW-Authenticate bearer-token flow. HEAD does not count against registry pull rate limits. Token-endpoint responses are classified (2xx without a usable token, 401/403, other) so a broken token service is not misreported as bad credentials. - REGISTRY_PULL_SECRET is sent only to the NICO_IMAGE_REGISTRY host and the Bearer token endpoint it advertises; refs on other hosts are probed anonymously so the secret never leaks to unrelated registries and no-auth registries simply answer 200. - Failures are distinguished as unauthorized/forbidden, tag not found, registry 5xx, and DNS/TLS/connect transport errors. They are hard errors only when credentials apply to the host; otherwise warnings, since images may be public, preloaded, or covered by existing imagePullSecrets. - If the registry host itself is unreachable, the existing warning is kept and per-image checks are skipped so air-gapped/preloaded installs are not blocked. Also fixes the pre-existing host probe: curl already prints 000 on transport failure, so '|| echo 000' corrupted the value and the unreachable path never triggered. - Docker Hub shorthand refs (org/image:tag) in values files are skipped since they do not map to a probeable /v2 endpoint; digest-pinned refs are supported. - Documented in the script header and helm-prereqs/README.md. Verified against live registries (anonymous bearer flow, 404, 401 with bad credentials, digest refs, DNS failure, values-file collector filter) and end-to-end on a fresh 3-node cluster where preflight validated the exact rendered core image with real credentials. Fixes NVIDIA#3367 Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
shayan1995
force-pushed
the
snamaghi/issue-3367-preflight-core-image-access
branch
from
July 17, 2026 20:52
079c679 to
d04258c
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
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
1 participant
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.
helm-prereqs/preflight.shonly probedhttps://<registry>/v2/for reachability, so preflight passed even when the supplied credentials could not pull the exact NICo Core imagesetup.shrenders. Setup then installed the full prerequisite stack before failing much later in [6/6] with thenico-api-migratepre-install job stuck inImagePullBackOff(#3367).Preflight now checks manifest access for the exact refs setup.sh will deploy, using the Docker Registry v2 API with the same conventions as setup.sh (
${NICO_IMAGE_REGISTRY}/nvmetal-carbide:${NICO_CORE_IMAGE_TAG},$oauthtoken+REGISTRY_PULL_SECRET), plus any literal registry-qualified image refs found in the core values file (unbound image/exporter, etc.):HEAD /v2/<repo>/manifests/<tag-or-digest>, following theWWW-Authenticatebearer-token flow (authenticated whenREGISTRY_PULL_SECRETis set, anonymous otherwise). HEAD does not count against registry pull rate limits.REGISTRY_PULL_SECRETis set; without a secret they stay warnings, since images may be public, preloaded, or covered by existingimagePullSecrets.org/image:tag) in values files are skipped since they do not map to a probeable/v2endpoint; digest-pinned refs are supported.Related issues
Fixes #3367
Type of Change
Breaking Changes
Testing
bash -nclean. Verified against live registries: public image passes via the anonymous bearer flow (ghcr.io, registry-1.docker.io), missing tag returns a 404 hard error, bad credentials on nvcr.io return a 401 hard error, DNS failure is a warning without a secret and an error with one, digest-pinned refs resolve,registry:5000/repo(port but no tag) is rejected, and the values-file collector keeps registry-hosted refs while dropping Hub shorthand, bare names, and comments.