diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index ef969bea..b8a573e8 100644 --- 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 { - // 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 { + 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. + 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 13e7257a..ff738ca2 100644 --- 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)) + } + }) })