change: discover_dhcp API v6 support#2988
Open
bcavnvidia wants to merge 1 commit into
Open
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. |
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:
WalkthroughAdds DHCPv6 information-request handling and SLAAC GUA observation to ChangesDHCPv6 Protocol Support
Sequence Diagram(s)sequenceDiagram
participant Caller as RPC Caller
participant discover_dhcp
participant parse_discovery_protocol
participant SegmentDB as network_segment DB
participant MachineInterfaceDB as machine_interface DB
participant v6 as dhcp::v6
Caller->>discover_dhcp: DhcpDiscovery{address_family, message_kind, duid}
discover_dhcp->>parse_discovery_protocol: validate (family, kind, DUID)
parse_discovery_protocol-->>discover_dhcp: (address_family, message_kind) or error
discover_dhcp->>SegmentDB: for_relay_all / exact dhcpv6_link_address match
SegmentDB-->>discover_dhcp: NetworkSegment
alt anonymous reserved DHCPv6 INFO request
discover_dhcp-->>Caller: options-only DhcpRecord (no DB writes)
else v6 observation (INFO-REQUEST, non-reserved)
discover_dhcp->>MachineInterfaceDB: create_without_addresses
discover_dhcp->>v6: observe_slaac_address(txn, interface_id, segment, mac)
v6->>MachineInterfaceDB: insert AllocationType::Slaac
v6->>MachineInterfaceDB: sync_hostname_after_address_assignment
discover_dhcp-->>Caller: options-only DHCPv6 DhcpRecord
else stateful DHCPv4 / DHCPv6 Solicit
discover_dhcp->>MachineInterfaceDB: find_or_create_machine_interface_for_family
discover_dhcp->>MachineInterfaceDB: ensure_dhcp_address_for_family
MachineInterfaceDB->>MachineInterfaceDB: sync_hostname_after_address_assignment
discover_dhcp-->>Caller: stateful DhcpRecord
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 4
🤖 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/api-core/src/dhcp/discover.rs`:
- Around line 624-668: The DHCPv6 observation flow in discover.rs is selecting
an authoritative segment, but the observed-interface creation and predicted
promotion paths are recomputing their own candidates and can persist state on a
different segment. Thread the already-selected segment (or its ID) from the
selection logic into the helper used for observed-interface creation and into
the promotion path, so the same NetworkSegment is used for validation,
persistence, and SLAAC/observed row updates. Update the relevant
discover/observation helper calls and any segment lookup logic so they consume
the passed-in selection instead of deriving a new one.
In `@crates/api-core/src/dhcp/v6.rs`:
- Around line 98-124: The SLAAC path in the DHCPv6 ownership check still has a
TOCTOU race because `find_by_address()` and
`machine_interface_address::insert()` are separated by awaits. Update this logic
in the DHCPv6 handler to use the shared DB helper for atomic ownership
validation and insert, or introduce one around `db::machine_interface_address`
that locks the owning segment/address, rechecks ownership, and performs the
write in one transaction so concurrent static/preallocation/SLAAC claims cannot
slip in between.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 716-731: The helper exact_dhcpv6_link_address_segment_ids
currently returns all matching segments, which lets ambiguous DHCPv6
link-address ownership slip through. Update this logic to detect when a single
relay matches more than one exact segment and return an error instead of an
arbitrary candidate, then propagate that error through the
reconciliation/new-row creation callers that consume this helper so they fail
fast on duplicate ownership.
In `@crates/api-db/src/network_segment.rs`:
- Around line 207-216: The lookup in the network segment selection path is using
`find` to return the first segment whose prefix matches `dhcpv6_link_address`,
which can silently hide duplicate exact matches. Update the matching logic in
this branch so it detects when more than one segment in `results` has a prefix
with the same `relay` address and treats that as an invalid/ambiguous
configuration instead of choosing one arbitrarily. Keep the existing flow around
`results`, `segment.prefixes`, and the `.ok_or_else(...)` error path, but make
the duplicate-check explicit before returning a segment.
🪄 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: 8ac15f8a-50b2-4b65-a9e5-2705af789df5
📒 Files selected for processing (18)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/mod.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/dhcp_lease_expiration.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/static_address_management.rscrates/api-db/src/dhcp_record.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/network_segment.rscrates/api-model/src/network_segment/mod.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp/src/mock_api_server.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/dhcp_record.rs
💤 Files with no reviewable changes (6)
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp-server/src/modes/dpu.rs
- crates/rpc/src/model/dhcp_record.rs
- crates/dhcp-server/src/main.rs
- crates/rpc/proto/forge.proto
- crates/rpc/src/lib.rs
🔐 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-06-29 22:47:58 UTC | Commit: 7867478 |
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/02_api_handler_dual_stack
branch
3 times, most recently
from
2d5c345 to
835672b
Compare
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 `@crates/api-db/src/machine_interface.rs`:
- Around line 697-713: The DHCP relay resolver in
network_segments_for_dhcp_relays is narrowing by host NIC type too early, which
can drop the authoritative exact dhcpv6_link_address match. Change the flow to
first fetch all relay candidates with db_network_segment::for_relay_all, then
let the exact DHCPv6 link-address segment win before considering type-based
fallback. Apply ExpectedHostNic::resolved_network_segment_type only to non-exact
prefix candidates, so mismatched exact links fail correctly instead of falling
back to a wrong segment.
In `@crates/rpc/proto/forge.proto`:
- Around line 3339-3348: The proto definition is reusing a reserved field
name/number in the delete request message, which will break generation and
compatibility checks. In forge.proto, update the message containing
common.InstanceId id, optional Issue issue, and optional bool is_repair_tenant
so that either the reserved entry for delete_attribution/field 4 is removed or
the optional DeleteAttribution delete_attribution declaration is deleted, but
not both retained together. Keep the reservation and remove the field if the API
is deprecated, or unreserve it if the field must remain.
In `@crates/rpc/proto/site_explorer.proto`:
- Around line 18-20: The protobuf definition has a conflicting reservation in
site_explorer.proto: the reserved field number and name in the reserved block
conflict with EndpointExplorationReport.last_exploration_error_schema, which
still uses the same identifier and tag. Remove or adjust the reservation so the
message field and reserved declarations are no longer overlapping, keeping
EndpointExplorationReport and its field number consistent for generated clients.
🪄 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: 8e061af5-e8a2-4e96-9dd7-6c7b761258b1
📒 Files selected for processing (19)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/mod.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/dhcp_lease_expiration.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/static_address_management.rscrates/api-db/src/dhcp_record.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/network_segment.rscrates/api-model/src/network_segment/mod.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp/src/mock_api_server.rscrates/rpc/proto/forge.protocrates/rpc/proto/site_explorer.protocrates/rpc/src/lib.rscrates/rpc/src/model/dhcp_record.rs
💤 Files with no reviewable changes (5)
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp-server/src/modes/dpu.rs
- crates/dhcp-server/src/main.rs
- crates/rpc/src/model/dhcp_record.rs
- crates/rpc/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/api-core/src/dhcp/mod.rs
- crates/api-db/src/dhcp_record.rs
- crates/api-core/src/handlers/machine_interface_address.rs
- crates/api-core/src/tests/dhcp_lease_expiration.rs
- crates/api-core/src/tests/static_address_management.rs
- crates/api-db/src/machine.rs
- crates/api-core/src/dhcp/v6.rs
- crates/api-model/src/network_segment/mod.rs
- crates/api-db/src/network_segment.rs
- crates/api-core/src/dhcp/discover.rs
- crates/api-core/src/tests/machine_dhcp.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@crates/api-db/src/machine_interface.rs`:
- Around line 697-713: The DHCP relay resolver in
network_segments_for_dhcp_relays is narrowing by host NIC type too early, which
can drop the authoritative exact dhcpv6_link_address match. Change the flow to
first fetch all relay candidates with db_network_segment::for_relay_all, then
let the exact DHCPv6 link-address segment win before considering type-based
fallback. Apply ExpectedHostNic::resolved_network_segment_type only to non-exact
prefix candidates, so mismatched exact links fail correctly instead of falling
back to a wrong segment.
In `@crates/rpc/proto/forge.proto`:
- Around line 3339-3348: The proto definition is reusing a reserved field
name/number in the delete request message, which will break generation and
compatibility checks. In forge.proto, update the message containing
common.InstanceId id, optional Issue issue, and optional bool is_repair_tenant
so that either the reserved entry for delete_attribution/field 4 is removed or
the optional DeleteAttribution delete_attribution declaration is deleted, but
not both retained together. Keep the reservation and remove the field if the API
is deprecated, or unreserve it if the field must remain.
In `@crates/rpc/proto/site_explorer.proto`:
- Around line 18-20: The protobuf definition has a conflicting reservation in
site_explorer.proto: the reserved field number and name in the reserved block
conflict with EndpointExplorationReport.last_exploration_error_schema, which
still uses the same identifier and tag. Remove or adjust the reservation so the
message field and reserved declarations are no longer overlapping, keeping
EndpointExplorationReport and its field number consistent for generated clients.
🪄 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: 8e061af5-e8a2-4e96-9dd7-6c7b761258b1
📒 Files selected for processing (19)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/mod.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/dhcp_lease_expiration.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/static_address_management.rscrates/api-db/src/dhcp_record.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/network_segment.rscrates/api-model/src/network_segment/mod.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp/src/mock_api_server.rscrates/rpc/proto/forge.protocrates/rpc/proto/site_explorer.protocrates/rpc/src/lib.rscrates/rpc/src/model/dhcp_record.rs
💤 Files with no reviewable changes (5)
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp-server/src/modes/dpu.rs
- crates/dhcp-server/src/main.rs
- crates/rpc/src/model/dhcp_record.rs
- crates/rpc/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/api-core/src/dhcp/mod.rs
- crates/api-db/src/dhcp_record.rs
- crates/api-core/src/handlers/machine_interface_address.rs
- crates/api-core/src/tests/dhcp_lease_expiration.rs
- crates/api-core/src/tests/static_address_management.rs
- crates/api-db/src/machine.rs
- crates/api-core/src/dhcp/v6.rs
- crates/api-model/src/network_segment/mod.rs
- crates/api-db/src/network_segment.rs
- crates/api-core/src/dhcp/discover.rs
- crates/api-core/src/tests/machine_dhcp.rs
🛑 Comments failed to post (3)
crates/api-db/src/machine_interface.rs (1)
697-713: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve DHCPv6 exact link-address authority before NIC-type narrowing.
for_segment_type_allfilters bynetwork_segment_typebefore callers compute exactdhcpv6_link_addressmatches. If the exact link-address segment has a different declared type, it is removed and later code can fall back to prefix candidates, assigning/reconciling the MAC to the wrong segment instead of failing the mismatch.Prefer resolving all relay candidates first, selecting exact DHCPv6 link-address matches as authoritative, and only applying
host_nictype narrowing to non-exact prefix fallback candidates.Proposed direction
pub async fn network_segments_for_dhcp_relays( txn: &mut PgConnection, relays: &[IpAddr], host_nic: Option<&ExpectedHostNic>, ) -> DatabaseResult<Vec<NetworkSegment>> { + let all_segments = db_network_segment::for_relay_all(txn, relays).await?; + let exact_segment_ids = exact_dhcpv6_link_address_segment_ids(&all_segments, relays); + if !exact_segment_ids.is_empty() { + return Ok(all_segments + .into_iter() + .filter(|segment| exact_segment_ids.contains(&segment.id)) + .collect()); + } + if let Some(network_segment_type) = host_nic.and_then(ExpectedHostNic::resolved_network_segment_type) { db_network_segment::for_segment_type_all(txn, relays, network_segment_type).await } else { - db_network_segment::for_relay_all(txn, relays).await + Ok(all_segments) } }As per path instructions, API/database changes should be reviewed for request validation, SQL/query correctness, schema/API compatibility, and tenant isolation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Resolve DHCP candidate network segments for a relay list and optional NIC type. pub async fn network_segments_for_dhcp_relays( txn: &mut PgConnection, relays: &[IpAddr], host_nic: Option<&ExpectedHostNic>, ) -> DatabaseResult<Vec<NetworkSegment>> { let all_segments = db_network_segment::for_relay_all(txn, relays).await?; let exact_segment_ids = exact_dhcpv6_link_address_segment_ids(&all_segments, relays); if !exact_segment_ids.is_empty() { return Ok(all_segments .into_iter() .filter(|segment| exact_segment_ids.contains(&segment.id)) .collect()); } // A declared NIC narrows segment selection to a specific type: when // the relay's prefix matches more than one segment, pick the one of // the declared typed `network_segment_type`, or the legacy `nic_type` // it supersedes. Otherwise the relay's matching segment(s) stand. if let Some(network_segment_type) = host_nic.and_then(ExpectedHostNic::resolved_network_segment_type) { db_network_segment::for_segment_type_all(txn, relays, network_segment_type).await } else { Ok(all_segments) } }🤖 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-db/src/machine_interface.rs` around lines 697 - 713, The DHCP relay resolver in network_segments_for_dhcp_relays is narrowing by host NIC type too early, which can drop the authoritative exact dhcpv6_link_address match. Change the flow to first fetch all relay candidates with db_network_segment::for_relay_all, then let the exact DHCPv6 link-address segment win before considering type-based fallback. Apply ExpectedHostNic::resolved_network_segment_type only to non-exact prefix candidates, so mismatched exact links fail correctly instead of falling back to a wrong segment.Source: Path instructions
crates/rpc/proto/forge.proto (1)
3339-3348: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Remove the reserved
delete_attributionfield declaration.Line 3348 reuses field number
4and namedelete_attributionafter both are reserved on Lines 3339-3340, so proto generation/breaking-change checks will fail. If the field is intentionally removed, delete the remaining declaration; otherwise, remove the reservation.Proposed fix
message InstanceReleaseRequest { reserved 4; reserved "delete_attribution"; common.InstanceId id = 1; // Optional issue information if tenant is reporting a problem optional Issue issue = 2; // Optional flag indicating the call is from repair tenant (default: false) optional bool is_repair_tenant = 3; - // Optional cloud-side attribution for who initiated the delete - optional DeleteAttribution delete_attribution = 4; }As per path instructions,
**/*.protochanges should be reviewed for wire compatibility, stable field numbers, and generated-client impact.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.reserved 4; reserved "delete_attribution"; common.InstanceId id = 1; // Optional issue information if tenant is reporting a problem optional Issue issue = 2; // Optional flag indicating the call is from repair tenant (default: false) optional bool is_repair_tenant = 3;🧰 Tools
🪛 GitHub Check: Proto Breaking Changes Check
[failure] 3348-3348:
use of reserved field number4
[failure] 3348-3348:
use of reserved message field name🤖 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/rpc/proto/forge.proto` around lines 3339 - 3348, The proto definition is reusing a reserved field name/number in the delete request message, which will break generation and compatibility checks. In forge.proto, update the message containing common.InstanceId id, optional Issue issue, and optional bool is_repair_tenant so that either the reserved entry for delete_attribution/field 4 is removed or the optional DeleteAttribution delete_attribution declaration is deleted, but not both retained together. Keep the reservation and remove the field if the API is deprecated, or unreserve it if the field must remain.Sources: Path instructions, Linters/SAST tools
crates/rpc/proto/site_explorer.proto (1)
18-20: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the conflicting reservation.
Lines 18-19 reserve field number
20and the namelast_exploration_error_schema, butEndpointExplorationReportstill defineslast_exploration_error_schema = 20on Line 53.protocrejects that combination, so generated clients will fail until either the reservation or the field is removed.Suggested fix
message EndpointExplorationReport { - reserved 20; - reserved "last_exploration_error_schema"; - // The type of the endpoint string endpoint_type = 1;As per path instructions, review protobuf definitions for wire compatibility, stable field numbers, and generated-client impact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🤖 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/rpc/proto/site_explorer.proto` around lines 18 - 20, The protobuf definition has a conflicting reservation in site_explorer.proto: the reserved field number and name in the reserved block conflict with EndpointExplorationReport.last_exploration_error_schema, which still uses the same identifier and tag. Remove or adjust the reservation so the message field and reserved declarations are no longer overlapping, keeping EndpointExplorationReport and its field number consistent for generated clients.Sources: Path instructions, Linters/SAST tools
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/tests/machine_dhcp.rs (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
MESSAGE_KIND_V6_REQUEST.
discover.rsnow routesV6Request, but the visible DHCPv6 test constants and stateful tests only exerciseV6SolicitandV6InfoRequest. Please add aV6Requestcase, preferably by table-driving one stateful DHCPv6 test over both stateful message kinds. As per coding guidelines, Rust tests should prefer table-driven style for operations with multiple input variants.🤖 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/tests/machine_dhcp.rs` around lines 50 - 54, The DHCPv6 test coverage in machine_dhcp.rs is missing MESSAGE_KIND_V6_REQUEST, so update the existing constants/tests around rpc::forge::MessageKind to include V6Request. Prefer refactoring the stateful DHCPv6 test logic into a table-driven test that iterates over both V6Solicit and V6Request, while keeping V6InfoRequest coverage separate if needed. Use the existing test symbols such as RPC_MESSAGE_KIND_V6_SOLICIT, RPC_MESSAGE_KIND_V6_INFO_REQUEST, and the stateful DHCPv6 test helper to locate and extend the right cases.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/api-core/src/tests/machine_dhcp.rs`:
- Around line 50-54: The DHCPv6 test coverage in machine_dhcp.rs is missing
MESSAGE_KIND_V6_REQUEST, so update the existing constants/tests around
rpc::forge::MessageKind to include V6Request. Prefer refactoring the stateful
DHCPv6 test logic into a table-driven test that iterates over both V6Solicit and
V6Request, while keeping V6InfoRequest coverage separate if needed. Use the
existing test symbols such as RPC_MESSAGE_KIND_V6_SOLICIT,
RPC_MESSAGE_KIND_V6_INFO_REQUEST, and the stateful DHCPv6 test helper to locate
and extend the right cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5de9f959-4e87-4c0f-869a-8414c30be7dc
📒 Files selected for processing (18)
crates/api-core/src/dhcp/discover.rscrates/api-core/src/dhcp/mod.rscrates/api-core/src/dhcp/v6.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/src/tests/dhcp_lease_expiration.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/static_address_management.rscrates/api-db/src/dhcp_record.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/network_segment.rscrates/api-model/src/network_segment/mod.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp/src/mock_api_server.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/rpc/src/model/dhcp_record.rs
💤 Files with no reviewable changes (5)
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp-server/src/main.rs
- crates/rpc/src/model/dhcp_record.rs
- crates/dhcp-server/src/modes/dpu.rs
- crates/rpc/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/api-core/src/dhcp/mod.rs
- crates/api-core/src/handlers/machine_interface_address.rs
- crates/api-model/src/network_segment/mod.rs
- crates/api-core/src/tests/dhcp_lease_expiration.rs
- crates/api-db/src/dhcp_record.rs
- crates/api-db/src/machine.rs
- crates/api-core/src/tests/static_address_management.rs
- crates/api-core/src/dhcp/v6.rs
- crates/api-db/src/network_segment.rs
- crates/api-db/src/machine_interface.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/02_api_handler_dual_stack
branch
2 times, most recently
from
910886b to
284cba8
Compare
…y and message kind to continue v6 support
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/02_api_handler_dual_stack
branch
from
284cba8 to
fd25a14
Compare
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
1 participant
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.
This implements Milestone 02 of the IPv6 DHCP plan:
discover_dhcpnow routes byaddress_familyandmessage_kind, supports DHCPv6 stateful requests, and adds the DHCPv6 INFORMATION-REQUEST/SLAAC observation path while preserving the existing IPv4 behaviorI tried to have code-comments keep things easier to follow, but here's a curated list of highlights:
Route DHCP discovery through explicit protocol fields:
Add DHCPv6 options-only response handling
Add SLAAC observation support:
/64Extend DHCPv6 relay resolution:
dhcpv6_link_addressequality can resolve segments even when the link-address is outside the managed prefixPreserve global MAC/segment consistency:
DHCPv6 lifetimes were intentionally removed from
DhcpRecord.The addition wasn't caught in the previous MR, and lifetimes are supplied by the DHCP server configuration rather than api-core. These fields can be safely removed because nothing depends on them yet.(Looks like our overzealous proto lint forces a reservation)NOTE: The proto changes seems to be triggering lint issues with the Go codebase, so I've had to include regenerated .pb.go files in this PR.
Related issues
#2384
Type of Change
Breaking Changes
Testing
Additional Notes
Closes #2384