Signed-off-by: Rishikpulhani <[email protected]>

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

Author: Rishikpulhani

src/commands/dhcp_proxy.rs

@@ -38,6 +38,8 @@ use tonic::{
     transport::Server, Code, Code::Internal, Code::InvalidArgument, Request, Response, Status,
 };
 
+type TaskData = (Arc<tokio::sync::Mutex<DhcpV4Service>>, 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<W: Write + Clear> {
     timeout_sender: Option<Arc<Mutex<Sender<i32>>>>,
     // 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<Mutex<HashMap<String, AbortHandle>>>,
+    task_map: Arc<Mutex<HashMap<String, TaskData>>>,
 }
 
 impl<W: Write + Clear> NetavarkProxyService<W> {
@@ -136,8 +138,8 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
         };
     }
 
-    /// When a container is shut down this method should be called. It will clear the lease information
-    /// from the caching system.
+    /// When a container is shut down this method should be called. It will release the
+    /// DHCP lease and clear the lease information from the caching system.
     async fn teardown(
         &self,
         request: Request<NetworkConfig>,
@@ -149,12 +151,25 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
         let cache = self.cache.clone();
         let tasks = self.task_map.clone();
 
-        let task = tasks
-            .lock()
-            .expect("lock tasks")
-            .remove(&nc.container_mac_addr);
-        if let Some(handle) = task {
-            handle.abort();
+        let maybe_service_arc = {
+            // Scope for the std::sync::MutexGuard
+            let mut tasks_guard = tasks.lock().expect("lock tasks");
+
+            if let Some((service_arc, handle)) = tasks_guard.remove(&nc.container_mac_addr) {
+                handle.abort();
+                Some(service_arc)
+            } else {
+                None
+            }
+        };
+        if let Some(service_arc) = maybe_service_arc {
+            let mut service = service_arc.lock().await;
+            if let Err(e) = service.release_lease() {
+                warn!(
+                    "Failed to send DHCPRELEASE for {}: {}",
+                    &nc.container_mac_addr, e
+                );
+            }
         }
 
         // Remove the client from the cache dir
@@ -406,7 +421,7 @@ async fn process_setup<W: Write + Clear>(
     network_config: NetworkConfig,
     timeout: u32,
     cache: Arc<Mutex<LeaseCache<W>>>,
-    tasks: Arc<Mutex<HashMap<String, AbortHandle>>>,
+    tasks: Arc<Mutex<HashMap<String, TaskData>>>,
 ) -> Result<NetavarkLease, Status> {
     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<W: Write + Clear>(
             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

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,53 @@ impl From<DhcpServiceError> for Status {
     }
 }
 
-pub async fn process_client_stream(mut client: DhcpV4Service) {
-    while let Some(lease) = client.client.next().await {
-        match lease {
-            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(
-                            &client.network_config.ns_path,
-                            &client.network_config.container_iface,
-                            old_lease,
-                            &lease,
-                        ) {
-                            Ok(_) => {}
-                            Err(err) => {
+pub async fn process_client_stream(service_arc: Arc<Mutex<DhcpV4Service>>) {
+    loop {
+        let lease_result = service_arc.lock().await.client.next().await;
+
+        if let Some(lease_result) = lease_result {
+            let mut client = service_arc.lock().await;
+            match lease_result {
+                Ok(lease) => {
+                    log::info!(
+                        "got new lease for mac {}: {:?}",
+                        &client.network_config.container_mac_addr,
+                        &lease
+                    );
+
+                    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
+                        {
+                            log::info!(
+                                "ip or gateway for mac {} changed, update address",
+                                &client.network_config.container_mac_addr
+                            );
+                            if let Err(err) = update_lease_ip(
+                                &client.network_config.ns_path,
+                                &client.network_config.container_iface,
+                                old_lease,
+                                &lease,
+                            ) {
                                 log::error!("{err}");
-                                continue;
                             }
                         }
                     }
+                    client.previous_lease = Some(lease);
+                }
+                Err(err) => {
+                    log::error!(
+                        "Failed to renew lease for {}: {err}",
+                        &client.network_config.container_mac_addr
+                    );
                 }
-                client.previous_lease = Some(lease)
             }
-            Err(err) => log::error!(
-                "Failed to renew lease for {}: {err}",
-                &client.network_config.container_mac_addr
-            ),
+        } else {
+            break;
         }
     }
 }
-
 fn update_lease_ip(
     netns: &str,
     interface: &str,