From 97e0fcb765525ba4b7963ffab3c745433be68427 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 27 May 2017 07:21:28 +0000 Subject: [PATCH 1/3] Refactor: move SetupIface code from weaveutil to net package --- net/veth.go | 26 ++++++++++++++++++++++++++ prog/weaveutil/netdev.go | 23 +---------------------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/net/veth.go b/net/veth.go index 76c0932e..0b78f07f 100644 --- a/net/veth.go +++ b/net/veth.go @@ -193,6 +193,32 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult return nil } +// SetupIface is the implementation of the 'setup-iface' call above, +// running in another process in the container's netns +func SetupIface(ifaceName, newIfName string) error { + ipt, err := iptables.New() + if err != nil { + return err + } + + link, err := netlink.LinkByName(ifaceName) + if err != nil { + return err + } + if err := netlink.LinkSetName(link, newIfName); err != nil { + return err + } + // This is only called by AttachContainer which is only called in host pid namespace + if err := ConfigureARPCache("/proc", newIfName); err != nil { + return err + } + if err := ipt.Append("filter", "INPUT", "-i", newIfName, "-d", "224.0.0.0/4", "-j", "DROP"); err != nil { + return err + } + + return nil +} + // NB: This function can be used only by a process that terminates immediately // after calling the function as it changes netns via WithNetNSLinkUnsafe. func DetachContainer(netNSPath, id, ifName string, cidrs []*net.IPNet) error { diff --git a/prog/weaveutil/netdev.go b/prog/weaveutil/netdev.go index 457e30e3..c6caa38d 100644 --- a/prog/weaveutil/netdev.go +++ b/prog/weaveutil/netdev.go @@ -6,7 +6,6 @@ import ( "strconv" "strings" - "github.com/coreos/go-iptables/iptables" "github.com/j-keck/arping" "github.com/vishvananda/netlink" @@ -48,27 +47,7 @@ func setupIface(args []string) error { ifaceName := args[0] newIfName := args[1] - ipt, err := iptables.New() - if err != nil { - return err - } - - link, err := netlink.LinkByName(ifaceName) - if err != nil { - return err - } - if err := netlink.LinkSetName(link, newIfName); err != nil { - return err - } - // This is only called by AttachContainer which is only called in host pid namespace - if err := weavenet.ConfigureARPCache("/proc", newIfName); err != nil { - return err - } - if err := ipt.Append("filter", "INPUT", "-i", newIfName, "-d", "224.0.0.0/4", "-j", "DROP"); err != nil { - return err - } - - return nil + return weavenet.SetupIface(ifaceName, newIfName) } func configureARP(args []string) error { From 28aa181480e38f20aa4acf4d2cf64cba42eaa3a3 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 26 May 2017 17:34:01 +0000 Subject: [PATCH 2/3] Move setting up addresses for a container, into weaveutil-invoked function This means we only change netns by execing nsenter: changing netns in Go is unsafe for long-running processes like the proxy. --- net/veth.go | 108 +++++++++++++++++++++------------------ prog/weaveutil/main.go | 1 + prog/weaveutil/netdev.go | 20 ++++++++ 3 files changed, 78 insertions(+), 51 deletions(-) diff --git a/net/veth.go b/net/veth.go index 0b78f07f..c42c2713 100644 --- a/net/veth.go +++ b/net/veth.go @@ -104,8 +104,6 @@ func interfaceExistsInNamespace(netNSPath string, ifName string) bool { return err == nil } -// NB: This function can be used only by a process that terminates immediately -// after calling the function as it changes netns via WithNetNSLinkUnsafe. func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMulticastRoute bool, cidrs []*net.IPNet, keepTXOn bool, hairpinMode bool) error { ns, err := netns.GetFromPath(netNSPath) if err != nil { @@ -113,11 +111,6 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult } defer ns.Close() - ipt, err := iptables.New() - if err != nil { - return err - } - if !interfaceExistsInNamespace(netNSPath, ifName) { maxIDLen := IFNAMSIZ - 1 - len(vethPrefix+"pl") if len(id) > maxIDLen { @@ -142,54 +135,67 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult } - if err := WithNetNSLinkUnsafe(ns, ifName, func(veth netlink.Link) error { - newAddresses, err := AddAddresses(veth, cidrs) - if err != nil { - return err - } + args := []string{ifName, fmt.Sprintf("%t", withMulticastRoute)} + for _, cidr := range cidrs { + args = append(args, cidr.String()) + } + if _, err := WithNetNS(netNSPath, "setup-iface-addrs", args...); err != nil { + return fmt.Errorf("error setting up interface addresses: %s", err) + } + return nil +} - // Add multicast ACCEPT rules for new subnets - for _, ipnet := range newAddresses { - acceptRule := []string{"-i", ifName, "-s", subnet(ipnet), "-d", "224.0.0.0/4", "-j", "ACCEPT"} - exists, err := ipt.Exists("filter", "INPUT", acceptRule...) - if err != nil { - return err - } - if !exists { - if err := ipt.Insert("filter", "INPUT", 1, acceptRule...); err != nil { - return err - } - } - } - - if err := netlink.LinkSetUp(veth); err != nil { - return err - } - for _, ipnet := range newAddresses { - // If we don't wait for a bit here, we see the arp fail to reach the bridge. - time.Sleep(1 * time.Millisecond) - arping.GratuitousArpOverIfaceByName(ipnet.IP, ifName) - } - if withMulticastRoute { - /* Route multicast packets across the weave network. - This must come last in 'attach'. If you change this, change weavewait to match. - - TODO: Add the MTU lock to prevent PMTU discovery for multicast - destinations. Without that, the kernel sets the DF flag on - multicast packets. Since RFC1122 prohibits sending of ICMP - errors for packets with multicast destinations, that causes - packets larger than the PMTU to be dropped silently. */ - - _, multicast, _ := net.ParseCIDR("224.0.0.0/4") - if err := AddRoute(veth, netlink.SCOPE_LINK, multicast, nil); err != nil { - return err - } - } - return nil - }); err != nil { +// SetupIfaceAddrs is the implementation of the 'setup-iface-addrs' call above, +// running in another process in the container's netns +func SetupIfaceAddrs(veth netlink.Link, withMulticastRoute bool, cidrs []*net.IPNet) error { + newAddresses, err := AddAddresses(veth, cidrs) + if err != nil { return err } + ifName := veth.Attrs().Name + ipt, err := iptables.New() + if err != nil { + return err + } + + // Add multicast ACCEPT rules for new subnets + for _, ipnet := range newAddresses { + acceptRule := []string{"-i", ifName, "-s", subnet(ipnet), "-d", "224.0.0.0/4", "-j", "ACCEPT"} + exists, err := ipt.Exists("filter", "INPUT", acceptRule...) + if err != nil { + return err + } + if !exists { + if err := ipt.Insert("filter", "INPUT", 1, acceptRule...); err != nil { + return err + } + } + } + + if err := netlink.LinkSetUp(veth); err != nil { + return err + } + for _, ipnet := range newAddresses { + // If we don't wait for a bit here, we see the arp fail to reach the bridge. + time.Sleep(1 * time.Millisecond) + arping.GratuitousArpOverIfaceByName(ipnet.IP, ifName) + } + if withMulticastRoute { + /* Route multicast packets across the weave network. + This must come last in 'attach'. If you change this, change weavewait to match. + + TODO: Add the MTU lock to prevent PMTU discovery for multicast + destinations. Without that, the kernel sets the DF flag on + multicast packets. Since RFC1122 prohibits sending of ICMP + errors for packets with multicast destinations, that causes + packets larger than the PMTU to be dropped silently. */ + + _, multicast, _ := net.ParseCIDR("224.0.0.0/4") + if err := AddRoute(veth, netlink.SCOPE_LINK, multicast, nil); err != nil { + return err + } + } return nil } diff --git a/prog/weaveutil/main.go b/prog/weaveutil/main.go index 57751428..525516ea 100644 --- a/prog/weaveutil/main.go +++ b/prog/weaveutil/main.go @@ -32,6 +32,7 @@ func init() { "check-iface": checkIface, "del-iface": delIface, "setup-iface": setupIface, + "setup-iface-addrs": setupIfaceAddrs, "list-netdevs": listNetDevs, "cni-net": cniNet, "cni-ipam": cniIPAM, diff --git a/prog/weaveutil/netdev.go b/prog/weaveutil/netdev.go index c6caa38d..af4e63a5 100644 --- a/prog/weaveutil/netdev.go +++ b/prog/weaveutil/netdev.go @@ -50,6 +50,26 @@ func setupIface(args []string) error { return weavenet.SetupIface(ifaceName, newIfName) } +// setupIfaceAddrs sets up addresses on an interface. It expects to be called inside the container's netns. +func setupIfaceAddrs(args []string) error { + if len(args) < 1 { + cmdUsage("setup-iface-addrs", " ...") + } + link, err := netlink.LinkByName(args[0]) + if err != nil { + return err + } + withMulticastRoute, err := strconv.ParseBool(args[1]) + if err != nil { + return err + } + cidrs, err := parseCIDRs(args[2:]) + if err != nil { + return err + } + return weavenet.SetupIfaceAddrs(link, withMulticastRoute, cidrs) +} + func configureARP(args []string) error { if len(args) != 2 { cmdUsage("configure-arp", " ") From 444d6be6c98d52903a9fd10a1011ff0cf00015e1 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 16 Jun 2017 13:21:07 +0000 Subject: [PATCH 3/3] Document the requirement for 'nsenter' --- site/cni-plugin.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/site/cni-plugin.md b/site/cni-plugin.md index a3d114a3..2e5b3efd 100644 --- a/site/cni-plugin.md +++ b/site/cni-plugin.md @@ -108,5 +108,8 @@ For more information, see the ### Caveats +- The program `nsenter` must be present on the host; it is required by + the CNI plugin. In some distros (e.g. Alpine, Ubuntu) it is in the + util-linux package - The Weave Net router container must be running for CNI to allocate addresses - The CNI plugin does not add entries to weaveDNS.