Add NIM supprot: Fix NIM auth issue for download nvcr.io images. #254
Conversation
- Add detection for NVIDIA NIM images (nvcr.io/nim/ prefix) - Create NIM-specific container lifecycle management - Configure NIM containers with GPU support, shared memory, and NGC API key - Proxy chat requests to NIM container's OpenAI-compatible API - Add tests for NIM detection and container naming - Support both single prompt and interactive chat modes for NIM - Add comprehensive NIM support documentation to README - Improve user feedback for NIM initialization status - Add GPU detection status messages - Improve timeout error message with troubleshooting tip Signed-off-by: Eric Curtin <eric.curtin@docker.com>
Reviewer's GuideThis PR enhances the NIM image pull command by extracting Docker credentials from Sequence diagram for NIM image pull authentication flowsequenceDiagram
actor User
participant CLI
participant DockerClient
participant DockerConfig
participant Registry
User->>CLI: Initiate NIM image pull
CLI->>DockerConfig: Retrieve auth for nvcr.io
alt Auth found in Docker config
DockerConfig-->>CLI: Return base64(username:password)
CLI->>CLI: Decode and reformat to {username, password} JSON
CLI->>DockerClient: Pull image with RegistryAuth
else No auth found
CLI->>CLI: Check NGC_API_KEY env var
alt NGC_API_KEY present
CLI->>CLI: Create auth with $oauthtoken:NGC_API_KEY
CLI->>DockerClient: Pull image with RegistryAuth
else NGC_API_KEY not present
CLI->>DockerClient: Pull image without auth
end
end
DockerClient->>Registry: Request image pull
Registry-->>DockerClient: Respond (success or error)
DockerClient-->>CLI: Return result
CLI-->>User: Show result
Class diagram for updated NIM image pull authentication logicclassDiagram
class NIMImagePuller {
+pullNIMImage(ctx, dockerClient, model, cmd) error
+nimContainerName(model string) string
+isNIMImage(model string) bool
}
class DockerConfig {
+getDockerAuth(registry string) (string, error)
}
NIMImagePuller --|> DockerConfig : uses
NIMImagePuller o-- DockerClient : dockerClient
NIMImagePuller o-- cmd : cmd
DockerConfig : Auths map[string]{Auth string}
DockerConfig : CredHelpers map[string]string
DockerConfig : config.json
DockerConfig : getDockerAuth(registry string)
NIMImagePuller : pullNIMImage(ctx, dockerClient, model, cmd)
NIMImagePuller : nimContainerName(model string)
NIMImagePuller : isNIMImage(model string)
DockerClient : ImagePull(ctx, model, options)
NIMImagePuller ..> DockerClient : calls ImagePull
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @DC-Shi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical authentication problem encountered when attempting to download NIM images from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The inline debug prints re-read the Docker config and print sensitive registry info; consider consolidating this into getDockerAuth and using a verbose logging flag instead of ad-hoc prints.
- getDockerAuth currently ignores credHelpers, so environments relying on Docker credential helpers will still fail—adding support for them would improve compatibility.
- When looking up entries in config.Auths, normalize registry names (e.g. with or without scheme) to avoid mismatches between how Docker writes registry keys and how you query them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline debug prints re-read the Docker config and print sensitive registry info; consider consolidating this into getDockerAuth and using a verbose logging flag instead of ad-hoc prints.
- getDockerAuth currently ignores credHelpers, so environments relying on Docker credential helpers will still fail—adding support for them would improve compatibility.
- When looking up entries in config.Auths, normalize registry names (e.g. with or without scheme) to avoid mismatches between how Docker writes registry keys and how you query them.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes authentication issues when pulling NIM images from nvcr.io by properly formatting credentials for the Docker API. The key change is transforming Docker config's base64-encoded username:password format into the JSON format required by the Docker client API.
Key changes:
- Added authentication retrieval from Docker config.json for nvcr.io registry
- Implemented fallback authentication using NGC_API_KEY environment variable
- Added debug logging to help troubleshoot authentication issues
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Create basic auth with NGC API key | ||
| // For nvcr.io, username is "$oauthtoken" and password is the NGC API key | ||
| auth := base64.StdEncoding.EncodeToString([]byte("$oauthtoken:" + ngcAPIKey)) | ||
| pullOptions.RegistryAuth = auth |
There was a problem hiding this comment.
The NGC_API_KEY authentication format is inconsistent with the Docker config authentication format. Line 168 uses the old username:password base64 format, but according to the PR description and lines 155-159, the correct format should be a JSON object {\"username\": \"$oauthtoken\", \"password\": ngcAPIKey} that is then base64-encoded.
| // Create basic auth with NGC API key | |
| // For nvcr.io, username is "$oauthtoken" and password is the NGC API key | |
| auth := base64.StdEncoding.EncodeToString([]byte("$oauthtoken:" + ngcAPIKey)) | |
| pullOptions.RegistryAuth = auth | |
| // Create Docker auth JSON with NGC API key | |
| // For nvcr.io, username is "$oauthtoken" and password is the NGC API key | |
| authJSON, _ := json.Marshal(map[string]string{ | |
| "username": "$oauthtoken", | |
| "password": ngcAPIKey, | |
| }) | |
| pullOptions.RegistryAuth = base64.StdEncoding.EncodeToString(authJSON) |
| authJSON, _ := json.Marshal(map[string]string{ | ||
| "username": username, | ||
| "password": password, | ||
| }) |
There was a problem hiding this comment.
The error from json.Marshal is silently ignored. While json.Marshal rarely fails for simple maps, the error should still be handled to maintain code robustness and provide better debugging information if marshaling does fail.
| configFile := filepath.Join(configDir, "config.json") | ||
|
|
||
| // Read Docker config file |
There was a problem hiding this comment.
The Docker config file reading and parsing logic is duplicated between the getDockerAuth function (lines 54-86) and the debug section in pullNIMImage (lines 119-137). Consider removing the duplicate logic in pullNIMImage and using the getDockerAuth function's results for debugging purposes.
| // Debug: Show what we're looking for | ||
| cmd.Printf("Looking for authentication for registry: nvcr.io\n") |
There was a problem hiding this comment.
Corrected spelling of 'supprot' to 'support' in the PR title.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix authentication issues when pulling NIM images from nvcr.io. The changes introduce a helper function to read Docker's config.json and correctly format the authentication token as a JSON payload. My review focuses on the correctness of the authentication logic and code quality.
I've found a few issues:
- The fallback authentication using
NGC_API_KEYdoes not use the correct JSON payload format, which will likely cause it to fail. - There is a significant amount of debug code, some of which is implemented unsafely by ignoring errors and duplicating logic. This should be removed.
Overall, the main change to fix the Docker config authentication is correct, but the related changes need some cleanup and a fix for the fallback mechanism.
| if authStr == "" { | ||
| cmd.Printf("Debug: No stored credentials found for nvcr.io. Checking Docker config...\n") | ||
| configDir := os.Getenv("DOCKER_CONFIG") | ||
| if configDir == "" { | ||
| homeDir, _ := os.UserHomeDir() | ||
| configDir = filepath.Join(homeDir, ".docker") | ||
| } | ||
| configFile := filepath.Join(configDir, "config.json") | ||
|
|
||
| if data, err := os.ReadFile(configFile); err == nil { | ||
| var config struct { | ||
| Auths map[string]interface{} `json:"auths"` | ||
| } | ||
| if json.Unmarshal(data, &config) == nil { | ||
| cmd.Printf("Debug: Found auth entries for: ") | ||
| for registry := range config.Auths { | ||
| cmd.Printf("%s ", registry) | ||
| } | ||
| cmd.Printf("\n") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code appears to be for debugging. It's inefficient because it re-reads and re-parses the Docker config file, duplicating logic from getDockerAuth. More importantly, it's unsafe as it ignores potential errors from os.UserHomeDir() on line 121 and json.Unmarshal() on line 130. This entire block should be removed.
| auth := base64.StdEncoding.EncodeToString([]byte("$oauthtoken:" + ngcAPIKey)) | ||
| pullOptions.RegistryAuth = auth |
There was a problem hiding this comment.
The authentication format for NGC_API_KEY is inconsistent with the fix for Docker config authentication. The PR correctly identifies that nvcr.io requires a base64-encoded JSON payload. However, this fallback logic is still using the older username:password string format. This will likely cause authentication to fail when NGC_API_KEY is used. The payload should be a JSON object with username as "$oauthtoken" and password as the ngcAPIKey.
| auth := base64.StdEncoding.EncodeToString([]byte("$oauthtoken:" + ngcAPIKey)) | |
| pullOptions.RegistryAuth = auth | |
| authJSON, _ := json.Marshal(map[string]string{"username": "$oauthtoken", "password": ngcAPIKey}) | |
| pullOptions.RegistryAuth = base64.StdEncoding.EncodeToString(authJSON) |
|
|
||
| reader, err := dockerClient.ImagePull(ctx, model, image.PullOptions{}) | ||
| // Debug: Show what we're looking for | ||
| cmd.Printf("Looking for authentication for registry: nvcr.io\n") |
There was a problem hiding this comment.
This Printf statement appears to be a debug message. To avoid cluttering the command-line output for users, it's best to remove such messages or make them conditional (e.g., by showing them only when a --verbose flag is used).
|
Thanks for working on this |
|
@DC-Shi you can change: docker:add-nim-support from DC-Shi:fix-nim-image-auth to main from DC-Shi:fix-nim-image-auth if you'd like, might be easier |
Got it, changed to docker:main |
2 participants
Fix the auth issue, the format should be username and password(token) combination:
The docker config.json gives
base64(username:password)so you need manually to make this.Summary by Sourcery
Fix NIM image pull authentication by reading Docker credentials and formatting them correctly, with a fallback to environment-based API key and enhanced diagnostics.
Bug Fixes:
Enhancements: