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,28 @@ 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 Some(lease) = service.previous_lease() {
+                debug!("Attempting to release lease for {}", &nc.container_mac_addr);
+                if let Err(e) = service.release_lease(&lease) {
+                    warn!(
+                        "Failed to send DHCPRELEASE for {}: {}",
+                        &nc.container_mac_addr, e
+                    );
+                }
+            }
         }
 
         // Remove the client from the cache dir
@@ -406,7 +424,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 +440,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,
@@ -83,6 +85,9 @@ impl DhcpV4Service {
             previous_lease: None,
         })
     }
+    pub fn previous_lease(&self) -> Option<MozimV4Lease> {
+        self.previous_lease.clone()
+    }
 
     /// Performs a DHCP DORA on a ipv4 network configuration.
     /// # Arguments
@@ -129,6 +134,19 @@ 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, lease: &MozimV4Lease) -> Result<(), DhcpServiceError> {
+        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()))
+    }
 }
 
 impl std::fmt::Display for DhcpServiceError {
@@ -149,46 +167,61 @@ 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) => {
-                                log::error!("{err}");
-                                continue;
+pub async fn process_client_stream(service_arc: Arc<Mutex<DhcpV4Service>>) {
+    loop {
+        let next_lease_result = {
+            let mut service = service_arc.lock().await;
+            service.client.next().await
+        };
+        if let Some(lease_result) = next_lease_result {
+            // Now that we have the result, we can re-lock to update the state.
+            // This is safe because this part doesn't involve `.await`.
+            match lease_result {
+                Ok(lease) => {
+                    let mut client = service_arc.lock().await;
+                    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) => {
+                                    log::error!("{err}");
+                                    continue;
+                                }
                             }
                         }
                     }
+                    client.previous_lease = Some(lease);
+                }
+                Err(err) => {
+                    let client = service_arc.lock().await;
+                    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 {
+            // The stream has ended (e.g., the client disconnected), so we exit the loop.
+            break;
         }
     }
 }

test-dhcp/003-teardown.bats

@@ -5,6 +5,11 @@
 
 load helpers
 
+function start_proxy() {
+  RUST_LOG=debug ip netns exec "$NS_NAME" $NETAVARK dhcp-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR" &>"$TMP_TESTDIR/proxy.log" &
+  PROXY_PID=$!
+}
+
 @test "basic teardown" {
       read -r -d '\0' input_config <<EOF
 {
@@ -30,6 +35,10 @@ EOF
        assert "$output" == "true"
        # Run teardown
        run_teardown "$input_config"
+       # Check the dnsmasq log to confirm it received the DHCPRELEASE message.
+       # The release is sent synchronously, but we sleep briefly to allow dnsmasq to flush its logs.
+       sleep 1
+       assert `grep -c "DHCPRELEASE(br0).*[[:space:]]${CONTAINER_MAC}" "$TMP_TESTDIR/dnsmasq.log"` == 1
        run_helper cat "$TMP_TESTDIR/nv-proxy.lease"
        # Check that the length of the lease file is now zero
        run_helper jq ". | length" <<<"$output"

test-dhcp/helpers.bash

@@ -375,7 +375,7 @@ function stop_dhcp() {
 }
 
 function start_proxy() {
-  RUST_LOG=info ip netns exec "$NS_NAME" $NETAVARK dhcp-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR" &>"$TMP_TESTDIR/proxy.log" &
+  RUST_LOG=debug ip netns exec "$NS_NAME" $NETAVARK dhcp-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR" &>"$TMP_TESTDIR/proxy.log" &
   PROXY_PID=$!
 }