feat(dhcpv6): Add DHCPv6 support for macvlan and bridge drivers #1323
Conversation
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
Reviewer's GuideThis PR adds full stateful DHCPv6 support alongside DHCPv4 by refactoring the proxy layer into a unified DhcpService enum, introducing a new DhcpV6Service using the mozim library, extending gRPC Lease messages for IPv6, adjusting network drivers to configure IPv6 sysctls, updating client code to perform sequential IPv4/IPv6 leases, and enhancing test scripts and dependencies to cover IPv6 scenarios. Class diagram for unified DHCP service and lease typesclassDiagram
class DhcpService {
+get_net_config()
+get_previous_lease()
+set_previous_lease(lease)
+release_lease()
}
class DhcpV4Service {
+client
+network_config
+previous_lease
+release_lease()
}
class DhcpV6Service {
+client
+network_config
+previous_lease
+get_lease()
+release_lease()
}
class MozimLease {
}
class MozimV4Lease {
}
class MozimV6Lease {
}
DhcpService <|-- DhcpV4Service
DhcpService <|-- DhcpV6Service
MozimLease <|-- MozimV4Lease
MozimLease <|-- MozimV6Lease
DhcpV4Service --> MozimV4Lease : previous_lease
DhcpV6Service --> MozimV6Lease : previous_lease
DhcpService --> MozimLease : get_previous_lease/set_previous_lease
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Rishikpulhani
changed the title
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
3 times, most recently
from
eba4a4b to
68032d7
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
5 similar comments
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
from
68032d7 to
3aa0fe1
Compare
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
7 times, most recently
from
29ba56a to
7b52545
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- lease_has_changed’s catch-all arm treats any mismatched pair as ‘changed’, which may hide logic errors—replace the wildcard with unreachable!() or explicit error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- lease_has_changed’s catch-all arm treats any mismatched pair as ‘changed’, which may hide logic errors—replace the wildcard with unreachable!() or explicit error handling.
## Individual Comments
### Comment 1
<location> `src/dhcp_proxy/dhcp_service.rs:296` </location>
<code_context>
+ (MozimLease::V6(old_v6), MozimLease::V6(new_v6)) => {
+ old_v6.addr != new_v6.addr || old_v6.prefix_len != new_v6.prefix_len
+ }
+ _ => true, // could have used unreachable!()
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using 'true' for mismatched lease types in lease_has_changed may mask logic errors.
Using 'true' here may conceal incorrect mixing of lease types. Instead, consider unreachable!() or logging to highlight these cases during development.
```suggestion
_ => unreachable!("Mismatched MozimLease types in lease_has_changed"),
```
</issue_to_address>
### Comment 2
<location> `src/dhcp_proxy/dhcp_service.rs:369` </location>
<code_context>
}
+ // NO gateway logic for IPv6. This is intentional.
}
+ _ => return Err(NetavarkError::msg("Lease type mismatch during IP update")),
}
-
</code_context>
<issue_to_address>
**suggestion:** Returning a generic error for lease type mismatch may obscure the root cause.
Include the actual lease types in the error message to make debugging easier.
```suggestion
_ => {
return Err(NetavarkError::msg(format!(
"Lease type mismatch during IP update: new lease type is {:?}, old lease type is {:?}",
new_lease, old_lease
)));
}
```
</issue_to_address>
### Comment 3
<location> `src/network/dhcp.rs:67-70` </location>
<code_context>
+
+ // If IPv4 fails but IPv6 is requested, should we continue or fail all?
+ // For now, we'll let it continue.
+ let v4_lease = match get_lease_from_proxy(nvp_config_v4) {
+ Ok(l) => l,
+ Err(e) => {
+ return Err(NetavarkError::msg(format!("unable to obtain lease: {e}")));
+ }
+ };
</code_context>
<issue_to_address>
**suggestion:** Failing immediately on IPv4 lease error may prevent IPv6-only containers from working.
If ipam.ipv6_enabled is true and no IPv4 addresses are needed, consider allowing the process to continue when IPv4 lease acquisition fails.
Suggested implementation:
```rust
let v4_lease = match get_lease_from_proxy(nvp_config_v4) {
Ok(l) => l,
Err(e) => {
// If IPv6 is enabled and no IPv4 addresses are needed, allow to continue
if ipam.ipv6_enabled && nvp_config_v4.is_none() {
// No IPv4 lease, but that's acceptable for IPv6-only containers
None
} else {
return Err(NetavarkError::msg(format!("unable to obtain lease: {e}")));
}
}
};
```
You may need to ensure that `nvp_config_v4` is an `Option` and that downstream code can handle `v4_lease` being `None`. If `v4_lease` is expected to always be present, update its type to `Option<LeaseType>` (or whatever type is returned by `get_lease_from_proxy`). Also, make sure that the logic for handling leases later in the code can handle the case where `v4_lease` is `None`.
</issue_to_address>
### Comment 4
<location> `test-dhcp/002-setup.bats:33-42` </location>
<code_context>
+: <<'DISABLED_TEST'
</code_context>
<issue_to_address>
**issue (testing):** IPv6 setup test is currently disabled.
Please enable the test to verify DHCPv6 functionality. If it cannot be enabled now, add a note explaining the reason and a plan for future activation.
</issue_to_address>
### Comment 5
<location> `test-dhcp/helpers.bash:344-353` </location>
<code_context>
+function run_dhcp() {
</code_context>
<issue_to_address>
**suggestion (testing):** No negative/edge case tests for DHCPv6 lease failures.
Please add tests for scenarios such as no available addresses, misconfigured dnsmasq, and timeouts to ensure proper error handling and messaging.
</issue_to_address>
### Comment 6
<location> `src/dhcp_proxy/dhcp_service.rs:300` </location>
<code_context>
+ }
+}
+
fn update_lease_ip(
netns: &str,
interface: &str,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the large match blocks in `update_lease_ip` and `lease_has_changed` into smaller helper functions for each IP version to improve readability and maintainability.
Here’s one way to tame the nesting in `update_lease_ip` (and the free‐fn `lease_has_changed`) by pulling each IP/version case into its own small helper. You’ll end up with a single `match` that just calls into two focused routines.
```rust
fn update_lease_ip(
netns: &str,
interface: &str,
old_lease: &MozimLease,
new_lease: &MozimLease,
) -> NetavarkResult<()> {
let (_, ns) = core_utils::open_netlink_sockets(netns)
.wrap("failed to open netlink socket in netns")?;
let mut sock = ns.netlink;
match (old_lease, new_lease) {
(MozimLease::V4(o), MozimLease::V4(n)) =>
update_lease_ip_v4(&mut sock, interface, o, n),
(MozimLease::V6(o), MozimLease::V6(n)) =>
update_lease_ip_v6(&mut sock, interface, o, n),
_ => Err(NetavarkError::msg("Lease type mismatch during IP update")),
}
}
fn update_lease_ip_v4(
sock: &mut NetlinkSocket, interface: &str,
old: &MozimV4Lease, new: &MozimV4Lease
) -> NetavarkResult<()> {
let old_net = ipnet::Ipv4Net::with_netmask(old.yiaddr, old.subnet_mask)?;
let new_net = ipnet::Ipv4Net::with_netmask(new.yiaddr, new.subnet_mask)?;
if new_net != old_net {
let link = sock.get_link(LinkID::Name(interface.to_string()))?
.wrap("get interface in netns")?;
sock.add_addr(link.header.index, &ipnet::IpNet::V4(new_net))
.wrap("add new addr")?;
sock.del_addr(link.header.index, &ipnet::IpNet::V4(old_net))
.wrap("remove old addr")?;
}
// ... default‐gateway logic here ...
Ok(())
}
fn update_lease_ip_v6(
sock: &mut NetlinkSocket, interface: &str,
old: &MozimV6Lease, new: &MozimV6Lease
) -> NetavarkResult<()> {
let old_net = ipnet::Ipv6Net::new(old.addr, old.prefix_len)?;
let new_net = ipnet::Ipv6Net::new(new.addr, new.prefix_len)?;
if new_net != old_net {
let link = sock.get_link(LinkID::Name(interface.to_string()))?
.wrap("get interface in netns")?;
sock.add_addr(link.header.index, &ipnet::IpNet::V6(new_net))?;
sock.del_addr(link.header.index, &ipnet::IpNet::V6(old_net))?;
}
Ok(())
}
```
And likewise push `lease_has_changed` onto the enum itself:
```rust
impl MozimLease {
fn has_changed(&self, other: &MozimLease) -> bool {
match (self, other) {
(MozimLease::V4(a), MozimLease::V4(b)) =>
a.yiaddr != b.yiaddr ||
a.subnet_mask != b.subnet_mask ||
a.gateways != b.gateways,
(MozimLease::V6(a), MozimLease::V6(b)) =>
a.addr != b.addr ||
a.prefix_len != b.prefix_len,
_ => true,
}
}
}
```
This flattening pulls each version’s logic out of one huge `match` block and into small, self‐contained functions, making both the control flow and each protocol’s behavior easier to read & maintain.
</issue_to_address>
### Comment 7
<location> `src/network/dhcp.rs:54` </location>
<code_context>
+ // --- Perform DHCPv4 Lease ---
+ // For now, we assume IPv4 is always desired unless explicitly disabled in future logic.
+ // You could make this conditional based on `ipam.container_addresses` if needed.
+ let nvp_config_v4 = NetworkConfig {
+ version: 0, // Explicitly set for IPv4
host_iface: host_network_interface.to_string(),
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a helper function for NetworkConfig creation and using Option::or to simplify merging logic.
```rust
// 1. Extract a helper to remove the v4/v6 NetworkConfig duplication:
fn build_network_config(
host_iface: &str,
container_iface: &str,
ns_path: &str,
mac: &str,
hostname: &str,
container_id: &str,
version: u8,
) -> NetworkConfig {
NetworkConfig {
version,
host_iface: host_iface.to_string(),
container_iface: container_iface.to_string(),
ns_path: ns_path.to_string(),
container_mac_addr: mac.to_string(),
host_name: hostname.to_string(),
container_id: container_id.to_string(),
domain_name: "".into(),
}
}
// Then in get_dhcp_lease:
let nvp_v4 = build_network_config(
host_network_interface,
container_network_interface,
ns_path,
container_macvlan_mac,
container_hostname,
container_id,
0,
);
let v4_lease = get_lease_from_proxy(nvp_v4)?;
// ...
if ipam.ipv6_enabled {
let nvp_v6 = build_network_config(
host_network_interface,
container_network_interface,
ns_path,
container_macvlan_mac,
container_hostname,
container_id,
1,
);
let (v6_subnet, v6_dns, v6_domain) = parse_lease(get_lease_from_proxy(nvp_v6)?)?;
subnets.push(v6_subnet);
// 2. Simplify the merge logic with `Option::or`:
all_dns_servers = all_dns_servers.or(v6_dns);
all_domain_names = all_domain_names.or(v6_domain);
}
```
</issue_to_address>
### Comment 8
<location> `src/dhcp_proxy/lib.rs:117` </location>
<code_context>
+ // DhcpV6Lease
+ // here the fields of DhcpV4Lease are repurposed tarry their equivalents in the DhcpV6Lease
+ impl From<DhcpV6Lease> for Lease {
+ fn from(l: DhcpV6Lease) -> Lease {
+ let mut domain_name = String::new();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the option-parsing loop into separate helper functions for clarity and maintainability.
```markdown
You can collapse that 70-line loop into three small, self-contained parsers. For example:
```rust
impl From<DhcpV6Lease> for Lease {
fn from(l: DhcpV6Lease) -> Lease {
let dns_servers = parse_dns_servers(&l.dhcp_opts);
let domain_name = parse_domain_search(&l.dhcp_opts);
let ntp_servers = parse_ntp_servers(&l.dhcp_opts);
Lease {
t1: l.t1,
t2: l.t2,
lease_time: l.valid_life,
yiaddr: l.addr.to_string(),
siaddr: l.srv_ip.to_string(),
srv_id: hex::encode(l.srv_duid),
is_v6: true,
domain_name,
dns_servers,
ntp_servers,
subnet_mask: l.prefix_len.to_string(),
mac_address: String::new(),
mtu: 0,
broadcast_addr:String::new(),
gateways: Vec::new(),
host_name: String::new(),
}
}
}
fn parse_dns_servers(opts: &[DhcpOption]) -> Vec<String> {
opts.iter()
.find_map(|opt| {
if let DhcpOption::DomainNameServers(servers) = opt {
Some(servers.iter().map(ToString::to_string).collect())
} else {
None
}
})
.unwrap_or_default()
}
fn parse_domain_search(opts: &[DhcpOption]) -> String {
opts.iter()
.find_map(|opt| {
if let DhcpOption::DomainSearchList(domains) = opt {
Some(domains.iter()
.map(ToString::to_string)
.collect::<Vec<_>>()
.join(" "))
} else {
None
}
})
.unwrap_or_default()
}
fn parse_ntp_servers(opts: &[DhcpOption]) -> Vec<String> {
opts.iter()
.filter_map(|opt| match opt {
DhcpOption::NtpServer(subopts) => Some(subopts),
_ => None,
})
.flat_map(|subopts| subopts.iter().filter_map(|sub| match sub {
NtpSuboption::ServerAddress(addr)
| NtpSuboption::MulticastAddress(addr) => Some(addr.to_string()),
NtpSuboption::FQDN(name) => Some(name.to_string()),
}))
.collect()
}
```
This keeps all the functionality but breaks the logic into focused, testable units.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| (MozimLease::V6(old_v6), MozimLease::V6(new_v6)) => { | ||
| old_v6.addr != new_v6.addr || old_v6.prefix_len != new_v6.prefix_len | ||
| } | ||
| _ => true, // could have used unreachable!() |
There was a problem hiding this comment.
suggestion (bug_risk): Using 'true' for mismatched lease types in lease_has_changed may mask logic errors.
Using 'true' here may conceal incorrect mixing of lease types. Instead, consider unreachable!() or logging to highlight these cases during development.
Suggested change
| _ => true, // could have used unreachable!() | |
| _ => unreachable!("Mismatched MozimLease types in lease_has_changed"), |
| } | ||
| // NO gateway logic for IPv6. This is intentional. | ||
| } | ||
| _ => return Err(NetavarkError::msg("Lease type mismatch during IP update")), |
There was a problem hiding this comment.
suggestion: Returning a generic error for lease type mismatch may obscure the root cause.
Include the actual lease types in the error message to make debugging easier.
Suggested change
| _ => return Err(NetavarkError::msg("Lease type mismatch during IP update")), | |
| _ => { | |
| return Err(NetavarkError::msg(format!( | |
| "Lease type mismatch during IP update: new lease type is {:?}, old lease type is {:?}", | |
| new_lease, old_lease | |
| ))); | |
| } |
Comment on lines
+67
to
+70
| let v4_lease = match get_lease_from_proxy(nvp_config_v4) { | ||
| Ok(l) => l, | ||
| Err(e) => { | ||
| return Err(NetavarkError::msg(format!("unable to obtain lease: {e}"))); |
There was a problem hiding this comment.
suggestion: Failing immediately on IPv4 lease error may prevent IPv6-only containers from working.
If ipam.ipv6_enabled is true and no IPv4 addresses are needed, consider allowing the process to continue when IPv4 lease acquisition fails.
Suggested implementation:
let v4_lease = match get_lease_from_proxy(nvp_config_v4) {
Ok(l) => l,
Err(e) => {
// If IPv6 is enabled and no IPv4 addresses are needed, allow to continue
if ipam.ipv6_enabled && nvp_config_v4.is_none() {
// No IPv4 lease, but that's acceptable for IPv6-only containers
None
} else {
return Err(NetavarkError::msg(format!("unable to obtain lease: {e}")));
}
}
};You may need to ensure that nvp_config_v4 is an Option and that downstream code can handle v4_lease being None. If v4_lease is expected to always be present, update its type to Option<LeaseType> (or whatever type is returned by get_lease_from_proxy). Also, make sure that the logic for handling leases later in the code can handle the case where v4_lease is None.
test-dhcp/002-setup.bats
Outdated
Comment on lines
33
to
42
| : <<'DISABLED_TEST' | ||
| @test "basic ipv6 setup" { | ||
| read -r -d '\0' input_config <<EOF | ||
| { | ||
| "host_iface": "veth1", | ||
| "container_iface": "veth0", | ||
| "container_mac_addr": "$CONTAINER_MAC", | ||
| "domain_name": "example.com", | ||
| "host_name": "foobar-v6", |
There was a problem hiding this comment.
issue (testing): IPv6 setup test is currently disabled.
Please enable the test to verify DHCPv6 functionality. If it cannot be enabled now, add a note explaining the reason and a plan for future activation.
Comment on lines
331
to
353
| function run_dhcp() { | ||
| gw=$(gateway_from_subnet "$SUBNET_CIDR") | ||
| stripped_subnet=$(strip_last_octet_from_subnet) | ||
|
|
||
| read -r -d '\0' dnsmasq_config <<EOF | ||
| local version=${1:-4} | ||
| local gw | ||
| gw=$(gateway_from_subnet "$SUBNET_CIDR") | ||
| local dnsmasq_config="" | ||
|
|
||
| if [ "$version" == "6" ]; then | ||
| # --- FIX IS HERE --- | ||
| # Get the IPv6 network prefix from the full CIDR. | ||
| # For example, turn "fd1d:5139:5cb5:1a99::/64" into "fd1d:5139:5cb5:1a99::" |
There was a problem hiding this comment.
suggestion (testing): No negative/edge case tests for DHCPv6 lease failures.
Please add tests for scenarios such as no available addresses, misconfigured dnsmasq, and timeouts to ensure proper error handling and messaging.
| } | ||
| } | ||
|
|
||
| fn update_lease_ip( |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the large match blocks in update_lease_ip and lease_has_changed into smaller helper functions for each IP version to improve readability and maintainability.
Here’s one way to tame the nesting in update_lease_ip (and the free‐fn lease_has_changed) by pulling each IP/version case into its own small helper. You’ll end up with a single match that just calls into two focused routines.
fn update_lease_ip(
netns: &str,
interface: &str,
old_lease: &MozimLease,
new_lease: &MozimLease,
) -> NetavarkResult<()> {
let (_, ns) = core_utils::open_netlink_sockets(netns)
.wrap("failed to open netlink socket in netns")?;
let mut sock = ns.netlink;
match (old_lease, new_lease) {
(MozimLease::V4(o), MozimLease::V4(n)) =>
update_lease_ip_v4(&mut sock, interface, o, n),
(MozimLease::V6(o), MozimLease::V6(n)) =>
update_lease_ip_v6(&mut sock, interface, o, n),
_ => Err(NetavarkError::msg("Lease type mismatch during IP update")),
}
}
fn update_lease_ip_v4(
sock: &mut NetlinkSocket, interface: &str,
old: &MozimV4Lease, new: &MozimV4Lease
) -> NetavarkResult<()> {
let old_net = ipnet::Ipv4Net::with_netmask(old.yiaddr, old.subnet_mask)?;
let new_net = ipnet::Ipv4Net::with_netmask(new.yiaddr, new.subnet_mask)?;
if new_net != old_net {
let link = sock.get_link(LinkID::Name(interface.to_string()))?
.wrap("get interface in netns")?;
sock.add_addr(link.header.index, &ipnet::IpNet::V4(new_net))
.wrap("add new addr")?;
sock.del_addr(link.header.index, &ipnet::IpNet::V4(old_net))
.wrap("remove old addr")?;
}
// ... default‐gateway logic here ...
Ok(())
}
fn update_lease_ip_v6(
sock: &mut NetlinkSocket, interface: &str,
old: &MozimV6Lease, new: &MozimV6Lease
) -> NetavarkResult<()> {
let old_net = ipnet::Ipv6Net::new(old.addr, old.prefix_len)?;
let new_net = ipnet::Ipv6Net::new(new.addr, new.prefix_len)?;
if new_net != old_net {
let link = sock.get_link(LinkID::Name(interface.to_string()))?
.wrap("get interface in netns")?;
sock.add_addr(link.header.index, &ipnet::IpNet::V6(new_net))?;
sock.del_addr(link.header.index, &ipnet::IpNet::V6(old_net))?;
}
Ok(())
}And likewise push lease_has_changed onto the enum itself:
impl MozimLease {
fn has_changed(&self, other: &MozimLease) -> bool {
match (self, other) {
(MozimLease::V4(a), MozimLease::V4(b)) =>
a.yiaddr != b.yiaddr ||
a.subnet_mask != b.subnet_mask ||
a.gateways != b.gateways,
(MozimLease::V6(a), MozimLease::V6(b)) =>
a.addr != b.addr ||
a.prefix_len != b.prefix_len,
_ => true,
}
}
}This flattening pulls each version’s logic out of one huge match block and into small, self‐contained functions, making both the control flow and each protocol’s behavior easier to read & maintain.
| // --- Perform DHCPv4 Lease --- | ||
| // For now, we assume IPv4 is always desired unless explicitly disabled in future logic. | ||
| // You could make this conditional based on `ipam.container_addresses` if needed. | ||
| let nvp_config_v4 = NetworkConfig { |
There was a problem hiding this comment.
issue (complexity): Consider extracting a helper function for NetworkConfig creation and using Option::or to simplify merging logic.
// 1. Extract a helper to remove the v4/v6 NetworkConfig duplication:
fn build_network_config(
host_iface: &str,
container_iface: &str,
ns_path: &str,
mac: &str,
hostname: &str,
container_id: &str,
version: u8,
) -> NetworkConfig {
NetworkConfig {
version,
host_iface: host_iface.to_string(),
container_iface: container_iface.to_string(),
ns_path: ns_path.to_string(),
container_mac_addr: mac.to_string(),
host_name: hostname.to_string(),
container_id: container_id.to_string(),
domain_name: "".into(),
}
}
// Then in get_dhcp_lease:
let nvp_v4 = build_network_config(
host_network_interface,
container_network_interface,
ns_path,
container_macvlan_mac,
container_hostname,
container_id,
0,
);
let v4_lease = get_lease_from_proxy(nvp_v4)?;
// ...
if ipam.ipv6_enabled {
let nvp_v6 = build_network_config(
host_network_interface,
container_network_interface,
ns_path,
container_macvlan_mac,
container_hostname,
container_id,
1,
);
let (v6_subnet, v6_dns, v6_domain) = parse_lease(get_lease_from_proxy(nvp_v6)?)?;
subnets.push(v6_subnet);
// 2. Simplify the merge logic with `Option::or`:
all_dns_servers = all_dns_servers.or(v6_dns);
all_domain_names = all_domain_names.or(v6_domain);
}|
|
||
| // DhcpV6Lease | ||
| // here the fields of DhcpV4Lease are repurposed tarry their equivalents in the DhcpV6Lease | ||
| impl From<DhcpV6Lease> for Lease { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the option-parsing loop into separate helper functions for clarity and maintainability.
You can collapse that 70-line loop into three small, self-contained parsers. For example:
```rust
impl From<DhcpV6Lease> for Lease {
fn from(l: DhcpV6Lease) -> Lease {
let dns_servers = parse_dns_servers(&l.dhcp_opts);
let domain_name = parse_domain_search(&l.dhcp_opts);
let ntp_servers = parse_ntp_servers(&l.dhcp_opts);
Lease {
t1: l.t1,
t2: l.t2,
lease_time: l.valid_life,
yiaddr: l.addr.to_string(),
siaddr: l.srv_ip.to_string(),
srv_id: hex::encode(l.srv_duid),
is_v6: true,
domain_name,
dns_servers,
ntp_servers,
subnet_mask: l.prefix_len.to_string(),
mac_address: String::new(),
mtu: 0,
broadcast_addr:String::new(),
gateways: Vec::new(),
host_name: String::new(),
}
}
}
fn parse_dns_servers(opts: &[DhcpOption]) -> Vec<String> {
opts.iter()
.find_map(|opt| {
if let DhcpOption::DomainNameServers(servers) = opt {
Some(servers.iter().map(ToString::to_string).collect())
} else {
None
}
})
.unwrap_or_default()
}
fn parse_domain_search(opts: &[DhcpOption]) -> String {
opts.iter()
.find_map(|opt| {
if let DhcpOption::DomainSearchList(domains) = opt {
Some(domains.iter()
.map(ToString::to_string)
.collect::<Vec<_>>()
.join(" "))
} else {
None
}
})
.unwrap_or_default()
}
fn parse_ntp_servers(opts: &[DhcpOption]) -> Vec<String> {
opts.iter()
.filter_map(|opt| match opt {
DhcpOption::NtpServer(subopts) => Some(subopts),
_ => None,
})
.flat_map(|subopts| subopts.iter().filter_map(|sub| match sub {
NtpSuboption::ServerAddress(addr)
| NtpSuboption::MulticastAddress(addr) => Some(addr.to_string()),
NtpSuboption::FQDN(name) => Some(name.to_string()),
}))
.collect()
}This keeps all the functionality but breaks the logic into focused, testable units.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rishikpulhani, sourcery-ai[bot] The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
6 times, most recently
from
fdfbbca to
2f136af
Compare
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
2 times, most recently
from
92215c5 to
16ed14e
Compare
Rishikpulhani
changed the title
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
from
16ed14e to
50eb1da
Compare
This commit introduces full support for stateful IPv6 address assignment via DHCPv6, resolving the issue where containers would not receive a global IPv6 address on macvlan networks. The implementation follows the standard IPv6 design of separating gateway discovery (via Router Advertisements) from stateful address assignment (via DHCPv6). Key changes include: - **Kernel Configuration:** Netavark now configures the container's kernel to accept Router Advertisements (`accept_ra=2`) for automatic gateway discovery, while disabling SLAAC (`autoconf=0`) to ensure a managed, deterministic network environment. - **DHCPv6 Service:** A new `DhcpV6Service` is added to the DHCP proxy. It uses the `mozim` library to acquire IPv6 leases and correctly generates a stable DUID-LL from the container's static MAC address to ensure a persistent network identity. - **gRPC Layer:** The gRPC `Lease` object and its `From` implementations have been updated to act as a universal carrier for both IPv4 and IPv6 lease information. - **Generic Proxy Logic:** Core functions like `process_client_stream` and `update_lease_ip` have been refactored to handle both DHCPv4 and DHCPv6 services generically, with conditional logic to correctly handle the differences between the protocols (e.g., gateway handling). Signed-off-by: Rishikpulhani <[email protected]>
Rishikpulhani
force-pushed
the
dhcp-ipv6-support
branch
from
50eb1da to
83b79ad
Compare
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.
Fixes #1072
This commit introduces full support for stateful IPv6 address assignment via DHCPv6, resolving the issue where containers would not receive a global IPv6 address on macvlan networks.
The implementation follows the standard IPv6 design of separating gateway discovery (via Router Advertisements) from stateful address assignment (via DHCPv6).
Design
The implementation follows the standard IPv6 model:
sysctl(accept_ra=1,autoconf=0) to automatically learn its default gateway from RAs while preventing SLAAC. Netavark does not parse RA messages itself.DhcpV6Servicein the DHCP proxy uses themozimlibrary to acquire a lease, providing the container with a stable IP address and other options like DNS.Key changes include:
Kernel Configuration: Netavark now configures the container's kernel to accept Router Advertisements (
accept_ra=1) for automatic gateway discovery, while disabling SLAAC (autoconf=0) to ensure a managed, deterministic network environment.DHCPv6 Service: A new
DhcpV6Serviceis added to the DHCP proxy. It uses themozimlibrary to acquire IPv6 leases and correctly generates a stable DUID-LL from the container's static MAC address to ensure a persistent network identity.gRPC Layer: The gRPC
Leaseobject and itsFromimplementations have been updated to act as a universal carrier for both IPv4 and IPv6 lease information.Generic Proxy Logic: Core functions like
process_client_streamandupdate_lease_iphave been refactored to handle both DHCPv4 and DHCPv6 services generically, with conditional logic to correctly handle the differences between the protocols (e.g., gateway handling).Summary by Sourcery
Add comprehensive DHCPv6 support alongside existing DHCPv4 functionality, allowing macvlan and bridge networks to obtain stateful IPv6 addresses via DHCPv6 while preserving deterministic gateway discovery via Router Advertisements.
New Features:
Enhancements:
Build:
Tests: