bridge: various fixes

* Don't set the MAC, send gratuitous arp instead
* Set the bridge's MAC to itself
* Only disable DAD when necessary

diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go
index ef969be..b8a573e 100644 (file)
--- a/plugins/main/bridge/bridge.go
+++ b/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 {
+               contVeth, err := net.InterfaceByName(args.IfName)
+               if 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.
-               // 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
+               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 {
-                       if err := ip.SetHWAddrByIP(args.IfName, result.IPs[0].Address.IP, nil /* TODO IPv6 */); err != nil {
-                               return err
+               // Send a gratuitous arp
+               for _, ipc := range result.IPs {
+                       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 {

diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go
index 13e7257..ff738ca 100644 (file)
--- a/plugins/main/bridge/bridge_test.go
+++ b/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))
+               }
+       })
 })