From 642242b5e7619ae1653f70a961a291a1b2e47b5d Mon Sep 17 00:00:00 2001 From: Rishikpulhani Date: Mon, 11 Aug 2025 13:16:12 +0530 Subject: [PATCH] feat(dhcp): Send DHCPRELEASE on container teardown When a container using DHCP is torn down, its lease is left active on the DHCP server until it expires. This can be problematic in environments with small IP pools or long lease times. In setups using Dynamic DNS (DDNS), it can also lead to stale DNS records. This change introduces the capability for netavark to send a DHCPRELEASE message to the DHCP server when a container's network is torn down. This is implemented by: - Adding a `release_lease` method to the `DhcpV4Service` in `dhcp_service.rs`, which wraps the `release` function from the underlying mozim client. - Wrapped DhcpV4Service inside Arc<> share that to the process_client_stream function and then also store it in the task map - used it in process_client_stream task and also in the teardown function to send the DHCPRELEASE message Signed-off-by: Rishikpulhani --- src/commands/dhcp_proxy.rs | 41 +++++++++++++++++------- src/dhcp_proxy/dhcp_service.rs | 57 +++++++++++++++++++++++----------- 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/commands/dhcp_proxy.rs b/src/commands/dhcp_proxy.rs index f91723816..e5bf723f8 100644 --- a/src/commands/dhcp_proxy.rs +++ b/src/commands/dhcp_proxy.rs @@ -38,6 +38,8 @@ use tonic::{ transport::Server, Code, Code::Internal, Code::InvalidArgument, Request, Response, Status, }; +type TaskData = (Arc>, AbortHandle); + #[derive(Debug)] /// This is the tonic netavark proxy service that is required to impl the Netavark Proxy trait which /// includes the gRPC methods defined in proto/proxy.proto. We can store a atomically referenced counted @@ -59,7 +61,7 @@ struct NetavarkProxyService { timeout_sender: Option>>>, // All dhcp poll will be spawned on a new task, keep track of it so // we can remove it on teardown. The key is the container mac. - task_map: Arc>>, + task_map: Arc>>, } impl NetavarkProxyService { @@ -136,8 +138,8 @@ impl NetavarkProxy for NetavarkProxyService, @@ -149,12 +151,25 @@ impl NetavarkProxy for NetavarkProxyService( network_config: NetworkConfig, timeout: u32, cache: Arc>>, - tasks: Arc>>, + tasks: Arc>>, ) -> Result { let container_network_interface = network_config.container_iface.clone(); let ns_path = network_config.ns_path.clone(); @@ -422,11 +437,13 @@ async fn process_setup( let mut service = DhcpV4Service::new(network_config, timeout)?; let lease = service.get_lease().await?; - let task = tokio::spawn(process_client_stream(service)); + let service_arc = Arc::new(tokio::sync::Mutex::new(service)); + let service_arc_clone = service_arc.clone(); + let task_handle = tokio::spawn(process_client_stream(service_arc_clone)); tasks .lock() .expect("lock tasks") - .insert(mac.to_string(), task.abort_handle()); + .insert(mac.to_string(), (service_arc, task_handle.abort_handle())); lease } //V6 TODO implement DHCPv6 diff --git a/src/dhcp_proxy/dhcp_service.rs b/src/dhcp_proxy/dhcp_service.rs index 95eb4aa10..636023bcc 100644 --- a/src/dhcp_proxy/dhcp_service.rs +++ b/src/dhcp_proxy/dhcp_service.rs @@ -11,8 +11,9 @@ use crate::network::netlink::Route; use crate::wrap; use log::debug; use mozim::{DhcpV4ClientAsync, DhcpV4Config, DhcpV4Lease as MozimV4Lease}; +use std::sync::Arc; +use tokio::sync::Mutex; use tokio_stream::StreamExt; - use tonic::{Code, Status}; /// The kind of DhcpServiceError that can be caused when finding a dhcp lease @@ -39,6 +40,7 @@ impl DhcpServiceError { } /// DHCP service is responsible for creating, handling, and managing the dhcp lease process. +#[derive(Debug)] pub struct DhcpV4Service { client: DhcpV4ClientAsync, network_config: NetworkConfig, @@ -129,6 +131,28 @@ impl DhcpV4Service { "Could not find a lease within the timeout limit".to_string(), )) } + + /// Sends a DHCPRELEASE message for the given lease. + /// This is a "best effort" operation and should not block teardown. + pub fn release_lease(&mut self) -> Result<(), DhcpServiceError> { + if let Some(lease) = &self.previous_lease { + debug!( + "Attempting to release lease for MAC: {}", + &self.network_config.container_mac_addr + ); + // Directly call the release function on the underlying mozim client. + self.client + .release(lease) + .map_err(|e| DhcpServiceError::new(Bug, e.to_string())) + } else { + // No previous lease recorded; nothing to release. Best-effort -> succeed silently. + debug!( + "No previous lease to release for MAC: {}", + &self.network_config.container_mac_addr + ); + Ok(()) + } + } } impl std::fmt::Display for DhcpServiceError { @@ -149,50 +173,47 @@ impl From for Status { } } -pub async fn process_client_stream(mut client: DhcpV4Service) { - while let Some(lease) = client.client.next().await { - match lease { +pub async fn process_client_stream(service_arc: Arc>) { + let mut client = service_arc.lock().await; + while let Some(lease_result) = client.client.next().await { + match lease_result { Ok(lease) => { log::info!( "got new lease for mac {}: {:?}", &client.network_config.container_mac_addr, &lease ); - // get previous lease and check if ip addr changed, if not we do not have to do anything + if let Some(old_lease) = &client.previous_lease { if old_lease.yiaddr != lease.yiaddr || old_lease.subnet_mask != lease.subnet_mask || old_lease.gateways != lease.gateways { - // ips do not match, remove old ones and assign new ones. log::info!( "ip or gateway for mac {} changed, update address", &client.network_config.container_mac_addr ); - match update_lease_ip( + if let Err(err) = update_lease_ip( &client.network_config.ns_path, &client.network_config.container_iface, old_lease, &lease, ) { - Ok(_) => {} - Err(err) => { - log::error!("{err}"); - continue; - } + log::error!("{err}"); } } } - client.previous_lease = Some(lease) + client.previous_lease = Some(lease); + } + Err(err) => { + log::error!( + "Failed to renew lease for {}: {err}", + &client.network_config.container_mac_addr + ); } - Err(err) => log::error!( - "Failed to renew lease for {}: {err}", - &client.network_config.container_mac_addr - ), } } } - fn update_lease_ip( netns: &str, interface: &str,