Skip to content

Fixes CI base image pulls when NGC refuses the inherited credential - #7332

Merged
kellyguo11 merged 3 commits into
isaac-sim:release/3.0.0-beta2from
myurasov-nv:myurasov/nvcr-anonymous-fallback-beta2
Aug 26, 2026
Merged

kellyguo11 merged 3 commits into
isaac-sim:release/3.0.0-beta2from
myurasov-nv:myurasov/nvcr-anonymous-fallback-beta2

Conversation

@myurasov-nv

@myurasov-nv myurasov-nv commented Aug 25, 2026 •

Copy link
Copy Markdown
Member

Currently Build Base Docker Image and Build cuRobo Docker Image fail with denied: Access Denied while resolving the Isaac Sim base image from public ("nvidia") NGC org.

Cause. ecr-build-push-pull copies the runner's persistent nvcr.io credentials for a private NGC org into the job. NGC declines that identity a pull token for the public nvidia/* catalog, so an image that needs no credentials becomes unpullable.

Fix. The setup step now confirms those credentials can read the configured base image, and retries anonymously when they cannot. The deps-hash step fails loudly on an unreadable digest, which until now corrupted the cache key in silence. docker-build receives the same fallback, and its login moves to --password-stdin.

Why this branch. Only release/3.0.0-beta2 pins a public base image, so the bug lives here alone. develop keeps this logic in .github/actions/_lib/, a path this branch does not have, so a fix there could never run here.

Before After
Base image pull uses only the runner's inherited private-org credentials, which NGC refuses for the public catalog, so the job fails with denied: Access Denied Credentials are tried first; if the registry refuses them the pull retries anonymously, and the public base image resolves

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Release backport

  • Backport this pull request to the active release branch after it merges into develop (not applicable: this targets a release branch directly, and the equivalent code on develop is already correct)

Screenshots

Not applicable; the change affects CI credential handling only.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format (not run locally, no dev setup on this machine; this PR's own pre-commit check passes)
  • I have made corresponding changes to the documentation (no documented interface changes)
  • My changes generate no new warnings (not assessable for a composite action)
  • I have added tests that prove my fix is effective or that my feature works (no harness executes composite actions; the CI run on this PR is the check)
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that) (no package under source/ touched)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@myurasov-nv

myurasov-nv commented Aug 25, 2026 •

Copy link
Copy Markdown
Member Author

Default branch. PRs opened with the default base land here, which is how the failure arrived.

Credential kept. Removing it would break fork PRs on develop, where the base image is private and the inherited credential is the only one. Credential-first, so it cannot regress a working branch.

Verified. Two GPU runners failed identically here, ruling out host state. YAML parsed; changed shell passed bash -n.

Live test. This PR is from a fork, so NGC_API_KEY is empty and its own base-image build exercises the fallback.

Untouched. docker-build/action.yml shares the pattern but has no callers here.

@myurasov-nv
myurasov-nv marked this pull request as ready for review August 25, 2026 04:40
@myurasov-nv
myurasov-nv requested a review from a team August 25, 2026 04:40
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes the composite Docker action retry public NGC base-image access anonymously when inherited credentials are refused, improves registry-login secret handling, and fails early when the base-image digest cannot be resolved.

  • Verifies the configured base image and switches to an empty Docker credential configuration only after anonymous access succeeds.
  • Retries manifest digest resolution three times and includes the digest in the dependency-cache identity.
  • Leaves a non-blocking robustness issue where successful stderr diagnostics can contaminate the digest-derived cache key.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking cache-key robustness issue if buildx emits diagnostics during a successful manifest inspection.

The credential fallback is constrained to a base image proven readable anonymously, and later ECR authentication is restored explicitly; only the unvalidated combined stdout/stderr digest capture needs attention.

Files Needing Attention: .github/actions/ecr-build-push-pull/action.yml

Important Files Changed

Filename Overview
.github/actions/ecr-build-push-pull/action.yml Adds credential fallback and fail-closed digest resolution; stderr should remain separate from the digest used in the cache key.

