diff --git a/src/commands/teardown.rs b/src/commands/teardown.rs index c7d91956..58aac12b 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 2d5aa54f..c1a967eb 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) -> 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) -> 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) -> 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 94e64802..3af0ed5f 100644 --- a/test/250-bridge-nftables.bats +++ b/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))