Merge pull request #833 from Luap99/bridge-con

bridge: force static mac on bridge interface

src/network/bridge.rs

@@ -482,10 +482,15 @@ fn create_interfaces(
     hostns_fd: BorrowedFd<'_>,
     netns_fd: BorrowedFd<'_>,
 ) -> NetavarkResult<String> {
-    let bridge = match host.get_link(netlink::LinkID::Name(
+    let (bridge_index, mac) = match host.get_link(netlink::LinkID::Name(
         data.bridge_interface_name.to_string(),
     )) {
-        Ok(bridge) => check_link_is_bridge(bridge, &data.bridge_interface_name)?,
+        Ok(bridge) => (
+            check_link_is_bridge(bridge, &data.bridge_interface_name)?
+                .header
+                .index,
+            None,
+        ),
         Err(err) => match err.unwrap() {
             NetavarkError::Netlink(e) => {
                 if -e.raw_code() != libc::ENODEV {
@@ -528,14 +533,27 @@ fn create_interfaces(
                     ))
                     .wrap("get bridge interface")?;
 
+                let mut mac = None;
+                for nla in link.nlas.into_iter() {
+                    if let Nla::Address(addr) = nla {
+                        mac = Some(addr);
+                    }
+                }
+                if mac.is_none() {
+                    return Err(NetavarkError::msg(
+                        "failed to get the mac address from the bridge interface",
+                    ));
+                }
+
                 for addr in &data.ipam.gateway_addresses {
                     host.add_addr(link.header.index, addr)
                         .wrap("add ip addr to bridge")?;
                 }
 
                 host.set_up(netlink::LinkID::ID(link.header.index))
                     .wrap("set bridge up")?;
-                link
+
+                (link.header.index, mac)
             }
             _ => return Err(err),
         },
@@ -545,19 +563,22 @@ fn create_interfaces(
         host,
         netns,
         data,
-        bridge.header.index,
+        bridge_index,
+        mac,
         internal,
         hostns_fd,
         netns_fd,
     )
 }
 
 /// return the container veth mac address
+#[allow(clippy::too_many_arguments)]
 fn create_veth_pair<'fd>(
     host: &mut netlink::Socket,
     netns: &mut netlink::Socket,
     data: &InternalData,
     primary_index: u32,
+    bridge_mac: Option<Vec<u8>>,
     internal: bool,
     hostns_fd: BorrowedFd<'fd>,
     netns_fd: BorrowedFd<'fd>,
@@ -621,6 +642,11 @@ fn create_veth_pair<'fd>(
             );
             core_utils::CoreUtils::apply_sysctl_value(disable_dad_in_container, "0")?;
         }
+        let enable_arp_notify = format!(
+            "/proc/sys/net/ipv4/conf/{}/arp_notify",
+            &data.container_interface_name
+        );
+        core_utils::CoreUtils::apply_sysctl_value(enable_arp_notify, "1")?;
         Ok::<(), NetavarkError>(())
     });
     // check the result and return error
@@ -641,6 +667,20 @@ fn create_veth_pair<'fd>(
     host.set_up(netlink::LinkID::ID(host_link))
         .wrap("failed to set host veth up")?;
 
+    // Ok this is extremely strange, by default the kernel will always choose the mac address with the
+    // lowest value from all connected interfaces for the bridge. This means as our veth interfaces are
+    // added and removed the bridge mac can change randomly which causes problems with ARP. This causes
+    // package loss until the old incorrect ARP entry is updated with the new bridge mac which for some
+    // reason can take a very long time, we noticed delays up to 100s.
+    // This here forces a static mac because we explicitly requested one even though we still just only
+    // set the same autogenerated one. Not that this must happen after the first veth interface is
+    // connected otherwise no connectivity is possible at all and I have no idea why but CNI does it
+    // also in the same way.
+    if let Some(m) = bridge_mac {
+        host.set_mac_address(netlink::LinkID::ID(primary_index), m)
+            .wrap("set static mac on bridge")?;
+    }
+
     for addr in &data.ipam.container_addresses {
         netns
             .add_addr(veth.header.index, addr)

src/network/netlink.rs

@@ -394,6 +394,22 @@ impl Socket {
         Ok(())
     }
 
+    pub fn set_mac_address(&mut self, id: LinkID, mac: Vec<u8>) -> NetavarkResult<()> {
+        let mut msg = LinkMessage::default();
+
+        match id {
+            LinkID::ID(id) => msg.header.index = id,
+            LinkID::Name(name) => msg.nlas.push(Nla::IfName(name)),
+        }
+
+        msg.nlas.push(Nla::Address(mac));
+
+        let result = self.make_netlink_request(RtnlMessage::SetLink(msg), NLM_F_ACK)?;
+        expect_netlink_result!(result, 0);
+
+        Ok(())
+    }
+
     fn make_netlink_request(
         &mut self,
         msg: RtnlMessage,

test/100-bridge-iptables.bats

@@ -51,6 +51,11 @@ fw_driver=iptables
     link_info="$output"
     assert_json "$link_info" '.[].flags[] | select(.=="UP")' == "UP" "Host bridge interface is up"
     assert_json "$link_info" ".[].linkinfo.info_kind" == "bridge" "The bridge interface is actually a bridge"
+    bridge_mac=$(jq -r '.[].address' <<<"$link_info")
+
+    run_in_host_netns ip -j link show veth0
+    veth_info="$output"
+    assert_json "$veth_info" ".[].address" != "$bridge_mac" "Bridge and Veth must have different mac address"
 
     ipaddr="10.88.0.1"
     run_in_host_netns ip addr show podman0
@@ -797,6 +802,7 @@ EOF
 
     # when the sysctl value is already set correctly we should not error
     run_in_host_netns sh -c "echo 1 > /proc/sys/net/ipv4/ip_forward"
+    run_in_container_netns sh -c "echo 1 > /proc/sys/net/ipv4/conf/default/arp_notify"
     run_in_host_netns mount -t proc -o ro,nosuid,nodev,noexec proc /proc
 
     run_netavark --file ${TESTSDIR}/testfiles/simplebridge.json setup $(get_container_netns_path)