Merge pull request #1060 from Luap99/delete-entries

aardvark: on start failure delete entries again

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));
             }
         }

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)

test/250-bridge-nftables.bats

@@ -259,6 +259,17 @@ export NETAVARK_FW=nftables
     assert "${lines[1]}" =~ ".*aardvark-dns --config $NETAVARK_TMPDIR/config/aardvark-dns -p 53 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))