fix(dpf): start without the site-wide BMC root credential#4167
fix(dpf): start without the site-wide BMC root credential#4167wminckler wants to merge 3 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe DPF SDK now applies BMC credentials idempotently, supports deferred initialization when credentials are unavailable, tracks optional refresh state, updates repository mocks and tests, and documents credential prerequisites and Secret synchronization. ChangesDPF BMC credential propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DpfSdk
participant BmcPasswordProvider
participant K8sConfigRepository
participant KubernetesApi
DpfSdk->>BmcPasswordProvider: Read site-wide BMC root password
BmcPasswordProvider-->>DpfSdk: Password or unavailable
DpfSdk->>K8sConfigRepository: apply_secret for a new password
K8sConfigRepository->>KubernetesApi: Server-side apply Secret
KubernetesApi-->>K8sConfigRepository: Apply result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a838452 to
708d0bd
Compare
708d0bd to
be36fff
Compare
|
Testing was done using the devspace env with additional hacks for DPF. If I can get the devspace hacks to be configurable and usable, I'll put those in a separate PR |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/dpf/src/sdk.rs (1)
3158-3272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the refresh and initialization cases into tables.
These tests map provider/write/refresh inputs to expected values or errors, so they should use
check_cases/check_values(orscenarios!) rather than seven near-duplicate test functions.As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs or errors.”
🤖 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/dpf/src/sdk.rs` around lines 3158 - 3272, The refresh and initialization tests are duplicated cases that should be consolidated into table-driven tests. Replace the seven individual tests around refresh_bmc_secret_if_changed and DpfSdkBuilder::build_without_resources with check_cases/check_values or scenarios!, grouping provider availability, previous password, write failure, refresh configuration, and expected values/errors while preserving all existing assertions and outcomes.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 `@docs/getting-started/quick-start.md`:
- Around line 582-583: Update the Site Explorer credential setup section in the
quick-start documentation to include the required DPU UEFI credential
provisioning command, or explicitly point to where that credential is
configured. Keep the existing site-wide BMC root and host UEFI setup
instructions intact and ensure both UEFI site defaults are documented before
Site Explorer is run.
In `@docs/manuals/dpf.md`:
- Around line 792-794: Update the Vault command example in the BMC credential
instructions so the password is entered through a secure prompt or protected
stdin/file workflow rather than embedded in the echo payload or any command
argument. Preserve the documented username and Vault path while ensuring the
password cannot appear in shell history or process arguments.
---
Nitpick comments:
In `@crates/dpf/src/sdk.rs`:
- Around line 3158-3272: The refresh and initialization tests are duplicated
cases that should be consolidated into table-driven tests. Replace the seven
individual tests around refresh_bmc_secret_if_changed and
DpfSdkBuilder::build_without_resources with check_cases/check_values or
scenarios!, grouping provider availability, previous password, write failure,
refresh configuration, and expected values/errors while preserving all existing
assertions and outcomes.
🪄 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: 79414dfe-9ce8-4eab-a580-070358a95739
📒 Files selected for processing (14)
crates/dpf/src/repository/kube.rscrates/dpf/src/repository/traits.rscrates/dpf/src/sdk.rscrates/dpf/src/test/helpers.rscrates/dpf/src/test/maintenance_flow.rscrates/dpf/src/test/sdk_device_registration.rscrates/dpf/src/test/sdk_initialization.rscrates/dpf/src/test/sdk_maintenance_hold.rscrates/dpf/src/test/sdk_provisioning_flow.rscrates/dpf/src/test/sdk_reboot_annotation.rsdocs/getting-started/prerequisites/bmc-oob-setup.mddocs/getting-started/quick-start.mddocs/manuals/dpf.mdhelm-prereqs/values.yaml
|
Re: the nitpick on consolidating The four cases aren't homogeneous input/output pairs: two drive the mock repository through different refresh sequences and assert on what was written, the other two assert build success vs. a specific error variant. A table would need per-case setup closures, which reads worse than the explicit tests and risks weakening assertions that currently pass. Happy to revisit if a reviewer disagrees.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/getting-started/quick-start.md`:
- Around line 580-581: Update the credential add-uefi example in the quick-start
documentation to avoid passing the UEFI password directly as a command-line
argument. Document the safer supported password-input method, or explicitly warn
about shell-history and process-argument exposure if no alternative exists.
🪄 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: cb471d55-0889-49fb-a172-6db4627e3846
📒 Files selected for processing (2)
docs/getting-started/quick-start.mddocs/manuals/dpf.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/manuals/dpf.md
Operators set the site-wide BMC root credential through the API, so on a fresh site it does not exist when nico-api first boots. DPF SDK init read it unconditionally and propagated the failure out of setup, aborting the boot before the listener started -- leaving no way to set the credential that would let the next boot succeed. Make the initial read best-effort when a BMC password refresh interval is configured: initialization continues without the Secret and the existing 60s refresh task writes it as soon as the credential appears, so no restart is needed. With no refresh task nothing would ever retry, so there the failure stays fatal. The refresh task could not have recovered as written: create_secret returned early when the Secret already existed, so a rotated password was never propagated to the cluster. Rename it to apply_secret and implement it as a server-side apply, matching apply_configmap. Document the credential as a DPF setup step, and note the Site Explorer precondition on the two getting-started pages that previously implied "after deploy" was the only option. Fixes NVIDIA#4130 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The quick-start credential block listed only the site-wide BMC root and the host UEFI default, while asserting that Site Explorer needs "all of these" -- so a reader following it still hit MissingCredentials on the missing dpu UEFI default. Add the `--kind=dpu` command and name all three explicitly. The Vault seeding example passed the BMC password inside an echo payload, putting it in shell history. Read it with `read -rs` and render the JSON with the printf builtin, so it reaches Vault without appearing on the terminal, in history, or in a process argument list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
16c98a3 to
b1dd505
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/manuals/dpf.md (1)
767-815: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd non-critical-environment validation guidance.
Because this changes experimental NICo startup gating and credential synchronization, add an explicit warning to validate the workflow in a non-critical environment before production deployment.
Based on coding guidelines, NICo features, APIs, and configurations must be thoroughly tested in non-critical environments first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manuals/dpf.md` around lines 767 - 815, Insert an explicit warning in section 3.6 about validating the BMC credential setup, startup gating, and synchronization workflow in a non-critical environment before deploying to production. Place it near the configuration instructions and preserve the existing production guidance.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 `@docs/manuals/dpf.md`:
- Around line 783-786: Update the documented carbide-api startup behavior to
state that deferred initialization applies only when a BMC password refresh task
or interval is configured; without that refresh configuration, a missing
credential remains fatal. Preserve the existing warning, delayed Secret write,
and no-restart behavior for the configured refresh-task case.
---
Nitpick comments:
In `@docs/manuals/dpf.md`:
- Around line 767-815: Insert an explicit warning in section 3.6 about
validating the BMC credential setup, startup gating, and synchronization
workflow in a non-critical environment before deploying to production. Place it
near the configuration instructions and preserve the existing production
guidance.
🪄 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: b8fb0d85-c54d-4f25-83ba-b93b5aaaf616
📒 Files selected for processing (13)
crates/dpf/src/repository/kube.rscrates/dpf/src/repository/traits.rscrates/dpf/src/sdk.rscrates/dpf/src/test/helpers.rscrates/dpf/src/test/maintenance_flow.rscrates/dpf/src/test/sdk_device_registration.rscrates/dpf/src/test/sdk_initialization.rscrates/dpf/src/test/sdk_maintenance_hold.rscrates/dpf/src/test/sdk_provisioning_flow.rscrates/dpf/src/test/sdk_reboot_annotation.rsdocs/getting-started/prerequisites/bmc-oob-setup.mddocs/getting-started/quick-start.mddocs/manuals/dpf.md
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/dpf/src/test/sdk_reboot_annotation.rs
- crates/dpf/src/test/sdk_maintenance_hold.rs
- crates/dpf/src/test/sdk_provisioning_flow.rs
- crates/dpf/src/test/sdk_initialization.rs
- docs/getting-started/prerequisites/bmc-oob-setup.md
- crates/dpf/src/repository/traits.rs
- crates/dpf/src/test/helpers.rs
- crates/dpf/src/test/sdk_device_registration.rs
- crates/dpf/src/repository/kube.rs
- crates/dpf/src/test/maintenance_flow.rs
- docs/getting-started/quick-start.md
- crates/dpf/src/sdk.rs
The DPF manual said carbide-api starts whether or not the site-wide BMC root credential is present. That holds only when a BMC password refresh interval is configured -- with no interval nothing retries the read, so the credential is still fatal to startup and must be seeded beforehand. State both cases. The nico-admin-cli credential commands accept a password only as an argument, so it reaches shell history and the process argument list. The CLI offers no stdin or prompt alternative, so note the exposure where the commands appear and point at the Vault seeding path, which avoids it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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)
docs/manuals/dpf.md (1)
801-805: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEscape the password before constructing JSON.
printfinterpolates the password directly into a JSON string, so passwords containing"or\can produce invalid or altered payloads. Encode the value through a JSON encoder while keeping it on stdin.Proposed fix
read -rs -p 'Site-wide BMC root password: ' BMC_ROOT_PASSWORD && echo -printf '{"UsernamePassword":{"username":"root","password":"%s"}}' \ - "${BMC_ROOT_PASSWORD}" \ +printf '%s' "${BMC_ROOT_PASSWORD}" \ + | jq -Rs '{UsernamePassword:{username:"root",password:.}}' \ | vault kv put <kv-mount>/machines/bmc/site/root - unset BMC_ROOT_PASSWORDAs per path instructions, documentation commands must be realistic and safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manuals/dpf.md` around lines 801 - 805, Update the password-to-JSON pipeline around BMC_ROOT_PASSWORD so the value is encoded with a JSON encoder before being inserted into the payload, preserving stdin input to vault kv put. Ensure passwords containing quotes or backslashes remain valid and unchanged, and keep the existing secret cleanup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/manuals/dpf.md`:
- Around line 801-805: Update the password-to-JSON pipeline around
BMC_ROOT_PASSWORD so the value is encoded with a JSON encoder before being
inserted into the payload, preserving stdin input to vault kv put. Ensure
passwords containing quotes or backslashes remain valid and unchanged, and keep
the existing secret cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8d388d45-a74e-423a-86b5-05d45e09c9e7
📒 Files selected for processing (2)
docs/getting-started/quick-start.mddocs/manuals/dpf.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/getting-started/quick-start.md
Backport of #4167 onto `release/v2.0`. Operators set the site-wide BMC root credential through the API, so on a fresh site it does not exist when nico-api first boots. DPF SDK initialization read that credential unconditionally and propagated the failure out of setup, aborting the boot before the listener started — which left no way to set the credential that would have let the next boot succeed. A fresh site with DPF enabled could not be brought up at all. This makes the initial read best-effort when a BMC password refresh interval is configured: initialization continues without the Secret, and the existing 60s refresh task writes it as soon as the credential appears, so no restart is needed. With no refresh task configured nothing would ever retry, so there the failure stays fatal. The refresh task could not have recovered as written: `create_secret` returned early when the Secret already existed, so a rotated password was never propagated to the cluster. It is renamed to `apply_secret` and implemented as a server-side apply, matching `apply_configmap`. All ten source files cherry-picked onto the `release/v2.0` base with no conflicts. The documentation changes carried by #4167 are omitted here, as release branches do not carry docs. ## Related issues #4130 ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) `cargo test -p carbide-dpf` on this branch: 98 passed, 0 failed. That includes the four new tests, each confirmed by name — the refresh task writing the Secret once the password appears, staying unwritten while it is unavailable, and build succeeding vs. failing when the password is unset with and without a refresh interval configured. The equivalent change on `main` was additionally verified against a local kind cluster running the DPF operator, with nico-api pointed at a Vault mount holding the UEFI site defaults but no `machines/bmc/site/root`. Startup reached DPF SDK initialization instead of aborting at the credential read: ``` level=ERROR The Vault server returned an error (status code 404) level=WARN site-wide BMC root not set; deferring lockdown IKM seed until it is configured level=INFO Initializing DPF SDK ``` That verification was performed on the `main`-based branch, not on this `release/v2.0` backport. ## Additional Notes The repository pre-commit hook and `pre-commit-verify-workspace` both invoke `cargo xtask lint-error-messages`, which does not exist on `release/v2.0`, so neither could run against this branch. CI is the first lint signal here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Operators set the site-wide BMC root credential through the API, so on a fresh
site it does not exist when nico-api first boots. DPF SDK initialization read
that credential unconditionally and propagated the failure out of setup,
aborting the boot before the listener started — which left no way to set the
credential that would have let the next boot succeed. A fresh site with DPF
enabled could not be brought up at all.
This makes the initial read best-effort when a BMC password refresh interval is
configured: initialization continues without the Secret, and the existing 60s
refresh task writes it as soon as the credential appears, so no restart is
needed. With no refresh task configured nothing would ever retry, so there the
failure stays fatal.
The refresh task could not have recovered as written:
create_secretreturnedearly when the Secret already existed, so a rotated password was never
propagated to the cluster. It is renamed to
apply_secretand implemented as aserver-side apply, matching
apply_configmap.Related issues
Fixes #4130
Type of Change
Breaking Changes
Testing
Four new unit tests cover the refresh task writing the Secret once the password
appears, staying unwritten while it is unavailable, and build succeeding vs.
failing when the password is unset with and without a refresh interval
configured.
cargo test -p carbide-dpf: 103 passed, 0 failed.Verified manually against a local kind cluster running the DPF operator
(v26.4.0), with nico-api pointed at a freshly created Vault KV mount that holds
the UEFI site defaults but no
machines/bmc/site/root. Startup reaches DPF SDKinitialization instead of aborting at the credential read:
Additional Notes
Docs updates ride along: the credential is documented as a DPF setup step, and
the Site Explorer precondition is noted on the two getting-started pages that
previously implied "after deploy" was the only option.
The devspace support for installing the DPF operator locally, which was
originally part of this PR, has moved to a follow-up branch: it needs further
fixes before it works end-to-end, and they should not hold up this fix.
🤖 Generated with Claude Code