feat(machine-controller): add host UEFI credential rotation - #4424
Merged
spydaNVIDIA merged 3 commits intoJul 31, 2026
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:
Summary by CodeRabbit
WalkthroughAdds an RPC and CLI for forcing or clearing host UEFI credential rotation. Persists rotation requests, adds family-aware rotation gates, implements a multi-step machine-controller flow with Redfish retries, and adds integration coverage for success and failure paths. ChangesUEFI rotation system
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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-31 01:17:45 UTC | Commit: 70e6145 |
![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: 2
🧹 Nitpick comments (4)
crates/api-core/src/handlers/uefi_credential_rotation.rs (1)
39-70: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueValidate
modebefore opening the transaction / resolving the target.
Mode::Unspecifiedis only rejected inside thematch(line 60), afterapi.txn_begin()andresolve_target(which issues a DB lookup whenbmc_macis set) have already run. Checking the mode upfront avoids an unnecessary transaction and query on malformed requests.♻️ Proposed reordering
log_request_data(&request); let req = request.into_inner(); let mode = req.mode(); + // An omitted `mode` decodes as `Unspecified`; reject it before doing any + // DB work rather than let a request fall through to an action it did not name. + if mode == Mode::Unspecified { + return Err( + CarbideError::InvalidArgument("mode must be set or clear".to_string()).into(), + ); + } + let mut txn = api.txn_begin().await?; let machine_id = resolve_target(&mut txn, req.machine_id, req.bmc_mac).await?; match mode { Mode::Set => { db::machine::set_uefi_credential_rotation_requested(&mut txn, machine_id).await?; } Mode::Clear => { db::machine::clear_uefi_credential_rotation_requested(&mut txn, machine_id).await?; } - // An omitted `mode` decodes as `Unspecified`; reject it rather than let a - // request fall through to an action it did not name. - Mode::Unspecified => { - return Err( - CarbideError::InvalidArgument("mode must be set or clear".to_string()).into(), - ); - } + Mode::Unspecified => unreachable!("rejected above"), };🤖 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/uefi_credential_rotation.rs` around lines 39 - 70, Validate req.mode() immediately after extracting the request and before calling api.txn_begin() or resolve_target, returning the existing InvalidArgument error for Mode::Unspecified. Then retain the Set/Clear match for database mutations without repeating the late Unspecified rejection.crates/credential-rotation/src/lib.rs (1)
563-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "BMC"-specific wording on the now-generalized
RotationGateAPI.
RotationGatewas generalized to be family-generic (struct doc at line 528 and constructors already reflect this), butrotation_pending's doc still says "Whether any BMC device currently lags the site-wide target, from the" androtation_needed's doc still says "Controller entry guard for one device:truewhen the device's BMC". Since this same gate now backsHostUefi(and future families), these BMC-specific references will mislead readers integrating a new family.📝 Proposed doc fix
- /// Whether *any* BMC device currently lags the site-wide target, from the + /// Whether *any* device currently lags the site-wide target for this gate's + /// configured credential family, from the /// cached aggregate (refreshed at most once per TTL window).- /// Controller entry guard for one device: `true` when the device's BMC - /// credential is behind the site-wide target and not quarantined, so the - /// controller should enter its BMC-rotation state. + /// Controller entry guard for one device: `true` when the device's + /// credential (for this gate's family) is behind the site-wide target and + /// not quarantined, so the controller should enter its rotation state.Also applies to: 597-603
🤖 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/credential-rotation/src/lib.rs` around lines 563 - 569, Update the documentation for RotationGate::rotation_pending and RotationGate::rotation_needed to use family-neutral terminology instead of “BMC” and “device.” Describe pending rotations and per-entry guards in terms of the generalized rotation family while preserving the existing target, pending, quarantined, and convergence behavior.crates/machine-controller/src/handler/host_uefi_rotation.rs (1)
174-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate into a table-driven test per repo convention.
These three tests all exercise
host_uefi_current_candidateswith different inputs and assert on the resultingVec<String>— exactly the shape the repo's coding guidelines call out forvalue_scenarios!/check_values. As per coding guidelines, "Use the carbide-test-support table-driven testing helpers for functions mapping inputs to outputs or errors; use scenarios! for Result, value_scenarios! for total operations, and direct check helpers when macros obscure intent." and "prefer table-driven tests using carbide-test-support scenarios such as scenarios!/value_scenarios! or explicit check_cases/check_values, especially for parsers, validators, and conversions."If the macros don't support async operations directly,
carbide-test-support'scheck_values/check_caseshelpers (mentioned in the same guideline) are the documented fallback.🤖 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-controller/src/handler/host_uefi_rotation.rs` around lines 174 - 220, Consolidate the three async tests for host_uefi_current_candidates into one table-driven test using the repository’s carbide-test-support value_scenarios!/check_values or documented async-compatible fallback. Keep each existing input combination and expected Vec<String> as a separate case, including ordering and deduplication behavior, and remove the redundant individual test functions.Source: Coding guidelines
crates/machine-controller/tests/integration/host_uefi_rotation.rs (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the unused iteration count.
All four call sites discard the returned
usize, and the trailing12is unreachable once the post-loop assertion holds. Returning()would remove the dead surface without changing behaviour.♻️ Proposed simplification
-async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) -> usize { - for iteration in 1..=12 { +async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) { + for _ in 1..=12 { if matches!(mh.host.machine().await.state.value, ManagedHostState::Ready) { - return iteration - 1; + return; } env.run_single_iteration().await; } assert!( matches!(mh.host.machine().await.state.value, ManagedHostState::Ready), "host UEFI rotation FSM did not return to Ready within the iteration budget, got {:?}", mh.host.machine().await.state.value, ); - 12 }🤖 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-controller/tests/integration/host_uefi_rotation.rs` around lines 114 - 127, Change run_until_ready to return () instead of usize, remove the iteration - 1 and trailing 12 return values, and retain the readiness loop and final assertion unchanged. Update all four call sites to use it without discarding a returned value.
🤖 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-controller/src/handler.rs`:
- Around line 5936-5971: Update set_rotating_host_uefi_password to pass the
already-read target from current_site_uefi_target into
resolve_site_uefi_credentials, and adjust that resolver’s signature and callers
as needed to accept and reuse the supplied target. Ensure candidate generation,
credential selection, and rotating_to_version all use the same target snapshot
during the rotation attempt.
In `@crates/redfish/src/libredfish/mod.rs`:
- Around line 313-336: Update the public trait method containing the
current-password candidate loop to validate current_password_candidates before
iteration and return RedfishClientCreationError::MissingArgument when it is
empty. Preserve the existing candidate-rotation behavior and last_err handling
for non-empty slices, removing the panic-prone expect assumption.
---
Nitpick comments:
In `@crates/api-core/src/handlers/uefi_credential_rotation.rs`:
- Around line 39-70: Validate req.mode() immediately after extracting the
request and before calling api.txn_begin() or resolve_target, returning the
existing InvalidArgument error for Mode::Unspecified. Then retain the Set/Clear
match for database mutations without repeating the late Unspecified rejection.
In `@crates/credential-rotation/src/lib.rs`:
- Around line 563-569: Update the documentation for
RotationGate::rotation_pending and RotationGate::rotation_needed to use
family-neutral terminology instead of “BMC” and “device.” Describe pending
rotations and per-entry guards in terms of the generalized rotation family while
preserving the existing target, pending, quarantined, and convergence behavior.
In `@crates/machine-controller/src/handler/host_uefi_rotation.rs`:
- Around line 174-220: Consolidate the three async tests for
host_uefi_current_candidates into one table-driven test using the repository’s
carbide-test-support value_scenarios!/check_values or documented
async-compatible fallback. Keep each existing input combination and expected
Vec<String> as a separate case, including ordering and deduplication behavior,
and remove the redundant individual test functions.
In `@crates/machine-controller/tests/integration/host_uefi_rotation.rs`:
- Around line 114-127: Change run_until_ready to return () instead of usize,
remove the iteration - 1 and trailing 12 return values, and retain the readiness
loop and final assertion unchanged. Update all four call sites to use it without
discarding a returned value.
🪄 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: 5f66b5da-f72c-4e3e-ba46-9d9fdd98c4dc
⛔ 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 (40)
crates/admin-cli/src/credential/force_uefi/args.rscrates/admin-cli/src/credential/force_uefi/cmd.rscrates/admin-cli/src/credential/force_uefi/mod.rscrates/admin-cli/src/credential/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/maintenance.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-db/migrations/20260731120000_uefi_credential_rotation_requested.sqlcrates/api-db/src/machine.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/test_support/machine_snapshot.rscrates/credential-rotation/src/lib.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/host_uefi_rotation.rscrates/machine-controller/tests/integration/main.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+5936
to
+5971
| async fn set_rotating_host_uefi_password( | ||
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | ||
| state: &ManagedHostStateSnapshot, | ||
| host_bmc_mac: mac_address::MacAddress, | ||
| redfish_client: &dyn Redfish, | ||
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | ||
| use db::credential_rotation::CredentialRotationType::HostUefi; | ||
|
|
||
| let db_pool = &ctx.services.db_pool; | ||
| let reader = ctx.services.redfish_client_pool.credential_reader(); | ||
|
|
||
| let target = current_site_uefi_target(db_pool, HostUefi).await?; | ||
|
|
||
| // The device's tracked current version selects the first authentication | ||
| // candidate; its prior attempt count sizes the backoff on failure. | ||
| let (current_version, prior_attempts) = { | ||
| let mut conn = db_pool.acquire().await?; | ||
| match db::credential_rotation::device_rotation_status(&mut conn, HostUefi, host_bmc_mac) | ||
| .await | ||
| .map_err(|e| { | ||
| StateHandlerError::GenericError(eyre!("read host uefi rotation status: {e}")) | ||
| })? { | ||
| Some(status) => ( | ||
| status.current_version.and_then(|v| u32::try_from(v).ok()), | ||
| status.rotate_attempts, | ||
| ), | ||
| None => (None, 0), | ||
| } | ||
| }; | ||
|
|
||
| let candidates = | ||
| host_uefi_rotation::host_uefi_current_candidates(reader, current_version, target).await?; | ||
| let Credentials::UsernamePassword { | ||
| password: new_password, | ||
| .. | ||
| } = resolve_site_uefi_credentials(db_pool, reader, HostUefi).await?; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the Redfish-layer rotation call probes/recovers when the
# device is already on a later candidate password (mirroring BMC's
# change_or_recover), which would bound the blast radius of the TOCTOU above.
rg -n -A 40 'fn rotate_host_uefi_password' crates/redfish/src/libredfish/mod.rsRepository: NVIDIA/infra-controller
Length of output: 2108
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rotation helper and call site ---'
sed -n '300,338p' crates/redfish/src/libredfish/mod.rs
sed -n '5936,6055p' crates/machine-controller/src/handler.rs
printf '%s\n' '--- credential resolution and rotation bookkeeping ---'
sed -n '5540,5610p' crates/machine-controller/src/handler.rs
rg -n -A 35 -B 15 'host_uefi_current_candidates|mark_device_rotating_to_version|finish_rotating_host_uefi|current_site_uefi_target' crates/machine-controller crates | head -n 260Repository: NVIDIA/infra-controller
Length of output: 35792
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Redfish UEFI password-change implementations ---'
rg -n -A 55 -B 15 'change_uefi_password' crates/redfish crates | head -n 320
printf '%s\n' '--- completion and version promotion ---'
sed -n '6040,6135p' crates/machine-controller/src/handler.rs
rg -n -A 45 -B 20 'mark_device_rotating_to_version|complete_device_rotation|current_version|rotating_to_version' crates/db crates | head -n 320Repository: NVIDIA/infra-controller
Length of output: 50379
Thread one UEFI target version through the rotation attempt.
The initial current_site_uefi_target read builds candidates and stages rotating_to_version, while resolve_site_uefi_credentials independently reads the target for the new password. A concurrent target advance can therefore apply v2 while recording v1. The Redfish fallback only retries password changes and does not guarantee recovery from this stale bookkeeping. Pass the already-read target into credential resolution.
🤖 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-controller/src/handler.rs` around lines 5936 - 5971, Update
set_rotating_host_uefi_password to pass the already-read target from
current_site_uefi_target into resolve_site_uefi_credentials, and adjust that
resolver’s signature and callers as needed to accept and reuse the supplied
target. Ensure candidate generation, credential selection, and
rotating_to_version all use the same target snapshot during the rotation
attempt.
Comment on lines
+313
to
+336
| let mut last_err = None; | ||
| for candidate in current_password_candidates { | ||
| match client | ||
| .change_uefi_password(candidate.as_str(), new_password.as_str()) | ||
| .await | ||
| { | ||
| Ok(job_id) => return Ok(job_id), | ||
| Err(e) => { | ||
| let redacted = | ||
| redact_passwords(e, &[new_password.as_str(), candidate.as_str()]); | ||
| tracing::warn!( | ||
| error = %redacted, | ||
| "host UEFI password change failed for a current-password candidate; trying the next" | ||
| ); | ||
| last_err = Some(redacted); | ||
| } | ||
| } | ||
| } | ||
| // The caller contract guarantees at least one candidate (empty string | ||
| // for a never-set host), so `last_err` is populated whenever the loop | ||
| // fell through without an Ok. | ||
| Err(RedfishClientCreationError::RedfishError(last_err.expect( | ||
| "rotate_host_uefi_password requires at least one current-password candidate", | ||
| ))) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect host_uefi_current_candidates and every rotate_host_uefi_password call site.
fd -t f 'host_uefi_rotation.rs' crates
ast-grep run --pattern 'pub(crate) async fn host_uefi_current_candidates($$$) { $$$ }' --lang rust crates || true
rg -nP -C 8 '\bhost_uefi_current_candidates\s*\(' crates
rg -nP -C 5 '\brotate_host_uefi_password\s*\(' cratesRepository: NVIDIA/infra-controller
Length of output: 10249
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate resolver ---'
cat -n crates/machine-controller/src/handler/host_uefi_rotation.rs | sed -n '90,165p'
printf '%s\n' '--- rotation call context ---'
cat -n crates/machine-controller/src/handler.rs | sed -n '5925,6005p'
printf '%s\n' '--- trait visibility and error definition ---'
rg -n -C 8 'rotate_host_uefi_password|enum RedfishClientCreationError|MissingArgument' crates/redfish crates/machine-controllerRepository: NVIDIA/infra-controller
Length of output: 13829
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- trait and implementation context ---'
cat -n crates/redfish/src/libredfish/mod.rs | sed -n '220,345p'
printf '%s\n' '--- all declarations and call sites ---'
rg -n -C 3 'trait .*Redfish|impl .*Redfish|rotate_host_uefi_password' crates/redfish crates/machine-controller
printf '%s\n' '--- standalone invariant probe ---'
python3 - <<'PY'
from itertools import product
# Model the resolver's control flow for all combinations of optional current
# version and target version. Each resolver result appends the factory-password
# fallback unconditionally.
for current_version, target_version in product([None, 0, 1], [0, 1]):
versions = []
if current_version is not None:
versions.append(current_version)
if target_version not in versions:
versions.append(target_version)
candidates = [f"secret:{v}" for v in versions] + [""]
assert candidates, (current_version, target_version)
assert candidates[-1] == ""
print("all modeled resolver inputs produce at least one candidate")
PYRepository: NVIDIA/infra-controller
Length of output: 19184
Guard the trait boundary against empty candidate slices.
Although host_uefi_current_candidates always appends the empty-password fallback, callers can still pass an empty slice to this public trait method. Return MissingArgument instead of allowing expect to panic.
🤖 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/redfish/src/libredfish/mod.rs` around lines 313 - 336, Update the
public trait method containing the current-password candidate loop to validate
current_password_candidates before iteration and return
RedfishClientCreationError::MissingArgument when it is empty. Preserve the
existing candidate-rotation behavior and last_err handling for non-empty slices,
removing the panic-prone expect assumption.
spydaNVIDIA
force-pushed
the
host_uefi_credential_rotation
branch
from
July 31, 2026 05:43
70e6145 to
02a8e6a
Compare
Contributor
Author
|
@coderabbitai full review please |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/machine-controller/tests/integration/host_uefi_rotation.rs (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
run_until_readyreturns an iteration count that no caller uses.The helper returns
usize, but all four call sites discard it. The value is also inconsistent: the loop returnsiteration - 1, while the post-loop path returns the literal12. Return()and drop the counter, or assert on the count in at least one test to give it a purpose.♻️ Proposed simplification
-/// Advance the controller until the host settles back in Ready, returning the -/// number of iterations taken. Bounded so a wedged FSM fails loudly instead of -/// hanging. -async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) -> usize { - for iteration in 1..=12 { +/// Advance the controller until the host settles back in Ready. Bounded so a +/// wedged FSM fails loudly instead of hanging. +async fn run_until_ready(env: &mut Env, mh: &TestManagedHost) { + for _ in 1..=12 { if matches!(mh.host.machine().await.state.value, ManagedHostState::Ready) { - return iteration - 1; + return; } env.run_single_iteration().await; } assert!( matches!(mh.host.machine().await.state.value, ManagedHostState::Ready), "host UEFI rotation FSM did not return to Ready within the iteration budget, got {:?}", mh.host.machine().await.state.value, ); - 12 }🤖 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-controller/tests/integration/host_uefi_rotation.rs` around lines 114 - 127, Update the run_until_ready helper to return unit instead of usize, remove the iteration-count return values, and retain the readiness assertion after the iteration budget is exhausted. Ensure all four call sites continue invoking the helper without handling a result.crates/machine-controller/src/handler/host_uefi_rotation.rs (1)
188-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a table-driven form for the three candidate tests.
The three tests call
host_uefi_current_candidateswith different inputs and expect different outputs. The repository standard is to use a table for that shape. Avalue_scenarios!-style table with a small seeding closure would keep the three cases in one place and make a fourth case cheap to add.As per coding guidelines: "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone 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-controller/src/handler/host_uefi_rotation.rs` around lines 188 - 234, Consolidate the three tests for host_uefi_current_candidates into a table-driven test using the repository’s value_scenarios!-style pattern and a small per-case seeding closure. Preserve each scenario’s inputs and expected candidate ordering, including deduplication when current equals target, while keeping the shared invocation and assertions in one test.Source: Coding guidelines
crates/machine-controller/src/io.rs (1)
344-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting the UEFI rotation sub-state.
RotatingHostUefiis a multi-tick state, but this mapping emits an empty sub-state. Operators then cannot tell whether a rotation is stuck atSetUefiPassword,WaitForPasswordJobCompletion, or elsewhere. Other multi-step states, such asBootConfiguring, emit a sub-state name. A smalluefi_setup_state_namehelper would close that gap without changing behavior.🤖 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-controller/src/io.rs` at line 344, Update the ManagedHostState::RotatingHostUefi mapping to report its current UEFI rotation sub-state instead of an empty string. Add a small uefi_setup_state_name helper that maps SetUefiPassword, WaitForPasswordJobCompletion, and the other relevant UEFI setup states to stable names, then use it in this mapping without changing state behavior.crates/api-core/src/handlers/uefi_credential_rotation.rs (1)
78-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing target-resolution logic with the BMC handler.
resolve_targetclosely mirrors the doc-referenced BMC pattern ("Unlike a BMC, a UEFI credential only ever belongs to a machine"). Ifcrates/api-core/src/handlers/bmc_credential_rotation.rshas near-identical id/MAC resolution logic, extracting a shared helper would reduce duplication between the two handlers.#!/bin/bash # Description: Compare resolve_target-style logic between BMC and UEFI credential rotation handlers. fd bmc_credential_rotation.rs crates/api-core --exec cat -n {}🤖 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/uefi_credential_rotation.rs` around lines 78 - 129, Compare resolve_target in the UEFI handler with the target-resolution logic in the BMC credential rotation handler. Extract the shared machine_id/BMC-MAC validation and reconciliation flow into a common helper, then update both handlers to reuse it while preserving their existing errors and behavior. Keep handler-specific resolution rules outside the shared helper.
🤖 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/force_uefi/cmd.rs`:
- Around line 39-44: Update the `println!` guidance in the force-converge
command to use the valid `rotation-status --type=host-uefi` value, while
preserving the existing `--mac-address` option and all other message content.
In `@crates/api-core/src/handlers/uefi_credential_rotation.rs`:
- Line 49: Validate the resolved target immediately after `resolve_target` in
the UEFI credential rotation handler and reject it when
`machine_type().is_dpu()` is true, returning the handler’s established
`NicoError` for unsupported requests. Keep host rotation behavior unchanged and
ensure the rejection occurs before `set_uefi_credential_rotation_requested` can
persist any state.
---
Nitpick comments:
In `@crates/api-core/src/handlers/uefi_credential_rotation.rs`:
- Around line 78-129: Compare resolve_target in the UEFI handler with the
target-resolution logic in the BMC credential rotation handler. Extract the
shared machine_id/BMC-MAC validation and reconciliation flow into a common
helper, then update both handlers to reuse it while preserving their existing
errors and behavior. Keep handler-specific resolution rules outside the shared
helper.
In `@crates/machine-controller/src/handler/host_uefi_rotation.rs`:
- Around line 188-234: Consolidate the three tests for
host_uefi_current_candidates into a table-driven test using the repository’s
value_scenarios!-style pattern and a small per-case seeding closure. Preserve
each scenario’s inputs and expected candidate ordering, including deduplication
when current equals target, while keeping the shared invocation and assertions
in one test.
In `@crates/machine-controller/src/io.rs`:
- Line 344: Update the ManagedHostState::RotatingHostUefi mapping to report its
current UEFI rotation sub-state instead of an empty string. Add a small
uefi_setup_state_name helper that maps SetUefiPassword,
WaitForPasswordJobCompletion, and the other relevant UEFI setup states to stable
names, then use it in this mapping without changing state behavior.
In `@crates/machine-controller/tests/integration/host_uefi_rotation.rs`:
- Around line 114-127: Update the run_until_ready helper to return unit instead
of usize, remove the iteration-count return values, and retain the readiness
assertion after the iteration budget is exhausted. Ensure all four call sites
continue invoking the helper without handling a result.
🪄 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: 40c5619b-3364-41fc-ac52-398224cc4b1e
⛔ 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 (40)
crates/admin-cli/src/credential/force_uefi/args.rscrates/admin-cli/src/credential/force_uefi/cmd.rscrates/admin-cli/src/credential/force_uefi/mod.rscrates/admin-cli/src/credential/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/maintenance.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-db/migrations/20260731120000_uefi_credential_rotation_requested.sqlcrates/api-db/src/machine.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/test_support/machine_snapshot.rscrates/credential-rotation/src/lib.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/host_uefi_rotation.rscrates/machine-controller/tests/integration/main.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+39
to
+44
| println!( | ||
| "Requested force-converge of {target}'s UEFI credential. The state controller rotates it \ | ||
| on its next sweep (a BIOS job plus a host power-cycle, bypassing backoff); confirm this \ | ||
| device converged with `credential rotation-status --type=host_uefi --mac-address \ | ||
| <bmc-mac>` (the per-device query, not the site-wide view).", | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the rotation-status command's flag names and accepted --type values.
fd rotation_status.rs crates/admin-cli --exec cat -n {}
rg -n 'HostUefi|host_uefi' crates/credential-rotation/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -i 'rotation.*status|status.*rotation|credential' crates/admin-cli -t f | head -200
printf '%s\n' '--- command declarations and flags ---'
rg -n -i 'rotation[-_ ]status|mac[-_]address|host_uefi|HostUefi|struct Args|derive\(.*Args' crates/admin-cli crates -g '*.rs' | head -300Repository: NVIDIA/infra-controller
Length of output: 29413
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- admin-cli credential files ---'
git ls-files 'crates/admin-cli/**' | rg -i 'credential|rotation|force_uefi'
printf '%s\n' '--- credential command references ---'
rg -n -i 'credential|rotation-status|rotation_status|force_uefi|force-uefi' crates/admin-cli/src crates/admin-cli/Cargo.toml -g '*.rs' -g '*.toml'
printf '%s\n' '--- force_uefi source ---'
fd -i 'force_uefi|credential' crates/admin-cli -t f -x sh -c 'echo "--- $1"; cat -n "$1"' sh {} \; | head -500Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- rotation-status arguments ---'
cat -n crates/admin-cli/src/credential/rotation_status/args.rs
printf '%s\n' '--- rotation credential value parsing ---'
sed -n '45,80p' crates/admin-cli/src/credential/common.rs
printf '%s\n' '--- rotation-status parser tests ---'
sed -n '285,330p' crates/admin-cli/src/credential/tests.rsRepository: NVIDIA/infra-controller
Length of output: 5040
Use the valid --type value. rotation-status accepts host-uefi, not host_uefi; keep --mac-address unchanged.
🤖 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/credential/force_uefi/cmd.rs` around lines 39 - 44,
Update the `println!` guidance in the force-converge command to use the valid
`rotation-status --type=host-uefi` value, while preserving the existing
`--mac-address` option and all other message content.
Source: Learnings
|
|
||
| let mut txn = api.txn_begin().await?; | ||
|
|
||
| let machine_id = resolve_target(&mut txn, req.machine_id, req.bmc_mac).await?; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject DPU targets until DPU UEFI rotation ships.
resolve_target accepts any resolved MachineId, including a DPU. The proto doc for UefiCredentialRotationRequest states a UEFI credential belongs to "a host, or a DPU once DPU UEFI rotation ships," but only host rotation is implemented in this PR. If an operator targets a DPU today, set_uefi_credential_rotation_requested persists the flag, and no state machine ever consumes or clears it. The request looks accepted, but it silently never converges, with no error to the operator.
Add an explicit check and reject DPU machine IDs now, using the same machine_type().is_dpu() accessor already used in crates/api-model/src/test_support/machine_snapshot.rs.
🛡️ Proposed fix to reject unsupported DPU targets
let machine_id = resolve_target(&mut txn, req.machine_id, req.bmc_mac).await?;
+ if machine_id.machine_type().is_dpu() {
+ return Err(CarbideError::InvalidArgument(
+ "the requested machine is a DPU; UEFI credential rotation for DPUs is not supported yet"
+ .to_string(),
+ )
+ .into());
+ }
+
match mode {As per path instructions for crates/api*/**: "Review API, model, database, and web changes for request validation, authorization boundaries, transaction safety, SQLx/query correctness, schema/API compatibility, tenant isolation, NicoError-based handler errors, and safe request logging."
Also applies to: 78-129
🤖 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/uefi_credential_rotation.rs` at line 49,
Validate the resolved target immediately after `resolve_target` in the UEFI
credential rotation handler and reject it when `machine_type().is_dpu()` is
true, returning the handler’s established `NicoError` for unsupported requests.
Keep host rotation behavior unchanged and ensure the rejection occurs before
`set_uefi_credential_rotation_requested` can persist any state.
Source: Path instructions
spydaNVIDIA
force-pushed
the
host_uefi_credential_rotation
branch
from
July 31, 2026 06:59
02a8e6a to
726020c
Compare
10 tasks
kensimon
approved these changes
Jul 31, 2026
kensimon
left a comment
kensimon
left a comment
Contributor
There was a problem hiding this comment.
Mostly just nitpicks here. I'll approve since they can be addressed in a followup, but if you find yourself already making more changes before your next push feel free to address.
spydaNVIDIA
force-pushed
the
host_uefi_credential_rotation
branch
from
July 31, 2026 17:23
726020c to
808a157
Compare
spydaNVIDIA
enabled auto-merge (squash)
July 31, 2026 17:23
Contributor
Author
|
@coderabbitai full review please |
Contributor
|
✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4424.docs.buildwithfern.com/infra-controller |
spydaNVIDIA
force-pushed
the
host_uefi_credential_rotation
branch
from
July 31, 2026 17:33
808a157 to
8aa4d20
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rs (1)
219-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated setup into the existing
stage_lagging_pmchelper.
ready_power_shelf_converges_pmc_to_site_targetrepeats, line for line, the staging steps already factored intostage_lagging_pmc(lines 180-198): seed the per-device secret, record convergence at v0, advance the target, and stage the rotate-to secret. Reuse the helper here as the other tests do. Duplicated setup logic drifts silently when one copy changes and the other does not.♻️ Proposed refactor
- // The PMC currently holds the per-device "old" secret. - env.redfish_sim.seed_user("root", "old"); - env.test_credential_manager - .set_credentials(&per_device_key(pmc_mac), &creds("root", "old")) - .await - .expect("staging the per-device secret should succeed"); - - // Stage a site-wide rotation to version 1: record the device converged at - // the v0 baseline, advance the target, and write the rotate-to secret that - // `RotateCredential` would have staged. - { - let mut conn = pool.acquire().await?; - record_device_converged(&mut conn, pmc_mac, BMC).await?; - set_next_target_version(&mut conn, BMC, 0, serde_json::json!({})) - .await? - .expect("target must advance from version 0"); - } - env.test_credential_manager - .set_credentials(&rotate_to_key(1), &creds("root", "new")) - .await - .expect("staging the rotate-to secret should succeed"); + stage_lagging_pmc(&env, &pool, pmc_mac).await?;🤖 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/tests/power_shelf_state_controller/bmc_rotation.rs` around lines 219 - 343, Refactor ready_power_shelf_converges_pmc_to_site_target to call the existing stage_lagging_pmc helper for all PMC lagging-state setup, including seeding the per-device secret, recording v0 convergence, advancing the target, and staging the rotate-to secret. Remove the duplicated inline setup while preserving the test’s subsequent pre-controller lag assertion and behavior.crates/machine-controller/src/handler/host_uefi_rotation.rs (2)
279-394: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
i32::try_frominstead oftarget as i32for the staged rotation version.Line 323 casts
target: u32toi32withas, which truncates silently iftargetexceedsi32::MAX. Line 302, a few lines earlier in the same function, already uses the safeu32::try_from(v).ok()pattern for the reverse conversion, and the BMC engine'srotate_bmc(crates/credential-rotation/src/lib.rs) usesu32::try_from(status.target_version).map_err(...)for the same kind of version conversion. Use the same fallible-conversion pattern here for consistency and to avoid a silently wrong staged version on overflow.As per coding guidelines: "Prefer
From/TryFromandFromStrconversions over bespoke conversion methods."♻️ Proposed fix
{ let mut conn = db_pool.acquire().await?; + let target_i32 = i32::try_from(target).map_err(|_| { + StateHandlerError::GenericError(eyre!( + "host uefi target version {target} does not fit in i32" + )) + })?; db::credential_rotation::mark_device_rotating_to_version( &mut conn, host_bmc_mac, HostUefi, - target as i32, + target_i32, ) .await .map_err(|e| { StateHandlerError::GenericError(eyre!("stage host uefi rotating_to_version: {e}")) })?; }🤖 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-controller/src/handler/host_uefi_rotation.rs` around lines 279 - 394, In set_rotating_host_uefi_password, replace the unchecked target as i32 conversion in mark_device_rotating_to_version with an i32::try_from(target) conversion. Propagate or map the conversion failure through StateHandlerError before staging the rotation, preserving the existing database call and error-handling flow.
458-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three tests into one asynchronous table-driven test.
Use
carbide_test_support::check_cases_asyncwith fresh seed data and expected candidates per case.value_scenarios!is not suitable becausehost_uefi_current_candidatesreturns an asynchronousResult.🤖 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-controller/src/handler/host_uefi_rotation.rs` around lines 458 - 529, Replace the three separate async tests in the tests module with one table-driven async test using carbide_test_support::check_cases_async. Define cases containing fresh seed data, current version, target version, and expected candidates, then seed a new MemoryCredentialStore for each case and invoke host_uefi_current_candidates through the async checker. Preserve all existing scenarios and assertions, and do not use value_scenarios! because the function returns an asynchronous Result.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rs`:
- Around line 219-343: Refactor ready_power_shelf_converges_pmc_to_site_target
to call the existing stage_lagging_pmc helper for all PMC lagging-state setup,
including seeding the per-device secret, recording v0 convergence, advancing the
target, and staging the rotate-to secret. Remove the duplicated inline setup
while preserving the test’s subsequent pre-controller lag assertion and
behavior.
In `@crates/machine-controller/src/handler/host_uefi_rotation.rs`:
- Around line 279-394: In set_rotating_host_uefi_password, replace the unchecked
target as i32 conversion in mark_device_rotating_to_version with an
i32::try_from(target) conversion. Propagate or map the conversion failure
through StateHandlerError before staging the rotation, preserving the existing
database call and error-handling flow.
- Around line 458-529: Replace the three separate async tests in the tests
module with one table-driven async test using
carbide_test_support::check_cases_async. Define cases containing fresh seed
data, current version, target version, and expected candidates, then seed a new
MemoryCredentialStore for each case and invoke host_uefi_current_candidates
through the async checker. Preserve all existing scenarios and assertions, and
do not use value_scenarios! because the function returns an asynchronous Result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4e31a7e-8d25-4c88-a96d-f76e06e235b5
⛔ 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 (47)
crates/admin-cli/src/credential/force_uefi/args.rscrates/admin-cli/src/credential/force_uefi/cmd.rscrates/admin-cli/src/credential/force_uefi/mod.rscrates/admin-cli/src/credential/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rscrates/api-core/src/tests/power_shelf_state_controller/error_state.rscrates/api-core/src/tests/power_shelf_state_controller/maintenance.rscrates/api-core/src/tests/power_shelf_state_controller/mod.rscrates/api-core/src/tests/power_shelf_state_controller/reprovisioning.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/maintenance.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-db/migrations/20260731120000_uefi_credential_rotation_requested.sqlcrates/api-db/src/machine.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/test_support/machine_snapshot.rscrates/credential-rotation/src/lib.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/host_uefi_rotation.rscrates/machine-controller/tests/integration/main.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/rotating_bmc.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (38)
- crates/api-core/src/handlers/mod.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/api-core/src/cfg/README.md
- crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs
- crates/machine-controller/src/handler/host_boot_config.rs
- crates/api-db/migrations/20260731120000_uefi_credential_rotation_requested.sql
- crates/api-core/src/test_support/default_config.rs
- crates/admin-cli/src/credential/mod.rs
- crates/api-core/src/setup.rs
- crates/api-model/src/machine/slas.rs
- crates/api-core/src/tests/switch_state_controller/bmc_rotation.rs
- crates/machine-controller/tests/integration/env.rs
- crates/api-model/src/machine/json.rs
- crates/admin-cli/src/credential/force_uefi/mod.rs
- crates/machine-controller/src/io.rs
- crates/switch-controller/src/context.rs
- crates/api-core/src/api.rs
- crates/api-core/src/handlers/uefi_credential_rotation.rs
- crates/machine-controller/src/handler/rotation.rs
- crates/switch-controller/src/rotating_bmc.rs
- crates/api-core/src/tests/switch_state_controller/maintenance.rs
- crates/api-core/src/tests/switch_state_controller/mod.rs
- crates/admin-cli/src/credential/force_uefi/args.rs
- crates/api-core/src/cfg/file.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-model/src/test_support/machine_snapshot.rs
- crates/machine-controller/tests/integration/host_uefi_rotation.rs
- crates/machine-controller/tests/integration/main.rs
- crates/machine-controller/src/config/mod.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/machine-controller/src/context.rs
- crates/api-db/src/machine.rs
- crates/redfish/src/libredfish/mod.rs
- crates/redfish/src/libredfish/test_support.rs
- crates/admin-cli/src/credential/force_uefi/cmd.rs
- crates/rpc/proto/forge.proto
- crates/machine-controller/src/handler.rs
- crates/credential-rotation/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/machine-controller/src/handler.rs (1)
6820-6827: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the outer
redfish_clientinstead of creating a second one.
handle_host_uefi_setupalready buildsredfish_clientat the top of the function (line 6699). This arm builds a second client for the same machine. The extra construction adds an avoidable round of client setup and creates two divergent code paths for the same connection.♻️ Proposed simplification
if let Some(job_id) = uefi_setup_info.uefi_password_jid.as_ref() { - let redfish_client = ctx - .services - .create_redfish_client_from_machine(&state.host_snapshot) - .await?; - let job_state = redfish_client .get_job_state(job_id)🤖 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-controller/src/handler.rs` around lines 6820 - 6827, In handle_host_uefi_setup, remove the inner create_redfish_client_from_machine call within the uefi_password_jid branch and reuse the redfish_client already initialized at the function's top. Keep the existing get_job_state flow and error propagation unchanged.crates/machine-controller/src/handler/host_uefi_rotation.rs (1)
482-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the candidate tests into a table-driven scenario set.
The three tests call
host_uefi_current_candidateswith different(current_version, target_version)inputs and assert the returned vector. The coding guidelines require table-driven tests throughcarbide-test-supportfor exactly this shape, usingvalue_scenarios!for total operations orcheck_valueswhere the macro would obscure intent. A table also makes the seeded secret set and the expected ordering visible in one place.As per coding guidelines: "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone tests" and "When writing tests, prefer table-driven tests using
carbide-test-supportscenarios such asscenarios!/value_scenarios!or explicitcheck_cases/check_values, especially for parsers, validators, and conversions."🤖 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-controller/src/handler/host_uefi_rotation.rs` around lines 482 - 528, Replace the three separate candidate tests with one table-driven test using carbide-test-support’s value_scenarios! or check_values pattern. Define each case’s seeded credentials, current_version, target_version, and expected candidate ordering in the table, then invoke host_uefi_current_candidates for every case and assert the result; preserve all three existing scenarios, including deduplication when versions match.Source: Coding guidelines
crates/credential-rotation/src/lib.rs (1)
607-659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale "BMC"-only wording in the method docs.
The doc comment on
rotation_pending(Line 607) says "Whether any BMC device currently lags the site-wide target." The doc comment onrotation_needed(Lines 641-643) says "the device's BMC credential is behind the site-wide target... enter its BMC-rotation state." Both methods now readself.family(Line 631, Line 657), an arbitraryCredentialRotationType. The struct-level docs at Lines 552-558 and 572-576 already describe the family-generic design. Update these two method docstrings to describe the gate's configured family instead of hard-coding "BMC," so a reader who consultsuefi_rotation_gateis not misled into thinking it only reports on BMC devices.♻️ Proposed doc fix
- /// Whether *any* BMC device currently lags the site-wide target, from the + /// Whether *any* device of the gate's configured credential family currently + /// lags the site-wide target, from the /// cached aggregate (refreshed at most once per TTL window).- /// Controller entry guard for one device: `true` when the device's BMC - /// credential is behind the site-wide target and not quarantined, so the - /// controller should enter its BMC-rotation state. + /// Controller entry guard for one device: `true` when the device's + /// credential for the gate's configured family is behind the site-wide + /// target and not quarantined, so the controller should enter that + /// family's rotation state.🤖 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/credential-rotation/src/lib.rs` around lines 607 - 659, Update the doc comments for rotation_pending and rotation_needed to describe credentials and rotation state for the gate’s configured CredentialRotationType family, removing BMC-specific wording. Preserve the existing behavior and clarify that rotation_needed applies to the configured family rather than only BMC credentials.
🤖 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/force_uefi/cmd.rs`:
- Around line 39-43: Update the force-converge status message in the UEFI
credential command to identify the machine controller as the component that
rotates the credential on its next sweep, replacing the incorrect
state-controller reference while preserving the remaining operational guidance.
In `@rest-api/proto/core/src/v1/nico_nico.proto`:
- Around line 388-397: Update TriggerUefiCredentialRotation to reject requests
whose resolved target is a DPU, allowing only host MachineId targets to proceed
with Set or Clear. Preserve the existing host request behavior and return the
established invalid-target error without persisting flags for rejected DPU
requests.
---
Nitpick comments:
In `@crates/credential-rotation/src/lib.rs`:
- Around line 607-659: Update the doc comments for rotation_pending and
rotation_needed to describe credentials and rotation state for the gate’s
configured CredentialRotationType family, removing BMC-specific wording.
Preserve the existing behavior and clarify that rotation_needed applies to the
configured family rather than only BMC credentials.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6820-6827: In handle_host_uefi_setup, remove the inner
create_redfish_client_from_machine call within the uefi_password_jid branch and
reuse the redfish_client already initialized at the function's top. Keep the
existing get_job_state flow and error propagation unchanged.
In `@crates/machine-controller/src/handler/host_uefi_rotation.rs`:
- Around line 482-528: Replace the three separate candidate tests with one
table-driven test using carbide-test-support’s value_scenarios! or check_values
pattern. Define each case’s seeded credentials, current_version, target_version,
and expected candidate ordering in the table, then invoke
host_uefi_current_candidates for every case and assert the result; preserve all
three existing scenarios, including deduplication when versions match.
🪄 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: e9c753c7-e4d2-426f-b8eb-8453b2ea28c0
⛔ 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 (47)
crates/admin-cli/src/credential/force_uefi/args.rscrates/admin-cli/src/credential/force_uefi/cmd.rscrates/admin-cli/src/credential/force_uefi/mod.rscrates/admin-cli/src/credential/mod.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/uefi_credential_rotation.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/power_shelf_state_controller/bmc_rotation.rscrates/api-core/src/tests/power_shelf_state_controller/error_state.rscrates/api-core/src/tests/power_shelf_state_controller/maintenance.rscrates/api-core/src/tests/power_shelf_state_controller/mod.rscrates/api-core/src/tests/power_shelf_state_controller/reprovisioning.rscrates/api-core/src/tests/switch_state_controller/bmc_rotation.rscrates/api-core/src/tests/switch_state_controller/maintenance.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-db/migrations/20260731143022_uefi_credential_rotation_requested.sqlcrates/api-db/src/machine.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/test_support/machine_snapshot.rscrates/credential-rotation/src/lib.rscrates/machine-controller/src/config/mod.rscrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/host_boot_config.rscrates/machine-controller/src/handler/host_uefi_rotation.rscrates/machine-controller/src/handler/rotation.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/host_uefi_rotation.rscrates/machine-controller/tests/integration/main.rscrates/power-shelf-controller/src/context.rscrates/power-shelf-controller/src/rotating_bmc.rscrates/redfish/src/libredfish/mod.rscrates/redfish/src/libredfish/test_support.rscrates/rpc/proto/forge.protocrates/switch-controller/src/context.rscrates/switch-controller/src/rotating_bmc.rsrest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+388
to
+397
| // Operator "force-converge this UEFI credential now" escape hatch for a single | ||
| // host (DPU support follows). This is asynchronous: the handler only persists | ||
| // (Set) or removes (Clear) the machine's `uefi_credential_rotation_requested` | ||
| // flag and returns; it performs no rotation itself. A later machine-controller | ||
| // sweep observes a set flag and rotates the UEFI password (a BIOS job plus a | ||
| // host power-cycle), bypassing the passive site-wide gate and the device's | ||
| // backoff quarantine, then clears the flag once it converges. Clear only | ||
| // withdraws a not-yet-consumed request -- it does not undo or reset any UEFI | ||
| // credential that a prior sweep already rotated. | ||
| rpc TriggerUefiCredentialRotation(UefiCredentialRotationRequest) returns (google.protobuf.Empty); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether resolve_target or trigger_uefi_credential_rotation validates machine type.
rg -n -A5 -B5 'fn resolve_target' crates/api-core/src/handlers/uefi_credential_rotation.rs
rg -n 'is_dpu|machine_type' crates/api-core/src/handlers/uefi_credential_rotation.rsRepository: NVIDIA/infra-controller
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- handler outline ---'
ast-grep outline crates/api-core/src/handlers/uefi_credential_rotation.rs
printf '%s\n' '--- resolver and handler implementation ---'
sed -n '1,240p' crates/api-core/src/handlers/uefi_credential_rotation.rs
printf '%s\n' '--- all relevant call sites and machine-type checks ---'
rg -n -A12 -B8 \
'resolve_target|trigger_uefi_credential_rotation|is_dpu|machine_type|MachineType' \
crates/api-core/src rest-api/proto/core/src/v1/nico_nico.proto
printf '%s\n' '--- referenced protobuf messages ---'
sed -n '5900,5960p' rest-api/proto/core/src/v1/nico_nico.proto
sed -n '340,410p' rest-api/proto/core/src/v1/nico_nico.protoRepository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- all flag consumers and writers ---'
rg -n -A8 -B8 \
'uefi_credential_rotation_requested|should_enter_host_uefi_rotation' \
crates db model 2>/dev/null || true
printf '%s\n' '--- machine-state-handler target filtering ---'
rg -n -A10 -B10 \
'machine_type\(\)\.is_host|is_host\(\)|ManagedHostState|MachineStateHandler' \
crates/api-core/src/machine_state_handler crates/api-core/src 2>/dev/null \
| rg -n -C3 'uefi|rotation|is_host|MachineStateHandler|ManagedHostState' \
| head -300
printf '%s\n' '--- focused behavioral evidence ---'
python3 - <<'PY'
from pathlib import Path
roots = [Path("crates"), Path("db"), Path("model")]
needles = (
"uefi_credential_rotation_requested",
"should_enter_host_uefi_rotation",
)
for root in roots:
if not root.exists():
continue
for path in root.rglob("*"):
if path.is_file() and path.suffix in {".rs", ".sql"}:
text = path.read_text(errors="replace")
for i, line in enumerate(text.splitlines(), 1):
if any(n in line for n in needles):
print(f"{path}:{i}:{line.strip()}")
PYRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- DPU UEFI rotation implementation ---'
rg -n -A12 -B12 \
'RotatingDpuUefi|DpuUefi|dpu.*uefi|uefi.*dpu' \
crates/machine-controller crates/api-model crates/api-db \
| head -500
printf '%s\n' '--- state transitions involving the force flag ---'
sed -n '820,865p' crates/api-model/src/machine/mod.rs
sed -n '1060,1135p' crates/machine-controller/src/handler.rs
sed -n '1,130p' crates/machine-controller/src/handler/host_uefi_rotation.rs
printf '%s\n' '--- DPU state-handler modules and builder wiring ---'
rg -n -A8 -B8 \
'dpu.*state|DpuInit|RotatingDpu|state_handler.*dpu|dpu.*handler' \
crates/machine-controller/src \
| head -500Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- every RotatingDpuUefi reference ---'
rg -n -A20 -B20 'RotatingDpuUefi' crates/machine-controller/src crates/api-model/src
printf '%s\n' '--- every read and clear of the request flag in machine-controller ---'
rg -n -A10 -B10 \
'uefi_credential_rotation_requested' \
crates/machine-controller/src
printf '%s\n' '--- DPU state enum and transition logic ---'
rg -n -A35 -B20 \
'enum DpuInitState|impl DpuInitState|DpuInitState::' \
crates/api-model/src crates/machine-controller/src/handler.rs \
| rg -n -C4 'RotatingDpuUefi|uefi_credential_rotation_requested|DpuInitState|next_state' \
| head -400Repository: NVIDIA/infra-controller
Length of output: 46599
Reject DPU targets in TriggerUefiCredentialRotation.
resolve_target accepts any MachineId, but the controller only consumes this flag for hosts. A Set request for a DPU persists indefinitely because no DPU rotation state consumes or clears it. Reject non-host targets until DPU UEFI rotation is implemented.
🤖 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 `@rest-api/proto/core/src/v1/nico_nico.proto` around lines 388 - 397, Update
TriggerUefiCredentialRotation to reject requests whose resolved target is a DPU,
allowing only host MachineId targets to proceed with Set or Clear. Preserve the
existing host request behavior and return the established invalid-target error
without persisting flags for rejected DPU requests.
spydaNVIDIA
force-pushed
the
host_uefi_credential_rotation
branch
from
July 31, 2026 20:02
8aa4d20 to
6efa17a
Compare
aswaroop-nv
approved these changes
Jul 31, 2026
10 tasks
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
3 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.
Converge a Ready pool host's UEFI (BIOS) password to the staged site-wide target via a new top-level RotatingHostUefi state, mirroring RotatingBmc. Because applying a UEFI password needs a BIOS job plus a full host power-cycle, the state carries the multi-tick UefiSetupInfo FSM (unlock -> set -> job -> power-cycle -> poll -> re-lock -> record), reusing
the ingestion setup flow. Passwords are site-wide uniform per version, so the current credential is derived from the tracked version (no per-device secret): the SET step walks current -> target -> empty candidates, stages rotating_to_version crash-safely, and quarantines with backoff on failure so a host never wedges.
Related issues
#367
Type of Change
Breaking Changes
Testing
Additional Notes