feat(machine-a-tron): support DHCP relay mode#3754
Conversation
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Summary by CodeRabbit
WalkthroughMachine-a-tron now supports configurable API-based or UDP-relayed DHCP. The change adds relay packet exchange and validation, routes state-machine requests through a shared DHCP client, and manages relay service startup and shutdown in production and local test harnesses. ChangesDHCP relay support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MachineATron
participant DhcpClient
participant UdpDhcpClient
participant DhcpServer
MachineATron->>DhcpClient: start(config, api_client)
DhcpClient->>UdpDhcpClient: start relay when configured
MachineATron->>DhcpClient: request_ip(request_info)
DhcpClient->>UdpDhcpClient: dispatch UDP-relay request
UdpDhcpClient->>DhcpServer: send DHCP Discover and Request
DhcpServer-->>UdpDhcpClient: return Offer and Ack
UdpDhcpClient-->>DhcpClient: return DHCP response info
DhcpClient-->>MachineATron: return assigned IP and interface data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/machine-a-tron/src/config.rs`:
- Around line 412-424: Add explicit deserialization validation for the empty
`DhcpType::Api` variant so any relay-only or other unexpected fields are
rejected when `type = "api"`. Preserve successful parsing of a field-free API
configuration and the existing `UdpRelay` field validation; implement the guard
at the `DhcpType` deserialization boundary rather than relying solely on
`deny_unknown_fields`.
🪄 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: 6ef9e08b-d7b5-4e22-85ea-77131a2b660d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/api-integration-tests/tests/lib.rscrates/api-test-helper/src/machine_a_tron.rscrates/machine-a-tron/Cargo.tomlcrates/machine-a-tron/config/mat.tomlcrates/machine-a-tron/src/config.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper_udp.rscrates/machine-a-tron/src/lib.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/main.rs
| #[derive(Clone, Copy, Debug, Deserialize, Serialize, Eq, PartialEq)] | ||
| #[serde(tag = "type", rename_all = "snake_case", deny_unknown_fields)] | ||
| pub enum DhcpType { | ||
| Api {}, | ||
| UdpRelay { | ||
| /// Destination for relayed DHCP packets. | ||
| server_address: SocketAddrV4, | ||
| /// Local UDP socket used to receive relayed DHCP replies. | ||
| listen_address: SocketAddrV4, | ||
| /// Reachable IPv4 address placed in DHCP `giaddr` for server replies. | ||
| advertise_address: Ipv4Addr, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant config file and serde version pins.
git ls-files 'crates/machine-a-tron/src/config.rs' 'Cargo.toml' 'Cargo.lock' | sed 's/^/FILE /'
echo
echo "=== config.rs excerpt ==="
sed -n '412,424p' crates/machine-a-tron/src/config.rs
echo
echo "=== serde version references ==="
rg -n 'serde( |"|=)|serde_json' Cargo.toml Cargo.lock crates -g 'Cargo.toml' -g 'Cargo.lock' -g '*.toml' -g '*.rs' | sed -n '1,120p'Repository: NVIDIA/infra-controller
Length of output: 4183
🌐 Web query:
serde internally tagged enum deny_unknown_fields empty struct variant unknown fields
💡 Result:
In Serde, the #[serde(deny_unknown_fields)] attribute often fails to behave as expected when used with internally tagged enums, particularly for unit variants or empty struct variants [1][2][3]. There are several known limitations and open issues regarding this combination: 1. Failure to Deny on Unit/Empty Struct Variants: When an internally tagged enum uses deny_unknown_fields, it often continues to accept unknown fields when deserializing unit variants (e.g., Variant or Variant {}) [1][2][3]. The deserializer may ignore extraneous fields in the input that do not match the expected structure, contrary to the attribute's purpose [1][2][3]. 2. Architectural Complexity: The implementation of deny_unknown_fields relies on the deserializer being able to track and consume all fields provided in the input [4][5]. Internally tagged enums, however, must often inspect the map/structure to determine which variant is being deserialized, which can interfere with the field-tracking mechanism [4][5]. 3. Flattening Conflicts: Using deny_unknown_fields in conjunction with #[serde(flatten)] is explicitly documented as unsupported [6][7]. This combination often leads to errors because the flattening mechanism buffers fields, making it difficult for the deserializer to determine which fields have been "consumed" versus which are genuinely "unknown" [4][5]. Due to these complexities, these behaviors are well-documented in the Serde issue tracker as limitations rather than bugs that have been resolved [1][2][3]. If strict validation is required, you may need to implement a custom Deserialize manual implementation or perform validation after deserialization.
Citations:
- 1: Internally tagged enum with
deny_unknown_fieldsaccepts unknown fields serde-rs/serde#2123 - 2:
#![serde(deny_unknown_fields)]does not work as expected on unit variants of tagged enum serde-rs/serde#2294 - 3: Unknown fields are denied for tagged newtype variant with unit struct or unit type serde-rs/serde#2304
- 4: Combination of flattened internally-tagged enum and deny_unknown_fields results in unsatisfiable requirements serde-rs/serde#1358
- 5: can serialize but can't deserilize when combined
tag = ""withflattenserde-rs/serde#2984 - 6: https://serde.rs/container-attrs
- 7: https://serde.rs/container-attrs.html
🌐 Web query:
serde 1.0.228 internally tagged enum deny_unknown_fields empty struct variant unknown fields
💡 Result:
In Serde version 1.0.228, using #[serde(deny_unknown_fields)] on an internally tagged enum does not reliably reject unknown fields when applied to unit variants or empty struct variants [1][2][3]. This is a known limitation in how Serde handles deny_unknown_fields for internally tagged enums [1][2][3]. When an enum is internally tagged, Serde must often parse the input to identify the variant [4][5]. In cases involving unit variants or variants that can be represented as empty structs, Serde may not correctly track or enforce the "unknown fields" constraint because it does not treat the variant's payload as a fixed set of fields that must be exhausted [1][3]. As a result, deserialization may succeed even when extraneous fields are present in the input, contrary to the expected behavior of deny_unknown_fields [1][2][3]. There is no standard workaround for this behavior within the current implementation of Serde, as changing it would potentially be a breaking change for existing deserialization logic [3]. Issues tracking this limitation have been discussed in the Serde issue tracker, noting that it particularly affects unit and empty struct variants [1][2][3].
Citations:
- 1:
#![serde(deny_unknown_fields)]does not work as expected on unit variants of tagged enum serde-rs/serde#2294 - 2: Internally tagged enum with
deny_unknown_fieldsaccepts unknown fields serde-rs/serde#2123 - 3: Unknown fields are denied for tagged newtype variant with unit struct or unit type serde-rs/serde#2304
- 4: https://serde.rs/enum-representations.html
- 5: Combination of flattened internally-tagged enum and deny_unknown_fields results in unsatisfiable requirements serde-rs/serde#1358
Add an explicit guard for stray relay fields on DhcpType::Api
deny_unknown_fields is not a safe contract here: DhcpType::Api {} is an internally tagged empty-struct variant, and Serde can still accept unknown keys on that arm. Add explicit validation so type = "api" with relay-only fields fails deterministically.
🤖 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/machine-a-tron/src/config.rs` around lines 412 - 424, Add explicit
deserialization validation for the empty `DhcpType::Api` variant so any
relay-only or other unexpected fields are rejected when `type = "api"`. Preserve
successful parsing of a field-free API configuration and the existing `UdpRelay`
field validation; implement the guard at the `DhcpType` deserialization boundary
rather than relying solely on `deny_unknown_fields`.
2 participants
Add a UDP DHCP relay mode to machine-a-tron so simulated machines can exercise the DHCP server through its packet interface instead of obtaining leases directly through the Carbide API.
The new mode performs the DHCP DISCOVER/OFFER and REQUEST/ACK exchanges over UDP, including relay-agent information and the vendor class derived from the simulated hardware. Responses are correlated and validated before completing requests, and pending transactions are cleaned up on timeout or cancellation.
DHCP configuration is represented as a tagged enum so each mode contains all of its required parameters. API mode remains the default for backward compatibility. The local test helper now starts and retains the configured DHCP service, allowing integration environments to use UDP relay mode as well.
Related issues
Closes: #3366
Type of Change
Breaking Changes
Testing
Additional Notes
UDP relay mode requires:
The advertised address is placed in
giaddr, while the machine’s target subnet is carried using the relay-agent link-selection option. In Kubernetes, the listener may bind to0.0.0.0, with the pod IP supplied as the advertised address.