feat(credential-rotation): add durable NVOS password rotation state#3591
Merged
Conversation
jayzhudev
added
the
rack lifecycle
jayzhudev
changed the title
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:
WalkthroughNVOS credential rotation now persists staged per-device operations, backend job IDs, retry attempts, and outcomes. NVOS status requires an initialized site-wide target and uses live switches. ComponentManager exposes backend rotation operations, while controller execution remains disabled and fails closed. ChangesNVOS credential rotation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SwitchController
participant CredentialRotationDB
participant ComponentManager
participant NVSwitchBackend
SwitchController->>CredentialRotationDB: stage rotation attempt
SwitchController->>ComponentManager: start switch password rotation
ComponentManager->>NVSwitchBackend: submit rotation request
NVSwitchBackend-->>ComponentManager: return job ID
ComponentManager-->>SwitchController: return job ID
SwitchController->>CredentialRotationDB: persist submitted job
SwitchController->>ComponentManager: fetch job status
ComponentManager->>NVSwitchBackend: query rotation job
NVSwitchBackend-->>ComponentManager: return job state
SwitchController->>CredentialRotationDB: reconcile device operation
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-db/src/credential_rotation/test_backfill.rs (1)
235-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the v0 assertion reject NULL versions.
current_version <> 0excludes NULL rows, so the test can pass even when a backfilled device has no confirmed version.Proposed fix
- "SELECT count(*) FROM device_credential_rotation WHERE current_version <> 0", + "SELECT count(*) FROM device_credential_rotation \ + WHERE current_version IS DISTINCT FROM 0",🤖 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-db/src/credential_rotation/test_backfill.rs` around lines 235 - 251, Update the non_v0 assertion in the backfill test to count NULL current_version values as invalid alongside nonzero versions. Ensure the query rejects any backfilled device whose current_version is not confirmed as 0, while preserving the existing assertion and message.
🧹 Nitpick comments (1)
crates/api-db/src/credential_rotation.rs (1)
1079-1130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every conditional-baseline outcome with table-driven tests.
The test exercises
TargetChangedandRecorded, but leavesAlreadyPresent,TargetMissing, and negative-version rejection unverified. These are public initialization and idempotency contracts.As per coding guidelines, functions mapping inputs to outcomes or errors 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-db/src/credential_rotation.rs` around lines 1079 - 1130, The test coverage around record_device_converged_if_target_matches is incomplete and not table-driven. Expand or add a table-driven test suite covering Recorded, AlreadyPresent, TargetChanged, TargetMissing, and rejection of negative authenticated versions, asserting each expected outcome or error while preserving the existing race scenario coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/credential_rotation.rs`:
- Around line 32-34: Update the NVOS rejection response in the relevant handler
to state that the end-to-end NVOS rotation path remains disabled, replacing the
outdated NICo ownership and REQ-6 explanation. Keep the surrounding rejection
behavior unchanged and align it with the module documentation.
---
Outside diff comments:
In `@crates/api-db/src/credential_rotation/test_backfill.rs`:
- Around line 235-251: Update the non_v0 assertion in the backfill test to count
NULL current_version values as invalid alongside nonzero versions. Ensure the
query rejects any backfilled device whose current_version is not confirmed as 0,
while preserving the existing assertion and message.
---
Nitpick comments:
In `@crates/api-db/src/credential_rotation.rs`:
- Around line 1079-1130: The test coverage around
record_device_converged_if_target_matches is incomplete and not table-driven.
Expand or add a table-driven test suite covering Recorded, AlreadyPresent,
TargetChanged, TargetMissing, and rejection of negative authenticated versions,
asserting each expected outcome or error while preserving the existing race
scenario coverage.
🪄 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: 67beced7-d749-4526-ad46-f3a423b10759
📒 Files selected for processing (7)
crates/api-core/src/handlers/credential_rotation.rscrates/api-db/migrations/20260715120000_nvos_credential_rotation_state.sqlcrates/api-db/src/credential_rotation.rscrates/api-db/src/credential_rotation/test_backfill.rscrates/component-manager/src/component_manager.rscrates/secrets/src/credentials.rscrates/switch-controller/src/configuring.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
spydaNVIDIA
reviewed
Jul 16, 2026
spydaNVIDIA
approved these changes
Jul 16, 2026
| /// Matching the staged target and attempt number prevents stale reconciliation | ||
| /// from releasing a newer operation. Returns `false` when that exact operation | ||
| /// is no longer active. | ||
| pub async fn record_device_rotation_reconciled_to_previous( |
Contributor
There was a problem hiding this comment.
record_device_rotation_failed may be a better name
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
nvos/password-rotation-03
branch
from
July 16, 2026 18:49
198bcd1 to
a370346
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3591.docs.buildwithfern.com/infra-controller |
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/src/credential_rotation.rs (1)
824-848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept a
DbReaderhere.
This is a single-query read-only helper, so it should follow the existingimpl DbReader<'_>pattern and stay callable from pooled readers, transactions, and connections.🤖 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-db/src/credential_rotation.rs` around lines 824 - 848, Update device_rotation_operation_state to use the existing DbReader<'_> abstraction instead of requiring &mut PgConnection, following the established impl DbReader pattern. Preserve the query, bindings, result type, and error mapping, while ensuring the helper remains callable with pooled readers, transactions, and direct connections.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-db/src/credential_rotation.rs`:
- Around line 894-927: Update nvos_device_rotation_status to distinguish a live
switch with no published site-wide NVOS target from an unknown device: preserve
the device row via the query’s join structure, return a status/result that lets
the handler produce MissingSitewideRotationTarget when no target exists, and
retain None/NotFound behavior for unknown devices. Ensure this also covers
requests made before publish_nvos_target.
---
Nitpick comments:
In `@crates/api-db/src/credential_rotation.rs`:
- Around line 824-848: Update device_rotation_operation_state to use the
existing DbReader<'_> abstraction instead of requiring &mut PgConnection,
following the established impl DbReader pattern. Preserve the query, bindings,
result type, and error mapping, while ensuring the helper remains callable with
pooled readers, transactions, and direct connections.
🪄 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: 57a8b665-e845-46ea-b3ca-5805e667314b
📒 Files selected for processing (8)
crates/api-core/src/handlers/credential_rotation.rscrates/api-db/migrations/20260715120000_nvos_credential_rotation_state.sqlcrates/api-db/src/credential_rotation.rscrates/api-db/src/credential_rotation/test_backfill.rscrates/api-db/src/migrations/mod.rscrates/component-manager/src/component_manager.rscrates/secrets/src/credentials.rscrates/switch-controller/src/configuring.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/api-db/migrations/20260715120000_nvos_credential_rotation_state.sql
- crates/api-db/src/credential_rotation/test_backfill.rs
- crates/component-manager/src/component_manager.rs
- crates/api-core/src/handlers/credential_rotation.rs
- crates/api-db/src/migrations/mod.rs
- crates/secrets/src/credentials.rs
jayzhudev
force-pushed
the
nvos/password-rotation-03
branch
from
July 16, 2026 19:42
aa9cbb1 to
e49ec8d
Compare
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
nvos/password-rotation-03
branch
from
July 16, 2026 19:45
e49ec8d to
51d4a3c
Compare
spydaNVIDIA
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This continues the previous PR #3409, which added
component-managerNVOS password rotation job contract. This PR adds restart-safe persistence for future controller orchestration. NVOS password rotation is not enabled yet.Review areas:
Key decisions:
Follow-up PRs will cover switch controller, target publication, activation, and admin-cli.
Related issues
Supports #2041 and #3522
Type of Change
Breaking Changes
Testing