fix(switch-controller): make NVOS credential gaps retryable during cert setup - #4169
Conversation
jayzhudev
added
the
rack lifecycle
WalkthroughCertificate setup now checks NVOS admin credentials before starting, seeds missing credentials when possible, waits when unavailable, and proceeds after credentials are imported. Tests cover credential persistence, rack association, and retry transitions. ChangesNVOS credential gating
Estimated code review effort: 3 (Moderate) | ~25 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-core/src/tests/switch_state_controller/mod.rs (1)
462-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
Error→waitbranch.The three tests cover
Available(pre-seeded), the expected-switch seeding path, andSkip→ retry. The remaining branch —NvosAdminCredentialStatus::Error(cause)mapped towaitatcrates/switch-controller/src/configuring.rsLines 397-403 — is untested, yet it is precisely the branch that decides whether an inconsistent switch stalls retryably or strands inError. A case with, say, an expected-switch row carrying a username but no password would pin the"waiting for valid NVOS admin credentials: {cause}"contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/switch_state_controller/mod.rs` around lines 462 - 470, Add a test in the switch state controller test suite covering an expected-switch credential row with a username but no password, causing NvosAdminCredentialStatus::Error(cause). Assert the controller transitions to ConfigureCertificateState::WaitForComplete and preserves the “waiting for valid NVOS admin credentials: {cause}” wait/retry behavior.Source: Path instructions
crates/switch-controller/src/configuring.rs (1)
373-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe preflight read duplicates the resolver's first step.
ensure_nvos_admin_credentialsalready opens with the identicalget_credentials(&key)read and the byte-identicalmap_errmessage (Lines 157-166), and it already preserves an existing credential rather than clobbering it (Lines 298-315). The local read is therefore a pure short-circuit whose only benefit is skipping the expected-switch/rotation DB queries — at the cost of duplicating the error string in two places, where they will inevitably drift.If the short-circuit is worth keeping, consider returning the read credential from a small shared helper so the error mapping lives in exactly one place; otherwise call the resolver unconditionally and let
Availablecover the existing-credential case.♻️ Sketch: unconditional resolver call
- let key = CredentialKey::SwitchNvosAdmin { bmc_mac_address }; - - let credential_exists = ctx - .services - .credential_manager - .get_credentials(&key) - .await - .map_err(|error| { - StateHandlerError::GenericError(eyre::eyre!( - "switch {switch_id}: failed to read NVOS credentials from credential store: {error}" - )) - })? - .is_some(); - - if !credential_exists { - match ensure_nvos_admin_credentials(switch_id, bmc_mac_address, ctx).await? { - NvosAdminCredentialStatus::Available => {} - NvosAdminCredentialStatus::Skip => { - // Stay in `ConfigureCertificate::Start` so periodic - // reconciliation retries after credentials are imported. - return Ok(StateHandlerOutcome::wait(format!( - "switch {switch_id}: waiting for NVOS admin credentials" - ))); - } - NvosAdminCredentialStatus::Error(cause) => { - // Expected-switch credential metadata can be corrected - // without resetting the switch controller state. - return Ok(StateHandlerOutcome::wait(format!( - "switch {switch_id}: waiting for valid NVOS admin credentials: {cause}" - ))); - } - } - } + match ensure_nvos_admin_credentials(switch_id, bmc_mac_address, ctx).await? { + NvosAdminCredentialStatus::Available => {} + NvosAdminCredentialStatus::Skip => { + // Stay in `ConfigureCertificate::Start` so periodic + // reconciliation retries after credentials are imported. + return Ok(StateHandlerOutcome::wait(format!( + "switch {switch_id}: waiting for NVOS admin credentials" + ))); + } + NvosAdminCredentialStatus::Error(cause) => { + // Expected-switch credential metadata can be corrected + // without resetting the switch controller state. + return Ok(StateHandlerOutcome::wait(format!( + "switch {switch_id}: waiting for valid NVOS admin credentials: {cause}" + ))); + } + }Note this trades the short-circuit for a per-iteration credential rewrite via
persist_nvos_admin_credentials; if that write cost is the reason for the guard, prefer the shared-helper option instead.🤖 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/switch-controller/src/configuring.rs` around lines 373 - 405, Remove the duplicated credential preflight read in the configuration flow and call ensure_nvos_admin_credentials unconditionally, letting its Available result handle existing credentials; preserve the existing Skip and Error wait outcomes. Alternatively, if retaining the short-circuit is required, extract the shared get_credentials and StateHandlerError mapping into a helper used by both paths so the read and error message are defined once.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/switch-controller/src/configuring.rs`:
- Around line 397-403: Separate unrecoverable credential inconsistencies from
recoverable NvosAdminCredentialStatus::Error cases. Update the status
construction for “No expected switch found…” and empty confirmed-password
conditions to use a distinct terminal variant, then map that variant in the
configuring state handler to an Error outcome. Preserve the existing wait
behavior for credentials that operators can import or correct.
---
Nitpick comments:
In `@crates/api-core/src/tests/switch_state_controller/mod.rs`:
- Around line 462-470: Add a test in the switch state controller test suite
covering an expected-switch credential row with a username but no password,
causing NvosAdminCredentialStatus::Error(cause). Assert the controller
transitions to ConfigureCertificateState::WaitForComplete and preserves the
“waiting for valid NVOS admin credentials: {cause}” wait/retry behavior.
In `@crates/switch-controller/src/configuring.rs`:
- Around line 373-405: Remove the duplicated credential preflight read in the
configuration flow and call ensure_nvos_admin_credentials unconditionally,
letting its Available result handle existing credentials; preserve the existing
Skip and Error wait outcomes. Alternatively, if retaining the short-circuit is
required, extract the shared get_credentials and StateHandlerError mapping into
a helper used by both paths so the read and error message are defined once.
🪄 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: 6fa34dd9-8fbe-432d-add3-4a45d6dc69d6
📒 Files selected for processing (2)
crates/api-core/src/tests/switch_state_controller/mod.rscrates/switch-controller/src/configuring.rs
…rt setup Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
04bf375 to
6858a0c
Compare
2 participants
Fix switch certificate setup so missing per-switch NVOS admin credentials do not strand a switch in non-retryable
Error.When certificate setup is actionable, the switch controller now checks for
SwitchNvosAdmincredentials before endpoint resolution. If the credential is missing, it uses the existing NVOS credential resolver to seed expected-switch bootstrap credentials when available. If no usable credential source exists yet, it stays inConfigureCertificate::StartwithWait, so later credential import can retry automatically.Existing imported credentials are preserved.
Related issues
Fixes #4168
Type of Change
Breaking Changes
Testing
Manual Testing Results
Validated both recovery paths against real hardware.
Method
SwitchNvosAdmincredential and expected-switch NVOS fields, then set lifecycle state toConfigureCertificate::Start.StartwithWait.SwitchNvosAdmincredential into Vault and waited for scheduled reconciliation.Missing credential is retryable
This shows the controller did not transition to lifecycle
Error.Imported credential self-heals
After Vault import, a later periodic controller pass completed the lifecycle without operator state reset.
Expected-switch bootstrap seeds the per-switch credential
This exercises the fix: resolve and persist expected-switch bootstrap credentials before certificate endpoint resolution.
RMS job completed
Note: RMS was run with
--insecure-switchmode during tests, hence it performed mTLS unset.