feat(dhcp): implemented DHCPv6 Kea callouts#3435
Draft
bcavnvidia wants to merge 1 commit into
Draft
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
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 end-to-end authoritative DHCPv6 support, including packet decoding, identity selection, protocol-scoped caching, Kea callouts, IPv6 options, lease lifecycle handling, metrics, test harnesses, integration tests, and container packaging. ChangesDHCPv6 runtime foundation
DHCPv6 discovery and caching
Kea DHCPv6 hooks
Test infrastructure and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Kea as Kea DHCPv6
participant Callouts as DHCPv6 callouts
participant Rust as machine_v6
participant Cache as DHCPv6 cache
participant API as Carbide API
Kea->>Callouts: pkt6_receive
Callouts->>Rust: carbide_pkt6_receive
Rust->>Cache: lookup cached machine
Rust->>API: discover_dhcp on cache miss
API-->>Rust: DHCPv6 machine record
Rust-->>Callouts: machine and hook result
Callouts->>Callouts: enforce address and set options
Callouts-->>Kea: DHCPv6 response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
from
6d8c5a9 to
d07e835
Compare
![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: 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 `@crates/dhcp/src/kea/callouts.cc`:
- Around line 1415-1421: Update the DHCPv6 send path in pkt6_send near the
existing LOG_CARBIDE_PKT6_SEND call to invoke carbide_increment_reply_sent after
the response is prepared, ensuring DHCPv6 replies receive a send-side count
comparable to pkt4_send. Reuse the existing counter unless the metrics design
explicitly requires separate protocol-specific tracking.
In `@crates/dhcp/src/kea/loader.cc`:
- Around line 97-100: Update the next_server type validation in the loader’s
parameter-validation flow to emit a LOG_ERROR describing the invalid
carbide-provisioning-server-ipv4 value before returning false. Match the logging
style and diagnostic detail used by the other validators in this file, while
preserving the existing rejection 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: 4478b164-5296-4189-bccd-19cdbf6b5023
📒 Files selected for processing (30)
crates/dhcp/Cargo.tomlcrates/dhcp/examples/kea-dhcp6-carbide.confcrates/dhcp/src/cache.rscrates/dhcp/src/discovery.rscrates/dhcp/src/discovery_v6.rscrates/dhcp/src/kea/callouts.cccrates/dhcp/src/kea/callouts.hcrates/dhcp/src/kea/loader.cccrates/dhcp/src/lease_expiration.rscrates/dhcp/src/lib.rscrates/dhcp/src/machine.rscrates/dhcp/src/machine_v6.rscrates/dhcp/src/metrics.rscrates/dhcp/src/mock_api_server.rscrates/dhcp/tests/common/dhcpv6_factory.rscrates/dhcp/tests/common/kea_v6.rscrates/dhcp/tests/common/mod.rscrates/dhcp/tests/identity_v6_test.rscrates/dhcp/tests/info_request_v6_test.rscrates/dhcp/tests/lease6_hook_test.rscrates/dhcp/tests/rapid_commit_v6_test.rsdev/deployment/localdev/Dockerfile.runtime-container.localdevdev/docker/Dockerfile.build-container-aarch64dev/docker/Dockerfile.build-container-x86_64dev/docker/Dockerfile.pxe-build-containerdev/docker/Dockerfile.release-container-aarch64dev/docker/Dockerfile.release-container-sa-x86_64dev/docker/Dockerfile.release-container-x86_64dev/docker/Dockerfile.runtime-container-aarch64dev/docker/Dockerfile.runtime-container-x86_64
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
from
d07e835 to
92a8d7c
Compare
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/dhcp/src/metrics.rs`:
- Around line 828-871: The v6 label contract test omits six documented
V6DropReason variants. Extend the table passed to
v6_drop_reason_renders_the_contract_strings_byte_identically with Success,
Ignore, ConfirmNotOnLink, InvalidPacket, InvalidMachinePointer, and
TooManyFailuresError, using each variant’s exact externally documented label
string.
🪄 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: b9b70afd-853b-465b-8ce2-2f72354d5037
📒 Files selected for processing (30)
crates/dhcp/Cargo.tomlcrates/dhcp/examples/kea-dhcp6-carbide.confcrates/dhcp/src/cache.rscrates/dhcp/src/discovery.rscrates/dhcp/src/discovery_v6.rscrates/dhcp/src/kea/callouts.cccrates/dhcp/src/kea/callouts.hcrates/dhcp/src/kea/loader.cccrates/dhcp/src/lease_expiration.rscrates/dhcp/src/lib.rscrates/dhcp/src/machine.rscrates/dhcp/src/machine_v6.rscrates/dhcp/src/metrics.rscrates/dhcp/src/mock_api_server.rscrates/dhcp/tests/common/dhcpv6_factory.rscrates/dhcp/tests/common/kea_v6.rscrates/dhcp/tests/common/mod.rscrates/dhcp/tests/identity_v6_test.rscrates/dhcp/tests/info_request_v6_test.rscrates/dhcp/tests/lease6_hook_test.rscrates/dhcp/tests/rapid_commit_v6_test.rsdev/deployment/localdev/Dockerfile.runtime-container.localdevdev/docker/Dockerfile.build-container-aarch64dev/docker/Dockerfile.build-container-x86_64dev/docker/Dockerfile.pxe-build-containerdev/docker/Dockerfile.release-container-aarch64dev/docker/Dockerfile.release-container-sa-x86_64dev/docker/Dockerfile.release-container-x86_64dev/docker/Dockerfile.runtime-container-aarch64dev/docker/Dockerfile.runtime-container-x86_64
🚧 Files skipped from review as they are similar to previous changes (22)
- dev/docker/Dockerfile.release-container-aarch64
- dev/docker/Dockerfile.release-container-x86_64
- dev/docker/Dockerfile.build-container-x86_64
- dev/docker/Dockerfile.runtime-container-aarch64
- crates/dhcp/examples/kea-dhcp6-carbide.conf
- crates/dhcp/tests/rapid_commit_v6_test.rs
- crates/dhcp/src/lease_expiration.rs
- crates/dhcp/src/kea/callouts.h
- crates/dhcp/src/discovery.rs
- crates/dhcp/src/discovery_v6.rs
- crates/dhcp/tests/info_request_v6_test.rs
- crates/dhcp/tests/identity_v6_test.rs
- crates/dhcp/Cargo.toml
- crates/dhcp/tests/common/kea_v6.rs
- crates/dhcp/src/machine_v6.rs
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp/src/machine.rs
- crates/dhcp/src/lib.rs
- crates/dhcp/tests/common/mod.rs
- crates/dhcp/src/cache.rs
- crates/dhcp/src/kea/callouts.cc
- crates/dhcp/tests/common/dhcpv6_factory.rs
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
from
92a8d7c to
9872f51
Compare
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
2 times, most recently
from
424d3f1 to
12e8c6e
Compare
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
from
12e8c6e to
7caf8a2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/dhcp/tests/info_request_v6_test.rs (1)
103-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the needless
.clone()at the call sites — takeresponseby reference.
assert_stateless_responsenever needs ownership ofresponse; every use inside the function body only borrows it. Theresponse.clone()calls at Line 129 and Line 172 exist solely to satisfy this by-value signature, and callers keep usingresponseafterward. Taking&v6::Messageremoves both clones without any functional change.♻️ Suggested fix
-fn assert_stateless_response(h: &Harness, response: v6::Message, expected_type: v6::MessageType) { +fn assert_stateless_response(h: &Harness, response: &v6::Message, expected_type: v6::MessageType) { // Stateless flows should call the API as observation and not carry IA_NA. assert_eq!(response.msg_type(), expected_type); - assert_eq!(DHCPv6Factory::ia_addr(&response), None); + assert_eq!(DHCPv6Factory::ia_addr(response), None); let discovery = h.api_server.discoveries().last().unwrap().clone(); assert_eq!( discovery.message_kind, Some(rpc::MessageKind::V6InfoRequest as i32) ); // The hook should still render service options on the response. - assert_hook_v6_dns_servers(&response); - assert_api_v6_domain_search(&response); - assert_hook_v6_ntp_servers(&response); + assert_hook_v6_dns_servers(response); + assert_api_v6_domain_search(response); + assert_hook_v6_ntp_servers(response); }And update the two call sites that currently clone:
- assert_stateless_response(&h, response.clone(), v6::MessageType::Reply); + assert_stateless_response(&h, &response, v6::MessageType::Reply);- assert_stateless_response(&h, response.clone(), v6::MessageType::Advertise); + assert_stateless_response(&h, &response, v6::MessageType::Advertise);Based on coding guidelines: "Avoid needless clones. Seeing
.clone()frequently indicates the ownership model may need rethinking. Prefer borrowing data or taking ownership of the value being cloned."🤖 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/tests/info_request_v6_test.rs` around lines 103 - 117, Change assert_stateless_response to accept response by reference, preserving its existing borrowed uses. Update both call sites that currently pass response.clone() to pass a reference instead, allowing callers to continue using the original response without cloning.Source: Coding guidelines
crates/dhcp/src/discovery_v6.rs (1)
317-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit the MAC mismatch as structured tracing fields.
Interpolation prevents efficient logfmt queries by
client_macandduid_mac.Proposed fix
- log::warn!("DHCPv6 option 79 MAC {client_mac} disagrees with DUID MAC {duid_mac}"); + tracing::warn!( + %client_mac, + %duid_mac, + "DHCPv6 option 79 MAC disagrees with DUID MAC" + );As per coding guidelines, “prefer placing common fields as attributes passed to tracing functions instead of using string interpolation.”
🤖 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/src/discovery_v6.rs` at line 317, Update the warning log at the DHCPv6 option 79 MAC mismatch site to pass client_mac and duid_mac as structured tracing fields/attributes, rather than interpolating them into the message. Keep the warning message descriptive and preserve both MAC values for logfmt queries.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/dhcp/src/discovery_v6.rs`:
- Line 317: Update the warning log at the DHCPv6 option 79 MAC mismatch site to
pass client_mac and duid_mac as structured tracing fields/attributes, rather
than interpolating them into the message. Keep the warning message descriptive
and preserve both MAC values for logfmt queries.
In `@crates/dhcp/tests/info_request_v6_test.rs`:
- Around line 103-117: Change assert_stateless_response to accept response by
reference, preserving its existing borrowed uses. Update both call sites that
currently pass response.clone() to pass a reference instead, allowing callers to
continue using the original response without cloning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6604628-7927-49c3-af27-7c9e985ad008
📒 Files selected for processing (30)
crates/dhcp/Cargo.tomlcrates/dhcp/examples/kea-dhcp6-carbide.confcrates/dhcp/src/cache.rscrates/dhcp/src/discovery.rscrates/dhcp/src/discovery_v6.rscrates/dhcp/src/kea/callouts.cccrates/dhcp/src/kea/callouts.hcrates/dhcp/src/kea/loader.cccrates/dhcp/src/lease_expiration.rscrates/dhcp/src/lib.rscrates/dhcp/src/machine.rscrates/dhcp/src/machine_v6.rscrates/dhcp/src/metrics.rscrates/dhcp/src/mock_api_server.rscrates/dhcp/tests/common/dhcpv6_factory.rscrates/dhcp/tests/common/kea_v6.rscrates/dhcp/tests/common/mod.rscrates/dhcp/tests/identity_v6_test.rscrates/dhcp/tests/info_request_v6_test.rscrates/dhcp/tests/lease6_hook_test.rscrates/dhcp/tests/rapid_commit_v6_test.rsdev/deployment/localdev/Dockerfile.runtime-container.localdevdev/docker/Dockerfile.build-container-aarch64dev/docker/Dockerfile.build-container-x86_64dev/docker/Dockerfile.pxe-build-containerdev/docker/Dockerfile.release-container-aarch64dev/docker/Dockerfile.release-container-sa-x86_64dev/docker/Dockerfile.release-container-x86_64dev/docker/Dockerfile.runtime-container-aarch64dev/docker/Dockerfile.runtime-container-x86_64
🚧 Files skipped from review as they are similar to previous changes (22)
- dev/docker/Dockerfile.release-container-aarch64
- dev/docker/Dockerfile.build-container-aarch64
- dev/docker/Dockerfile.build-container-x86_64
- crates/dhcp/tests/rapid_commit_v6_test.rs
- dev/docker/Dockerfile.release-container-x86_64
- crates/dhcp/Cargo.toml
- crates/dhcp/src/discovery.rs
- crates/dhcp/src/lease_expiration.rs
- crates/dhcp/tests/lease6_hook_test.rs
- crates/dhcp/src/kea/callouts.h
- crates/dhcp/src/machine_v6.rs
- crates/dhcp/tests/identity_v6_test.rs
- crates/dhcp/src/cache.rs
- crates/dhcp/tests/common/mod.rs
- crates/dhcp/tests/common/kea_v6.rs
- crates/dhcp/src/kea/loader.cc
- crates/dhcp/src/mock_api_server.rs
- crates/dhcp/src/lib.rs
- crates/dhcp/src/machine.rs
- crates/dhcp/src/kea/callouts.cc
- crates/dhcp/src/metrics.rs
- crates/dhcp/tests/common/dhcpv6_factory.rs
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
2 times, most recently
from
1c06a84 to
e29e880
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3435.docs.buildwithfern.com/infra-controller |
bcavnvidia
force-pushed
the
IPV6_DHCP_PLAN/03_kea_v6_callouts
branch
from
e29e880 to
058da8c
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.
Adds DHCPv6 support to the Kea hook path by decoding DHCPv6 client identity and relay metadata in Rust, routing v6 discoveries through the existing Carbide DHCP API/cache flow, and wiring Kea pkt6_receive, lease6_select, pkt6_send, and lease6_expire.
Also adds v6 hook configuration, DHCPv6 option rendering, address-family-aware cache keys, v6 mock API responses, and an example kea-dhcp6 hook config.
Related issues
#2385
Type of Change
Breaking Changes
Testing
Additional Notes
Closes #2385