feat(dhcp-server): support configurable UDP ports#3699
Conversation
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Summary by CodeRabbit
WalkthroughDHCP server CLI arguments now configure the IPv4 listen address and relay response port. These values propagate through server configuration to UDP binding and relayed packet routing, with tests covering defaults, overrides, IPv6 rejection, and destination ports. ChangesDHCP socket configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DHCPServer
participant Config
participant PacketHandler
participant UDPSocket
CLI->>DHCPServer: Provide listen_addr and relay_response_port
DHCPServer->>Config: Store relay_response_port
DHCPServer->>UDPSocket: Bind listen_addr per interface
PacketHandler->>Config: Read relay_response_port
PacketHandler->>UDPSocket: Send relayed response to configured port
🚥 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/dhcp-server/src/command_line.rs (1)
85-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the CLI parser test table-driven.
dhcp_port_argumentshard-codes several argv/result cases in one linear test. Since this maps inputs to parsed values and errors, use a table of cases (check_cases/check_valuesor an equivalent) covering defaults, overrides, IPv6 rejection, and port-zero rejection.As per coding guidelines, tests mapping inputs to outputs or errors should be table-driven.
🤖 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 85 - 121, Convert the dhcp_port_arguments test into a table-driven structure with separate cases for defaults, overridden values, IPv6 listen-address rejection, and port-zero rejection. Add reusable helpers or equivalent case handling to assert expected parsed values and errors, while preserving the current defaults and override expectations.Source: Coding guidelines
crates/dhcp-server/src/packet_handler.rs (1)
191-198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for direct-client unicast routing.
The configurable port should affect only relayed packets. Current assertions cover a custom relay destination and broadcast port 68, but no case exercises
giaddr == 0withciaddr != 0; add one asserting that direct-client replies still target port 68.Based on path instructions, DHCP/networking changes should include tests for routing correctness and boundary behavior.
🤖 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/packet_handler.rs` around lines 191 - 198, Add a routing test covering the direct-client case in decide_dst_ip: use a packet with giaddr set to 0 and ciaddr nonzero, configure a custom relay response port, and assert the destination remains the client address on port 68. Keep existing relayed and broadcast assertions unchanged.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.
Inline comments:
In `@crates/dhcp-server/src/command_line.rs`:
- Around line 28-40: Update the argument parsing for listen_addr and
relay_response_port in the command-line configuration to reject port 0 before
server startup. Validate the embedded SocketAddrV4 port and the
relay_response_port value, preserving valid non-zero ports, and add regression
cases covering zero-port inputs for both fields.
---
Nitpick comments:
In `@crates/dhcp-server/src/command_line.rs`:
- Around line 85-121: Convert the dhcp_port_arguments test into a table-driven
structure with separate cases for defaults, overridden values, IPv6
listen-address rejection, and port-zero rejection. Add reusable helpers or
equivalent case handling to assert expected parsed values and errors, while
preserving the current defaults and override expectations.
In `@crates/dhcp-server/src/packet_handler.rs`:
- Around line 191-198: Add a routing test covering the direct-client case in
decide_dst_ip: use a packet with giaddr set to 0 and ciaddr nonzero, configure a
custom relay response port, and assert the destination remains the client
address on port 68. Keep existing relayed and broadcast assertions 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: c36e4a20-9371-4c0e-b9d8-cedb3553fef3
📒 Files selected for processing (4)
crates/dhcp-server/src/command_line.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rs
| #[arg( | ||
| long, | ||
| help = "UDP address where the DHCP server listens.", | ||
| default_value = "0.0.0.0:67" | ||
| )] | ||
| pub listen_addr: SocketAddrV4, | ||
|
|
||
| #[arg( | ||
| long, | ||
| help = "UDP destination port for responses to DHCP relays.", | ||
| default_value_t = 67 | ||
| )] | ||
| pub relay_response_port: u16, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject port 0 during argument parsing.
Both new fields currently accept zero: SocketAddrV4 permits :0, causing an ephemeral server bind, while relay_response_port = 0 is not a usable DHCP relay destination. Validate both ports as non-zero before starting the server and add regression cases.
As per path instructions, DHCP/networking changes must be reviewed for address-allocation correctness and boundary behavior.
🤖 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, Update the
argument parsing for listen_addr and relay_response_port in the command-line
configuration to reject port 0 before server startup. Validate the embedded
SocketAddrV4 port and the relay_response_port value, preserving valid non-zero
ports, and add regression cases covering zero-port inputs for both fields.
Source: Path instructions
1 participant
Allow
forge-dhcp-serverto use configurable UDP ports instead of requiring the standard privileged DHCP ports in every environment.The server previously hardcoded both its listen address to
0.0.0.0:67and relayed response destinations to UDP port 67. This prevented local and integration environments from exercising the real DHCP packet path without elevated privileges.This change adds:
--listen-addr, defaulting to0.0.0.0:67.--relay-response-port, defaulting to67.The defaults preserve existing production behavior. Direct client and broadcast responses continue to use UDP port 68. Because the server creates an IPv4 socket, IPv6 listen addresses are rejected during argument parsing instead of failing later during socket setup.
Related issues
#3366
Type of Change
Breaking Changes
Testing
Additional Notes