bridge: unmanaged aardvark-dns fix #1304
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR enhances the netlink address dump to support interface-level filtering and updates the bridge driver to bind aardvark-dns to the bridge’s own IPs in unmanaged mode (falling back to gateway IPs otherwise), with accompanying test and example updates. Sequence diagram for aardvark-dns binding in unmanaged bridge modesequenceDiagram
participant Bridge
participant Socket
participant AardvarkDNS
Bridge->>Socket: dump_addresses(Some(LinkID::Name(bridge_interface_name)))
Socket-->>Bridge: Vec<AddressMessage> (filtered by interface)
Bridge->>AardvarkDNS: bind to bridge IPs
AardvarkDNS-->>Bridge: success/failure
ER diagram for AardvarkEntry struct field changeerDiagram
AardvarkEntry {
string network_name
string container_id
IpAddr[] network_gateways
string[] network_dns_servers
IpAddr[] container_ips_v4
IpAddr[] container_ips_v6
}
%% network_gateways now holds bridge IPs in unmanaged mode, not gateway IPs
Class diagram for updated Socket and Bridge classesclassDiagram
class Socket {
+dump_addresses(interface: Option<LinkID>) : Vec<AddressMessage>
+set_netlink_get_strict_chk(true)
}
class Bridge {
+NetworkDriver
+info
+bind_addr: Vec<IpAddr>
}
Socket <|-- Bridge
LinkID <.. Socket
AddressMessage <.. Socket
IpAddr <.. Bridge
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @shivkr6 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/network/netlink.rs:117` </location>
<code_context>
pub fn new() -> NetavarkResult<Socket> {
let mut socket = wrap!(netlink_sys::Socket::new(NETLINK_ROUTE), "open")?;
let addr = &SocketAddr::new(0, 0);
+ // Needs to be enabled for dump filtering to work
+ socket.set_netlink_get_strict_chk(true)?;
wrap!(socket.bind(addr), "bind")?;
wrap!(socket.connect(addr), "connect")?;
</code_context>
<issue_to_address>
Enabling strict netlink checking may affect compatibility with older kernels.
Since netlink_get_strict_chk may not be available on all kernels, please handle potential failures gracefully or specify the minimum supported kernel version in the documentation.
</issue_to_address>
### Comment 2
<location> `src/network/netlink.rs:414` </location>
<code_context>
- pub fn dump_addresses(&mut self) -> NetavarkResult<Vec<AddressMessage>> {
- let msg = AddressMessage::default();
+ // If interface's LinkID is supplied, then only the ip addresses of that specific interface is returned. Otherwise all ip addresses of all interfaces are returned
+ pub fn dump_addresses(
+ &mut self,
+ interface: Option<LinkID>,
+ ) -> NetavarkResult<Vec<AddressMessage>> {
+ let mut msg = AddressMessage::default();
- let results = self
</code_context>
<issue_to_address>
Consider validating LinkID::Name resolution failure.
If get_link does not find the interface, consider returning a clearer error or handling the failure to improve debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Some(LinkID::Name(name)) => {
let link_message = self.get_link(LinkID::Name(name))?;
msg.header.index = link_message.header.index;
}
=======
Some(LinkID::Name(name)) => {
let link_message = match self.get_link(LinkID::Name(name)) {
Ok(link) => link,
Err(e) => {
// Return a clear error if the interface name cannot be resolved
return Err(anyhow::anyhow!(
"Failed to resolve interface name '{}': {}",
name,
e
));
}
};
msg.header.index = link_message.header.index;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/network/netlink.rs:415` </location>
<code_context>
+ ) -> NetavarkResult<Vec<AddressMessage>> {
+ let mut msg = AddressMessage::default();
- let results = self
- .make_netlink_request(RouteNetlinkMessage::GetAddress(msg), NLM_F_DUMP | NLM_F_ACK)?;
+ match interface {
+ Some(LinkID::ID(id)) => {
</code_context>
<issue_to_address>
Dropping NLM_F_ACK flag may affect error reporting for netlink requests.
Without NLM_F_ACK, kernel errors may not be reported explicitly. Please confirm this is intentional and that error handling remains robust.
</issue_to_address>
### Comment 4
<location> `src/test/netlink.rs:190` </location>
<code_context>
}
}
}
+ #[test]
+ fn test_dump_addr_filter() {
+ test_setup!();
+ let mut sock = Socket::new().expect("Socket::new()");
</code_context>
<issue_to_address>
Consider adding tests for IPv6 addresses and multiple addresses on the same interface.
Please add test cases for interfaces with multiple IPv4 and IPv6 addresses to verify correct filtering and address parsing.
</issue_to_address>
### Comment 5
<location> `src/test/netlink.rs` </location>
<code_context>
+ let addresses = sock
+ .dump_addresses(Some(LinkID::Name("test1".to_string())))
+ .expect("dump_address_filter failed");
</code_context>
<issue_to_address>
Test does not cover the case where no addresses are present on the interface.
Add a test where the interface exists but has no IP addresses, to confirm dump_addresses returns an empty result or handles it correctly.
</issue_to_address>
### Comment 6
<location> `src/test/netlink.rs:179` </location>
<code_context>
- let addresses = sock.dump_addresses().expect("dump_addresses failed");
+ let addresses = sock.dump_addresses(None).expect("dump_addresses failed");
for nla in addresses[0].attributes.iter() {
if let address::AddressAttribute::Address(ip) = nla {
assert_eq!(ip, &IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)))
</code_context>
<issue_to_address>
Test assumes addresses[0] exists; consider adding assertions for empty results.
Add an assertion to verify that addresses is not empty before accessing addresses[0] to avoid potential panics and improve test robustness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let addresses = sock.dump_addresses(None).expect("dump_addresses failed");
for nla in addresses[0].attributes.iter() {
if let address::AddressAttribute::Address(ip) = nla {
assert_eq!(ip, &IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)))
}
}
=======
let addresses = sock.dump_addresses(None).expect("dump_addresses failed");
assert!(!addresses.is_empty(), "No addresses returned, expected at least one");
for nla in addresses[0].attributes.iter() {
if let address::AddressAttribute::Address(ip) = nla {
assert_eq!(ip, &IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)))
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `src/network/bridge.rs:258` </location>
<code_context>
- .collect();
+ // Fixes #1177: In unmanaged mode, the gateway IP may not be on the host.
+ // We need to find an IP on the bridge itself for aardvark-dns to bind to.
+ let bind_addr: Vec<IpAddr> = if data.mode == BridgeMode::Unmanaged {
+ let addresses = host_sock
+ .dump_addresses(Some(LinkID::Name(data.bridge_interface_name.clone())))?;
</code_context>
<issue_to_address>
Consider extracting the IP address collection logic into a helper function to simplify and flatten the main code.
```rust
// helper at top of the file
fn collect_bridge_ips(
addrs: Vec<netlink_packet_route::address::AddressMessage>,
) -> Vec<IpAddr> {
addrs
.into_iter()
.flat_map(|msg| msg.attributes)
.filter_map(|attr| {
if let netlink_packet_route::address::AddressAttribute::Address(ip) = attr {
Some(ip)
} else {
None
}
})
.collect()
}
// … inside your method
let bind_addr: Vec<IpAddr> = if data.mode == BridgeMode::Unmanaged {
let addrs = host_sock!
.dump_addresses(Some(LinkID::Name(data.bridge_interface_name.clone())))?;
let ips = collect_bridge_ips(addrs);
if ips.is_empty() {
return Err(NetavarkError::msg(format!(
"bridge '{}' in unmanaged mode has no IP addresses…",
data.bridge_interface_name,
)));
}
ips
} else {
data.ipam
.gateway_addresses
.iter()
.map(|ipnet| ipnet.addr())
.collect()
};
```
This pulls out the nested loops and `if let` into a tiny helper using `flat_map`/`filter_map`, making the main code flatter and easier to follow without losing any functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I have tested the modified dump_addresses() function but have not tested the fix in the second commit. Can someone please test if it works? |
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
from
51c389d to
c98368d
Compare
|
Let me know if I should add more tests or extract the IP address collection logic into a helper function as suggested by sourcery-ai. |
Luap99
reviewed
Aug 11, 2025
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
2 times, most recently
from
f5bc7c1 to
c534d89
Compare
shivkr6
commented
Aug 13, 2025
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
2 times, most recently
from
636216d to
2d8e758
Compare
|
I had to write 2 separate tests in test/620-bridge-mode.bats because after the following lines, I have to perform a teardown because of the following error: and if I perform a teardown with the configuration file (test/testfiles/bridge-unmanaged-dns.json) I get the following error. This error does not happen if I specify the config file with dns disabled. I guess this is because when we ran netavark before, it already created the veth pair and the teardown doesn't work with dns enabled because we got an error in netavark before it could create entries for aardvark-dns. |
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
from
2d8e758 to
3f7f7bb
Compare
Luap99
reviewed
Aug 15, 2025
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
from
3f7f7bb to
6c834d7
Compare
Luap99
reviewed
Aug 18, 2025
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
from
6c834d7 to
b20f884
Compare
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
2 times, most recently
from
5e32765 to
237ea44
Compare
…ot on the host Find all the IPv4 and IPv6 addresses except link local IPv6 addressses of the bridge with the modified dump_addresses() function. If the dns is enabled and the bridge mode is unmanaged, then the bind IP of aardvark-dns is changed to the IP addresses of the bridge instead of the gateway. If there is no usable IP address on the bridge then we just fail with a clear error that the user must disable dns (--disable-dns) when creating the network. Fixes: containers#1177 Signed-off-by: Shivang K Raghuvanshi <[email protected]>
shivkr6
force-pushed
the
bridge-unmanaged-dns-fix
branch
from
237ea44 to
8655e62
Compare
|
PTAL @Luap99 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, shivkr6, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mheon PTAL |
|
/lgtm |
|
Thanks! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bind aardvark-dns to the ip addresses of the bridge instead of the gateway when gateway IP addresses are not on the host.
Fixes: #1177
I have written a unit test for this function verifying that the function filters the IP addresses of an interface correctly. This function works for all interfaces, not just bridges.
I guess we don't have to check if the bridge's link is valid with validate_bridge_link() because netavark does that when it creates a veth pair? Please correct me if I am wrong.
I think we should also change the field name from
network_gatewaystobind_ipsin theAardvarkEntry struct?Summary by Sourcery
Improve bridge DNS handling by binding to bridge IPs in unmanaged mode, extend netlink address dumping to support interface filters and strict checking, and add corresponding tests and example updates
Bug Fixes:
Enhancements:
Tests:
Chores: