Merge pull request #62 from squeed/bridge-fixes

bridge: various fixes

plugins/main/bridge/bridge.go

@@ -32,6 +32,7 @@ import (
 	"github.com/containernetworking/plugins/pkg/ipam"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/utils"
+	"github.com/j-keck/arping"
 	"github.com/vishvananda/netlink"
 )
 
@@ -172,6 +173,13 @@ func ensureBridgeAddr(br *netlink.Bridge, family int, ipn *net.IPNet, forceAddre
 	if err := netlink.AddrAdd(br, addr); err != nil {
 		return fmt.Errorf("could not add IP address to %q: %v", br.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.HardwareAddr); err != nil {
+		return fmt.Errorf("could not set bridge's mac: %v", err)
+	}
+
 	return nil
 }
 
@@ -294,8 +302,15 @@ func setupBridge(n *NetConf) (*netlink.Bridge, *current.Interface, error) {
 }
 
 // disableIPV6DAD disables IPv6 Duplicate Address Detection (DAD)
-// for an interface.
+// for an interface, if the interface does not support enhanced_dad.
+// We do this because interfaces with hairpin mode will see their own DAD packets
 func disableIPV6DAD(ifName string) error {
+	// ehanced_dad sends a nonce with the DAD packets, so that we can safely
+	// ignore ourselves
+	enh, err := ioutil.ReadFile(fmt.Sprintf("/proc/sys/net/ipv6/conf/%s/enhanced_dad", ifName))
+	if err == nil && string(enh) == "1\n" {
+		return nil
+	}
 	f := fmt.Sprintf("/proc/sys/net/ipv6/conf/%s/accept_dad", ifName)
 	return ioutil.WriteFile(f, []byte("0"), 0644)
 }
@@ -363,32 +378,34 @@ func cmdAdd(args *skel.CmdArgs) error {
 
 	// Configure the container hardware address and IP address(es)
 	if err := netns.Do(func(_ ns.NetNS) error {
-		// Disable IPv6 DAD just in case hairpin mode is enabled on the
+		contVeth, err := net.InterfaceByName(args.IfName)
-		// bridge. Hairpin mode causes echos of neighbor solicitation
+		if err != nil {
-		// packets, which causes DAD failures.
-		// TODO: (short term) Disable DAD conditional on actual hairpin mode
-		// TODO: (long term) Use enhanced DAD when that becomes available in kernels.
-		if err := disableIPV6DAD(args.IfName); err != nil {
 			return err
 		}
 
+		// Disable IPv6 DAD just in case hairpin mode is enabled on the
+		// bridge. Hairpin mode causes echos of neighbor solicitation
+		// packets, which causes DAD failures.
+		for _, ipc := range result.IPs {
+			if ipc.Version == "6" && (n.HairpinMode || n.PromiscMode) {
+				if err := disableIPV6DAD(args.IfName); err != nil {
+					return err
+				}
+				break
+			}
+		}
+
+		// Add the IP to the interface
 		if err := ipam.ConfigureIface(args.IfName, result); err != nil {
 			return err
 		}
 
-		if result.IPs[0].Address.IP.To4() != nil {
+		// Send a gratuitous arp
-			if err := ip.SetHWAddrByIP(args.IfName, result.IPs[0].Address.IP, nil /* TODO IPv6 */); err != nil {
+		for _, ipc := range result.IPs {
-				return err
+			if ipc.Version == "4" {
+				_ = arping.GratuitousArpOverIface(ipc.Address.IP, *contVeth)
 			}
 		}
-
-		// Refetch the veth since its MAC address may changed
-		link, err := netlink.LinkByName(args.IfName)
-		if err != nil {
-			return fmt.Errorf("could not lookup %q: %v", args.IfName, err)
-		}
-		containerInterface.Mac = link.Attrs().HardwareAddr.String()
-
 		return nil
 	}); err != nil {
 		return err
@@ -415,12 +432,6 @@ func cmdAdd(args *skel.CmdArgs) error {
 				}
 			}
 		}
-
-		if firstV4Addr != nil {
-			if err := ip.SetHWAddrByIP(n.BrName, firstV4Addr, nil /* TODO IPv6 */); err != nil {
-				return err
-			}
-		}
 	}
 
 	if n.IPMasq {

plugins/main/bridge/bridge_test.go

@@ -25,7 +25,6 @@ import (
 	"github.com/containernetworking/cni/pkg/types/current"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/containernetworking/plugins/pkg/testutils"
-	"github.com/containernetworking/plugins/pkg/utils/hwaddr"
 
 	"github.com/vishvananda/netlink"
 
@@ -266,6 +265,7 @@ func (tester *testerV03x) cmdAddTest(tc testCase) {
 		Expect(link.Attrs().Name).To(Equal(BRNAME))
 		Expect(link).To(BeAssignableToTypeOf(&netlink.Bridge{}))
 		Expect(link.Attrs().HardwareAddr.String()).To(Equal(result.Interfaces[0].Mac))
+		bridgeMAC := link.Attrs().HardwareAddr.String()
 
 		// Ensure bridge has expected gateway address(es)
 		addrs, err := netlink.AddrList(link, netlink.FAMILY_ALL)
@@ -274,10 +274,6 @@ func (tester *testerV03x) cmdAddTest(tc testCase) {
 		for _, cidr := range tc.expGWCIDRs {
 			ip, subnet, err := net.ParseCIDR(cidr)
 			Expect(err).NotTo(HaveOccurred())
-			if ip.To4() != nil {
-				hwAddr := fmt.Sprintf("%s", link.Attrs().HardwareAddr)
-				Expect(hwAddr).To(HavePrefix(hwaddr.PrivateMACPrefixString))
-			}
 
 			found := false
 			subnetPrefix, subnetBits := subnet.Mask.Size()
@@ -300,6 +296,12 @@ func (tester *testerV03x) cmdAddTest(tc testCase) {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(link).To(BeAssignableToTypeOf(&netlink.Veth{}))
 		tester.vethName = result.Interfaces[1].Name
+
+		// Check that the bridge has a different mac from the veth
+		// If not, it means the bridge has an unstable mac and will change
+		// as ifs are added and removed
+		Expect(link.Attrs().HardwareAddr.String()).NotTo(Equal(bridgeMAC))
+
 		return nil
 	})
 	Expect(err).NotTo(HaveOccurred())
@@ -314,14 +316,11 @@ func (tester *testerV03x) cmdAddTest(tc testCase) {
 		Expect(link).To(BeAssignableToTypeOf(&netlink.Veth{}))
 
 		expCIDRsV4, expCIDRsV6 := tc.expectedCIDRs()
-		if expCIDRsV4 != nil {
-			hwAddr := fmt.Sprintf("%s", link.Attrs().HardwareAddr)
-			Expect(hwAddr).To(HavePrefix(hwaddr.PrivateMACPrefixString))
-		}
 		addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(len(addrs)).To(Equal(len(expCIDRsV4)))
 		addrs, err = netlink.AddrList(link, netlink.FAMILY_V6)
+		Expect(len(addrs)).To(Equal(len(expCIDRsV6) + 1)) //add one for the link-local
 		Expect(err).NotTo(HaveOccurred())
 		// Ignore link local address which may or may not be
 		// ready when we read addresses.
@@ -442,10 +441,6 @@ func (tester *testerV01xOr02x) cmdAddTest(tc testCase) {
 		for _, cidr := range tc.expGWCIDRs {
 			ip, subnet, err := net.ParseCIDR(cidr)
 			Expect(err).NotTo(HaveOccurred())
-			if ip.To4() != nil {
-				hwAddr := fmt.Sprintf("%s", link.Attrs().HardwareAddr)
-				Expect(hwAddr).To(HavePrefix(hwaddr.PrivateMACPrefixString))
-			}
 
 			found := false
 			subnetPrefix, subnetBits := subnet.Mask.Size()
@@ -479,10 +474,6 @@ func (tester *testerV01xOr02x) cmdAddTest(tc testCase) {
 		Expect(link).To(BeAssignableToTypeOf(&netlink.Veth{}))
 
 		expCIDRsV4, expCIDRsV6 := tc.expectedCIDRs()
-		if expCIDRsV4 != nil {
-			hwAddr := fmt.Sprintf("%s", link.Attrs().HardwareAddr)
-			Expect(hwAddr).To(HavePrefix(hwaddr.PrivateMACPrefixString))
-		}
 		addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(len(addrs)).To(Equal(len(expCIDRsV4)))
@@ -892,4 +883,28 @@ var _ = Describe("bridge Operations", func() {
 		})
 		Expect(err).NotTo(HaveOccurred())
 	})
+	It("creates a bridge with a stable MAC addresses", func() {
+		testCases := []testCase{
+			{
+				subnet: "10.1.2.0/24",
+			},
+			{
+				subnet: "2001:db8:42::/64",
+			},
+		}
+
+		for _, tc := range testCases {
+			tc.cniVersion = "0.3.1"
+			_, _, err := setupBridge(tc.netConf())
+			link, err := netlink.LinkByName(BRNAME)
+			Expect(err).NotTo(HaveOccurred())
+			origMac := link.Attrs().HardwareAddr
+
+			cmdAddDelTest(originalNS, tc)
+
+			link, err = netlink.LinkByName(BRNAME)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(origMac))
+		}
+	})
 })