From 4f37d9fa3d03187cabdaae08bada7fa89930adf2 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Wed, 14 Aug 2024 15:20:38 +0200
Subject: [PATCH] aardvark: on start failure delete entries again

If we fail to start make sure to remove the entries again to not leak
them in case following starts might work.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 src/commands/teardown.rs      |  2 +-
 src/dns/aardvark.rs           | 22 ++++++++++++++++------
 test/250-bridge-nftables.bats | 11 +++++++++++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/commands/teardown.rs b/src/commands/teardown.rs
index 8bbafa998..5bb628522 100644
--- a/src/commands/teardown.rs
+++ b/src/commands/teardown.rs
@@ -67,7 +67,7 @@ impl Teardown {
             let path = Path::new(&config_dir).join("aardvark-dns");
 
             let aardvark_interface = Aardvark::new(path, rootless, aardvark_bin, dns_port);
-            if let Err(err) = aardvark_interface.delete_from_netavark_entries(aardvark_entries) {
+            if let Err(err) = aardvark_interface.delete_from_netavark_entries(&aardvark_entries) {
                 error_list.push(NetavarkError::wrap("remove aardvark entries", err));
             }
         }
diff --git a/src/dns/aardvark.rs b/src/dns/aardvark.rs
index 2d5aa54f2..c1a967ebf 100644
--- a/src/dns/aardvark.rs
+++ b/src/dns/aardvark.rs
@@ -211,7 +211,7 @@ impl Aardvark {
         Ok(())
     }
 
-    pub fn commit_entries(&self, entries: Vec<AardvarkEntry>) -> Result<()> {
+    pub fn commit_entries(&self, entries: &[AardvarkEntry]) -> Result<()> {
         // Acquire fs lock to ensure other instance of aardvark cannot commit
         // or start aardvark instance till already running instance has not
         // completed its `commit` phase.
@@ -240,7 +240,7 @@ impl Aardvark {
             ));
         }
 
-        for entry in &entries {
+        for entry in entries {
             let mut path = Path::new(&self.config).join(entry.network_name);
             if entry.is_internal {
                 let new_path = Path::new(&self.config).join(entry.network_name.to_owned() + "%int");
@@ -344,8 +344,18 @@ impl Aardvark {
 
     pub fn commit_netavark_entries(&self, entries: Vec<AardvarkEntry>) -> NetavarkResult<()> {
         if !entries.is_empty() {
-            self.commit_entries(entries)?;
-            self.notify(true, false)?;
+            self.commit_entries(&entries)?;
+            match self.notify(true, false) {
+                Ok(_) => (),
+                Err(e) => {
+                    if let Err(err) = self.delete_from_netavark_entries(&entries) {
+                        log::warn!(
+                            "Failed to delete aardvark-dns entries after failed start: {err}"
+                        );
+                    };
+                    return Err(e);
+                }
+            };
         }
         Ok(())
     }
@@ -450,8 +460,8 @@ impl Aardvark {
         Ok(())
     }
 
-    pub fn delete_from_netavark_entries(&self, entries: Vec<AardvarkEntry>) -> NetavarkResult<()> {
-        for entry in &entries {
+    pub fn delete_from_netavark_entries(&self, entries: &[AardvarkEntry]) -> NetavarkResult<()> {
+        for entry in entries {
             self.delete_entry(entry.container_id, entry.network_name)?;
         }
         self.notify(false, false)
diff --git a/test/250-bridge-nftables.bats b/test/250-bridge-nftables.bats
index c5687acd0..88086e82b 100644
--- a/test/250-bridge-nftables.bats
+++ b/test/250-bridge-nftables.bats
@@ -258,6 +258,17 @@ export NETAVARK_FW=nftables
     assert "${lines[1]}" =~ ".*aardvark-dns --config $NETAVARK_TMPDIR/config/aardvark-dns -p $dns_port run" "aardvark not running or bad options"
 }
 
+@test "$fw_driver - aardvark-dns entries after startup failure" {
+    # force failure with invalid aardvark-dns binary
+    expected_rc=1 run_netavark --aardvark-binary ${TESTSDIR} --file ${TESTSDIR}/testfiles/dualstack-bridge-custom-dns-server.json \
+        setup $(get_container_netns_path)
+    assert "$output" =~ "aardvark-dns failed to start: Failed to find executable" "netavark error"
+
+    # check aardvark config must not exists after error
+    run_helper ls "$NETAVARK_TMPDIR/config/aardvark-dns"
+    assert "$output" == "" "No aardvark entries"
+}
+
 @test "$fw_driver - bridge driver must generate config for aardvark with multiple custom dns server" {
     # get a random port directly to avoid low ports e.g. 53 would not create nftables
     dns_port=$((RANDOM+10000))