feat(machine-validation): add container registry credential support#3867
Merged
ianderson-nvidia merged 1 commit intoJul 23, 2026
Merged
Conversation
Contributor
|
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 ignored due to path filters (2)
📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (9)
Summary by CodeRabbit
WalkthroughAdds container registry credential storage, Forge RPC endpoints, RBAC permissions, an admin CLI command, and machine-validation support for authenticated ChangesContainer registry credentials
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant ApiClient
participant ForgeAPI
participant CredentialManager
participant MachineValidation
participant nerdctl
AdminCLI->>ApiClient: Set registry credentials
ApiClient->>ForgeAPI: SetContainerRegistryCredential
ForgeAPI->>CredentialManager: Store username/password
MachineValidation->>ForgeAPI: GetContainerRegistryCredential
ForgeAPI->>CredentialManager: Read registry credentials
CredentialManager-->>ForgeAPI: Credentials or not found
ForgeAPI-->>MachineValidation: Credential response
MachineValidation->>nerdctl: Login with password on stdin
MachineValidation->>nerdctl: Pull container image
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/rpc/build.rsast-grep timed out on this file Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-22 21:05:02 UTC | Commit: 080edcb |
ianderson-nvidia
force-pushed
the
encrypt_container_registry_auth
branch
3 times, most recently
from
July 22, 2026 21:11
fac8d71 to
b342a22
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3867.docs.buildwithfern.com/infra-controller |
![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: 5
🧹 Nitpick comments (1)
crates/machine-validation/src/machine_validation.rs (1)
795-829: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the parser tests to a table-driven case set.
These are input-to-output/error mappings and should share one table covering valid hosts, ports,
localhost, bare names, and Docker Hub shorthands. As per coding guidelines, “Write tests in table-driven style”; as per path instructions, “Use table-driven tests.”🤖 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 `@crates/machine-validation/src/machine_validation.rs` around lines 795 - 829, Convert the separate extract_registry tests into one table-driven test covering valid registries, port-bearing hosts, localhost, bare image names, and Docker Hub shorthands. Define each case with its input and expected registry or error outcome, then iterate through the cases while preserving the current assertions and test coverage in extract_registry.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/admin-cli/src/credential/registry/args.rs`:
- Around line 40-41: Update the credential argument definition for password to
remove the secret-bearing --password command-line option and its help text.
Replace it with a secure hidden prompt and/or password-stdin input path,
ensuring registry credentials are never accepted or documented through argv
while preserving the existing password value flow.
In `@crates/api-core/src/handlers/credential.rs`:
- Around line 769-781: Update the credential handler around get_credentials to
validate registry before accessing the secret store, map lookup or decryption
failures to NicoError::Internal rather than InvalidArgument, and return missing
credentials as the project NotFound error converted with .into() instead of
constructing Status directly.
In `@crates/machine-validation/src/machine_validation.rs`:
- Line 465: Update the structured logging call for the nerdctl login failure to
rename the field from code to exit_code, while preserving the existing
status.code() value and log message.
- Around line 409-415: The registry-host detection logic should also recognize
the exact hostname “localhost” as an explicit registry. Update the condition in
the surrounding image-reference parsing function to accept first == "localhost"
alongside dotted or port-qualified hosts, and add or update tests covering
localhost/repository:tag credential lookup.
- Around line 449-460: Update the nerdctl login child-process handling around
child.stdin and child.wait to prevent indefinite blocking: drain or redirect the
piped stdout/stderr while waiting, enforce a timeout, and on expiry kill and
reap the child before returning. Preserve the existing password-write error
logging and normal exit-status handling.
---
Nitpick comments:
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 795-829: Convert the separate extract_registry tests into one
table-driven test covering valid registries, port-bearing hosts, localhost, bare
image names, and Docker Hub shorthands. Define each case with its input and
expected registry or error outcome, then iterate through the cases while
preserving the current assertions and test coverage in extract_registry.
🪄 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: 201eccad-a8bf-4932-a5ea-b8def4e071dd
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (13)
crates/admin-cli/src/credential/mod.rscrates/admin-cli/src/credential/registry/args.rscrates/admin-cli/src/credential/registry/cmd.rscrates/admin-cli/src/credential/registry/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/credential.rscrates/machine-validation/src/machine_validation.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/secrets/src/credentials.rsrest-api/proto/core/src/v1/nico_nico.proto
ianderson-nvidia
force-pushed
the
encrypt_container_registry_auth
branch
from
July 22, 2026 21:41
b342a22 to
ce1d2ee
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/machine-validation/src/machine_validation.rs (2)
480-481: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid bare discarded
Results.Use the repository’s explicit intentional-ignore convention rather than
let _ =for the kill/reap results. As per coding guidelines, “avoid discarding results withlet _unused = foo(); use.ok()when failure is intentionally ignored.”🤖 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 `@crates/machine-validation/src/machine_validation.rs` around lines 480 - 481, Update the child cleanup calls to use the repository’s intentional-ignore convention: replace the bare discarded Results from child.kill().await and child.wait().await with .ok(), preserving the existing kill-then-wait order and behavior.Source: Coding guidelines
810-853: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate registry parsing cases into a table-driven test.
These cases map inputs to successful values or errors and should use the repository’s scenario helpers in one table-driven test.
🤖 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 `@crates/machine-validation/src/machine_validation.rs` around lines 810 - 853, Consolidate the separate registry extraction tests around MachineValidation::extract_registry into one table-driven test using the repository’s existing scenario helpers. Include successful expected values for known registries, port-based and bare localhost, plus error expectations for bare image names and Docker Hub shorthand; remove the redundant individual test functions.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.
Inline comments:
In `@crates/machine-validation/scripts/seed_go_health_validation.sql`:
- Around line 36-39: Update the documented post-apply CLI command in the seed
script to use the correctly spelled --show-unverified flag instead of
--show-un-verfied.
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 489-493: Update the image-pull command construction around
TokioCmd so it invokes nerdctl directly instead of launching sh with -c. Pass
"-n", "default", "pull", and image_name as separate arguments, ensuring
image_name remains a single argument and cannot be interpreted as shell syntax.
---
Nitpick comments:
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 480-481: Update the child cleanup calls to use the repository’s
intentional-ignore convention: replace the bare discarded Results from
child.kill().await and child.wait().await with .ok(), preserving the existing
kill-then-wait order and behavior.
- Around line 810-853: Consolidate the separate registry extraction tests around
MachineValidation::extract_registry into one table-driven test using the
repository’s existing scenario helpers. Include successful expected values for
known registries, port-based and bare localhost, plus error expectations for
bare image names and Docker Hub shorthand; remove the redundant individual test
functions.
🪄 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: 2c7a3865-05cc-482e-93d8-e2a0f292fc84
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (14)
crates/admin-cli/src/credential/mod.rscrates/admin-cli/src/credential/registry/args.rscrates/admin-cli/src/credential/registry/cmd.rscrates/admin-cli/src/credential/registry/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/credential.rscrates/machine-validation/scripts/seed_go_health_validation.sqlcrates/machine-validation/src/machine_validation.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/secrets/src/credentials.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/admin-cli/src/credential/registry/cmd.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/admin-cli/src/credential/registry/mod.rs
- crates/admin-cli/src/credential/registry/args.rs
- crates/admin-cli/src/credential/mod.rs
- crates/api-core/src/api.rs
- crates/api-core/src/handlers/credential.rs
- crates/rpc/proto/forge.proto
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/admin-cli/src/rpc.rs
- crates/secrets/src/credentials.rs
ianderson-nvidia
force-pushed
the
encrypt_container_registry_auth
branch
2 times, most recently
from
July 22, 2026 22:03
d9b69c3 to
2b880c6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/machine-validation/src/machine_validation.rs (1)
478-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
.ok()overlet _ =for intentionally discarded results.As per coding guidelines, "avoid discarding results with
let _unused = foo(); use.ok()when failure is intentionally ignored." Applies directly to the kill/wait cleanup on timeout.♻️ Proposed fix
- let _ = child.kill().await; - let _ = child.wait().await; + child.kill().await.ok(); + child.wait().await.ok();🤖 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 `@crates/machine-validation/src/machine_validation.rs` around lines 478 - 483, Update the timeout cleanup in the nerdctl login handling to call `.ok()` on the intentionally ignored results from `child.kill().await` and `child.wait().await`, replacing both `let _ =` bindings while preserving the existing cleanup order.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.
Inline comments:
In `@crates/api-core/src/handlers/credential.rs`:
- Around line 794-802: Update set_container_registry_credential to validate
req.registry with the same non-empty guard used by
get_container_registry_credential before constructing
CredentialKey::ContainerRegistry. Return the matching invalid-argument error for
an empty registry, and preserve the existing credential persistence flow for
valid registries.
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 360-371: Update resolve_registry_credential to capture and log the
error returned by self.create_forge_client().await before returning None. Use
the existing error! logging style and include the forge-client build failure
details, while preserving the current unauthenticated fallback behavior.
---
Nitpick comments:
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 478-483: Update the timeout cleanup in the nerdctl login handling
to call `.ok()` on the intentionally ignored results from `child.kill().await`
and `child.wait().await`, replacing both `let _ =` bindings while preserving the
existing cleanup order.
🪄 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: cf31a780-83ae-4fdd-9544-1d0ef2ede024
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (13)
crates/admin-cli/src/credential/mod.rscrates/admin-cli/src/credential/registry/args.rscrates/admin-cli/src/credential/registry/cmd.rscrates/admin-cli/src/credential/registry/mod.rscrates/admin-cli/src/rpc.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/credential.rscrates/machine-validation/src/machine_validation.rscrates/rpc/build.rscrates/rpc/proto/forge.protocrates/secrets/src/credentials.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/admin-cli/src/credential/registry/args.rs
- crates/api-core/src/api.rs
- crates/rpc/build.rs
- crates/rpc/proto/forge.proto
- crates/admin-cli/src/credential/registry/cmd.rs
- crates/admin-cli/src/rpc.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/admin-cli/src/credential/registry/mod.rs
- crates/secrets/src/credentials.rs
Introduce authenticated container image pulls for machine validations
tests backed by private registries.
Credential storage:
- Add `CredentialKey::ContainerRegistry { registry }` to the
envelope-encrypted secrets journal, keyed at
`container_registries/<hostname>/auth`
- Add `GetContainerRegistryCredential` / `SetContainerRegistryCredential`
RPCs with RBAC: Scout can read; only NicoAdminCLI can write
- Add `nico-admin-cli credential registry set` to store credentials
- Updated rest-api protobufs to match new rpc and message for
ContainerRegistry
Pull path:
- `create_forge_client` now takes `&self` instead of consuming `self`,
eliminating the caller-side clones throughout `machine_validation.rs`
- `pull_container` resolves credentials from the NICO API before each
pull and passes them to `nerdctl login --password-stdin`, keeping the
password out of process arguments and logs
- `extract_registry` parses the registry hostname from an OCI image
reference and returns an error for bare names and Docker Hub shorthands
that have no explicit registry component
ianderson-nvidia
force-pushed
the
encrypt_container_registry_auth
branch
from
July 22, 2026 22:20
2b880c6 to
ba3f3c7
Compare
chet
approved these changes
Jul 23, 2026
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.
Introduce authenticated pulls of container images for machine validation tests, backed by private registries.
Credential storage:
CredentialKey::ContainerRegistry { registry }to the envelope-encrypted secrets journal, keyed atcontainer_registries/<hostname>/authGetContainerRegistryCredential/SetContainerRegistryCredentialRPCs with RBAC: Scout can read; only NicoAdminCLI can writenico-admin-cli credential registry setto store credentialsPull path:
create_forge_clientnow takes&selfinstead of consumingself, eliminating the caller-side clones throughoutmachine_validation.rspull_containerresolves credentials from the NICO API before each pull and passes them tonerdctl login --password-stdin, keeping the password out of process arguments and logsextract_registryparses the registry hostname from an OCI image reference and returns an error for bare names and Docker Hub shorthands that have no explicit registry componentRelated issues
#3655
Type of Change
Testing
Additional Notes