feat(nvos): support password rotation#3706
Open
jayzhudev wants to merge 9 commits into
Open
Conversation
jayzhudev
added
the
rack lifecycle
Contributor
Summary by CodeRabbit
WalkthroughNVOS credential rotation is enabled end to end, including version-zero target staging, verified credential readback, repeat-safe component-manager dispatch, retry-fenced database state, and switch-controller reconciliation. Expected-switch updates now support validated sparse patches. ChangesNVOS credential rotation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant API
participant CredentialStore
participant SwitchController
participant ComponentManager
AdminCLI->>API: publish NVOS target
API->>CredentialStore: stage versioned secret
API->>SwitchController: enqueue switch reconciliation
SwitchController->>CredentialStore: resolve current credentials
SwitchController->>ComponentManager: ensure current-to-target rotation
ComponentManager-->>SwitchController: job status
SwitchController->>CredentialStore: persist completed credentials
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/handlers/expected_switch.rs (1)
112-120: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist NVOS credentials for add and replace-all mutations.
These handlers validate and store the credential pair in the database but never update
SwitchNvosAdmin. Existing switches can therefore retain stale credentials indefinitely, particularly after replace-all. Apply the same credential-store write/readback contract used by update, with explicit retry or compensation for partial bulk failures.As per path instructions, API changes must preserve transaction safety and schema/API compatibility.
Also applies to: 313-321
🤖 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/expected_switch.rs` around lines 112 - 120, Update the add and replace-all handlers around db_expected_switch::create and txn.commit to persist validated NVOS credentials through the same credential-store write/readback contract used by update. Ensure SwitchNvosAdmin is updated only after the database mutation succeeds, and add explicit retry or compensation so partial replace-all failures cannot leave database and NVOS credentials inconsistent. Preserve transaction safety and existing schema/API behavior.Source: Path instructions
🧹 Nitpick comments (2)
crates/api-core/src/tests/expected_switch.rs (1)
462-475: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify persistence through direct writer readback.
get_credentialsmay resolve through an effective reader, cache, or fallback. Useget_credentials_from_writerhere so this test proves thatupdate_expected_switchpersisted the credential to the configured writer.Proposed fix
let credentials = env .api .credential_manager - .get_credentials(&CredentialKey::SwitchNvosAdmin { bmc_mac_address }) + .get_credentials_from_writer(&CredentialKey::SwitchNvosAdmin { + bmc_mac_address, + }) .await .expect("unable to read NVOS admin credentials");🤖 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/expected_switch.rs` around lines 462 - 475, Update the credential readback in the expected-switch test to call CredentialManager’s get_credentials_from_writer method instead of get_credentials. Keep the existing CredentialKey::SwitchNvosAdmin lookup, await, error message, and assertions unchanged so the test validates persistence in the configured writer.crates/admin-cli/src/expected_switch/update/cmd.rs (1)
133-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTable-drive the sparse merge contract.
This single case misses the independent omitted/provided behavior for identifiers, BMC and NVOS credential pairs, metadata, rack ID, IPs, and credential retention. Cover those axes with
check_casesorscenarios!.As per coding guidelines, functions mapping inputs to observable outputs should use table-driven tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/expected_switch/update/cmd.rs` around lines 133 - 155, Expand the test coverage for merge_update_request with table-driven cases using check_cases or scenarios!, covering independent omitted/provided behavior for identifiers, BMC and NVOS credential pairs, metadata, rack ID, IP addresses, and credential retention. Replace the single merge_update_request_preserves_endpoint_fields_for_credential_only_update case with cases that assert each sparse-merge outcome while preserving the existing expected request values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/credential/rotate/args.rs`:
- Around line 34-35: Update the credential rotation help example for the NVOS
target password to use the prescribed shell-safe placeholder “mypassword”
instead of the unquoted password containing “!”. Keep the command structure and
copy-pasteable formatting unchanged.
In `@crates/admin-cli/src/expected_switch/update/cmd.rs`:
- Around line 33-36: Replace the client-side read-modify-write sequence around
get_expected_switch, merge_update_request, and update_expected_switch with an
atomic server-side sparse patch, or pass the fetched revision and enforce
compare-and-swap rejection for stale updates. Ensure concurrent changes are not
overwritten by stale fields while preserving the requested patch semantics.
In `@crates/api-core/src/handlers/component_manager.rs`:
- Around line 78-79: Update the ComponentManagerError-to-gRPC mapping so
RejectedBeforeDispatch remains a definitive non-retryable response while
OperationOutcomeUnknown uses a distinct retry-preserving status or structured
status detail. Revise the associated status-code table to document this
distinction consistently with the implementation.
In `@crates/component-manager/src/rms.rs`:
- Around line 2063-2068: Update the RackManagerError::ApiInvocationError mapping
for tonic::Code::Unimplemented to return the existing Unsupported error
classification instead of RejectedBeforeDispatch. Preserve target-specific
rejection handling for other failures and ensure staged rotations can defer this
unsupported RMS capability.
In `@crates/switch-controller/src/configuring.rs`:
- Around line 82-96: Reorder the configuring flow so
reconcile_nvos_password_rotation runs its no-op gates for missing
component-manager, unsupported rotation, and an uninitialized or absent
published target before ensure_nvos_admin_credentials is called. Keep credential
resolution only on actionable rotation paths, preserve existing state outcomes,
and add coverage for unsupported and uninitialized-target cases while ensuring
ConfiguringState::RotateOsPassword recovers safely.
---
Outside diff comments:
In `@crates/api-core/src/handlers/expected_switch.rs`:
- Around line 112-120: Update the add and replace-all handlers around
db_expected_switch::create and txn.commit to persist validated NVOS credentials
through the same credential-store write/readback contract used by update. Ensure
SwitchNvosAdmin is updated only after the database mutation succeeds, and add
explicit retry or compensation so partial replace-all failures cannot leave
database and NVOS credentials inconsistent. Preserve transaction safety and
existing schema/API behavior.
---
Nitpick comments:
In `@crates/admin-cli/src/expected_switch/update/cmd.rs`:
- Around line 133-155: Expand the test coverage for merge_update_request with
table-driven cases using check_cases or scenarios!, covering independent
omitted/provided behavior for identifiers, BMC and NVOS credential pairs,
metadata, rack ID, IP addresses, and credential retention. Replace the single
merge_update_request_preserves_endpoint_fields_for_credential_only_update case
with cases that assert each sparse-merge outcome while preserving the existing
expected request values.
In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 462-475: Update the credential readback in the expected-switch
test to call CredentialManager’s get_credentials_from_writer method instead of
get_credentials. Keep the existing CredentialKey::SwitchNvosAdmin lookup, await,
error message, and assertions unchanged so the test validates persistence in the
configured writer.
🪄 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: 7f0e6878-fa9f-4313-b1a2-3796897a7584
📒 Files selected for processing (30)
crates/admin-cli/src/credential/common.rscrates/admin-cli/src/credential/rotate/args.rscrates/admin-cli/src/credential/tests.rscrates/admin-cli/src/expected_switch/update/cmd.rscrates/api-core/src/credentials/bmc_session_manager.rscrates/api-core/src/errors.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/credential_rotation.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/secrets/mod.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-core/tests/integration/credential_rotation.rscrates/api-db/src/credential_rotation.rscrates/component-manager/src/component_manager.rscrates/component-manager/src/config.rscrates/component-manager/src/error.rscrates/component-manager/src/mock.rscrates/component-manager/src/nv_switch_manager.rscrates/component-manager/src/rms.rscrates/secrets/src/credentials.rscrates/secrets/src/forge_vault.rscrates/secrets/src/memory_credentials.rscrates/secrets/src/test_support/credentials.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/lib.rscrates/switch-controller/src/nvos_password_rotation.rscrates/switch-controller/src/ready.rs
Add direct writer readback for security-sensitive persistence checks. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Persist staged targets and backend attempts with CAS updates. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Send repeat-safe current-to-target requests through RMS. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Apply sparse patches and verify credential-store writes. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Retain current and target credentials across repeat-safe retries. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Reconcile in Configuring and reschedule drift from Ready. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Publish versioned targets and report fleet convergence. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Exercise partial, failed, lost-job, and target-active retries. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
nvos/password-rotation-04
branch
from
July 18, 2026 23:04
158baba to
e69fc6c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/api-core/src/tests/expected_switch.rs (1)
129-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover empty NVOS credential values in the table.
Add
(Some(""), Some("password"))and(Some("admin"), Some(""))cases to exercise the new non-empty validation branches.As per coding guidelines, validation input variants should use table-driven tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/expected_switch.rs` around lines 129 - 137, Add table-driven cases to the credential validation test’s input array, alongside the existing nvos_username/nvos_password combinations: one with an empty username and non-empty password, and one with a non-empty username and empty password. Mark both cases invalid and preserve the existing table-driven structure.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 32-61: Replace the raw update-mask constants and ALL in
crates/rpc/src/forge_api_client.rs:32-61 with an ExpectedSwitchUpdateField enum
implementing Display and FromStr. Update forge API client functions in
crates/rpc/src/forge_api_client.rs:64-170 to use typed masks internally and
serialize fields only at the metadata boundary. In
crates/api-core/src/handlers/expected_switch.rs:33-125, parse masks into
HashSet<ExpectedSwitchUpdateField> and match enum variants during merging.
In `@crates/switch-controller/src/nvos_password_rotation.rs`:
- Around line 549-563: Update nvos_password_rotation.rs in the current_version
eligibility check to detect validated expected-switch username or password
changes after confirmation, not only password rotation. In configuring.rs, apply
the explicit expected-switch credential override policy so configured username
and password values replace confirmed/existing values when provided. In
crates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rs, add
coverage for post-convergence username-only and password-only updates; these
sites require test changes validating credential-only reconciliation.
- Around line 167-187: Update the NVOS rotation flow around
persist_nvos_admin_credentials and record_device_rotation_succeeded so failures
after target credential persistence cannot leave recovery using the old
current_version. Persist a durable completion-pending state or otherwise retain
the staged target credential before promotion, and make
ensure_nvos_admin_credentials recovery preserve credentials matching that target
until database promotion completes.
---
Nitpick comments:
In `@crates/api-core/src/tests/expected_switch.rs`:
- Around line 129-137: Add table-driven cases to the credential validation
test’s input array, alongside the existing nvos_username/nvos_password
combinations: one with an empty username and non-empty password, and one with a
non-empty username and empty password. Mark both cases invalid and preserve the
existing table-driven structure.
🪄 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: 95d41731-4c54-463c-ad84-00d5e30c1f98
📒 Files selected for processing (31)
crates/admin-cli/src/credential/common.rscrates/admin-cli/src/credential/rotate/args.rscrates/admin-cli/src/credential/tests.rscrates/admin-cli/src/expected_switch/update/cmd.rscrates/api-core/src/credentials/bmc_session_manager.rscrates/api-core/src/errors.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/credential_rotation.rscrates/api-core/src/handlers/expected_switch.rscrates/api-core/src/secrets/mod.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/switch_state_controller/mod.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/api-core/tests/integration/credential_rotation.rscrates/api-db/src/credential_rotation.rscrates/api-db/src/expected_switch.rscrates/component-manager/src/component_manager.rscrates/component-manager/src/config.rscrates/component-manager/src/error.rscrates/component-manager/src/mock.rscrates/component-manager/src/nv_switch_manager.rscrates/component-manager/src/rms.rscrates/rpc/src/forge_api_client.rscrates/secrets/src/credentials.rscrates/secrets/src/forge_vault.rscrates/secrets/src/memory_credentials.rscrates/secrets/src/test_support/credentials.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/lib.rscrates/switch-controller/src/nvos_password_rotation.rscrates/switch-controller/src/ready.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/switch-controller/src/lib.rs
- crates/api-core/src/tests/switch_state_controller/mod.rs
- crates/secrets/src/forge_vault.rs
- crates/component-manager/src/component_manager.rs
- crates/api-core/src/errors.rs
- crates/api-core/src/secrets/mod.rs
- crates/api-core/src/credentials/bmc_session_manager.rs
- crates/secrets/src/memory_credentials.rs
- crates/admin-cli/src/credential/common.rs
- crates/secrets/src/credentials.rs
- crates/api-core/src/handlers/credential_rotation.rs
- crates/component-manager/src/mock.rs
- crates/api-core/tests/integration/credential_rotation.rs
- crates/component-manager/src/nv_switch_manager.rs
- crates/component-manager/src/rms.rs
- crates/api-db/src/credential_rotation.rs
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rpc/src/forge_api_client.rs (1)
215-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle an empty update mask before constructing the request.
expected_switch_update_mask(Default::default())returns an empty vector, but this serializes it as"". The handler then attempts to parse an empty field and rejects the request. Return a no-op or an explicit local validation error instead.Proposed no-op behavior
) -> Result<(), Status> { + if update_mask.is_empty() { + return Ok(()); + } + let update_mask = update_mask🤖 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/src/forge_api_client.rs` around lines 215 - 228, Handle an empty update_mask at the start of expected_switch_update_mask before parsing or constructing the tonic request, returning success as a no-op. Preserve the existing serialization, validation error, metadata insertion, and request flow for non-empty masks.
🧹 Nitpick comments (1)
crates/rpc/src/forge_api_client.rs (1)
376-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise every update-field wire mapping.
This checks only
NvosUsername; mismatchedDisplay/FromStrarms for the other variants would pass unnoticed. Usecheck_valueswith every(variant, wire_name)pair and verify both directions.As per coding guidelines, functions mapping inputs to outputs should use table-driven tests with
check_valueswhen appropriate.🤖 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/src/forge_api_client.rs` around lines 376 - 383, Update expected_switch_update_field_round_trips to use the table-driven check_values helper with every ExpectedSwitchUpdateField variant and its wire-name string, validating both Display and FromStr directions for each pair while retaining the unknown-value error assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 215-228: Handle an empty update_mask at the start of
expected_switch_update_mask before parsing or constructing the tonic request,
returning success as a no-op. Preserve the existing serialization, validation
error, metadata insertion, and request flow for non-empty masks.
---
Nitpick comments:
In `@crates/rpc/src/forge_api_client.rs`:
- Around line 376-383: Update expected_switch_update_field_round_trips to use
the table-driven check_values helper with every ExpectedSwitchUpdateField
variant and its wire-name string, validating both Display and FromStr directions
for each pair while retaining the unknown-value error assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1dcf2bc-865e-440b-8d63-3544397ec925
📒 Files selected for processing (5)
crates/api-core/src/handlers/expected_switch.rscrates/api-core/src/tests/expected_switch.rscrates/api-core/src/tests/switch_state_controller/nvos_password_rotation.rscrates/rpc/src/forge_api_client.rscrates/switch-controller/src/configuring.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-core/src/tests/expected_switch.rs
- crates/api-core/src/handlers/expected_switch.rs
- crates/switch-controller/src/configuring.rs
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
1 participant
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.
This continues #3409 and completes end-to-end NVOS password rotation support.
The main flow is:
Passwords remain in the configured credential backend. The database stores versions, attempt metadata, transient job IDs, and redacted failure information.
Expected-switch validates NVOS credential pairs and supports updating the current credential used for reconciliation. If a confirmed per-switch credential is missing, the controller restores it from the last confirmed versioned target instead of falling back to a stale bootstrap password. Support for
admin-cliis included for NVOS password rotation and credential-only expected-switch updates.Related issues
Supports #2041 and #3522.
Type of Change
Breaking Changes
Testing