fix(dhcp-server): make API client certificates configurable#3704
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. |
WalkthroughThe DHCP server gains CLI-configurable Forge TLS paths, propagates the resulting client configuration into RPC discovery, uses the configured listen address for UDP sockets, and validates relay response routing and broadcast destinations. ChangesDHCP configuration flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Args
participant init
participant ApiConfig
participant ForgeTlsClient
Args->>init: provide Forge TLS paths
init->>ApiConfig: create configuration from forge_client_config
ApiConfig->>ForgeTlsClient: configure TLS client
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
be013f4 to
8702ad4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/dhcp-server/src/command_line.rs (1)
28-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject port zero for DHCP sockets and relay responses.
Both fields accept
0:listen_addrcan bind an ephemeral UDP port, whilerelay_response_portcan route replies to UDP port zero. Sincecrates/dhcp-server/src/main.rsuses these values for socket binding and packet destinations, this can make the server unreachable or relay replies undeliverable. Reject zero during parsing/config validation and add regression tests for both options.🤖 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/dhcp-server/src/command_line.rs` around lines 28 - 40, Reject port zero for both the listen_addr and relay_response_port CLI options before they reach socket binding or relay destination construction. Add parsing or configuration validation that accepts ports 1–65535 while producing clear errors for zero, and add regression tests covering --listen-addr with port 0 and --relay-response-port 0.
🧹 Nitpick comments (1)
crates/dhcp-server/src/command_line.rs (1)
111-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the CLI parser tests table-driven.
This test mixes several input/output/error mappings in one imperative sequence. Refactor the cases into a table (or the repository’s
check_cases/scenario helpers), and add the symmetric key-without-certificate case so both credential requirements are covered.As per coding guidelines,
**/*.{rs,toml}requires table-driven tests for mappings 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/dhcp-server/src/command_line.rs` around lines 111 - 163, Refactor the dhcp_port_arguments test into table-driven cases covering default values, overridden ports, TLS credentials, and invalid inputs. Use the repository’s established check_cases/scenario helper if available, and add the missing invalid case where --client-key-path is provided without --client-cert-path; preserve the existing expected outputs and errors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/dhcp-server/src/command_line.rs`:
- Around line 28-40: Reject port zero for both the listen_addr and
relay_response_port CLI options before they reach socket binding or relay
destination construction. Add parsing or configuration validation that accepts
ports 1–65535 while producing clear errors for zero, and add regression tests
covering --listen-addr with port 0 and --relay-response-port 0.
---
Nitpick comments:
In `@crates/dhcp-server/src/command_line.rs`:
- Around line 111-163: Refactor the dhcp_port_arguments test into table-driven
cases covering default values, overridden ports, TLS credentials, and invalid
inputs. Use the repository’s established check_cases/scenario helper if
available, and add the missing invalid case where --client-key-path is provided
without --client-cert-path; preserve the existing expected outputs and errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ecb5b78-d522-45b9-b2b6-8cc8d4a04256
📒 Files selected for processing (3)
crates/dhcp-server/src/command_line.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/rpc/client.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dhcp-server/src/rpc/client.rs
- crates/dhcp-server/src/main.rs
1 participant
The DHCP server currently uses hardcoded
/opt/forgecertificate paths when calling the Carbide API. This prevents controller mode from running in environments where certificates are mounted elsewhere, including local development.Add command-line options for the Carbide API root CA, client certificate, and client key. The resolved TLS configuration is created once during startup and reused for DHCP discovery calls. Existing
/opt/forgepaths remain the defaults, preserving deployed behavior.The client certificate and key must be provided together to avoid starting with an incomplete mTLS identity.
Related issues
#3366
Type of Change
Breaking Changes
Testing
Additional Notes
Existing deployments do not need to specify the new arguments. When omitted, the DHCP server continues using the standard
/opt/forgecertificate paths.