From 222058b490000246306210646c50a790f0be3a4c Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 17 Jun 2026 13:05:21 +0200 Subject: [PATCH 1/2] bridge: always pin the bridge MAC address The ensureAddr function pins the bridge's MAC address so it doesn't change as ports come and go. The pinning happens after the gateway address is set, so if a run set the address but then failed before pinning, subsequent runs never reattempted to pin the MAC, leaving behind a bridge with a floating MAC. Always pin the MAC, even if the gateway address is already present, regardless of the outcomes of previous attempts. Signed-off-by: Tom Wieczorek --- plugins/main/bridge/bridge.go | 12 ++++-- plugins/main/bridge/bridge_test.go | 67 ++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 476e6d5fd..860ba8d1a 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -271,11 +271,13 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool) } ipnStr := ipn.String() + addrFound := false for _, a := range addrs { // string comp is actually easiest for doing IPNet comps if a.IPNet.String() == ipnStr { - return nil + addrFound = true + break } // Multiple IPv6 addresses are allowed on the bridge if the @@ -293,9 +295,11 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool) } } - addr := &netlink.Addr{IPNet: ipn, Label: ""} - if err := netlink.AddrAdd(br, addr); err != nil && err != syscall.EEXIST { - return fmt.Errorf("could not add IP address %s to %q: %v", ipnStr, br.Attrs().Name, err) + if !addrFound { + addr := &netlink.Addr{IPNet: ipn, Label: ""} + if err := netlink.AddrAdd(br, addr); err != nil && err != syscall.EEXIST { + return fmt.Errorf("could not add IP address %s to %q: %v", ipnStr, br.Attrs().Name, err) + } } // Set the bridge's MAC to itself. Otherwise, the bridge will take the diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index 3ed19022f..cd3f562f3 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/plugins/main/bridge/bridge_test.go @@ -21,6 +21,7 @@ import ( "net" "os" "strings" + "sync" "github.com/coreos/go-iptables/iptables" "github.com/networkplumbing/go-nft/nft" @@ -407,6 +408,30 @@ func (tc testCase) expectedCIDRs() ([]*net.IPNet, []*net.IPNet) { return cidrsV4, cidrsV6 } +// Attaches a veth port with an optional fixed MAC address to a bridge in the +// current network namespace. To be cleaned up via the returned function. +func attachVethPort(br netlink.Link, mac string) (func() error, error) { + linkAttrs := netlink.NewLinkAttrs() + linkAttrs.Name = "testport0" + if mac != "" { + hwAddr, err := net.ParseMAC(mac) + if err != nil { + return nil, err + } + + linkAttrs.HardwareAddr = hwAddr + } + veth := &netlink.Veth{LinkAttrs: linkAttrs, PeerName: "testport1"} + if err := netlink.LinkAdd(veth); err != nil { + return nil, err + } + if err := netlink.LinkSetMaster(veth, br); err != nil { + return nil, err + } + + return sync.OnceValue(func() error { return netlink.LinkDel(veth) }), nil +} + // delBridgeAddrs() deletes addresses from the bridge func delBridgeAddrs(testNS ns.NetNS) { err := testNS.Do(func(ns.NetNS) error { @@ -2377,6 +2402,48 @@ var _ = Describe("bridge Operations", func() { }) Expect(err).NotTo(HaveOccurred()) }) + + It(fmt.Sprintf("[%s] (%d) keeps a stable MAC even if the gateway address already exists", ver, i), func() { + err := originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + tc.cniVersion = ver + br, _, err := setupBridge(tc.netConf()) + Expect(err).NotTo(HaveOccurred()) + link, err := netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + originalMAC := link.Attrs().HardwareAddr + + // Pre-add the gateway address the plugin is going to + // configure, as left behind by an ADD that failed midway. + _, subnet, err := net.ParseCIDR(tc.subnet) + Expect(err).NotTo(HaveOccurred()) + gwIP := calcGatewayIP(subnet) + err = netlink.AddrAdd(br, &netlink.Addr{ + IPNet: &net.IPNet{IP: gwIP, Mask: subnet.Mask}, + }) + Expect(err).NotTo(HaveOccurred()) + + cmdAddDelTest(originalNS, targetNS, tc, dataDir) + + // The MAC must have been pinned nevertheless: it neither + // got zeroed when the container veth was removed, nor + // does it change with port churn. + link, err = netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + Expect(link.Attrs().HardwareAddr).To(Equal(originalMAC)) + + cleanupVethPort, err := attachVethPort(br, "02:00:00:00:00:01") + Expect(err).NotTo(HaveOccurred()) + defer func() { Expect(cleanupVethPort()).To(Succeed()) }() + + link, err = netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + Expect(link.Attrs().HardwareAddr).To(Equal(originalMAC)) + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) } It(fmt.Sprintf("[%s] uses an explicit MAC addresses for the container iface (from CNI_ARGS)", ver), func() { From e63b1f706facdf7683f5ab600f9335d91c654d65 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 17 Jun 2026 13:24:48 +0200 Subject: [PATCH 2/2] bridge: re-generate a bridge MAC when the kernel has zeroed it out The kernel zeroes a bridge's MAC address when its last port is removed. The ensureAddr function pins the bridge's MAC address using the address read when the bridge was looked up, which is before the veth is attached. So for an existing bridge that had previously lost all its ports, the address it tried to pin is all zeros, and pinning an all-zero address fails with EINVAL. In fact, this can happen in practice: an ADD that fails has its veth cleaned up, which, if it was the first ADD, leaves the bridge portless and hence its MAC zeroed, so the next ADD fails to pin it. Detect that case and regenerate a random MAC address the same way the kernel does. This is fine as it only happens if the bridge has no ports, so no peer can hold stale state about the old address. Signed-off-by: Tom Wieczorek --- plugins/main/bridge/bridge.go | 41 +++++++++++++- plugins/main/bridge/bridge_test.go | 89 ++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 860ba8d1a..8d89c20dd 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -15,6 +15,7 @@ package main import ( + "crypto/rand" "encoding/json" "errors" "fmt" @@ -303,8 +304,27 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool) } // Set the bridge's MAC to itself. Otherwise, the bridge will take the - // lowest-numbered mac on the bridge, and will change as ifs churn - if err := netlink.LinkSetHardwareAddr(br, br.Attrs().HardwareAddr); err != nil { + // lowest-numbered mac on the bridge, and will change as ifs churn. The + // cached attrs hold the MAC the bridge had before any veths were attached + // during this invocation. + hwAddr := br.Attrs().HardwareAddr + + // Check whether the kernel has zeroed the bridge's MAC. + // This only happens when the bridge's last port was detached. + if isZeroMAC(hwAddr) { + // There are no neighbors that could hold stale state about the previous + // MAC, so it's fine to generate a suitable stand-in the same way the + // kernel does. + // https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/etherdevice.h?h=v7.1#n237 + hwAddr = make(net.HardwareAddr, 6) + if _, err := rand.Read(hwAddr); err != nil { + return fmt.Errorf("failed to generate random MAC address: %w", err) + } + hwAddr[0] &= 0xfe // clear multicast bit + hwAddr[0] |= 0x02 // set local assignment bit (IEEE802) + } + + if err := netlink.LinkSetHardwareAddr(br, hwAddr); err != nil { return fmt.Errorf("could not set bridge's mac: %v", err) } @@ -676,6 +696,10 @@ func cmdAdd(args *skel.CmdArgs) error { if err != nil { return fmt.Errorf("failed to set bridge addr: %v", err) } + // Reload the bridge so it reflects the MAC that ensureAddr just pinned. + if br, err = bridgeByName(n.BrName); err != nil { + return err + } } } @@ -1120,3 +1144,16 @@ func cmdStatus(args *skel.CmdArgs) error { return nil } + +// Indicates whether a hardware address is nil, empty, or consists solely of +// zeros. Technically, the kernel will always report such addresses as an array +// of zeros, but vishvananda/netlink will "normalize" them to nil. +func isZeroMAC(mac net.HardwareAddr) bool { + for _, b := range mac { + if b != 0 { + return false + } + } + + return true +} diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index cd3f562f3..5cd1b9801 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/plugins/main/bridge/bridge_test.go @@ -17,6 +17,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "net" "os" @@ -27,6 +28,8 @@ import ( "github.com/networkplumbing/go-nft/nft" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + gomegaformat "github.com/onsi/gomega/format" + gomegatypes "github.com/onsi/gomega/types" "github.com/vishvananda/netlink" "github.com/vishvananda/netlink/nl" "sigs.k8s.io/knftables" @@ -2446,6 +2449,70 @@ var _ = Describe("bridge Operations", func() { }) } + It(fmt.Sprintf("[%s] keeps a stable MAC even if the bridge previously lost all its ports", ver), func() { + err := originalNS.Do(func(ns.NetNS) error { + defer GinkgoRecover() + + // Test dual-stack here: A dual-stack ADD configures a + // gateway per family, so it sets the bridge MAC more than + // once in a single invocation. + tc := testCase{ + cniVersion: ver, + ranges: []rangeInfo{ + {subnet: "10.1.2.0/24"}, + {subnet: "2001:db8:42::/64"}, + }, + expGWCIDRs: []string{"10.1.2.1/24", "2001:db8:42::1/64"}, + } + + br, _, err := setupBridge(tc.netConf()) + Expect(err).NotTo(HaveOccurred()) + + // Attach and remove a port so the kernel first generates, + // and then zeroes the bridge's MAC. + cleanupVethPort, err := attachVethPort(br, "") + Expect(err).NotTo(HaveOccurred()) + defer func() { Expect(cleanupVethPort()).To(Succeed()) }() + link, err := netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + kernelMAC := link.Attrs().HardwareAddr + Expect(kernelMAC).NotTo(beAZeroMAC(), "kernel didn't generate a MAC") + Expect(kernelMAC[0]&0x01).To(BeZero(), "kernel-generated MAC is not unicast") + Expect(kernelMAC[0]&0x02).NotTo(BeZero(), "kernel-generated MAC is not locally administered") + Expect(cleanupVethPort()).To(Succeed()) + link, err = netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + // If the expectation below fails, it may be because the + // kernel behavior has changed and it keeps the original + // address. + Expect(link.Attrs().HardwareAddr).To(beAZeroMAC(), "kernel should have zeroed the MAC after the bridge lost its last port") + + cmdAddDelTest(originalNS, targetNS, tc, dataDir) + + // The plugin must have generated a valid, locally administered + // unicast MAC address. This MAC address must have survived the + // removal of the container veth during DEL. + link, err = netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + bridgeMAC := link.Attrs().HardwareAddr + Expect(bridgeMAC).NotTo(beAZeroMAC(), "bridge plugin should have generated a MAC") + Expect(bridgeMAC).NotTo(Equal(kernelMAC), "generated MAC is the same as the kernel-generated one") + Expect(bridgeMAC[0]&0x01).To(BeZero(), "generated MAC should be unicast") + Expect(bridgeMAC[0]&0x02).NotTo(BeZero(), "generated MAC should be locally administered") + + // A second pod's ADD on the same bridge must keep that MAC. + // Every pod on the node shares the bridge. Between the two + // ADDs, it briefly had no ports. Because the generated MAC + // address was pinned, the kernel does not zero it. + cmdAddDelTest(originalNS, targetNS, tc, dataDir) + link, err = netlinksafe.LinkByName(BRNAME) + Expect(err).NotTo(HaveOccurred()) + Expect(link.Attrs().HardwareAddr).To(Equal(bridgeMAC), "generated MAC changed, but it should have been pinned") + return nil + }) + Expect(err).NotTo(HaveOccurred()) + }) + It(fmt.Sprintf("[%s] uses an explicit MAC addresses for the container iface (from CNI_ARGS)", ver), func() { err := originalNS.Do(func(ns.NetNS) error { defer GinkgoRecover() @@ -2824,3 +2891,25 @@ func assertMacSpoofCheckRules(assert func(actual interface{}, expectedLen int)) "macspoofchk-dummy-0-eth0", )), 2) } + +func beAZeroMAC() gomegatypes.GomegaMatcher { + return zeroMACMatcher{} +} + +type zeroMACMatcher struct{} + +func (zeroMACMatcher) Match(actual any) (bool, error) { + if addr, ok := actual.(net.HardwareAddr); ok { + return isZeroMAC(addr), nil + } + + return false, errors.New("expected a net.HardwareAddr") +} + +func (zeroMACMatcher) FailureMessage(actual any) string { + return gomegaformat.Message(fmt.Sprint(actual), "to be a zero MAC") +} + +func (zeroMACMatcher) NegatedFailureMessage(actual any) string { + return gomegaformat.Message(fmt.Sprint(actual), "not to be a zero MAC") +}