feat: make DPU bootstrap CA configurable#3588
Open
chet wants to merge 1 commit into
Open
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 configurable bootstrap CA sourcing through legacy download, embedded, and mounted modes. The change updates DPU agent provisioning, DPF configuration and service wiring, PXE cloud-init behavior, Helm projections, CA validation, runtime paths, build staging, and related tests. ChangesBootstrap CA configuration and source propagation
DPU agent acquisition and installation
PXE serving and embedded provisioning
DPU agent Helm projection
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DPF
participant PXE
participant CloudInit
participant DPUAgent
DPF->>PXE: bootstrap_ca_source in discovery instructions
PXE->>CloudInit: render source-specific user-data
CloudInit->>DPUAgent: invoke init-container with CA options
DPUAgent->>DPUAgent: acquire, validate, and atomically install CA
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3588.docs.buildwithfern.com/infra-controller |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
🔐 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-15 22:38:24 UTC | Commit: 5d5b849 |
![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
🧹 Nitpick comments (2)
pxe/common_files/stage-bootstrap-ca.sh (1)
78-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
installis not atomic against concurrent new readers of$destination.GNU
install(1)unlinks the existing destination and then writes the new content in place; per upstream coreutils discussion, "install(1) will unlink() an existing destination first, before writing the new version. That should handle atomicity for existing processes that have the file open", but "new openers of the file may get a partially written version". Since the PR explicitly calls for "atomic replacement" of the bootstrap CA, if$destinationcan ever be read while this script re-runs, a reader could observe a truncated/invalid bundle.🔒 Suggested atomic replacement
-install -m 0644 "$source" "$destination" +tmp_destination="$(dirname "$destination")/.$(basename "$destination").tmp.$$" +install -m 0644 "$source" "$tmp_destination" +mv -f "$tmp_destination" "$destination"Please confirm whether
$destinationis ever read concurrently with this script's execution; if it's purely a single-threaded build-time artifact, this is lower priority, but the fix is cheap enough to apply defensively either way.🤖 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 `@pxe/common_files/stage-bootstrap-ca.sh` at line 78, Replace the direct install to $destination in the bootstrap CA update flow with an atomic replacement: install the content into a temporary file in the same directory as $destination, then rename it over $destination after a successful write. Ensure cleanup on failure and preserve the existing file mode and destination behavior.bluefield/charts/nico-dpu-agent/templates/daemonset.yaml (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented coupling between
certsDirand mounted bootstrap CA.Requiring
certsDir=/opt/forgeforbootstrapCa.source: mountedrepurposes a general-purpose value (certsDir, otherwise governing the unrelatednico-certshostPath used for the agent's own TLS identity) as a hidden precondition for an unrelated feature. Without an inline comment, operators hitting thisfailwon't understand why changingbootstrapCa.sourcealso forces a change to their cert directory layout.📝 Suggested clarification
{{- if empty $bootstrapCaObjectKey -}} {{- fail "bootstrapCa.object.key must not be empty when bootstrapCa.source is mounted" -}} {{- end -}} + {{- /* The DPU agent init-container resolves the mounted trust anchor relative to + certsDir, so certsDir must stay at the fixed path the agent expects. */ -}} {{- if ne $certsDir "/opt/forge" -}} {{- fail "bootstrapCa.source mounted requires certsDir=/opt/forge" -}} {{- end -}}Please confirm whether the DPU-agent init-container actually reads the mounted CA from a
certsDir-relative path (not shown in the reviewed files) — if so, a comment closes the gap; if not, consider decoupling this check fromcertsDirentirely.Also applies to: 29-31
🤖 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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml` at line 8, Verify that the DPU-agent init-container reads the mounted bootstrap CA from a path relative to certsDir. If confirmed, add an inline comment near the certsDir default and the bootstrapCa.source validation explaining that mounted bootstrap CA requires certsDir=/opt/forge because of this path dependency; otherwise decouple the validation from certsDir and use the actual CA mount-path requirement.
🤖 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/agent/src/lib.rs`:
- Around line 116-129: Update download_bootstrap_ca to use an explicit request
timeout instead of reqwest::get’s default client. Configure the timeout before
sending the request, preserving the existing URL context and error propagation
so an unresponsive endpoint returns an error within the defined limit.
---
Nitpick comments:
In `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Line 8: Verify that the DPU-agent init-container reads the mounted bootstrap
CA from a path relative to certsDir. If confirmed, add an inline comment near
the certsDir default and the bootstrapCa.source validation explaining that
mounted bootstrap CA requires certsDir=/opt/forge because of this path
dependency; otherwise decouple the validation from certsDir and use the actual
CA mount-path requirement.
In `@pxe/common_files/stage-bootstrap-ca.sh`:
- Line 78: Replace the direct install to $destination in the bootstrap CA update
flow with an atomic replacement: install the content into a temporary file in
the same directory as $destination, then rename it over $destination after a
successful write. Ensure cleanup on failure and preserve the existing file mode
and destination behavior.
🪄 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: 90c41663-ad16-4cec-b01c-4f0a118762ca
📒 Files selected for processing (39)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-data
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/api-core/src/cfg/file.rs (1)
4781-4821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository table-test helpers.
Replace the manual case vectors and loops with
check_valuesorvalue_scenarios!for these input-to-output/error matrices. As per coding guidelines, “Use carbide-test-support macros” for table-driven mapping tests.Also applies to: 4839-4871
🤖 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/cfg/file.rs` around lines 4781 - 4821, Replace the manual cases vector and iteration in deserialize_dpf_dpu_agent_bootstrap_ca_sources, including the additional covered test block, with the repository’s check_values or value_scenarios! helper. Express each TOML input and expected DpfDpuAgentBootstrapCa result as table entries, preserving validation assertions and named failure context through the helper’s established pattern.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/agent/src/command_line.rs`:
- Around line 88-90: Restrict the bootstrap_ca_url argument defined on the CLI
options struct to HTTP and HTTPS schemes, rejecting unsupported schemes during
argument parsing before the downloader runs. Update the CLI tests to cover a
non-HTTP(S) URL and verify it fails validation.
In `@crates/api-core/src/dpf_services.rs`:
- Around line 501-540: Update
dpu_agent_bootstrap_ca_helm_values_follow_site_policy to use the
value_scenarios! table format, preserving the existing legacy and Secret cases,
and add a ConfigMap-mounted DpfDpuAgentBootstrapCa case with expected
bootstrapCa serialization using object kind "ConfigMap".
In `@crates/api-core/src/setup.rs`:
- Around line 690-694: Lowercase the error message passed to eyre! in the DPF
bootstrap CA validation within setup configuration, changing its initial
“Invalid” wording to lowercase while preserving the existing error details and
validation behavior.
In `@crates/pxe/src/config.rs`:
- Around line 43-44: Update the bootstrap CA path resolution around
bootstrap_root_ca_path so an environment value that is missing or empty falls
back to forge_root_ca_path. Extend the existing configuration table tests with
an empty FORGE_BOOTSTRAP_ROOT_CAFILE_PATH scenario and assert the legacy
fallback path is used.
In `@docs/provisioning/ip-and-network-configuration.md`:
- Line 294: Update the nico-pxe.nico endpoint-table row to document both DPF
modes: URL-based CA override and mounted mode using a Secret/ConfigMap that
bypasses the URL, while preserving the existing compatibility-default context
for other consumers.
In `@helm/charts/nico-pxe/values.yaml`:
- Around line 110-112: Update the CA bundle configuration comment near the
optional root CA settings to say that at most one of configMapName and
secretName may be set. Preserve the documented behavior that when neither is
configured, PXE serves FORGE_ROOT_CAFILE_PATH as in legacy deployments.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4781-4821: Replace the manual cases vector and iteration in
deserialize_dpf_dpu_agent_bootstrap_ca_sources, including the additional covered
test block, with the repository’s check_values or value_scenarios! helper.
Express each TOML input and expected DpfDpuAgentBootstrapCa result as table
entries, preserving validation assertions and named failure context through the
helper’s established pattern.
🪄 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: e5910504-09e0-4aa3-b6b3-c85867cef208
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (40)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Around line 91-92: Update the bootstrap-ca-url argument in the DaemonSet
template to quote the complete rendered argument using Helm’s quote function,
including the interpolated $bootstrapCaUrl value. Preserve the existing
conditional and argument prefix.
In `@pxe/templates/user-data`:
- Around line 455-465: Update the embedded and mounted branches in the bootstrap
CA selection logic to validate that the nonempty CA file contains a parseable
PEM certificate before installing or accepting it, rather than relying only on
the `-s` check. Reject malformed nonempty input with an error and nonzero exit,
and add coverage for invalid nonempty CA data in both modes.
🪄 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: 25bb8e4e-d47c-4193-b5c8-422ca92ee3a6
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (40)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (29)
- crates/agent/src/tests/mod.rs
- helm/values.yaml
- crates/host-support/src/lib.rs
- crates/pxe/src/routes/tls.rs
- dev/docker/Dockerfile.build-artifacts-container-aarch64
- crates/api-core/src/setup.rs
- crates/api-core/src/handlers/pxe.rs
- crates/pxe/src/metrics.rs
- helm/charts/nico-pxe/values.yaml
- crates/agent/Cargo.toml
- crates/api-core/src/handlers/client_resolution.rs
- book/src/configuration/configurability.md
- pxe/common_files/stage-bootstrap-ca.sh
- docs/dpu-management/dpu_configuration.md
- crates/api-core/src/test_support/default_config.rs
- crates/pxe/src/common.rs
- crates/agent/src/tests/bootstrap_ca.rs
- docs/provisioning/ip-and-network-configuration.md
- docs/bootable_artifacts.md
- crates/agent/src/lib.rs
- crates/api-core/src/dpf_services.rs
- crates/api-core/src/cfg/README.md
- crates/pxe/src/config.rs
- pxe/Makefile.toml
- crates/agent/src/command_line.rs
- crates/pxe/src/routes/cloud_init.rs
- crates/api-core/src/cfg/file.rs
- crates/api-core/src/ipxe.rs
- crates/api-core/src/tests/ipxe.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pxe/common_files/stage-bootstrap-ca_test.sh (1)
84-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover successful replacement of an existing CA file.
Success cases only stage into fresh paths. Prepopulate a destination with the single-cert bundle, stage
multi_cainto it, then assert the bytes and mode changed. This protects the replacement path in addition to existing failure-preservation and stale-removal coverage.🤖 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 `@pxe/common_files/stage-bootstrap-ca_test.sh` around lines 84 - 96, Extend the staging tests after the existing success cases to prepopulate a destination with valid_ca, run stage_script with multi_ca targeting that destination, and assert the destination bytes match multi_ca and its file mode is updated as expected. Keep the existing failure-preservation and stale-removal checks unchanged.
🤖 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 `@pxe/common_files/stage-bootstrap-ca_test.sh`:
- Line 39: Update the permission assertion in the stage-bootstrap-ca test to use
a small POSIX sh helper that obtains the file mode through GNU/BSD-compatible
stat options, then compare the normalized result with 0644 while preserving the
existing failure message.
---
Nitpick comments:
In `@pxe/common_files/stage-bootstrap-ca_test.sh`:
- Around line 84-96: Extend the staging tests after the existing success cases
to prepopulate a destination with valid_ca, run stage_script with multi_ca
targeting that destination, and assert the destination bytes match multi_ca and
its file mode is updated as expected. Keep the existing failure-preservation and
stale-removal checks unchanged.
🪄 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: 95bdc355-c0ba-4d74-ab58-31a77e1f1e52
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (40)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (35)
- crates/host-support/src/lib.rs
- crates/pxe/src/common.rs
- dev/docker/Dockerfile.build-artifacts-container-aarch64
- crates/api-core/src/handlers/client_resolution.rs
- crates/api-core/src/handlers/pxe.rs
- crates/pxe/src/metrics.rs
- helm/values.yaml
- helm/charts/nico-pxe/values.yaml
- crates/agent/src/tests/mod.rs
- bluefield/charts/nico-dpu-agent/values.yaml
- crates/host-support/src/bootstrap_ca.rs
- docs/bootable_artifacts.md
- crates/rpc/proto/forge.proto
- helm/charts/nico-pxe/tests/bootstrap_root_ca_test.yaml
- docs/provisioning/ip-and-network-configuration.md
- crates/api-core/src/test_support/default_config.rs
- crates/pxe/src/routes/tls.rs
- crates/api-core/src/setup.rs
- docs/dpu-management/dpu_configuration.md
- crates/agent/Cargo.toml
- crates/agent/src/command_line.rs
- crates/agent/src/tests/bootstrap_ca.rs
- pxe/common_files/stage-bootstrap-ca.sh
- pxe/templates/user-data
- crates/pxe/src/config.rs
- crates/api-core/src/tests/ipxe.rs
- bluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yaml
- book/src/configuration/configurability.md
- crates/api-core/src/cfg/README.md
- crates/api-core/src/dpf_services.rs
- crates/pxe/src/routes/cloud_init.rs
- pxe/Makefile.toml
- crates/agent/src/lib.rs
- crates/api-core/src/ipxe.rs
- docs/manuals/dpf.md
Contributor
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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Line 7: Update the bootstrapCaObjectKey initialization in the daemonset
template to use hasKey on bootstrapCaObject, preserving an explicitly configured
empty key instead of applying the "ca.crt" default. Continue defaulting to
"ca.crt" only when the key is omitted, so the existing later validation can fail
for an empty 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: 8c5f955a-9ce6-4230-8f66-9d3179f96cfb
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (41)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (35)
- crates/api-core/src/handlers/pxe.rs
- dev/docker/Dockerfile.build-artifacts-container-aarch64
- helm/values.yaml
- docs/dpu-management/dpu_configuration.md
- crates/api-core/src/test_support/default_config.rs
- crates/rpc/proto/forge.proto
- crates/agent/src/tests/mod.rs
- pxe/common_files/stage-bootstrap-ca.sh
- helm/charts/nico-pxe/tests/bootstrap_root_ca_test.yaml
- crates/api-core/src/handlers/client_resolution.rs
- crates/pxe/src/metrics.rs
- crates/host-support/src/lib.rs
- crates/host-support/src/bootstrap_ca.rs
- crates/api-core/src/setup.rs
- crates/pxe/src/common.rs
- crates/agent/src/tests/bootstrap_ca.rs
- pxe/templates/user-data
- crates/pxe/src/routes/tls.rs
- bluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yaml
- book/src/configuration/configurability.md
- bluefield/charts/nico-dpu-agent/values.yaml
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/agent/Cargo.toml
- crates/api-core/src/dpf_services.rs
- crates/pxe/src/routes/cloud_init.rs
- crates/pxe/src/config.rs
- docs/provisioning/ip-and-network-configuration.md
- docs/bootable_artifacts.md
- crates/agent/src/lib.rs
- pxe/Makefile.toml
- crates/api-core/src/ipxe.rs
- docs/manuals/dpf.md
- crates/api-core/src/tests/ipxe.rs
- crates/agent/src/command_line.rs
- crates/api-core/src/cfg/README.md
chet
force-pushed
the
gh-issue-3571
branch
2 times, most recently
from
July 15, 2026 23:43
c6cc72a to
32d2ab2
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/agent/src/command_line.rs (1)
656-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the bootstrap-CA URL cases into one table-driven test.
These tests repeat the same
Options::try_parse_fromoperation for valid, malformed, and unsupported URLs. Usescenarios!with expected success or error text to keep the parser contract in one table.As per coding guidelines, input-to-output and input-to-error mappings must use the repository’s table-driven test utilities.
🤖 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/agent/src/command_line.rs` around lines 656 - 697, Consolidate the three bootstrap-CA URL tests into one table-driven test using the repository’s scenarios! utility. Define cases covering the custom HTTPS URL, malformed input, and unsupported file scheme, with each case asserting either the parsed URL or the expected error text; reuse the existing Options::try_parse_from and init-container parsing assertions within the scenario loop.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/agent/src/tests/bootstrap_ca.rs`:
- Around line 42-51: Update TestServer::shutdown to ignore or otherwise tolerate
the oneshot sender’s send error, ensuring teardown does not panic when the
server has already exited. Always proceed to await self.task so any actual
server-task failure is reported through the join handle.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 1094-1099: Update the mounted-source validation in validate() to
verify that name and key values are valid Kubernetes object names and data keys,
not merely non-empty strings. Reuse the repository’s existing Kubernetes
validation helpers or conventions, and return clear validation errors for
invalid values while preserving the current mounted-source checks.
In `@pxe/templates/user-data`:
- Around line 506-513: Update install_embedded_bootstrap_ca so the validated CA
is written to a temporary file in the destination directory, then atomically
renamed over the destination only after install succeeds. Preserve the existing
permissions and validation behavior, and clean up any temporary artifact when
installation fails.
---
Nitpick comments:
In `@crates/agent/src/command_line.rs`:
- Around line 656-697: Consolidate the three bootstrap-CA URL tests into one
table-driven test using the repository’s scenarios! utility. Define cases
covering the custom HTTPS URL, malformed input, and unsupported file scheme,
with each case asserting either the parsed URL or the expected error text; reuse
the existing Options::try_parse_from and init-container parsing assertions
within the scenario loop.
🪄 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: 8fca158f-413b-4eae-b727-555f07a38dfa
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (41)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/agent/src/tests/bootstrap_ca.rs (2)
24-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CancellationTokenfor test-server shutdown.This
oneshot::Sender<()>is used only for cancellation. Replace it withtokio_util::sync::CancellationToken, cancel it inshutdown, and await the task as today. As per coding guidelines, “use CancellationToken rather than oneshot::Sender<()> for cancellation.”🤖 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/agent/src/tests/bootstrap_ca.rs` around lines 24 - 52, Replace TestServer’s shutdown oneshot::Sender<()> with tokio_util::sync::CancellationToken, clone the token when constructing the test server task, and trigger cancellation via cancel() in TestServer::shutdown before awaiting the task. Remove the now-unused oneshot import while preserving the existing task-await behavior.Sources: Coding guidelines, Path instructions
159-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s table-test helper.
These cases invoke the same operation with variant inputs; express them with
check_cases/scenarios!rather than a manual loop. As per coding guidelines, “use table-driven tests … with carbide-test-support macros.”🤖 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/agent/src/tests/bootstrap_ca.rs` around lines 159 - 187, Replace the manual cases array and loop in the bootstrap CA test with the repository’s table-test helper using check_cases/scenarios! and carbide-test-support macros. Preserve every existing input scenario and assertion, including failure expectations and verification that the existing trust anchor remains unchanged.Sources: Coding guidelines, Path instructions
crates/api-core/src/ipxe.rs (1)
687-729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
value_scenarios!for this mapping test.This helper is a total input-to-output mapping and should use the repository-standard table macro instead of a manual array loop.
As per coding guidelines, use
value_scenarios!for total operations.🤖 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/ipxe.rs` around lines 687 - 729, Replace the manual cases array and loop in bootstrap_ca_kernel_arguments_apply_only_to_dpus with the repository-standard value_scenarios! macro, preserving all existing BootstrapCaSource/MachineType inputs, expected strings, and descriptive case names.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.
Nitpick comments:
In `@crates/agent/src/tests/bootstrap_ca.rs`:
- Around line 24-52: Replace TestServer’s shutdown oneshot::Sender<()> with
tokio_util::sync::CancellationToken, clone the token when constructing the test
server task, and trigger cancellation via cancel() in TestServer::shutdown
before awaiting the task. Remove the now-unused oneshot import while preserving
the existing task-await behavior.
- Around line 159-187: Replace the manual cases array and loop in the bootstrap
CA test with the repository’s table-test helper using check_cases/scenarios! and
carbide-test-support macros. Preserve every existing input scenario and
assertion, including failure expectations and verification that the existing
trust anchor remains unchanged.
In `@crates/api-core/src/ipxe.rs`:
- Around line 687-729: Replace the manual cases array and loop in
bootstrap_ca_kernel_arguments_apply_only_to_dpus with the repository-standard
value_scenarios! macro, preserving all existing BootstrapCaSource/MachineType
inputs, expected strings, and descriptive case names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50230be2-506e-4723-9260-0cd3584eb786
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (41)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlbook/src/configuration/configurability.mdcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/ipxe.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64docs/bootable_artifacts.mddocs/dpu-management/dpu_configuration.mddocs/manuals/dpf.mddocs/provisioning/ip-and-network-configuration.mdhelm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3588.docs.buildwithfern.com/infra-controller |
🔐 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-16 22:34:49 UTC | Commit: ea2271b |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
Transient external failure on |
bcavnvidia
approved these changes
Jul 17, 2026
nick-vetro
approved these changes
Jul 17, 2026
10 tasks
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@bluefield/charts/nico-dpu-agent/templates/daemonset.yaml`:
- Around line 216-231: Make mounted CA rotation update the DaemonSet pod
template by rendering a configurable CA revision or checksum alongside the
bootstrap CA volume in the daemonset template, so Secret/ConfigMap content
changes trigger reprovisioning. In bluefield/charts/nico-dpu-agent/values.yaml
lines 22-34, expose and document the operator-facing rotation value, or require
and document immutable versioned CA object names; ensure the chosen behavior is
applied consistently at both sites.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 1116-1130: Update is_valid_kubernetes_object_name to require every
dot-separated label to be at most 63 characters, while retaining the existing
whole-name limit and character validation. Adjust the related tests by replacing
the single-label 253-character success case with a valid dotted 253-character
name, and add a rejection case for a 64-character label. Ensure the
configuration validation remains safe for Kubernetes Secret/ConfigMap rollouts.
In `@pxe/common_files/stage-bootstrap-ca.sh`:
- Around line 44-66: Update the certificate boundary checks in
pxe/common_files/stage-bootstrap-ca.sh lines 44-66 and pxe/templates/user-data
lines 473-495 to detect BEGIN/END markers with optional leading whitespace,
while still rejecting non-certificate PEM blocks. Add indented private-key
rejection cases in pxe/common_files/stage-bootstrap-ca_test.sh lines 85-90 and
pxe/common_files/runtime-bootstrap-ca_test.sh lines 87-90, then regenerate the
PXE artifacts safely.
🪄 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: 00eb36ec-66c6-435f-a8ba-e0ba1a34b636
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (33)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64helm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
wminckler
reviewed
Jul 17, 2026
| let response = reqwest::get(url).await?; | ||
| async fn download_bootstrap_ca(url: &url::Url) -> eyre::Result<Vec<u8>> { | ||
| download_bootstrap_ca_with_timeout(url, BOOTSTRAP_CA_DOWNLOAD_TIMEOUT).await | ||
| } |
Contributor
There was a problem hiding this comment.
this function seems unnecessary
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
Contributor
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 `@crates/agent/src/lib.rs`:
- Around line 107-113: Update the Mounted branch of the bootstrap CA loading
logic to read no more than MAX_BOOTSTRAP_CA_BYTES + 1 bytes, detect and reject
files exceeding the limit before validation, and preserve the existing
read-error context. Add a test covering an oversized mounted CA file and assert
it is rejected.
🪄 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: 48137b5c-48ae-4824-9d84-91e15ec9ca2b
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (33)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64helm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
DPUs currently download and install whatever CA nico-pxe serves over unauthenticated bootstrap networking. The served bundle is also tied to nico-pxe's runtime CA, which leaves operators without a supported site-managed trust source and makes intermediate rotation fragile. This adds backward-compatible bootstrap CA policies for non-DPF and DPF provisioning. It preserves legacy download as the default while allowing operators to select explicit BFB embedding or mounted CA bundles. This separates nico-pxe's served bootstrap bundle from its outbound API trust and validates CA material before installation. Operators can migrate site by site using configuration such as: ```toml # Non-DPF, after building the site BFB with BOOTSTRAP_CA_PATH. [dpu_config] bootstrap_ca_source = "embedded" # DPF, using an existing Secret in the dpu-agent workload namespace. [dpf.dpu_agent_bootstrap_ca] source = "mounted" object_kind = "secret" name = "nico-bootstrap-ca" key = "ca.crt" ``` This supports NVIDIA#3571 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/agent/src/tests/bootstrap_ca.rs (1)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CancellationTokenfor test-server shutdown.
Replace the oneshot cancellation channel with the repository-standard cancellation primitive.Proposed fix
-use tokio::sync::oneshot; use tokio::task::JoinHandle; +use tokio_util::sync::CancellationToken; struct TestServer { url: Url, - shutdown: oneshot::Sender<()>, + shutdown: CancellationToken, task: JoinHandle<()>, } impl TestServer { async fn shutdown(self) { - let _ = self.shutdown.send(()); + self.shutdown.cancel(); self.task.await.unwrap(); } } - let (shutdown, shutdown_receiver) = oneshot::channel(); + let shutdown = CancellationToken::new(); + let shutdown_receiver = shutdown.clone(); @@ .with_graceful_shutdown(async move { - shutdown_receiver.await.ok(); + shutdown_receiver.cancelled().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/agent/src/tests/bootstrap_ca.rs` around lines 24 - 25, Replace the oneshot-based shutdown mechanism in the bootstrap CA test server with tokio-util’s repository-standard CancellationToken. Update the server task and its shutdown signaling to clone, cancel, and await the token appropriately, removing the oneshot import and related channel handling while preserving graceful test-server shutdown.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/pxe/src/config.rs`:
- Around line 41-42: Update the error string returned when reading
FORGE_ROOT_CAFILE_PATH in the configuration loading flow to begin with lowercase
“could not extract” and preserve the remaining wording. Update the corresponding
exact match in config_error_kind to use the same lowercase phrase.
---
Nitpick comments:
In `@crates/agent/src/tests/bootstrap_ca.rs`:
- Around line 24-25: Replace the oneshot-based shutdown mechanism in the
bootstrap CA test server with tokio-util’s repository-standard
CancellationToken. Update the server task and its shutdown signaling to clone,
cancel, and await the token appropriately, removing the oneshot import and
related channel handling while preserving graceful test-server shutdown.
🪄 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: f0343b8b-aae5-4315-a2da-3363b9daa80c
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (34)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64helm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/pxe/src/routes/cloud_init.rs (1)
402-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
scenarios!withOutcomehere.parse_bootstrap_ca_sourcereturnsResult, so this table fits the fallible-test pattern better thancheck_values.🤖 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/pxe/src/routes/cloud_init.rs` around lines 402 - 427, Update the bootstrap_ca_source_protobuf_values_fail_closed test to use the scenarios! macro with Outcome entries instead of check_values. Keep the existing parse_bootstrap_ca_source inputs and expected success/error results unchanged, including the unknown value failure.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.
Nitpick comments:
In `@crates/pxe/src/routes/cloud_init.rs`:
- Around line 402-427: Update the
bootstrap_ca_source_protobuf_values_fail_closed test to use the scenarios! macro
with Outcome entries instead of check_values. Keep the existing
parse_bootstrap_ca_source inputs and expected success/error results unchanged,
including the unknown value failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b298ccf-d9fe-46ef-ac79-176365d4a116
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (34)
bluefield/charts/nico-dpu-agent/templates/daemonset.yamlbluefield/charts/nico-dpu-agent/tests/bootstrap_ca_test.yamlbluefield/charts/nico-dpu-agent/values.yamlcrates/agent/Cargo.tomlcrates/agent/src/command_line.rscrates/agent/src/lib.rscrates/agent/src/tests/bootstrap_ca.rscrates/agent/src/tests/mod.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/dpf_services.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/default_config.rscrates/api-core/src/tests/ipxe.rscrates/host-support/src/bootstrap_ca.rscrates/host-support/src/lib.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/metrics.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/tls.rscrates/rpc/proto/forge.protodev/docker/Dockerfile.build-artifacts-container-aarch64helm/charts/nico-pxe/templates/deployment.yamlhelm/charts/nico-pxe/tests/bootstrap_root_ca_test.yamlhelm/charts/nico-pxe/values.yamlhelm/values.yamlpxe/Makefile.tomlpxe/common_files/runtime-bootstrap-ca_test.shpxe/common_files/stage-bootstrap-ca.shpxe/common_files/stage-bootstrap-ca_test.shpxe/templates/user-datarest-api/proto/core/src/v1/nico_nico.proto
Contributor
|
isn't there a standard way of supplying a root ca via k8s? why is there the option to use a secret vs a config-map? |
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
4 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.
DPUs currently download and install the CA served by
nico-pxeover unauthenticated bootstrap networking. The served bundle is also tied to the PXE workload's outbound API trust, leaving operators without a supported site-managed trust source and making intermediate rotation fragile.This PR makes bootstrap CA selection an explicit, backward-compatible product surface across non-DPF and DPF provisioning. It preserves the historical download when no new configuration is supplied, while allowing operators to migrate site by site to an explicit BFB-embedded bundle or an operator-mounted bundle.
This also separates the CA served by
/api/v0/tls/root_cafromnico-pxe's outbound API trust and validates CA material before installation. It does not treat a CA downloaded over the same unauthenticated path as self-authenticating. Operator documentation and rollout guidance are split into #3685 so this code can merge independently.Related issues
Type of Change
Breaking Changes
No configuration means the old behavior. The default
nico-pxeandnico-dpu-agentHelm manifests were compared withmainand remain byte-for-byte identical. Sites can deploy the new build first and opt into a new trust source later without a coordinatedforgedchange.Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
Focused host-support, PXE, dpu-agent, API, and DB-backed cloud-init tests
CA staging tests for single/multi-certificate bundles, CRLF input, malformed PEM, private keys, atomic replacement, permissions, and stale-file removal
Download tests for HTTP status, size, timeout, and URL-scheme handling
Helm lint and default/configured/invalid renders for both charts and the umbrella chart
cargo make clippycargo make carbide-lintscargo make check-workspace-depscargo make check-format-nightlygit diff --checkAdditional Notes
Operator configuration examples
These are independent recipes; they are not meant to be combined.
Keep the historical non-DPF download explicitly (omitting the field is equivalent):
Keep legacy download but serve a stable operator-owned ConfigMap bundle from the NICo umbrella chart:
Use an existing Secret instead when that matches the site's bundle-distribution workflow:
When deploying the
nico-pxesubchart directly, omit the umbrella key:For a non-Helm deployment, mount the bundle into the
nico-pxecontainer and point the process at it:export FORGE_BOOTSTRAP_ROOT_CAFILE_PATH=/etc/nico/bootstrap/roots.pemBuild a site-specific non-DPF BFB and select its embedded bundle:
Use a bundle installed by the non-DPF provisioning environment at
/opt/forge/forge_root.pem:Retain DPF download with a complete HTTP(S) endpoint override:
Mount a DPF Secret:
Or use a ConfigMap already created in each target DPU cluster:
The shared DPF image intentionally does not embed a site CA. Non-DPF embedding requires the explicit build input above and an integrity-protected artifact/boot chain. Mounted modes fail closed rather than silently returning to the network download.