diff --git a/common/utils.go b/common/utils.go index 7086cd9f54..51129500a6 100644 --- a/common/utils.go +++ b/common/utils.go @@ -48,14 +48,14 @@ func FindNetDevs(processID int, match func(link netlink.Link) bool) ([]NetDev, e } defer ns.Close() - err = weavenet.WithNetNS(ns, func() error { + err = weavenet.WithNetNSUnsafe(ns, func() error { return forEachLink(func(link netlink.Link) error { if match(link) { netDev, err := linkToNetDev(link) if err != nil { return err } - netDevs = append(netDevs, *netDev) + netDevs = append(netDevs, netDev) } return nil }) @@ -77,21 +77,54 @@ func forEachLink(f func(netlink.Link) error) error { return nil } -func linkToNetDev(link netlink.Link) (*NetDev, error) { +func linkToNetDev(link netlink.Link) (NetDev, error) { addrs, err := netlink.AddrList(link, netlink.FAMILY_V4) if err != nil { - return nil, err + return NetDev{}, err } - netDev := &NetDev{Name: link.Attrs().Name, MAC: link.Attrs().HardwareAddr} + netDev := NetDev{Name: link.Attrs().Name, MAC: link.Attrs().HardwareAddr} for _, addr := range addrs { netDev.CIDRs = append(netDev.CIDRs, addr.IPNet) } return netDev, nil } -// Lookup the weave interface of a container -func GetWeaveNetDevs(processID int) ([]NetDev, error) { +// ConnectedToBridgePredicate returns a function which is used to query whether +// a given link is a veth interface which one end is connected to a bridge. +// The returned function should be called from a container network namespace which +// the bridge does NOT belong to. +func ConnectedToBridgePredicate(bridgeName string) (func(link netlink.Link) bool, error) { + indexes := make(map[int]struct{}) + + // Scan devices in root namespace to find those attached to weave bridge + err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName, + func(br netlink.Link) error { + return forEachLink(func(link netlink.Link) error { + if link.Attrs().MasterIndex == br.Attrs().Index { + peerIndex := link.Attrs().ParentIndex + if peerIndex == 0 { + // perhaps running on an older kernel where ParentIndex doesn't work. + // as fall-back, assume the indexes are consecutive + peerIndex = link.Attrs().Index - 1 + } + indexes[peerIndex] = struct{}{} + } + return nil + }) + }) + if err != nil { + return nil, err + } + + return func(link netlink.Link) bool { + _, isveth := link.(*netlink.Veth) + _, found := indexes[link.Attrs().Index] + return isveth && found + }, nil +} + +func GetNetDevsWithPredicate(processID int, predicate func(link netlink.Link) bool) ([]NetDev, error) { // Bail out if this container is running in the root namespace nsToplevel, err := netns.GetFromPid(1) if err != nil { @@ -105,39 +138,28 @@ func GetWeaveNetDevs(processID int) ([]NetDev, error) { return nil, nil } - weaveBridge, err := netlink.LinkByName("weave") - if err != nil { - return nil, fmt.Errorf("Cannot find weave bridge: %s", err) - } - // Scan devices in root namespace to find those attached to weave bridge - indexes := make(map[int]struct{}) - err = forEachLink(func(link netlink.Link) error { - if link.Attrs().MasterIndex == weaveBridge.Attrs().Index { - peerIndex := link.Attrs().ParentIndex - if peerIndex == 0 { - // perhaps running on an older kernel where ParentIndex doesn't work. - // as fall-back, assume the indexes are consecutive - peerIndex = link.Attrs().Index - 1 - } - indexes[peerIndex] = struct{}{} - } - return nil - }) + return FindNetDevs(processID, predicate) +} + +// Lookup the weave interface of a container +func GetWeaveNetDevs(processID int) ([]NetDev, error) { + p, err := ConnectedToBridgePredicate("weave") if err != nil { return nil, err } - return FindNetDevs(processID, func(link netlink.Link) bool { - _, isveth := link.(*netlink.Veth) - _, found := indexes[link.Attrs().Index] - return isveth && found - }) + + return GetNetDevsWithPredicate(processID, p) } -// Get the weave bridge interface -func GetBridgeNetDev(bridgeName string) ([]NetDev, error) { - return FindNetDevs(1, func(link netlink.Link) bool { - return link.Attrs().Name == bridgeName - }) +// Get the weave bridge interface. +// NB: Should be called from the root network namespace. +func GetBridgeNetDev(bridgeName string) (NetDev, error) { + link, err := netlink.LinkByName(bridgeName) + if err != nil { + return NetDev{}, err + } + + return linkToNetDev(link) } // Do post-attach configuration of all veths we have created diff --git a/net/netns.go b/net/netns.go index aeb8214c79..b067690938 100644 --- a/net/netns.go +++ b/net/netns.go @@ -7,7 +7,19 @@ import ( "github.com/vishvananda/netns" ) -func WithNetNS(ns netns.NsHandle, work func() error) error { +// 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 { runtime.LockOSThread() defer runtime.UnlockOSThread() @@ -26,8 +38,8 @@ func WithNetNS(ns netns.NsHandle, work func() error) error { return err } -func WithNetNSLink(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error { - return WithNetNS(ns, func() error { +func WithNetNSLinkUnsafe(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error { + return WithNetNSUnsafe(ns, func() error { link, err := netlink.LinkByName(ifName) if err != nil { return err @@ -35,3 +47,13 @@ func WithNetNSLink(ns netns.NsHandle, ifName string, work func(link netlink.Link return work(link) }) } + +func WithNetNSLinkByPidUnsafe(pid int, ifName string, work func(link netlink.Link) error) error { + ns, err := netns.GetFromPid(pid) + if err != nil { + return err + } + defer ns.Close() + + return WithNetNSLinkUnsafe(ns, ifName, work) +} diff --git a/net/veth.go b/net/veth.go index fbca71c3a3..49c1f96a64 100644 --- a/net/veth.go +++ b/net/veth.go @@ -104,7 +104,7 @@ const ( ) func interfaceExistsInNamespace(ns netns.NsHandle, ifName string) bool { - err := WithNetNS(ns, func() error { + err := WithNetNSUnsafe(ns, func() error { _, err := netlink.LinkByName(ifName) return err }) @@ -122,7 +122,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int, if err := netlink.LinkSetNsFd(veth, int(ns)); err != nil { return fmt.Errorf("failed to move veth to container netns: %s", err) } - if err := WithNetNS(ns, func() error { + if err := WithNetNSUnsafe(ns, func() error { if err := netlink.LinkSetName(veth, ifName); err != nil { return err } @@ -140,7 +140,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int, } } - if err := WithNetNSLink(ns, ifName, func(veth netlink.Link) error { + if err := WithNetNSLinkUnsafe(ns, ifName, func(veth netlink.Link) error { newAddresses, err := AddAddresses(veth, cidrs) if err != nil { return err @@ -177,7 +177,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int, } func DetachContainer(ns netns.NsHandle, id, ifName string, cidrs []*net.IPNet) error { - return WithNetNSLink(ns, ifName, func(veth netlink.Link) error { + return WithNetNSLinkUnsafe(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) diff --git a/plugin/net/cni.go b/plugin/net/cni.go index ec73b5f0af..566675a7be 100644 --- a/plugin/net/cni.go +++ b/plugin/net/cni.go @@ -95,7 +95,7 @@ func (c *CNIPlugin) CmdAdd(args *skel.CmdArgs) error { if err := weavenet.AttachContainer(ns, id, args.IfName, conf.BrName, conf.MTU, false, []*net.IPNet{&result.IP4.IP}, false); err != nil { return err } - if err := weavenet.WithNetNSLink(ns, args.IfName, func(link netlink.Link) error { + if err := weavenet.WithNetNSLinkUnsafe(ns, args.IfName, func(link netlink.Link) error { return setupRoutes(link, args.IfName, result.IP4.IP, result.IP4.Gateway, result.IP4.Routes) }); err != nil { return fmt.Errorf("error setting up routes: %s", err) @@ -131,23 +131,20 @@ func setupRoutes(link netlink.Link, name string, ipnet net.IPNet, gw net.IP, rou } func findBridgeIP(bridgeName string, subnet net.IPNet) (net.IP, error) { - netdevs, err := common.GetBridgeNetDev(bridgeName) + netdev, err := common.GetBridgeNetDev(bridgeName) if err != nil { return nil, fmt.Errorf("Failed to get netdev for %q bridge: %s", bridgeName, err) } - if len(netdevs) == 0 { - return nil, fmt.Errorf("Could not find %q bridge", bridgeName) - } - if len(netdevs[0].CIDRs) == 0 { + if len(netdev.CIDRs) == 0 { return nil, fmt.Errorf("Bridge %q has no IP addresses; did you forget to run 'weave expose'?", bridgeName) } - for _, cidr := range netdevs[0].CIDRs { + for _, cidr := range netdev.CIDRs { if subnet.Contains(cidr.IP) { return cidr.IP, nil } } // None in the required subnet; just return the first one - return netdevs[0].CIDRs[0].IP, nil + return netdev.CIDRs[0].IP, nil } func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error { @@ -161,7 +158,7 @@ func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error { return err } defer ns.Close() - err = weavenet.WithNetNS(ns, func() error { + err = weavenet.WithNetNSUnsafe(ns, func() error { link, err := netlink.LinkByName(args.IfName) if err != nil { return err diff --git a/prog/weaveutil/addrs.go b/prog/weaveutil/addrs.go index 9c0d45f5b1..efdf535780 100644 --- a/prog/weaveutil/addrs.go +++ b/prog/weaveutil/addrs.go @@ -2,7 +2,9 @@ package main import ( "fmt" + "github.com/fsouza/go-dockerclient" + "github.com/vishvananda/netlink" "github.com/weaveworks/weave/common" ) @@ -12,46 +14,66 @@ func containerAddrs(args []string) error { } bridgeName := args[0] - c, err := docker.NewVersionedClientFromEnv("1.18") + client, err := docker.NewVersionedClientFromEnv("1.18") if err != nil { return err } - for _, containerID := range args[1:] { - netDevs, err := getNetDevs(bridgeName, c, containerID) - if err != nil { + pred, err := common.ConnectedToBridgePredicate(bridgeName) + if err != nil { + return err + } + + var containerIDs []string + containers := make(map[string]*docker.Container) + + for _, cid := range args[1:] { + if cid == "weave:expose" { + netDev, err := common.GetBridgeNetDev(bridgeName) + if err != nil { + return err + } + printNetDevs(cid, []common.NetDev{netDev}) + continue + } + if containers[cid], err = client.InspectContainer(cid); err != nil { if _, ok := err.(*docker.NoSuchContainer); ok { continue } return err } + // To output in the right order, we keep the list of container IDs + containerIDs = append(containerIDs, cid) + } - for _, netDev := range netDevs { - fmt.Printf("%12s %s %s", containerID, netDev.Name, netDev.MAC.String()) - for _, cidr := range netDev.CIDRs { - prefixLength, _ := cidr.Mask.Size() - fmt.Printf(" %v/%v", cidr.IP, prefixLength) - } - fmt.Println() + // NB: Because network namespaces (netns) are changed many times inside the loop, + // it's NOT safe to exec any code depending on the root netns without + // wrapping with WithNetNS*. + for _, cid := range containerIDs { + netDevs, err := getNetDevs(client, containers[cid], pred) + if err != nil { + return err } + printNetDevs(cid, netDevs) } return nil } -func getNetDevs(bridgeName string, c *docker.Client, containerID string) ([]common.NetDev, error) { - if containerID == "weave:expose" { - return common.GetBridgeNetDev(bridgeName) - } - - container, err := c.InspectContainer(containerID) - if err != nil { - return nil, err - } - +func getNetDevs(c *docker.Client, container *docker.Container, pred func(netlink.Link) bool) ([]common.NetDev, error) { if container.State.Pid == 0 { return nil, nil } + return common.GetNetDevsWithPredicate(container.State.Pid, pred) +} - return common.GetWeaveNetDevs(container.State.Pid) +func printNetDevs(cid string, netDevs []common.NetDev) { + for _, netDev := range netDevs { + fmt.Printf("%12s %s %s", cid, netDev.Name, netDev.MAC.String()) + for _, cidr := range netDev.CIDRs { + prefixLength, _ := cidr.Mask.Size() + fmt.Printf(" %v/%v", cidr.IP, prefixLength) + } + fmt.Println() + } } diff --git a/proxy/proxy.go b/proxy/proxy.go index 83c4ce10c4..53985ffcc9 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -149,14 +149,14 @@ func NewProxy(c Config) (*Proxy, error) { p.client = client.Client if !p.WithoutDNS { - netDevs, err := common.GetBridgeNetDev(c.DockerBridge) + netDev, err := common.GetBridgeNetDev(c.DockerBridge) if err != nil { return nil, err } - if len(netDevs) != 1 || len(netDevs[0].CIDRs) != 1 { + if len(netDev.CIDRs) != 1 { return nil, fmt.Errorf("Could not obtain address of %s", c.DockerBridge) } - p.dockerBridgeIP = netDevs[0].CIDRs[0].IP.String() + p.dockerBridgeIP = netDev.CIDRs[0].IP.String() } p.hostnameMatchRegexp, err = regexp.Compile(c.HostnameMatch)