setup: on av errors cleanup again

When aardvark-dns error out at the end we already configured interfaces
+ firewall rules for the driver but because we return a error podman
considers this a failure and teardown is never called.

Netavark should always cleanup on its own. So on errors make sure to
tear down the drivers again.

Fixes containers#1121

Signed-off-by: Paul Holzinger <[email protected]>

src/commands/setup.rs

@@ -3,8 +3,8 @@ use crate::commands::get_config_dir;
 use crate::dns::aardvark::Aardvark;
 use crate::error::{NetavarkError, NetavarkResult};
 use crate::firewall;
-use crate::network::driver::{get_network_driver, DriverInfo};
-use crate::network::netlink::LinkID;
+use crate::network::driver::{get_network_driver, DriverInfo, NetworkDriver};
+use crate::network::netlink::{self, LinkID};
 use crate::network::{self};
 use crate::network::{core_utils, types};
 
@@ -109,17 +109,11 @@ impl Setup {
                     Ok((s, a)) => (s, a),
                     Err(e) => {
                         // now teardown the already setup drivers
-                        for dri in drivers.iter().take(i) {
-                            match dri.teardown((&mut hostns.netlink, &mut netns.netlink)) {
-                                Ok(_) => {}
-                                Err(e) => {
-                                    error!(
-                                    "failed to cleanup previous networks after setup failed: {}",
-                                    e
-                                )
-                                }
-                            };
-                        }
+                        teardown_drivers(
+                            drivers.iter().take(i),
+                            &mut hostns.netlink,
+                            &mut netns.netlink,
+                        );
                         return Err(e);
                     }
                 };
@@ -139,22 +133,19 @@ impl Setup {
                     // ignore error when path already exists
                     Err(ref e) if e.kind() == std::io::ErrorKind::AlreadyExists => {}
                     Err(e) => {
-                        return Err(std::io::Error::new(
-                            std::io::ErrorKind::Other,
-                            format!("failed to create aardvark-dns directory: {e}"),
-                        )
-                        .into());
+                        teardown_drivers(drivers.iter(), &mut hostns.netlink, &mut netns.netlink);
+                        return Err(NetavarkError::wrap(
+                            format!("failed to create aardvark-dns directory {}", path.display()),
+                            NetavarkError::Io(e),
+                        ));
                     }
                 }
 
                 let aardvark_interface = Aardvark::new(path, rootless, aardvark_bin, dns_port);
 
                 if let Err(er) = aardvark_interface.commit_netavark_entries(aardvark_entries) {
-                    return Err(std::io::Error::new(
-                        std::io::ErrorKind::Other,
-                        format!("Error while applying dns entries: {er}"),
-                    )
-                    .into());
+                    teardown_drivers(drivers.iter(), &mut hostns.netlink, &mut netns.netlink);
+                    return Err(NetavarkError::wrap("error while applying dns entries", er));
                 }
             } else {
                 info!(
@@ -170,3 +161,18 @@ impl Setup {
         Ok(())
     }
 }
+
+fn teardown_drivers<'a, I>(drivers: I, host: &mut netlink::Socket, netns: &mut netlink::Socket)
+where
+    I: Iterator<Item = &'a Box<dyn NetworkDriver + 'a>>,
+{
+    for driver in drivers {
+        if let Err(e) = driver.teardown((host, netns)) {
+            error!(
+                "failed to cleanup network {} after setup failed: {}",
+                driver.network_name(),
+                e
+            );
+        };
+    }
+}

test/100-bridge-iptables.bats

@@ -1088,3 +1088,12 @@ function check_simple_bridge_iptables() {
     assert "${lines[3]}" == "-A NETAVARK_FORWARD -s 10.88.0.0/16 -j ACCEPT" "NETAVARK_FORWARD rule 3"
     assert "${#lines[@]}" = 4 "too many NETAVARK_FORWARD rules"
 }
+
+@test "$fw_driver - aardvark-dns error cleanup" {
+    expected_rc=1 run_netavark -a /usr/bin/false --file ${TESTSDIR}/testfiles/dualstack-bridge-custom-dns-server.json setup $(get_container_netns_path)
+    assert_json ".error" "error while applying dns entries: IO error: aardvark-dns exited unexpectedly without error message" "aardvark-dns error"
+    run_in_host_netns iptables -S
+    assert "$output" !~ "10.89.3.0/24" "leaked network iptables rules after setup error"
+    run_in_host_netns iptables -S -t nat
+    assert "$output" !~ "10.89.3.0/24" "leaked network iptables NAT rules after setup error"
+}

test/250-bridge-nftables.bats

@@ -1003,3 +1003,13 @@ function check_simple_bridge_nftables() {
 
     expected_rc=1 run_in_host_netns nft list chain inet netavark $chain
 }
+
+
+@test "$fw_driver - aardvark-dns error cleanup" {
+    expected_rc=1 run_netavark -a /usr/bin/false --file ${TESTSDIR}/testfiles/dualstack-bridge-custom-dns-server.json setup $(get_container_netns_path)
+    assert_json ".error" "error while applying dns entries: IO error: aardvark-dns exited unexpectedly without error message" "aardvark-dns error"
+
+    run_in_host_netns nft list table inet netavark
+    assert "$output" !~ "10.89.3.0/24" "leaked network nft rules after setup error"
+    assert "$output" !~ "fd10:88:a::/64" "leaked network nft rules after setup error"
+}