bridge: reliably pin the MAC on a bridge that lost its ports

plugins/main/bridge/bridge.go

@@ -15,6 +15,7 @@
 package main
 
 import (
+	"crypto/rand"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -271,11 +272,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,14 +296,35 @@ 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
-	// 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)
 	}
 
@@ -672,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
+						}
 					}
 				}
 
@@ -1116,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
+}

plugins/main/bridge/bridge_test.go

@@ -17,15 +17,19 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net"
 	"os"
 	"strings"
+	"sync"
 
 	"github.com/coreos/go-iptables/iptables"
 	"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"
@@ -407,6 +411,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,8 +2405,114 @@ 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] 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()
@@ -2757,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")
+}