Reviews (1): Last reviewed commit: "Fall back to anonymous pulls when NGC re..." | Re-trigger Greptile

Comment thread .github/actions/ecr-build-push-pull/action.yml Outdated
@myurasov-nv
myurasov-nv force-pushed the myurasov/nvcr-anonymous-fallback-beta2 branch from 4e6b6db to ded7a06 Compare August 25, 2026 04:44

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

The action correctly probes inherited nvcr.io credentials and falls back to anonymous access when needed, while making an unreadable base-image digest fatal rather than weakening the dependency-cache identity. One digest-capture issue should be fixed before merge: stderr is currently incorporated into cache-key data.

  • Design and architecture: Credential selection remains contained in the setup step, and exporting the final DOCKER_CONFIG after probing preserves its use by later ECR and build steps. Requiring a resolved manifest digest is consistent with maintaining a reliable dependency-cache identity.
  • API: No action inputs or outputs were renamed or removed. Operational behavior intentionally changes by allowing anonymous base-image access after credential refusal and by failing when the base-image digest cannot be resolved.
  • Implementation: The credential probe, anonymous fallback, and temporary-directory handling are coherent. However, the digest command redirects stderr into INSPECT_OUT; on a successful command that emits a warning or diagnostic, this text is concatenated with stdout and then used in BASE_IMAGE_UNIQ_ID and DEPS_HASH. Capture failure diagnostics separately while deriving the digest from stdout only.

Minor fixes needed. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

Comment thread .github/actions/ecr-build-push-pull/action.yml Outdated
@myurasov-nv

Copy link
Copy Markdown
Member Author

Live proof on this PR. Build Base Docker Image is green on runner ip-10-0-2-166-gpu, and its log shows the new path doing exactly what it was written for:

Secret source: None
🟠 NGC_API_KEY not set - skipping nvcr.io login (normal for fork PRs)
🟠 Inherited credentials cannot read nvcr.io/nvidia/isaac-sim:6.0.1; using anonymous access
🔵 Base image ID: nvcr.io/nvidia/isaac-sim:6.0.1:sha256:783444c706538aa76cf5126e911ddc5e618779e6105305ad4af4260362a30aa9
🟠 Deps cache MISS 😿😿😿 (90e6b555c18579b9). Will build now. 🐢🐢🐢

The third line is the bug and the fix in one place. The runner's inherited credentials genuinely cannot read the public base image, which is the denied: Access Denied this PR exists to remove, and the anonymous retry recovers from it. Before this change that same condition ended the job.

Two details worth noting. The run hit a deps-cache miss, so it resolved the base image and completed a full build rather than short-circuiting on a cached layer — the strongest form of the test available here. And the Base image ID line carries a clean sha256: with no diagnostic text and no retry warnings, which confirms the stderr fix from the review above behaves correctly on the success path.

@myurasov-nv

Copy link
Copy Markdown
Member Author

Still failing on this branch. #7342, a backport opened against release/3.0.0-beta2 yesterday at 13:14Z, failed Build Base Docker Image and Build cuRobo Docker Image for the reason this PR fixes, with nothing to do with its own contents:

🔵 Base image ID: nvcr.io/nvidia/isaac-sim:6.0.1
#4 ERROR: failed to authorize: failed to fetch oauth token: denied: Access Denied
ERROR: failed to build: failed to solve: failed to fetch oauth token: denied: Access Denied

That log also shows the second defect this PR closes: the Base image ID line carries no sha256:, so the digest read failed silently and the deps-cache key was built from the bare tag.

Ready for a decision. 33 checks pass on bd660e3, both bot findings are addressed with no open threads, and .github/actions on the base branch has not changed since June 28, so there is no conflict to resolve.

Review. @kellyguo11 you own /.github/ on this branch, and the other listed codeowner no longer has access to the repository, so approval routes to you. Every PR opened against the default base keeps hitting this until the fix lands.

@kellyguo11
kellyguo11 merged commit b25387f into isaac-sim:release/3.0.0-beta2 Aug 26, 2026
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants