From 827bdd94924e154c2f62bef58e501fb33b102e27 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Fri, 26 May 2023 11:51:20 +0200
Subject: [PATCH 1/7] dhcp-proxy: remove some unused traits

Note sure what this should do but it is not used so just remove it.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/dhcp_proxy/dhcp_service.rs | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/src/dhcp_proxy/dhcp_service.rs b/src/dhcp_proxy/dhcp_service.rs
index 067970ecb..91db24a2a 100644
--- a/src/dhcp_proxy/dhcp_service.rs
+++ b/src/dhcp_proxy/dhcp_service.rs
@@ -8,7 +8,6 @@ use log::{debug, warn};
 use mozim::{
     DhcpError, DhcpV4ClientAsync, DhcpV4Config, DhcpV4Lease as MozimV4Lease, DhcpV4Lease, ErrorKind,
 };
-use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
 use tokio_stream::StreamExt;
 
 use tonic::{Code, Status};
@@ -50,32 +49,6 @@ pub struct DhcpService {
     network_config: NetworkConfig,
 }
 
-trait IP4Conv {
-    fn from(self) -> Ipv4Addr;
-}
-
-impl IP4Conv for IpAddr {
-    fn from(self) -> Ipv4Addr {
-        if let IpAddr::V4(ipv4) = self {
-            return ipv4;
-        }
-        Ipv4Addr::from(0)
-    }
-}
-
-trait IP6Conv {
-    fn from(self) -> Ipv6Addr;
-}
-
-impl IP6Conv for IpAddr {
-    fn from(self) -> Ipv6Addr {
-        if let IpAddr::V6(ipv6) = self {
-            return ipv6;
-        }
-        Ipv6Addr::from(0)
-    }
-}
-
 impl DhcpService {
     pub fn new(nc: &NetworkConfig, timeout: &u32) -> Result<DhcpService, DhcpServiceError> {
         let client = Self::create_client(nc, timeout)?;

From f7e950b496500a3e073512ab774c0ca3a95a8f5d Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Fri, 26 May 2023 14:18:03 +0200
Subject: [PATCH 2/7] dhcp-proxy: drop macaddr dependency

We already have our own validation functions in netavark, there is no
need for this extra dep.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 Cargo.lock                 |  7 -------
 Cargo.toml                 |  1 -
 src/commands/dhcp_proxy.rs | 31 ++++++++++++-------------------
 src/dhcp_proxy/cache.rs    | 15 ++++++++-------
 4 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 11d8051f7..99a6af56d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1098,12 +1098,6 @@ version = "0.4.18"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "518ef76f2f87365916b142844c16d8fefd85039bc5699050210a7778ee1cd1de"
 
-[[package]]
-name = "macaddr"
-version = "1.0.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "baee0bbc17ce759db233beb01648088061bf678383130602a298e6998eedb2d8"
-
 [[package]]
 name = "matches"
 version = "0.1.10"
@@ -1218,7 +1212,6 @@ dependencies = [
  "iptables",
  "libc",
  "log",
- "macaddr",
  "mozim",
  "netlink-packet-core",
  "netlink-packet-route",
diff --git a/Cargo.toml b/Cargo.toml
index 32a71dcf8..55b221cb6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -64,7 +64,6 @@ futures-core = "0.3.28"
 futures-util = "0.3.28"
 nispor = "1.2.10"
 http = "0.2.9"
-macaddr = "1.0.1"
 tower = { version = "0.4" }
 
 [build-dependencies]
diff --git a/src/commands/dhcp_proxy.rs b/src/commands/dhcp_proxy.rs
index 486b655be..976f1caf6 100644
--- a/src/commands/dhcp_proxy.rs
+++ b/src/commands/dhcp_proxy.rs
@@ -1,9 +1,5 @@
 #![cfg_attr(not(unix), allow(unused_imports))]
 
-use clap::Parser;
-use log::{debug, error, warn};
-use macaddr::MacAddr;
-
 use crate::dhcp_proxy::cache::{Clear, LeaseCache};
 use crate::dhcp_proxy::dhcp_service::DhcpService;
 use crate::dhcp_proxy::ip;
@@ -15,13 +11,15 @@ use crate::dhcp_proxy::proxy_conf::{
     get_cache_fqname, get_proxy_sock_fqname, DEFAULT_INACTIVITY_TIMEOUT, DEFAULT_TIMEOUT,
 };
 use crate::error::{NetavarkError, NetavarkResult};
+use crate::network::core_utils;
+use clap::Parser;
+use log::{debug, error, warn};
 
 use std::fs::File;
 use std::io::Write;
 use std::os::unix::io::FromRawFd;
 use std::os::unix::net::UnixListener as stdUnixListener;
 use std::path::{Path, PathBuf};
-use std::str::FromStr;
 use std::sync::{Arc, Mutex};
 use std::{env, fs};
 #[cfg(unix)]
@@ -34,7 +32,9 @@ use tokio::sync::{mpsc, oneshot};
 use tokio::time::{timeout, Duration};
 #[cfg(unix)]
 use tokio_stream::wrappers::UnixListenerStream;
-use tonic::{transport::Server, Code, Code::Internal, Request, Response, Status};
+use tonic::{
+    transport::Server, Code, Code::Internal, Code::InvalidArgument, Request, Response, Status,
+};
 
 #[derive(Debug)]
 /// This is the tonic netavark proxy service that is required to impl the Netavark Proxy trait which
@@ -393,26 +393,19 @@ async fn process_setup<W: Write + Clear>(
 ) -> Result<NetavarkLease, Status> {
     let container_network_interface = network_config.container_iface.clone();
     let ns_path = network_config.ns_path.clone();
-    // Check mac address and add it to nc
-    let mac_addr = network_config.container_mac_addr.clone();
-    if mac_addr.is_empty() {
-        return Err(Status::new(
-            Code::InvalidArgument,
-            "No mac address provided",
-        ));
-    }
-    if MacAddr::from_str(&mac_addr).is_err() {
-        return Err(Status::new(Code::InvalidArgument, "Invalid mac address"));
-    };
+
+    // test if mac is valid
+    core_utils::CoreUtils::decode_address_from_hex(&network_config.container_mac_addr)
+        .map_err(|e| Status::new(InvalidArgument, format!("{e}")))?;
     let nv_lease = DhcpService::new(&network_config, timeout)?
         .get_lease()
         .await?;
-    debug!("found a lease for {:?}", mac_addr);
+    debug!("found a lease for {:?}", &network_config.container_mac_addr);
 
     if let Err(e) = cache
         .lock()
         .expect("Could not unlock cache. A thread was poisoned")
-        .add_lease(&mac_addr, &nv_lease)
+        .add_lease(&network_config.container_mac_addr, &nv_lease)
     {
         return Err(Status::new(
             Internal,
diff --git a/src/dhcp_proxy/cache.rs b/src/dhcp_proxy/cache.rs
index 5b4fd724a..2fd97e694 100644
--- a/src/dhcp_proxy/cache.rs
+++ b/src/dhcp_proxy/cache.rs
@@ -175,7 +175,7 @@ impl<W: Write + Clear> LeaseCache<W> {
 mod cache_tests {
     use super::super::cache::LeaseCache;
     use super::super::lib::g_rpc::{Lease as NetavarkLease, Lease};
-    use macaddr::MacAddr6;
+    use crate::network::core_utils;
     use rand::{thread_rng, Rng};
     use std::collections::HashMap;
     use std::io::Cursor;
@@ -192,16 +192,17 @@ mod cache_tests {
         )
     }
     // Create a single random mac address
-    fn random_macaddr() -> MacAddr6 {
+    fn random_macaddr() -> String {
         let mut rng = thread_rng();
-        MacAddr6::new(
+        let bytes = vec![
             rng.gen::<u8>(),
             rng.gen::<u8>(),
             rng.gen::<u8>(),
             rng.gen::<u8>(),
             rng.gen::<u8>(),
             rng.gen::<u8>(),
-        )
+        ];
+        core_utils::CoreUtils::encode_address_to_hex(&bytes)
     }
     // Create a single random lease
     fn random_lease(mac_address: &String) -> Lease {
@@ -262,7 +263,7 @@ mod cache_tests {
 
         for i in 0..range {
             // Create a random mac address to create a random lease of that mac address
-            let mac_address = random_macaddr().to_string();
+            let mac_address = random_macaddr();
             macaddrs.push(mac_address.clone());
             let lease = random_lease(&mac_address);
 
@@ -305,7 +306,7 @@ mod cache_tests {
         let range = setup.range;
         for i in 0..range {
             // Create a random mac address to create a random lease of that mac address
-            let mac_address = random_macaddr().to_string();
+            let mac_address = random_macaddr();
             macaddrs.push(mac_address.clone());
             let lease = random_lease(&mac_address);
 
@@ -392,7 +393,7 @@ mod cache_tests {
 
         for i in 0..range {
             // Create a random mac address to create a random lease of that mac address
-            let mac_address = random_macaddr().to_string();
+            let mac_address = random_macaddr();
             macaddrs.push(mac_address.clone());
             let lease = random_lease(&mac_address);
 

From 1a5f6748d6c051024145e2e8abb24a14749602e1 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Mon, 5 Jun 2023 13:52:27 +0200
Subject: [PATCH 3/7] dhcp-proxy: simplify code

Restructure code to only switch once for ipv6/v4. sharing code is likely
not possible anyway to the big differences in the protocols.

This should make it easier for us to use the async stream implemenation
to allow release after the lease expires.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/commands/dhcp_proxy.rs     |  34 +++++----
 src/dhcp_proxy/dhcp_service.rs | 132 ++++++---------------------------
 2 files changed, 43 insertions(+), 123 deletions(-)

diff --git a/src/commands/dhcp_proxy.rs b/src/commands/dhcp_proxy.rs
index 976f1caf6..2d8f901d7 100644
--- a/src/commands/dhcp_proxy.rs
+++ b/src/commands/dhcp_proxy.rs
@@ -1,7 +1,7 @@
 #![cfg_attr(not(unix), allow(unused_imports))]
 
 use crate::dhcp_proxy::cache::{Clear, LeaseCache};
-use crate::dhcp_proxy::dhcp_service::DhcpService;
+use crate::dhcp_proxy::dhcp_service::DhcpV4Service;
 use crate::dhcp_proxy::ip;
 use crate::dhcp_proxy::lib::g_rpc::netavark_proxy_server::{NetavarkProxy, NetavarkProxyServer};
 use crate::dhcp_proxy::lib::g_rpc::{
@@ -100,7 +100,7 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
                 log::debug!("Request dropped, aborting DORA");
                 return Err(Status::new(Code::Aborted, "client disconnected"));
             }
-            let get_lease = process_setup(network_config, &timeout, cache);
+            let get_lease = process_setup(network_config, timeout, cache);
             // watch the client and the lease, which ever finishes first return
             let get_lease: NetavarkLease = tokio::select! {
                 _ = &mut rx => {
@@ -139,7 +139,6 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
         let nc = request.into_inner();
 
         let cache = self.cache.clone();
-        let timeout = self.dora_timeout;
 
         std::thread::spawn(move || {
             // Remove the client from the cache dir
@@ -150,11 +149,6 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
                 .remove_lease(&nc.container_mac_addr)
                 .map_err(|e| Status::internal(e.to_string()))?;
 
-            // Send the DHCP release message
-            DhcpService::new(&nc, &timeout)?
-                .release_lease(&lease)
-                .map_err(|e| Status::internal(e.to_string()))?;
-
             Ok(Response::new(lease))
         })
         .join()
@@ -388,7 +382,7 @@ fn is_catch_empty<W: Write + Clear>(current_cache: Arc<Mutex<LeaseCache<W>>>) ->
 /// returns: Result<Lease, Status>
 async fn process_setup<W: Write + Clear>(
     network_config: NetworkConfig,
-    timeout: &u32,
+    timeout: u32,
     cache: Arc<Mutex<LeaseCache<W>>>,
 ) -> Result<NetavarkLease, Status> {
     let container_network_interface = network_config.container_iface.clone();
@@ -397,15 +391,27 @@ async fn process_setup<W: Write + Clear>(
     // test if mac is valid
     core_utils::CoreUtils::decode_address_from_hex(&network_config.container_mac_addr)
         .map_err(|e| Status::new(InvalidArgument, format!("{e}")))?;
-    let nv_lease = DhcpService::new(&network_config, timeout)?
-        .get_lease()
-        .await?;
-    debug!("found a lease for {:?}", &network_config.container_mac_addr);
+    let mac = &network_config.container_mac_addr.clone();
+
+    let nv_lease = match network_config.version {
+        //V4
+        0 => {
+            let mut service = DhcpV4Service::new(network_config, timeout)?;
+            service.get_lease().await?
+        }
+        //V6 TODO implement DHCPv6
+        1 => {
+            return Err(Status::new(InvalidArgument, "ipv6 not yet supported"));
+        }
+        _ => {
+            return Err(Status::new(InvalidArgument, "invalid protocol version"));
+        }
+    };
 
     if let Err(e) = cache
         .lock()
         .expect("Could not unlock cache. A thread was poisoned")
-        .add_lease(&network_config.container_mac_addr, &nv_lease)
+        .add_lease(mac, &nv_lease)
     {
         return Err(Status::new(
             Internal,
diff --git a/src/dhcp_proxy/dhcp_service.rs b/src/dhcp_proxy/dhcp_service.rs
index 91db24a2a..0335a2f7c 100644
--- a/src/dhcp_proxy/dhcp_service.rs
+++ b/src/dhcp_proxy/dhcp_service.rs
@@ -1,13 +1,10 @@
 use crate::dhcp_proxy::dhcp_service::DhcpServiceErrorKind::{
     Bug, InvalidArgument, NoLease, Timeout,
 };
-use std::convert::TryFrom;
 
-use crate::dhcp_proxy::lib::g_rpc::{Lease as NetavarkLease, Lease, NetworkConfig};
-use log::{debug, warn};
-use mozim::{
-    DhcpError, DhcpV4ClientAsync, DhcpV4Config, DhcpV4Lease as MozimV4Lease, DhcpV4Lease, ErrorKind,
-};
+use crate::dhcp_proxy::lib::g_rpc::{Lease as NetavarkLease, NetworkConfig};
+use log::debug;
+use mozim::{DhcpV4ClientAsync, DhcpV4Config, DhcpV4Lease as MozimV4Lease};
 use tokio_stream::StreamExt;
 
 use tonic::{Code, Status};
@@ -44,55 +41,24 @@ pub enum DhcpClient {
 }
 
 /// DHCP service is responsible for creating, handling, and managing the dhcp lease process.
-pub struct DhcpService {
-    client: Option<DhcpClient>,
+pub struct DhcpV4Service {
+    client: DhcpV4ClientAsync,
     network_config: NetworkConfig,
 }
 
-impl DhcpService {
-    pub fn new(nc: &NetworkConfig, timeout: &u32) -> Result<DhcpService, DhcpServiceError> {
-        let client = Self::create_client(nc, timeout)?;
-        Ok(DhcpService {
-            client: Some(client),
-            network_config: nc.clone(),
+impl DhcpV4Service {
+    pub fn new(nc: NetworkConfig, timeout: u32) -> Result<Self, DhcpServiceError> {
+        let mut config = DhcpV4Config::new_proxy(&nc.host_iface, &nc.container_mac_addr);
+        config.set_timeout(timeout);
+        let client = match DhcpV4ClientAsync::init(config, None) {
+            Ok(client) => Ok(client),
+            Err(err) => Err(DhcpServiceError::new(InvalidArgument, err.to_string())),
+        }?;
+        Ok(Self {
+            client: client,
+            network_config: nc,
         })
     }
-    /// Based on the IP version, use the dhcp client to process a dhcp lease using DORA.
-    /// Note: By using process you pass ownership of the dhcp service.
-    pub async fn get_lease(mut self) -> Result<NetavarkLease, DhcpServiceError> {
-        // match the ip version to create the correct dhcp client
-        if let Some(client) = self.client.take() {
-            return match client {
-                DhcpClient::V4Client(v4_client) => self.get_v4_lease(*v4_client).await,
-                DhcpClient::V6Client() => self.get_v6_lease(),
-            };
-        }
-        Err(DhcpServiceError::new(
-            Bug,
-            "Could not initiate dhcp client".to_string(),
-        ))
-    }
-
-    pub fn release_lease(mut self, lease: &Lease) -> Result<(), DhcpError> {
-        // match the ip version to create the correct dhcp client
-        if let Some(client) = self.client.take() {
-            return match client {
-                DhcpClient::V4Client(_v4_client) => {
-                    let _v4_lease = DhcpV4Lease::try_from(lease.clone())?;
-                    Ok(())
-                    // TODO add release to mozim for async client
-                    // v4_client.release(&v4_lease)
-                }
-                DhcpClient::V6Client() => self.release_v6_lease(),
-            };
-        }
-        // Releasing a lease is not a fatal error
-        warn!(
-            "Unable to release lease for {}",
-            self.network_config.container_mac_addr
-        );
-        Ok(())
-    }
 
     /// Performs a DHCP DORA on a ipv4 network configuration.
     /// # Arguments
@@ -101,11 +67,8 @@ impl DhcpService {
     ///
     /// returns: Result<Lease, DhcpSearchError>. Either finds a lease successfully, finds no lease, or fails
     ///
-    async fn get_v4_lease(
-        &self,
-        mut client: DhcpV4ClientAsync,
-    ) -> Result<NetavarkLease, DhcpServiceError> {
-        if let Some(Ok(lease)) = client.next().await {
+    pub async fn get_lease(&mut self) -> Result<NetavarkLease, DhcpServiceError> {
+        if let Some(Ok(lease)) = self.client.next().await {
             debug!(
                 "successfully found a lease for {:?}",
                 &self.network_config.container_mac_addr
@@ -115,6 +78,11 @@ impl DhcpService {
             netavark_lease.add_domain_name(&self.network_config.domain_name);
             netavark_lease.add_mac_address(&self.network_config.container_mac_addr);
 
+            debug!(
+                "found a lease for {:?}, {:?}",
+                &self.network_config.container_mac_addr, &netavark_lease
+            );
+
             return Ok(netavark_lease);
         }
         Err(DhcpServiceError::new(
@@ -122,60 +90,6 @@ impl DhcpService {
             "Could not find a lease within the timeout limit".to_string(),
         ))
     }
-
-    /// TODO
-    /// Performs a DHCP DORA on a IPv6 network configuration.
-    /// # Arguments
-    ///
-    /// * `client`: a Ipv6 mozim dhcp client. When this method is called, it takes ownership of client.
-    ///
-    /// returns: Result<NetavarkLease, DhcpSearchError>. Either finds a lease successfully, finds no lease, or fails
-    fn get_v6_lease(&self) -> Result<NetavarkLease, DhcpServiceError> {
-        log::error!("ipv6 dhcp requests are unimplemented.");
-        Err(DhcpServiceError::new(
-            Bug,
-            "ipv6 dhcp requests are unimplemented.".to_string(),
-        ))
-    }
-
-    /// Create a DHCP client
-    /// # Arguments
-    ///
-    /// * `iface`: network interface name
-    /// * `version`: Version - can be Ipv4 or Ipv6
-    ///
-    /// returns: Result<DhcpClient, DhcpError>. DhcpClient is either a V4 or V6 client.
-    fn create_client(nc: &NetworkConfig, timeout: &u32) -> Result<DhcpClient, DhcpServiceError> {
-        let version = &nc.version;
-        let iface = &nc.host_iface;
-        match version {
-            //V4
-            0 => {
-                let mut config = DhcpV4Config::new_proxy(iface, &nc.container_mac_addr);
-                config.set_timeout(*timeout);
-                match DhcpV4ClientAsync::init(config, None) {
-                    Ok(client) => Ok(DhcpClient::V4Client(Box::new(client))),
-                    Err(err) => Err(DhcpServiceError::new(InvalidArgument, err.to_string())),
-                }
-            }
-            //V6 TODO implement DHCPv6
-            1 => {
-                unimplemented!();
-            }
-            // No valid version found in the network configuration sent by the client
-            _ => Err(DhcpServiceError::new(
-                InvalidArgument,
-                String::from("Must select a valid IP protocol 0=v4, 1=v6"),
-            )),
-        }
-    }
-
-    fn release_v6_lease(&self) -> Result<(), DhcpError> {
-        Err(DhcpError::new(
-            ErrorKind::Bug,
-            "not implemented".to_string(),
-        ))
-    }
 }
 
 impl std::fmt::Display for DhcpServiceError {

From 44b5a2856b8a1273790d1489a13f5db2c580bc06 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Tue, 6 Jun 2023 16:00:44 +0200
Subject: [PATCH 4/7] dhcp-proxy: actually implement renewing leases

Right now we never tried to renew a lease, this means if th dhcp server
starts serving new ips we will not notice it.
Note that this commit does not change the ip addresses yet on the
contianer interface, this will be done in a follow up commit.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/commands/dhcp_proxy.rs     | 49 +++++++++++++++++++++++-----------
 src/dhcp_proxy/dhcp_service.rs | 48 +++++++++++++++++++++++----------
 2 files changed, 68 insertions(+), 29 deletions(-)

diff --git a/src/commands/dhcp_proxy.rs b/src/commands/dhcp_proxy.rs
index 2d8f901d7..121adb842 100644
--- a/src/commands/dhcp_proxy.rs
+++ b/src/commands/dhcp_proxy.rs
@@ -1,7 +1,7 @@
 #![cfg_attr(not(unix), allow(unused_imports))]
 
 use crate::dhcp_proxy::cache::{Clear, LeaseCache};
-use crate::dhcp_proxy::dhcp_service::DhcpV4Service;
+use crate::dhcp_proxy::dhcp_service::{process_client_stream, DhcpV4Service};
 use crate::dhcp_proxy::ip;
 use crate::dhcp_proxy::lib::g_rpc::netavark_proxy_server::{NetavarkProxy, NetavarkProxyServer};
 use crate::dhcp_proxy::lib::g_rpc::{
@@ -14,7 +14,9 @@ use crate::error::{NetavarkError, NetavarkResult};
 use crate::network::core_utils;
 use clap::Parser;
 use log::{debug, error, warn};
+use tokio::task::AbortHandle;
 
+use std::collections::HashMap;
 use std::fs::File;
 use std::io::Write;
 use std::os::unix::io::FromRawFd;
@@ -55,6 +57,9 @@ struct NetavarkProxyService<W: Write + Clear> {
     dora_timeout: u32,
     // channel send-side for resetting the inactivity timeout
     timeout_sender: 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>>>,
 }
 
 impl<W: Write + Clear> NetavarkProxyService<W> {
@@ -88,6 +93,7 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
 
         let cache = self.cache.clone();
         let timeout = self.dora_timeout;
+        let task_map = self.task_map.clone();
 
         // setup client side streaming
         let network_config = request.into_inner();
@@ -100,7 +106,7 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
                 log::debug!("Request dropped, aborting DORA");
                 return Err(Status::new(Code::Aborted, "client disconnected"));
             }
-            let get_lease = process_setup(network_config, timeout, cache);
+            let get_lease = process_setup(network_config, timeout, cache, task_map);
             // watch the client and the lease, which ever finishes first return
             let get_lease: NetavarkLease = tokio::select! {
                 _ = &mut rx => {
@@ -139,20 +145,24 @@ impl<W: Write + Clear + Send + 'static> NetavarkProxy for NetavarkProxyService<W
         let nc = request.into_inner();
 
         let cache = self.cache.clone();
+        let tasks = self.task_map.clone();
 
-        std::thread::spawn(move || {
-            // Remove the client from the cache dir
-            let lease = cache
-                .clone()
-                .lock()
-                .expect("Could not unlock cache. A thread was poisoned")
-                .remove_lease(&nc.container_mac_addr)
-                .map_err(|e| Status::internal(e.to_string()))?;
+        let task = tasks
+            .lock()
+            .expect("lock tasks")
+            .remove(&nc.container_mac_addr);
+        if let Some(handle) = task {
+            handle.abort();
+        }
 
-            Ok(Response::new(lease))
-        })
-        .join()
-        .expect("Error joining thread")
+        // Remove the client from the cache dir
+        let lease = cache
+            .lock()
+            .expect("Could not unlock cache. A thread was poisoned")
+            .remove_lease(&nc.container_mac_addr)
+            .map_err(|e| Status::internal(e.to_string()))?;
+
+        Ok(Response::new(lease))
     }
 
     /// On teardown of the proxy the cache will be cleared gracefully.
@@ -280,6 +290,7 @@ pub async fn serve(opts: Opts) -> NetavarkResult<()> {
         cache: cache.clone(),
         dora_timeout,
         timeout_sender: Arc::new(Mutex::new(activity_timeout_tx.clone())),
+        task_map: Arc::new(Mutex::new(HashMap::new())),
     };
 
     let server = Server::builder()
@@ -384,6 +395,7 @@ async fn process_setup<W: Write + Clear>(
     network_config: NetworkConfig,
     timeout: u32,
     cache: Arc<Mutex<LeaseCache<W>>>,
+    tasks: Arc<Mutex<HashMap<String, AbortHandle>>>,
 ) -> Result<NetavarkLease, Status> {
     let container_network_interface = network_config.container_iface.clone();
     let ns_path = network_config.ns_path.clone();
@@ -397,7 +409,14 @@ async fn process_setup<W: Write + Clear>(
         //V4
         0 => {
             let mut service = DhcpV4Service::new(network_config, timeout)?;
-            service.get_lease().await?
+
+            let lease = service.get_lease().await?;
+            let task = tokio::spawn(process_client_stream(service));
+            tasks
+                .lock()
+                .expect("lock tasks")
+                .insert(mac.to_string(), task.abort_handle());
+            lease
         }
         //V6 TODO implement DHCPv6
         1 => {
diff --git a/src/dhcp_proxy/dhcp_service.rs b/src/dhcp_proxy/dhcp_service.rs
index 0335a2f7c..19000c14d 100644
--- a/src/dhcp_proxy/dhcp_service.rs
+++ b/src/dhcp_proxy/dhcp_service.rs
@@ -32,18 +32,11 @@ impl DhcpServiceError {
     }
 }
 
-/// The dhcp client can either be a Ipv4 or Ipv6.
-///
-/// These clients are managed differently. so it is important to keep these separate.
-pub enum DhcpClient {
-    V4Client(Box<DhcpV4ClientAsync>),
-    V6Client(/*TODO implement v6 client*/),
-}
-
 /// DHCP service is responsible for creating, handling, and managing the dhcp lease process.
 pub struct DhcpV4Service {
     client: DhcpV4ClientAsync,
     network_config: NetworkConfig,
+    previous_lease: Option<NetavarkLease>,
 }
 
 impl DhcpV4Service {
@@ -55,8 +48,9 @@ impl DhcpV4Service {
             Err(err) => Err(DhcpServiceError::new(InvalidArgument, err.to_string())),
         }?;
         Ok(Self {
-            client: client,
+            client,
             network_config: nc,
+            previous_lease: None,
         })
     }
 
@@ -69,11 +63,6 @@ impl DhcpV4Service {
     ///
     pub async fn get_lease(&mut self) -> Result<NetavarkLease, DhcpServiceError> {
         if let Some(Ok(lease)) = self.client.next().await {
-            debug!(
-                "successfully found a lease for {:?}",
-                &self.network_config.container_mac_addr
-            );
-
             let mut netavark_lease = <NetavarkLease as From<MozimV4Lease>>::from(lease);
             netavark_lease.add_domain_name(&self.network_config.domain_name);
             netavark_lease.add_mac_address(&self.network_config.container_mac_addr);
@@ -82,6 +71,7 @@ impl DhcpV4Service {
                 "found a lease for {:?}, {:?}",
                 &self.network_config.container_mac_addr, &netavark_lease
             );
+            self.previous_lease = Some(netavark_lease.clone());
 
             return Ok(netavark_lease);
         }
@@ -109,3 +99,33 @@ 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::debug!(
+                    "got new lease for mac {}: {:?}",
+                    &client.network_config.container_mac_addr,
+                    &lease
+                );
+                let lease = NetavarkLease::from(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.gateways != lease.gateways
+                        || old_lease.subnet_mask != lease.subnet_mask
+                    {
+                        // ips do not match, remove old ones and assign new ones.
+                        log::error!("ips do not match, reassign not implemented")
+                    }
+                }
+                client.previous_lease = Some(lease)
+            }
+            Err(err) => log::error!(
+                "Failed to renew lease for {}: {err}",
+                &client.network_config.container_mac_addr
+            ),
+        }
+    }
+}

From 2a17957e4bc9f3fa0e4eba97138497254f8ce80e Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Wed, 7 Jun 2023 13:49:25 +0200
Subject: [PATCH 5/7] test-dhcp: fix broekn has_ip() check

This never check anything, pipes with these run functions do not work!

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 test-dhcp/002-setup.bats | 2 +-
 test-dhcp/helpers.bash   | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/test-dhcp/002-setup.bats b/test-dhcp/002-setup.bats
index 0af25a1a8..52437eea5 100644
--- a/test-dhcp/002-setup.bats
+++ b/test-dhcp/002-setup.bats
@@ -24,7 +24,7 @@ EOF
         # Check that gateway provided is the first IP in the subnet
         assert `echo "$output" | jq -r .siaddr` == $(gateway_from_subnet "$SUBNET_CIDR")
         container_ip=$(echo "$output" | jq -r .yiaddr)
-        has_ip "$container_ip"
+        has_ip "$container_ip" veth0
 }
 
 
diff --git a/test-dhcp/helpers.bash b/test-dhcp/helpers.bash
index 81aa60938..7d16eba4b 100644
--- a/test-dhcp/helpers.bash
+++ b/test-dhcp/helpers.bash
@@ -495,5 +495,8 @@ function random_string() {
 
 function has_ip() {
   local container_ip=$1
-  run_in_container_netns ip -j address show tun0 | jq .[0].addr_info | jq -c 'map(select(.local | contains("$container_ip")))'
+  local interface=$2
+  run_in_container_netns ip -j address show $interface
+  addr_info=$(jq '.[0].addr_info' <<<"$output")
+  assert "$addr_info" =~ "$container_ip" "ip not set on interface $interface"
 }

From 72aa0ddb470162aaad99dc801a0726c9ec02e30d Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Wed, 7 Jun 2023 19:06:47 +0200
Subject: [PATCH 6/7] dhcp-proxy: apply new ip address/gateway

If we get a new lease with different ips or a new gateway we have to
update the contianer netns with the new addresses. The added tests takes
over 2m because the minimal lease time that dnsmasq supports is 2m so we
have to love with that for now.

One outstanding issue is that podman has no idea that things changed, it
will continue to show incorrect network info in podman inspect. This
never worked with CNI either so it should be ok for now.

However I think it would be a great improvement for long running
containers if we could somehow update the satus in podman. I think we
need some hidden podman command callback where we can feed podman the
new info.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/dhcp_proxy/dhcp_service.rs | 96 +++++++++++++++++++++++++++++++---
 test-dhcp/005-renew.bats       | 59 +++++++++++++++++++++
 test-dhcp/helpers.bash         | 19 ++++---
 3 files changed, 159 insertions(+), 15 deletions(-)
 create mode 100644 test-dhcp/005-renew.bats

diff --git a/src/dhcp_proxy/dhcp_service.rs b/src/dhcp_proxy/dhcp_service.rs
index 19000c14d..acc50c5f6 100644
--- a/src/dhcp_proxy/dhcp_service.rs
+++ b/src/dhcp_proxy/dhcp_service.rs
@@ -1,8 +1,14 @@
+use std::net::Ipv4Addr;
+
 use crate::dhcp_proxy::dhcp_service::DhcpServiceErrorKind::{
     Bug, InvalidArgument, NoLease, Timeout,
 };
 
 use crate::dhcp_proxy::lib::g_rpc::{Lease as NetavarkLease, NetworkConfig};
+use crate::error::{ErrorWrap, NetavarkError, NetavarkResult};
+use crate::network::core_utils;
+use crate::network::netlink::Route;
+use crate::wrap;
 use log::debug;
 use mozim::{DhcpV4ClientAsync, DhcpV4Config, DhcpV4Lease as MozimV4Lease};
 use tokio_stream::StreamExt;
@@ -36,7 +42,7 @@ impl DhcpServiceError {
 pub struct DhcpV4Service {
     client: DhcpV4ClientAsync,
     network_config: NetworkConfig,
-    previous_lease: Option<NetavarkLease>,
+    previous_lease: Option<MozimV4Lease>,
 }
 
 impl DhcpV4Service {
@@ -63,7 +69,7 @@ impl DhcpV4Service {
     ///
     pub async fn get_lease(&mut self) -> Result<NetavarkLease, DhcpServiceError> {
         if let Some(Ok(lease)) = self.client.next().await {
-            let mut netavark_lease = <NetavarkLease as From<MozimV4Lease>>::from(lease);
+            let mut netavark_lease = <NetavarkLease as From<MozimV4Lease>>::from(lease.clone());
             netavark_lease.add_domain_name(&self.network_config.domain_name);
             netavark_lease.add_mac_address(&self.network_config.container_mac_addr);
 
@@ -71,7 +77,7 @@ impl DhcpV4Service {
                 "found a lease for {:?}, {:?}",
                 &self.network_config.container_mac_addr, &netavark_lease
             );
-            self.previous_lease = Some(netavark_lease.clone());
+            self.previous_lease = Some(lease);
 
             return Ok(netavark_lease);
         }
@@ -104,20 +110,34 @@ pub async fn process_client_stream(mut client: DhcpV4Service) {
     while let Some(lease) = client.client.next().await {
         match lease {
             Ok(lease) => {
-                log::debug!(
+                log::info!(
                     "got new lease for mac {}: {:?}",
                     &client.network_config.container_mac_addr,
                     &lease
                 );
-                let lease = NetavarkLease::from(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.gateways != lease.gateways
                         || old_lease.subnet_mask != lease.subnet_mask
+                        || old_lease.gateways != lease.gateways
                     {
                         // ips do not match, remove old ones and assign new ones.
-                        log::error!("ips do not match, reassign not implemented")
+                        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)
@@ -129,3 +149,65 @@ pub async fn process_client_stream(mut client: DhcpV4Service) {
         }
     }
 }
+
+fn update_lease_ip(
+    netns: &str,
+    interface: &str,
+    old_lease: &MozimV4Lease,
+    new_lease: &MozimV4Lease,
+) -> NetavarkResult<()> {
+    let (_, netns) =
+        core_utils::open_netlink_sockets(netns).wrap("failed to open netlink socket in netns")?;
+    let mut sock = netns.netlink;
+    let old_net = wrap!(
+        ipnet::Ipv4Net::with_netmask(old_lease.yiaddr, old_lease.subnet_mask),
+        "create ipnet from old lease"
+    )?;
+    let new_net = wrap!(
+        ipnet::Ipv4Net::with_netmask(new_lease.yiaddr, new_lease.subnet_mask),
+        "create ipnet from new lease"
+    )?;
+
+    if new_net != old_net {
+        let link = sock
+            .get_link(crate::network::netlink::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 addrs")?;
+    }
+    if new_lease.gateways != old_lease.gateways {
+        if let Some(gws) = &old_lease.gateways {
+            let old_gw = gws.first();
+            if let Some(gw) = old_gw {
+                let route = Route::Ipv4 {
+                    dest: ipnet::Ipv4Net::new(Ipv4Addr::new(0, 0, 0, 0), 0)?,
+                    gw: *gw,
+                    metric: None,
+                };
+                match sock.del_route(&route) {
+                    Ok(_) => {}
+                    Err(err) => match err.unwrap() {
+                        // special case do not error if route does not exists
+                        NetavarkError::Netlink(e) if -e.code == libc::ESRCH => {}
+                        _ => return Err(err).wrap("delete old default route"),
+                    },
+                };
+            }
+        }
+        if let Some(gws) = &new_lease.gateways {
+            let new_gw = gws.first();
+            if let Some(gw) = new_gw {
+                let route = Route::Ipv4 {
+                    dest: ipnet::Ipv4Net::new(Ipv4Addr::new(0, 0, 0, 0), 0)?,
+                    gw: *gw,
+                    metric: None,
+                };
+                sock.add_route(&route)?;
+            }
+        }
+    }
+
+    Ok(())
+}
diff --git a/test-dhcp/005-renew.bats b/test-dhcp/005-renew.bats
new file mode 100644
index 000000000..b549c58b6
--- /dev/null
+++ b/test-dhcp/005-renew.bats
@@ -0,0 +1,59 @@
+#!/usr/bin/env bats   -*- bats -*-
+#
+# Test that release is working after lease timeout
+#
+
+load helpers
+
+
+@test "release after timeout" {
+      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",
+  "version": 0,
+  "ns_path": "$NS_PATH"
+}
+  \0
+EOF
+
+
+    run_setup "$input_config"
+    ip_before=$(jq -r '.yiaddr' <<<"$output")
+    gw_before=$(jq -r '.gateways[0]' <<<"$output")
+    has_ip "$ip_before" veth0
+    run_in_container_netns ip -j route show default
+    assert "$output" =~ "$gw_before"
+
+
+    # stop dhcp and restart with new subnet to get a new ip on the next lease
+    stop_dhcp
+    run_in_container_netns ip add del $(gateway_from_subnet) dev br0
+    run_in_container_netns ip addr
+    run_in_container_netns ip route
+
+    # get new subnet
+    SUBNET_CIDR=$(random_subnet)
+    run_in_container_netns ip addr add $(gateway_from_subnet) dev br0
+    stripped_subnet=$(strip_last_octet_from_subnet)
+    run_dhcp
+
+    run_in_container_netns ip addr
+    run_in_container_netns ip route
+
+    # Sigh, minimum lease time in dnsmasq is 2m, give some extra time for the
+    # lease roundtrip and ip changes to be applied
+    sleep 125
+    # after two minutes we should have a new lease and assigned the new ip
+    has_ip "$stripped_subnet" veth0
+
+    # make sure we got the new gateway set as well
+    run_in_container_netns ip -j route show default
+    assert "$output" =~ "$(gateway_from_subnet)"
+
+    # extra check to make sure we got our expected log
+    run_helper grep "ip or gateway for mac $CONTAINER_MAC changed" "$TMP_TESTDIR/proxy.log"
+}
diff --git a/test-dhcp/helpers.bash b/test-dhcp/helpers.bash
index 7d16eba4b..b43e088f8 100644
--- a/test-dhcp/helpers.bash
+++ b/test-dhcp/helpers.bash
@@ -263,6 +263,7 @@ function basic_teardown(){
   remove_bridge "br0"
   stop_dhcp "$DNSMASQ_PID"
   run_in_container_netns ip link set lo down
+  rm -rf "$TMP_TESTDIR"
 }
 
 
@@ -276,7 +277,6 @@ function basic_setup() {
   add_veth "veth1" "br0"
   run_in_container_netns ip link set lo up
   run_dhcp "$TESTSDIR/dnsmasqfiles"
-  DNSMASQ_PID="$output"
   start_proxy
 }
 
@@ -346,7 +346,7 @@ port=0
 
 
 # To enable dnsmasq's DHCP server functionality.
-dhcp-range=${stripped_subnet}50,${stripped_subnet}59,255.255.255.0,12h
+dhcp-range=${stripped_subnet}50,${stripped_subnet}59,255.255.255.0,2m
 
 # Set gateway as Router. Following two lines are identical.
 dhcp-option=3,$gw
@@ -362,27 +362,30 @@ log-dhcp    # log dhcp related messages.
 \0
 EOF
   dnsmasq_testdir="${TMP_TESTDIR}/dnsmasq"
-  DNSMASQ_PIDFILE="${TMP_TESTDIR}/dns.PID"
-  mkdir $dnsmasq_testdir
+  mkdir -p $dnsmasq_testdir
   echo "$dnsmasq_config" > "$dnsmasq_testdir/test.conf"
 
-   run_in_container_netns dnsmasq --log-debug --log-queries --conf-dir "${dnsmasq_testdir}" -x "${DNSMASQ_PIDFILE}" &
+  ip netns exec "${NS_NAME}" dnsmasq --log-debug --log-dhcp --no-daemon --conf-dir "${dnsmasq_testdir}" &>>"$TMP_TESTDIR/dnsmasq.log" &
+  DNSMASQ_PID=$!
 }
 
 #
 #  stop_dhcp 27231
 #
 function stop_dhcp() {
-  run_helper cat "$DNSMASQ_PIDFILE"
-  kill -9 "$output"
+  echo "dnsmasq log:"
+  cat "${TMP_TESTDIR}/dnsmasq.log"
+  kill -9 "$DNSMASQ_PID"
 }
 
 function start_proxy() {
-  ip netns exec "$NS_NAME" $NETAVARK dhcp-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR"  &
+  RUST_LOG=info ip netns exec "$NS_NAME" $NETAVARK dhcp-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR" &>"$TMP_TESTDIR/proxy.log" &
   PROXY_PID=$!
 }
 
 function stop_proxy(){
+  echo "proxy log:"
+  cat "$TMP_TESTDIR/proxy.log"
   kill -9 $PROXY_PID
 }
 

From 871096c14c8647db60e814306425ae3dc3aed312 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Wed, 7 Jun 2023 19:14:34 +0200
Subject: [PATCH 7/7] netlink: fix incorrect info log for del_route

This code path is also used when a route is deleted, make sure to move
the log at the correct place.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/network/netlink.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/network/netlink.rs b/src/network/netlink.rs
index f379bed55..b5c1a6fe9 100644
--- a/src/network/netlink.rs
+++ b/src/network/netlink.rs
@@ -250,8 +250,6 @@ impl Socket {
         msg.header.scope = RT_SCOPE_UNIVERSE;
         msg.header.kind = RTN_UNICAST;
 
-        info!("Adding route {}", route);
-
         let (dest_vec, dest_prefix, gateway_vec, final_metric) = match route {
             Route::Ipv4 { dest, gw, metric } => {
                 msg.header.address_family = AF_INET as u8;
@@ -285,6 +283,7 @@ impl Socket {
 
     pub fn add_route(&mut self, route: &Route) -> NetavarkResult<()> {
         let msg = Self::create_route_msg(route);
+        info!("Adding route {}", route);
 
         let result =
             self.make_netlink_request(RtnlMessage::NewRoute(msg), NLM_F_ACK | NLM_F_CREATE)?;
@@ -295,6 +294,7 @@ impl Socket {
 
     pub fn del_route(&mut self, route: &Route) -> NetavarkResult<()> {
         let msg = Self::create_route_msg(route);
+        info!("Deleting route {}", route);
 
         let result = self.make_netlink_request(RtnlMessage::DelRoute(msg), NLM_F_ACK)?;
         expect_netlink_result!(result, 0);