Merge pull request #3291 from weaveworks/withnetns-inproc

Remove subprocess for changing netns

net/arp.go

@@ -10,7 +10,7 @@ import (
 // Configure the ARP cache parameters for the given interface.  This
 // makes containers react more quickly to a change in the MAC address
 // associated with an IP address.
-func ConfigureARPCache(procPath, name string) error {
+func configureARPCache(procPath, name string) error {
 	if err := sysctl(procPath, fmt.Sprintf("net/ipv4/neigh/%s/base_reachable_time", name), "5"); err != nil {
 		return err
 	}

net/bridge.go

@@ -275,7 +275,7 @@ func EnsureBridge(procPath string, config *BridgeConfig, log *logrus.Logger) (Br
 		return bridgeType, err
 	}
 
-	if err := ConfigureARPCache(procPath, config.WeaveBridgeName); err != nil {
+	if err := configureARPCache(procPath, config.WeaveBridgeName); err != nil {
 		return bridgeType, errors.Wrapf(err, "configuring ARP cache on bridge %q", config.WeaveBridgeName)
 	}
 

net/netdev.go

@@ -1,11 +1,9 @@
 package net
 
 import (
-	"encoding/json"
 	"fmt"
 	"net"
 	"os"
-	"strconv"
 
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
@@ -17,7 +15,7 @@ type Dev struct {
 	CIDRs []*net.IPNet     `json:"CIDRs,omitempty"`
 }
 
-func LinkToNetDev(link netlink.Link) (Dev, error) {
+func linkToNetDev(link netlink.Link) (Dev, error) {
 	addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
 	if err != nil {
 		return Dev{}, err
@@ -90,16 +88,30 @@ func GetNetDevsByVethPeerIds(processID int, peerIDs []int) ([]Dev, error) {
 		return nil, nil
 	}
 
+	// convert list of peerIDs into a map for faster lookup
+	indexes := make(map[int]struct{})
+	for _, id := range peerIDs {
+		indexes[id] = struct{}{}
+	}
+
 	var netdevs []Dev
-	peersStr := make([]string, len(peerIDs))
-	for i, id := range peerIDs {
-		peersStr[i] = strconv.Itoa(id)
-	}
-	nds, err := WithNetNSByPid(processID, "list-netdevs", peersStr...)
-	if err != nil {
-		return nil, fmt.Errorf("list-netdevs failed: %s", err)
-	}
-	err = json.Unmarshal(nds, &netdevs)
+	err = WithNetNS(netnsContainer, func() error {
+		links, err := netlink.LinkList()
+		if err != nil {
+			return err
+		}
+
+		for _, link := range links {
+			if _, found := indexes[link.Attrs().Index]; found {
+				netdev, err := linkToNetDev(link)
+				if err != nil {
+					return err
+				}
+				netdevs = append(netdevs, netdev)
+			}
+		}
+		return nil
+	})
 
 	return netdevs, err
 }
@@ -112,5 +124,5 @@ func GetBridgeNetDev(bridgeName string) (Dev, error) {
 		return Dev{}, err
 	}
 
-	return LinkToNetDev(link)
+	return linkToNetDev(link)
 }

net/netns.go

@@ -1,10 +1,10 @@
+// +build go1.10
+
 package net
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
-	"os/exec"
 	"path/filepath"
 	"runtime"
 
@@ -14,19 +14,7 @@ import (
 
 var ErrLinkNotFound = errors.New("Link not found")
 
-// NB: The following function is unsafe, because:
-//     - It changes a network namespace (netns) of an OS thread which runs
-//       the function. During execution, the Go runtime might clone a new OS thread
-//       for scheduling other go-routines, thus they might end up running in
-//       a "wrong" netns.
-//     - runtime.LockOSThread does not guarantee that a spawned go-routine on
-//       the locked thread will be run by it. Thus, the work function is
-//       not allowed to spawn any go-routine which is dependent on the given netns.
-
-//     Please see https://github.com/weaveworks/weave/issues/2388#issuecomment-228365069
-//     for more details and make sure that you understand the implications before
-//     using the function!
-func WithNetNSUnsafe(ns netns.NsHandle, work func() error) error {
+func WithNetNS(ns netns.NsHandle, work func() error) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
@@ -45,8 +33,8 @@ func WithNetNSUnsafe(ns netns.NsHandle, work func() error) error {
 	return err
 }
 
-func WithNetNSLinkUnsafe(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error {
-	return WithNetNSUnsafe(ns, func() error {
+func WithNetNSLink(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error {
+	return WithNetNS(ns, func() error {
 		link, err := netlink.LinkByName(ifName)
 		if err != nil {
 			if err.Error() == errors.New("Link not found").Error() {
@@ -58,26 +46,12 @@ func WithNetNSLinkUnsafe(ns netns.NsHandle, ifName string, work func(link netlin
 	})
 }
 
-var WeaveUtilCmd = "weaveutil"
-
-// A safe version of WithNetNS* which creates a process executing
-// "nsenter --net=<ns-path> weaveutil <cmd> [args]".
-func WithNetNS(nsPath string, cmd string, args ...string) ([]byte, error) {
-	var stdout, stderr bytes.Buffer
-
-	args = append([]string{"--net=" + nsPath, WeaveUtilCmd, cmd}, args...)
-	c := exec.Command("nsenter", args...)
-	c.Stdout = &stdout
-	c.Stderr = &stderr
-	if err := c.Run(); err != nil {
-		return nil, fmt.Errorf("%s: %s", string(stderr.Bytes()), err)
+func WithNetNSByPath(path string, work func() error) error {
+	ns, err := netns.GetFromPath(path)
+	if err != nil {
+		return err
 	}
-
-	return stdout.Bytes(), nil
-}
-
-func WithNetNSByPid(pid int, cmd string, args ...string) ([]byte, error) {
-	return WithNetNS(NSPathByPid(pid), cmd, args...)
+	return WithNetNS(ns, work)
 }
 
 func NSPathByPid(pid int) string {

net/veth.go

@@ -104,7 +104,10 @@ const (
 )
 
 func interfaceExistsInNamespace(netNSPath string, ifName string) bool {
-	_, err := WithNetNS(netNSPath, "check-iface", ifName)
+	err := WithNetNSByPath(netNSPath, func() error {
+		_, err := netlink.LinkByName(ifName)
+		return err
+	})
 	return err == nil
 }
 
@@ -125,7 +128,9 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult
 			if err := netlink.LinkSetNsFd(veth, int(ns)); err != nil {
 				return fmt.Errorf("failed to move veth to container netns: %s", err)
 			}
-			if _, err := WithNetNS(netNSPath, "setup-iface", peerName, ifName); err != nil {
+			if err := WithNetNS(ns, func() error {
+				return setupIface(peerName, ifName)
+			}); err != nil {
 				return fmt.Errorf("error setting up interface: %s", err)
 			}
 			return nil
@@ -139,19 +144,16 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult
 
 	}
 
-	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 {
+	if err := WithNetNSLink(ns, ifName, func(veth netlink.Link) error {
+		return setupIfaceAddrs(veth, withMulticastRoute, cidrs)
+	}); err != nil {
 		return fmt.Errorf("error setting up interface addresses: %s", err)
 	}
 	return 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 {
+// setupIfaceAddrs expects to be called 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
@@ -203,9 +205,8 @@ func SetupIfaceAddrs(veth netlink.Link, withMulticastRoute bool, cidrs []*net.IP
 	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 {
+// setupIface expects to be called in the container's netns
+func setupIface(ifaceName, newIfName string) error {
 	ipt, err := iptables.New()
 	if err != nil {
 		return err
@@ -219,14 +220,32 @@ func SetupIface(ifaceName, newIfName string) error {
 		return err
 	}
 	// This is only called by AttachContainer which is only called in host pid namespace
-	if err := ConfigureARPCache("/proc", newIfName); err != nil {
+	if err := configureARPCache("/proc", newIfName); err != nil {
 		return err
 	}
 	return ipt.Append("filter", "INPUT", "-i", newIfName, "-d", "224.0.0.0/4", "-j", "DROP")
 }
 
-// NB: This function can be used only by a process that terminates immediately
-//     after calling the function as it changes netns via WithNetNSLinkUnsafe.
+// configureARP is a helper for the Docker plugin which doesn't set the addresses itself
+func ConfigureARP(prefix, rootPath string) error {
+	links, err := netlink.LinkList()
+	if err != nil {
+		return err
+	}
+	for _, link := range links {
+		ifName := link.Attrs().Name
+		if strings.HasPrefix(ifName, prefix) {
+			configureARPCache(rootPath+"/proc", ifName)
+			if addrs, err := netlink.AddrList(link, netlink.FAMILY_V4); err == nil {
+				for _, addr := range addrs {
+					arping.GratuitousArpOverIfaceByName(addr.IPNet.IP, ifName)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 func DetachContainer(netNSPath, id, ifName string, cidrs []*net.IPNet) error {
 	ns, err := netns.GetFromPath(netNSPath)
 	if err != nil {
@@ -239,7 +258,7 @@ func DetachContainer(netNSPath, id, ifName string, cidrs []*net.IPNet) error {
 		return err
 	}
 
-	return WithNetNSLinkUnsafe(ns, ifName, func(veth netlink.Link) error {
+	return WithNetNSLink(ns, ifName, func(veth netlink.Link) error {
 		existingAddrs, err := netlink.AddrList(veth, netlink.FAMILY_V4)
 		if err != nil {
 			return fmt.Errorf("failed to get IP address for %q: %v", veth.Attrs().Name, err)

plugin/net/cni.go

@@ -130,7 +130,7 @@ func (c *CNIPlugin) CmdAdd(args *skel.CmdArgs) error {
 	if err := weavenet.AttachContainer(args.Netns, id, args.IfName, conf.BrName, conf.MTU, false, []*net.IPNet{&ip.Address}, false, conf.HairpinMode); err != nil {
 		return err
 	}
-	if err := weavenet.WithNetNSLinkUnsafe(ns, args.IfName, func(link netlink.Link) error {
+	if err := weavenet.WithNetNSLink(ns, args.IfName, func(link netlink.Link) error {
 		return setupRoutes(link, args.IfName, ip.Address, ip.Gateway, result.Routes)
 	}); err != nil {
 		return fmt.Errorf("error setting up routes: %s", err)
@@ -177,7 +177,13 @@ func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error {
 
 	// As of CNI 0.3 spec, runtimes can send blank if they just want the address deallocated
 	if args.Netns != "" {
-		if _, err = weavenet.WithNetNS(args.Netns, "del-iface", args.IfName); err != nil {
+		if err = weavenet.WithNetNSByPath(args.Netns, func() error {
+			link, err := netlink.LinkByName(args.IfName)
+			if err != nil {
+				return err
+			}
+			return netlink.LinkDel(link)
+		}); err != nil {
 			// We log the error and carry on instead of returning nil, as there may still be resources to free in IPAM.
 			logOnStderr(fmt.Errorf("error removing interface %q: %s", args.IfName, err))
 		}

plugin/net/watcher.go

@@ -49,7 +49,9 @@ func (w *watcher) ContainerStarted(id string) {
 				rootDir = "/host"
 			}
 			netNSPath := weavenet.NSPathByPidWithRoot(rootDir, info.State.Pid)
-			if _, err := weavenet.WithNetNS(netNSPath, "configure-arp", weavenet.VethName, rootDir); err != nil {
+			if err := weavenet.WithNetNSByPath(netNSPath, func() error {
+				return weavenet.ConfigureARP(weavenet.VethName, rootDir)
+			}); err != nil {
 				w.driver.warn("ContainerStarted", "unable to configure interfaces: %s", err)
 			}
 		}

prog/weaveutil/main.go

@@ -4,10 +4,7 @@ package main
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
-
-	weavenet "github.com/weaveworks/weave/net"
 )
 
 var commands map[string]func([]string) error
@@ -36,12 +33,6 @@ func init() {
 		"create-volume-container":  createVolumeContainer,
 		"attach-container":         attach,
 		"detach-container":         detach,
-		"configure-arp":            configureARP,
-		"check-iface":              checkIface,
-		"del-iface":                delIface,
-		"setup-iface":              setupIface,
-		"setup-iface-addrs":        setupIfaceAddrs,
-		"list-netdevs":             listNetDevs,
 		"cni-net":                  cniNet,
 		"cni-ipam":                 cniIPAM,
 		"bridge-ip":                bridgeIP,
@@ -55,14 +46,6 @@ func init() {
 }
 
 func main() {
-	// force re-exec of this binary
-	selfPath, err := filepath.EvalSymlinks("/proc/self/exe")
-	if err != nil {
-		fmt.Fprintln(os.Stderr, err)
-		os.Exit(1)
-	}
-	weavenet.WeaveUtilCmd = selfPath
-
 	// If no args passed, act as CNI plugin based on executable name
 	switch {
 	case len(os.Args) == 1 && strings.HasSuffix(os.Args[0], "weave-ipam"):

prog/weaveutil/netdev.go

@@ -1,141 +0,0 @@
-package main
-
-import (
-	"encoding/json"
-	"fmt"
-	"strconv"
-	"strings"
-
-	"github.com/j-keck/arping"
-	"github.com/vishvananda/netlink"
-
-	weavenet "github.com/weaveworks/weave/net"
-)
-
-// checkIface returns an error if the given interface cannot be found.
-func checkIface(args []string) error {
-	if len(args) != 1 {
-		cmdUsage("check-iface", "<iface-name>")
-	}
-	ifaceName := args[0]
-
-	if _, err := netlink.LinkByName(ifaceName); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func delIface(args []string) error {
-	if len(args) != 1 {
-		cmdUsage("del-iface", "<iface-name>")
-	}
-	ifName := args[0]
-
-	link, err := netlink.LinkByName(ifName)
-	if err != nil {
-		return err
-	}
-	return netlink.LinkDel(link)
-}
-
-// setupIface renames the given iface and configures its ARP cache settings.
-func setupIface(args []string) error {
-	if len(args) != 2 {
-		cmdUsage("setup-iface", "<iface-name> <new-iface-name>")
-	}
-	ifaceName := args[0]
-	newIfName := args[1]
-
-	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", "<iface-name> <with-multicast> <cidr>...")
-	}
-	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", "<iface-name-prefix> <root-path>")
-	}
-	prefix := args[0]
-	rootPath := args[1]
-
-	links, err := netlink.LinkList()
-	if err != nil {
-		return err
-	}
-	for _, link := range links {
-		ifName := link.Attrs().Name
-		if strings.HasPrefix(ifName, prefix) {
-			weavenet.ConfigureARPCache(rootPath+"/proc", ifName)
-			if addrs, err := netlink.AddrList(link, netlink.FAMILY_V4); err == nil {
-				for _, addr := range addrs {
-					arping.GratuitousArpOverIfaceByName(addr.IPNet.IP, ifName)
-				}
-			}
-		}
-	}
-
-	return nil
-}
-
-// listNetDevs outputs network ifaces identified by the given indexes
-// in the format of weavenet.Dev.
-func listNetDevs(args []string) error {
-	if len(args) == 0 {
-		cmdUsage("list-netdevs", "<iface-index>[ <iface-index>]")
-	}
-
-	indexes := make(map[int]struct{})
-	for _, index := range args {
-		if index != "" {
-			id, err := strconv.Atoi(index)
-			if err != nil {
-				return err
-			}
-			indexes[id] = struct{}{}
-		}
-	}
-
-	links, err := netlink.LinkList()
-	if err != nil {
-		return err
-	}
-
-	var netdevs []weavenet.Dev
-
-	for _, link := range links {
-		if _, found := indexes[link.Attrs().Index]; found {
-			netdev, err := weavenet.LinkToNetDev(link)
-			if err != nil {
-				return err
-			}
-			netdevs = append(netdevs, netdev)
-		}
-	}
-
-	nds, err := json.Marshal(netdevs)
-	if err != nil {
-		return err
-	}
-	fmt.Println(string(nds))
-
-	return nil
-}