refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse#3113
refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse#3113spydaNVIDIA wants to merge 2 commits into
Conversation
…dor to RedfishClientPool for rotation reuse
Summary by CodeRabbit
WalkthroughThis PR adds two new BMC administration capabilities—setting a BMC root password out-of-band and probing a BMC's Redfish vendor—spanning proto definitions, Redfish client pool logic, site-explorer wiring, API handlers, admin-cli commands, and documentation. ChangesBMC Root Password & Vendor Probing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCli
participant Api
participant BmcEndpointExplorer
participant Vault
participant RedfishClient
AdminCli->>Api: SetBmcRootPassword(request)
Api->>BmcEndpointExplorer: set_bmc_root_password(address, interface, new_password)
BmcEndpointExplorer->>Vault: load current root credentials
Vault-->>BmcEndpointExplorer: credentials
BmcEndpointExplorer->>RedfishClient: probe_bmc_vendor(address, credentials)
RedfishClient-->>BmcEndpointExplorer: vendor
BmcEndpointExplorer->>RedfishClient: set_bmc_root_password(address, vendor, credentials, new_password)
RedfishClient-->>BmcEndpointExplorer: Ok(())
BmcEndpointExplorer->>Vault: persist new credentials
BmcEndpointExplorer-->>Api: Ok(())
Api-->>AdminCli: SetBmcRootPasswordResponse
Compact metadata
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3113.docs.buildwithfern.com/infra-controller |
🔐 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-02 23:24:26 UTC | Commit: 4504c6d |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
crates/admin-cli/src/bmc_machine/set_root_password/args.rs (1)
51-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
bmc_endpoint_requestconstruction logic.This
From<Args>body is byte-for-byte identical (modulo the extranew_passwordfield) to the one incrates/admin-cli/src/bmc_machine/probe_vendor/args.rs(lines 45-61). Consider extracting a shared helper (e.g.fn bmc_endpoint_request_from(ip: Option<String>, mac: Option<MacAddress>) -> Option<forgerpc::BmcEndpointRequest>) in a common module for both commands. This also gives a single place to fix the MAC-only bug noted above instead of two.🤖 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/admin-cli/src/bmc_machine/set_root_password/args.rs` around lines 51 - 68, The `From<Args>` implementation for `SetBmcRootPasswordRequest` duplicates the same `bmc_endpoint_request` building logic used by `probe_vendor::args::From<Args>`, so factor that shared logic into a common helper and reuse it from both command conversions. Introduce a reusable function for constructing `forgerpc::BmcEndpointRequest` from the IP/MAC inputs, then call it from `Args::from` here and in the probe vendor args conversion, keeping `machine_id` and `new_password` handling local to `SetBmcRootPasswordRequest`.docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the generic BMC IP placeholder for
--ip-address.This flag targets the BMC's own IP (not a host-targeted flag), so it should use
192.0.2.10rather than192.0.2.20, which is reserved for host-targeted commands such asmlx-devices --host.📝 Proposed fix
-nico-admin-cli bmc-machine probe-vendor --ip-address 192.0.2.20 +nico-admin-cli bmc-machine probe-vendor --ip-address 192.0.2.10Based on learnings, "Use
192.0.2.10as the canonical placeholder for the DPU/generic BMC IP address" while "192.0.2.20" is reserved for host-targeted flags.🤖 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/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md` at line 54, Update the example for nico-admin-cli bmc-machine probe-vendor so the --ip-address placeholder uses the generic BMC/DPU IP value 192.0.2.10 instead of 192.0.2.20. The issue is in the probe-vendor command example, and the fix is to replace the host-targeted placeholder with the canonical BMC IP placeholder consistently in this document.Source: Learnings
docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame BMC IP placeholder inconsistency as
probe-vendor.md.
--ip-addresstargets the BMC directly here too, so192.0.2.10(generic/DPU BMC IP) should be used instead of192.0.2.20(reserved for host-targeted commands).📝 Proposed fix
-nico-admin-cli bmc-machine set-root-password --ip-address 192.0.2.20 --new-password mynewpassword +nico-admin-cli bmc-machine set-root-password --ip-address 192.0.2.10 --new-password mynewpasswordBased on learnings, "Use
192.0.2.10as the canonical placeholder for the DPU/generic BMC IP address" while "192.0.2.20" is the host BMC placeholder.🤖 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/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md` at line 60, The BMC IP placeholder in the set-root-password example is inconsistent with the canonical BMC-targeted placeholder. Update the example under the set-root-password command so it uses the generic/DPU BMC address placeholder 192.0.2.10 for the --ip-address argument, matching the convention used for direct BMC commands. Keep the command text otherwise unchanged and ensure this aligns with the placeholder usage in the related BMC docs, including the set-root-password example.Source: Learnings
crates/api-core/src/handlers/bmc_endpoint_explorer.rs (1)
988-1055: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding table-driven tests for the new request→response adapters.
No tests are visible for
set_bmc_root_passwordorprobe_bmc_vendorin this diff. Both are straightforward request-validation/adapter functions well-suited to table-driven coverage of the variousvalidate_and_complete_bmc_endpoint_request/resolve_bmc_interfacefailure paths (missing machine, IP/MAC mismatch, missing BMC IP, etc.).As per path instructions, "prefer table-driven tests for functions that map inputs to outputs/errors (useful for request->response adapters introduced by the RPC contract)."
🤖 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/api-core/src/handlers/bmc_endpoint_explorer.rs` around lines 988 - 1055, Add table-driven tests for the new request-to-response adapter handlers `set_bmc_root_password` and `probe_bmc_vendor`, since the diff adds validation/translation logic without visible coverage. Exercise the main success path plus the failure cases from `validate_and_complete_bmc_endpoint_request` and `resolve_bmc_interface`, including missing machine ID, IP/MAC mismatch, and missing BMC IP. Keep the tests focused on the adapter behavior and expected mapped responses/errors so regressions in these request handlers are caught.Source: 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/bmc_machine/probe_vendor/args.rs`:
- Around line 32-33: The `probe-vendor` help text currently shows a MAC-only
example even though `probe_vendor` always populates
`bmc_endpoint_request.ip_address` and the API requires it. Update the example in
`args.rs` to include `--ip-address`, or remove the `--mac-address`-only example
entirely, so the usage shown by `ProbeVendorArgs` matches the actual
`probe-vendor` behavior.
In `@crates/admin-cli/src/bmc_machine/set_root_password/args.rs`:
- Around line 34-36: The `set_root_password::args` example currently suggests
`--mac-address` can be used alone, but the `BmcEndpointRequest` built by this
flow still ends up with an empty `ip_address` and is rejected. Update the
`set_root_password` argument handling so the BMC IP is resolved before
constructing the request, using the MAC-based lookup path in the relevant
`BmcEndpointRequest`/handler flow, or remove the MAC-only example if that path
is not supported.
In `@crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs`:
- Around line 22-26: The `set_root_password` function is passing the local
`Args` type directly into `set_bmc_root_password`, but the RPC client expects a
`::rpc::forge::SetBmcRootPasswordRequest`. Update the call in
`set_root_password` to convert `args` using its existing `Into` conversion
before invoking `api_client.0.set_bmc_root_password`, so the request type
matches what the client method requires.
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 1012-1015: The handler logic in the endpoint explorer is
collapsing all EndpointExplorationError cases into CarbideError::internal, which
loses the user-input vs system-failure distinction. Update the error mapping in
the root password flow and the other affected handler to inspect the underlying
EndpointExplorationError and return the appropriate CarbideError variant for bad
password/missing credential cases versus genuine BMC/network failures. Follow
the same pattern used by map_redfish_client_creation_error and map_redfish_error
so callers can reliably distinguish actionable 4xx-like errors from internal
5xx-like errors.
- Around line 988-993: The request logging in set_bmc_root_password is exposing
the plaintext new_password because log_request_data(&request) logs the full
Debug payload before redaction. Update this path to use
log_request_data_redacted(...) or first call into_inner() and log a redacted
copy of rpc::SetBmcRootPasswordRequest so the sensitive field is never written
to logs.
In `@crates/rpc/proto/forge.proto`:
- Around line 7897-7905: The SetBmcRootPasswordRequest currently logs sensitive
data because set_bmc_root_password calls log_request_data(&request), which
captures the full Debug output including new_password. Update the
bmc_endpoint_explorer::set_bmc_root_password path to use the redacted logging
helper or explicitly omit/filter new_password before logging, so the request
still has context without exposing the plaintext BMC root password.
In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 1215-1240: The `set_bmc_root_password` flow can leave the BMC
rotated but the vault stale if `credential_client.set_bmc_root_credentials`
fails after the device password change succeeds. Update this path in
`set_bmc_root_password` to handle post-rotation persistence failures explicitly,
either by retrying the credential write with backoff or by returning a dedicated
error such as `EndpointExplorationError::CredentialPersistFailedAfterRotation`.
Make sure the error includes enough context to distinguish this from a generic
explorer failure so operators can quickly identify and remediate the
partial-failure case.
---
Nitpick comments:
In `@crates/admin-cli/src/bmc_machine/set_root_password/args.rs`:
- Around line 51-68: The `From<Args>` implementation for
`SetBmcRootPasswordRequest` duplicates the same `bmc_endpoint_request` building
logic used by `probe_vendor::args::From<Args>`, so factor that shared logic into
a common helper and reuse it from both command conversions. Introduce a reusable
function for constructing `forgerpc::BmcEndpointRequest` from the IP/MAC inputs,
then call it from `Args::from` here and in the probe vendor args conversion,
keeping `machine_id` and `new_password` handling local to
`SetBmcRootPasswordRequest`.
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 988-1055: Add table-driven tests for the new request-to-response
adapter handlers `set_bmc_root_password` and `probe_bmc_vendor`, since the diff
adds validation/translation logic without visible coverage. Exercise the main
success path plus the failure cases from
`validate_and_complete_bmc_endpoint_request` and `resolve_bmc_interface`,
including missing machine ID, IP/MAC mismatch, and missing BMC IP. Keep the
tests focused on the adapter behavior and expected mapped responses/errors so
regressions in these request handlers are caught.
In
`@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.md`:
- Line 54: Update the example for nico-admin-cli bmc-machine probe-vendor so the
--ip-address placeholder uses the generic BMC/DPU IP value 192.0.2.10 instead of
192.0.2.20. The issue is in the probe-vendor command example, and the fix is to
replace the host-targeted placeholder with the canonical BMC IP placeholder
consistently in this document.
In
`@docs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.md`:
- Line 60: The BMC IP placeholder in the set-root-password example is
inconsistent with the canonical BMC-targeted placeholder. Update the example
under the set-root-password command so it uses the generic/DPU BMC address
placeholder 192.0.2.10 for the --ip-address argument, matching the convention
used for direct BMC commands. Keep the command text otherwise unchanged and
ensure this aligns with the placeholder usage in the related BMC docs, including
the set-root-password example.
🪄 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: f547ec4d-ef1f-407e-97bd-f6d1f3e3640d
📒 Files selected for processing (20)
crates/admin-cli/src/bmc_machine/mod.rscrates/admin-cli/src/bmc_machine/probe_vendor/args.rscrates/admin-cli/src/bmc_machine/probe_vendor/cmd.rscrates/admin-cli/src/bmc_machine/probe_vendor/mod.rscrates/admin-cli/src/bmc_machine/set_root_password/args.rscrates/admin-cli/src/bmc_machine/set_root_password/cmd.rscrates/admin-cli/src/bmc_machine/set_root_password/mod.rscrates/api-core/src/api.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/forge.protocrates/secrets/src/credentials.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/endpoint_explorer.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rsdocs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-probe-vendor.mddocs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine-set-root-password.mddocs/manuals/nico-admin-cli/commands/bmc-machine/bmc-machine.md
| Target the BMC by MAC address: | ||
| $ nico-admin-cli bmc-machine probe-vendor --mac-address 00:11:22:33:44:55 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Rust's std::net::IpAddr FromStr implementation error on an empty string?
💡 Result:
Yes, Rust's std::net::IpAddr FromStr implementation errors on an empty string [1][2]. The FromStr implementation for IpAddr parses the input string by calling internal parsing routines (specifically parse_ascii) [1][2]. Because an empty string does not contain a valid IPv4 or IPv6 address format, it fails to parse and returns an AddrParseError [1][2]. In Rust, calling "".parse:: will return Err(AddrParseError).
Citations:
- 1: https://doc.rust-lang.org/stable/src/core/net/parser.rs.html
- 2: https://github.com/rust-lang/rust/blob/master/library/core/src/net/parser.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the specific region mentioned in the review.
git ls-files 'crates/admin-cli/src/bmc_machine/probe_vendor/**' 'crates/admin-cli/src/**' | sed -n '1,120p'
echo '--- args.rs ---'
wc -l crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
cat -n crates/admin-cli/src/bmc_machine/probe_vendor/args.rs | sed -n '1,220p'
echo '--- search bmc_endpoint_request usage ---'
rg -n "bmc_endpoint_request|probe_vendor|ip_address.parse\\(" crates -g '*.rs'Repository: NVIDIA/infra-controller
Length of output: 23532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1090,1165p' crates/api-core/src/handlers/bmc_endpoint_explorer.rs
echo '--- tests or callers around mac-only/bmc endpoint request ---'
rg -n "mac_address: Some|ip_address: \"\"|Can supply _only_ ip_address or all fields|mac-only|probe-vendor" crates/api-core/src crates/admin-cli/src -g '*.rs'Repository: NVIDIA/infra-controller
Length of output: 6933
Remove the MAC-only example or require --ip-address
probe-vendor always sends bmc_endpoint_request.ip_address, and the API parses it unconditionally. With only --mac-address, that field becomes "", so the request is rejected. The help text should either include --ip-address here or drop this example.
🤖 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/admin-cli/src/bmc_machine/probe_vendor/args.rs` around lines 32 - 33,
The `probe-vendor` help text currently shows a MAC-only example even though
`probe_vendor` always populates `bmc_endpoint_request.ip_address` and the API
requires it. Update the example in `args.rs` to include `--ip-address`, or
remove the `--mac-address`-only example entirely, so the usage shown by
`ProbeVendorArgs` matches the actual `probe-vendor` behavior.
| Target the BMC by MAC address: | ||
| $ nico-admin-cli bmc-machine set-root-password \ | ||
| --mac-address 00:11:22:33:44:55 --new-password mynewpassword |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'crates/admin-cli/src/bmc_machine/**' | sort
printf '\n== Search for BmcEndpointRequest and relevant builders ==\n'
rg -n "BmcEndpointRequest|ip_address|mac_address|new-password|probe_vendor|set_root_password" crates/admin-cli/src -S
printf '\n== Outline candidate files ==\n'
ast-grep outline crates/admin-cli/src/bmc_machine/set_root_password/args.rs --view expanded || true
ast-grep outline crates/admin-cli/src/bmc_machine/probe_vendor/args.rs --view expanded || trueRepository: NVIDIA/infra-controller
Length of output: 39455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
crates/admin-cli/src/bmc_machine/set_root_password/args.rs
crates/admin-cli/src/bmc_machine/probe_vendor/args.rs
)
for f in "${files[@]}"; do
echo "===== $f ====="
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
echo
done
printf '\n===== request-building search =====\n'
rg -n "struct BmcEndpointRequest|impl .*BmcEndpointRequest|from_args|Into<BmcEndpointRequest>|ip_address:|mac_address:" crates/admin-cli/src -SRepository: NVIDIA/infra-controller
Length of output: 23931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== set_root_password args ====="
sed -n '1,220p' crates/admin-cli/src/bmc_machine/set_root_password/args.rs | cat -n
echo "===== probe_vendor args ====="
sed -n '1,220p' crates/admin-cli/src/bmc_machine/probe_vendor/args.rs | cat -n
echo "===== all BmcEndpointRequest references ====="
rg -n "BmcEndpointRequest|ip_address|mac_address" crates/admin-cli/src -SRepository: NVIDIA/infra-controller
Length of output: 40350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the server-side handling of BmcEndpointRequest and how empty ip_address is treated.
rg -n "bmc_endpoint_request|BmcEndpointRequest|ip_address\.parse|mac_address" crates api -S
# Inspect the request type definitions around the relevant fields in admin-cli rpc wrappers.
sed -n '630,700p' crates/admin-cli/src/rpc.rs | cat -n
sed -n '2470,2515p' crates/admin-cli/src/rpc.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "bmc_endpoint_request|BmcEndpointRequest|ip_address\.parse|mac_address" crates api -S
printf '\n===== rpc.rs excerpts =====\n'
sed -n '630,700p' crates/admin-cli/src/rpc.rs | cat -n
printf '\n===== rpc.rs BMC methods =====\n'
sed -n '2470,2515p' crates/admin-cli/src/rpc.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SetBmcRootPasswordRequest|ProbeBmcVendorRequest|BmcEndpointRequest|set_root_password|probe_vendor" crates -SRepository: NVIDIA/infra-controller
Length of output: 12602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=crates/api-core/src/handlers/bmc_endpoint_explorer.rs
printf '\n== outline ==\n'
ast-grep outline "$f" --view expanded || true
printf '\n== request resolution excerpt ==\n'
sed -n '1090,1195p' "$f" | cat -n
printf '\n== set/probe handlers excerpt ==\n'
sed -n '980,1045p' "$f" | cat -nRepository: NVIDIA/infra-controller
Length of output: 10255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=crates/api-core/src/handlers/bmc_endpoint_explorer.rs
printf '\n== request resolution excerpt ==\n'
sed -n '1090,1195p' "$f" | cat -n
printf '\n== set/probe handlers excerpt ==\n'
sed -n '980,1045p' "$f" | cat -nRepository: NVIDIA/infra-controller
Length of output: 7873
Resolve the BMC IP before building the root-password request. --mac-address alone still builds BmcEndpointRequest with ip_address: "", and this handler rejects requests without a populated IP. Either look up the IP from the MAC here or remove the MAC-only example.
🤖 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/admin-cli/src/bmc_machine/set_root_password/args.rs` around lines 34 -
36, The `set_root_password::args` example currently suggests `--mac-address` can
be used alone, but the `BmcEndpointRequest` built by this flow still ends up
with an empty `ip_address` and is rejected. Update the `set_root_password`
argument handling so the BMC IP is resolved before constructing the request,
using the MAC-based lookup path in the relevant `BmcEndpointRequest`/handler
flow, or remove the MAC-only example if that path is not supported.
| pub async fn set_root_password(args: Args, api_client: &ApiClient) -> CarbideCliResult<()> { | ||
| api_client.0.set_bmc_root_password(args).await?; | ||
| println!("BMC root password updated"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ApiClient's set_bmc_root_password signature and how other cmd.rs files call similar RPCs.
rg -n "struct ApiClient" -A 20 crates/admin-cli/src
rg -n "fn set_bmc_root_password" -B2 -A 10
rg -n "api_client\.0\.\w+\(" -A 1 crates/admin-cli/srcRepository: NVIDIA/infra-controller
Length of output: 1762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== cmd.rs ==\n'
cat -n crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs
printf '\n== args.rs ==\n'
cat -n crates/admin-cli/src/bmc_machine/set_root_password/args.rs
printf '\n== rpc.rs relevant section ==\n'
sed -n '1,220p' crates/admin-cli/src/rpc.rs
printf '\n== search similar call patterns ==\n'
rg -n "set_bmc_root_password|\.into\(\)\)\.await\?|\.into\(\)\)" crates/admin-cli/src -SRepository: NVIDIA/infra-controller
Length of output: 21341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate set_bmc_root_password definitions ==\n'
rg -n "set_bmc_root_password" .
printf '\n== locate ForgeApiClient definition source ==\n'
rg -n "struct ForgeApiClient|impl ForgeApiClient" .Repository: NVIDIA/infra-controller
Length of output: 3648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== forge_api_client.rs relevant section ==\n'
rg -n "set_bmc_root_password" -A 12 -B 8 crates/rpc/src/forge_api_client.rsRepository: NVIDIA/infra-controller
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all references to SetBmcRootPasswordRequest ==\n'
rg -n "SetBmcRootPasswordRequest|set_bmc_root_password" .
printf '\n== rpc crate file list around forge client ==\n'
fd -a "forge_api_client.rs|forge.*client|generated" crates/rpc -t f
printf '\n== forge_api_client.rs outline ==\n'
ast-grep outline crates/rpc/src/forge_api_client.rs --view expandedRepository: NVIDIA/infra-controller
Length of output: 5136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== forge_api_client.rs first 260 lines ==\n'
sed -n '1,260p' crates/rpc/src/forge_api_client.rs
printf '\n== forge_tls_client.rs first 260 lines ==\n'
sed -n '1,260p' crates/rpc/src/forge_tls_client.rs
printf '\n== any generated client type names in rpc crate ==\n'
rg -n "ForgeApiClient|set_bmc_root_password|IntoRequest<.*SetBmcRootPasswordRequest" crates/rpc/src crates/rpc/proto -SRepository: NVIDIA/infra-controller
Length of output: 17184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search generated proto client in repo ==\n'
rg -n "pub async fn set_bmc_root_password|fn set_bmc_root_password|SetBmcRootPasswordRequest" crates/rpc -g'!**/target/**' -S
printf '\n== list proto/gen files under crates/rpc ==\n'
fd -a -t f . crates/rpc
printf '\n== inspect tonic IntoRequest blanket impl in toolchain docs? ==\n'
python3 - <<'PY'
import sys, pkgutil
print("python ok")
PYRepository: NVIDIA/infra-controller
Length of output: 6775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all set_bmc_root_password symbols ==\n'
rg -n "set_bmc_root_password|SetBmcRootPasswordRequest" crates -S
printf '\n== rpc crate tree ==\n'
fd -a -t f . crates/rpc
printf '\n== generated proto module hints ==\n'
rg -n "mod protos|pub mod protos|include_proto|tonic::include_proto" crates/rpc/src crates/rpc -SRepository: NVIDIA/infra-controller
Length of output: 10381
🌐 Web query:
tonic IntoRequest blanket impl From IntoRequest Request<T> generated client method accepts custom type implementing From
💡 Result:
In tonic, the client methods generated for RPC calls accept any type that implements the IntoRequest trait [1][2]. This trait is designed to allow convenience when calling RPC methods, enabling you to pass either the raw message type T or a pre-constructed Request [1]. Tonic provides a blanket implementation of IntoRequest for all types T [1][3]. Specifically, the implementation impl<T> IntoRequest<T> for T allows a raw message to be automatically wrapped in a Request via Request::new(self) [1]. There is also a separate blanket implementation for Request<T> itself, which simply returns the request instance unchanged [1][3]. Regarding your question about using a custom type that implements From: The IntoRequest trait does not natively use the standard From or Into traits for its conversion logic [1][4]. Because the blanket implementation impl<T> IntoRequest<T> for T is provided for all types, attempting to implement IntoRequest for a custom type that also implements From for the underlying message type may conflict with the existing blanket implementation if not handled correctly. If you have a custom type and want to pass it to a generated tonic client method, you have two primary options: 1. Manual Conversion: Convert your custom type to the expected message type T (using your From/Into implementation) before passing it to the client method. 2. Implement IntoRequest: While you can theoretically implement IntoRequest for your custom type, doing so requires care because of potential coherence issues with the existing blanket implementation in the tonic crate. In summary, tonic does not rely on a From-based blanket implementation for IntoRequest; it uses a direct blanket implementation for any type T to wrap it in a Request [1]. Custom type support is best achieved by converting to the target message type before the call, rather than relying on an automatic trait-based conversion at the client method boundary [1].
Citations:
- 1: https://docs.rs/tonic/latest/src/tonic/request.rs.html
- 2: Consider an
IntoRequesttrait grpc/grpc-rust#34 - 3: https://docs.rs/tonic/latest/tonic/struct.Request.html
- 4: feat: Add
IntoRequestandIntoStreamingRequesttraits grpc/grpc-rust#66
Convert Args before calling the RPC client
crates/admin-cli/src/bmc_machine/set_root_password/cmd.rs:23 — set_bmc_root_password takes ::rpc::forge::SetBmcRootPasswordRequest, so pass args.into() here; Args only defines the conversion, it is not the request type itself.
🤖 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/admin-cli/src/bmc_machine/set_root_password/cmd.rs` around lines 22 -
26, The `set_root_password` function is passing the local `Args` type directly
into `set_bmc_root_password`, but the RPC client expects a
`::rpc::forge::SetBmcRootPasswordRequest`. Update the call in
`set_root_password` to convert `args` using its existing `Into` conversion
before invoking `api_client.0.set_bmc_root_password`, so the request type
matches what the client method requires.
| pub(crate) async fn set_bmc_root_password( | ||
| api: &Api, | ||
| request: Request<rpc::SetBmcRootPasswordRequest>, | ||
| ) -> Result<Response<rpc::SetBmcRootPasswordResponse>, Status> { | ||
| log_request_data(&request); | ||
| let req = request.into_inner(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A 15 'fn log_request_data' --type=rust
rg -n 'new_password' crates/rpc/proto/forge.protoRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- bmc_endpoint_explorer outline ---'
ast-grep outline crates/api-core/src/handlers/bmc_endpoint_explorer.rs --view expanded || true
echo '--- handler slice ---'
sed -n '980,1065p' crates/api-core/src/handlers/bmc_endpoint_explorer.rs | cat -n
echo '--- log_request_data search ---'
rg -n -A 25 -B 5 'fn log_request_data|log_request_data\(' crates/api-core crates -g '!target' || true
echo '--- proto field search ---'
rg -n -A 5 -B 5 'new_password|SetBmcRootPasswordRequest|SetBmcRootPasswordResponse' crates -g '!target' || trueRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- handler excerpt ---'
sed -n '988,1065p' crates/api-core/src/handlers/bmc_endpoint_explorer.rs | cat -n
echo '--- log_request_data definition(s) ---'
rg -n -A 40 -B 10 'fn log_request_data' crates/api-core crates -g '!target' | sed -n '1,220p'
echo '--- request schema for password field ---'
rg -n -A 30 -B 10 'message SetBmcRootPasswordRequest|new_password|current_password|password' crates/rpc crates -g '!target' | sed -n '1,220p'
echo '--- EndpointExplorationError mapping ---'
rg -n -A 40 -B 10 'enum EndpointExplorationError|match .*EndpointExplorationError|CarbideError::internal\(e\.to_string\(\)\)' crates/api-core/src/handlers/bmc_endpoint_explorer.rs crates -g '!target' | sed -n '1,260p'Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A 40 -B 10 'enum EndpointExplorationError|type EndpointExplorationError|struct .*EndpointExploration|impl .*EndpointExplorationError|set_bmc_root_password\(' crates/api-core crates -g '!target' | sed -n '1,240p'Repository: NVIDIA/infra-controller
Length of output: 22184
Redact new_password before request logging. log_request_data(&request) records request.get_ref() via Debug here, so the plaintext BMC root password can end up in the span. Switch to log_request_data_redacted(...) or log a redacted copy after into_inner() so credentials never reach logs.
🤖 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/api-core/src/handlers/bmc_endpoint_explorer.rs` around lines 988 -
993, The request logging in set_bmc_root_password is exposing the plaintext
new_password because log_request_data(&request) logs the full Debug payload
before redaction. Update this path to use log_request_data_redacted(...) or
first call into_inner() and log a redacted copy of
rpc::SetBmcRootPasswordRequest so the sensitive field is never written to logs.
Source: Path instructions
| api.endpoint_explorer | ||
| .set_bmc_root_password(bmc_addr, &machine_interface, &req.new_password) | ||
| .await | ||
| .map_err(|e| CarbideError::internal(e.to_string()))?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Blanket CarbideError::internal mapping loses the invalid-argument vs system-error distinction.
Both new handlers collapse every EndpointExplorationError (e.g. bad current password/Unauthorized, missing vault entry/MissingCredentials, genuine network/BMC failure) into CarbideError::internal. Callers (admin-cli, other services) can no longer distinguish "you supplied the wrong current password" (a 4xx-like, user-actionable condition) from "the BMC/network is down" (a genuine 5xx). This is the exact "avoid — constructing Status directly, bypassing NicoError error mapping" anti-pattern flagged in the referenced STYLE_GUIDE, applied in spirit to error variant selection.
Consider mapping the underlying EndpointExplorationError to the appropriate CarbideError variant (similar to how map_redfish_client_creation_error/map_redfish_error differentiate causes elsewhere in redfish.rs) instead of a single catch-all .internal().
As per path instructions, "Prefer NicoError-based error construction inside handlers... choose the NicoError variant based on whether failures are user-input/invalid-argument (4xx-like) vs internal/system issues."
Also applies to: 1044-1048
🤖 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/api-core/src/handlers/bmc_endpoint_explorer.rs` around lines 1012 -
1015, The handler logic in the endpoint explorer is collapsing all
EndpointExplorationError cases into CarbideError::internal, which loses the
user-input vs system-failure distinction. Update the error mapping in the root
password flow and the other affected handler to inspect the underlying
EndpointExplorationError and return the appropriate CarbideError variant for bad
password/missing credential cases versus genuine BMC/network failures. Follow
the same pattern used by map_redfish_client_creation_error and map_redfish_error
so callers can reliably distinguish actionable 4xx-like errors from internal
5xx-like errors.
Source: Path instructions
| // Must provide either machine_id or ip/mac pair | ||
| message SetBmcRootPasswordRequest { | ||
| optional BmcEndpointRequest bmc_endpoint_request = 1; | ||
| optional string machine_id = 2; | ||
| // New BMC root password to set. The server authenticates with the currently | ||
| // stored root credentials, probes the vendor, sets the new password, and | ||
| // records it as the device's per-BMC credential. | ||
| string new_password = 3; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect log_request_data and confirm SetBmcRootPasswordRequest.new_password is filtered.
rg -nP --type=rust -C4 'fn\s+log_request_data'
rg -nP --type=rust -C6 'set_bmc_root_password' crates/api-core/src/handlers/bmc_endpoint_explorer.rsRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate log_request_data and handler ==="
rg -n --hidden --glob '!target' 'log_request_data|set_bmc_root_password|SetBmcRootPasswordRequest' crates .github book 2>/dev/null || true
echo
echo "=== find likely handler files ==="
fd -a 'bmc_endpoint_explorer.rs|log_request_data|forge.proto' crates . 2>/dev/null || trueRepository: NVIDIA/infra-controller
Length of output: 39922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== crate file list around api-core handlers ==="
fd -a . crates/api-core/src/handlers 2>/dev/null | sed -n '1,200p'
echo
echo "=== search for request logging helpers ==="
rg -n --hidden --glob '!target' 'fn\s+log_request_data|log_request_data\(' crates/api-core crates/api-model crates/api-db 2>/dev/null || trueRepository: NVIDIA/infra-controller
Length of output: 33862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== api.rs log helpers ==="
sed -n '3510,3555p' crates/api-core/src/api.rs | cat -n
echo
echo "=== bmc_endpoint_explorer set_bmc_root_password ==="
sed -n '980,1020p' crates/api-core/src/handlers/bmc_endpoint_explorer.rs | cat -n
echo
echo "=== request type definition in proto ==="
sed -n '7890,7915p' crates/rpc/proto/forge.proto | cat -nRepository: NVIDIA/infra-controller
Length of output: 4604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate generated/request type Rust definitions ==="
fd -a 'forge.rs|mod.rs' crates/rpc crates/api-core 2>/dev/null | sed -n '1,120p'
echo
echo "=== search for SetBmcRootPasswordRequest in Rust sources ==="
rg -n --hidden --glob '!target' 'SetBmcRootPasswordRequest|new_password' crates/rpc crates/api-core crates/admin-cli crates/site-explorer 2>/dev/null | sed -n '1,220p'Repository: NVIDIA/infra-controller
Length of output: 5407
Redact new_password before logging this request. crates/api-core/src/handlers/bmc_endpoint_explorer.rs::set_bmc_root_password calls log_request_data(&request), and that helper records the request’s full Debug form. This will emit the plaintext BMC root password into structured logs. Use the redacted logging path here or filter the field explicitly.
🤖 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/rpc/proto/forge.proto` around lines 7897 - 7905, The
SetBmcRootPasswordRequest currently logs sensitive data because
set_bmc_root_password calls log_request_data(&request), which captures the full
Debug output including new_password. Update the
bmc_endpoint_explorer::set_bmc_root_password path to use the redacted logging
helper or explicitly omit/filter new_password before logging, so the request
still has context without exposing the plaintext BMC root password.
Source: Path instructions
| // Resolve the dispatch vendor `set_bmc_root_password` branches on using | ||
| // the current credentials, then set the new password on the device. | ||
| let vendor = self | ||
| .redfish_client | ||
| .probe_bmc_vendor(bmc_ip_address, current_credentials.clone()) | ||
| .await?; | ||
| let new_credentials = self | ||
| .set_bmc_root_password( | ||
| bmc_ip_address, | ||
| vendor, | ||
| current_credentials, | ||
| new_password.to_string(), | ||
| ) | ||
| .await?; | ||
|
|
||
| // Persist the new per-device credential so NICo can still reach the BMC. | ||
| // Deliberately does NOT record rotation convergence (unlike | ||
| // `set_bmc_root_credentials`): this is an out-of-band set, so the | ||
| // credential-rotation engine will reassert the site-wide password on | ||
| // its next pass rather than treating this device as converged. | ||
| self.credential_client | ||
| .set_bmc_root_credentials(bmc_mac_address, &new_credentials) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Partial-failure risk: BMC lockout if vault persistence fails after device rotation.
Lines 1221-1237 perform two dependent external writes: rotate the password on the physical BMC, then persist the new credential to the vault. If the vault write in Line 1236 fails (network blip, vault outage) after the on-device rotation in Line 1222 already succeeded, the stored credential becomes stale and every subsequent get_bmc_root_credentials call will authenticate with the wrong password — effectively locking NICo out of that BMC until an operator manually intervenes.
Consider retrying the vault persistence step with backoff before propagating the error, or emitting a distinct, actionable error/alert (e.g. EndpointExplorationError::CredentialPersistFailedAfterRotation) so operators can immediately recognize and remediate this specific failure mode rather than treating it as a generic explorer error.
🤖 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/site-explorer/src/bmc_endpoint_explorer.rs` around lines 1215 - 1240,
The `set_bmc_root_password` flow can leave the BMC rotated but the vault stale
if `credential_client.set_bmc_root_credentials` fails after the device password
change succeeds. Update this path in `set_bmc_root_password` to handle
post-rotation persistence failures explicitly, either by retrying the credential
write with backoff or by returning a dedicated error such as
`EndpointExplorationError::CredentialPersistFailedAfterRotation`. Make sure the
error includes enough context to distinguish this from a generic explorer
failure so operators can quickly identify and remediate the partial-failure
case.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
refactor(redfish): move BMC set_bmc_root_password + add probe_bmc_vendor to RedfishClientPool for rotation reuse
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes