diff --git a/libnetwork/osl/interface_linux.go b/libnetwork/osl/interface_linux.go index f7474b74a8..baa0923eab 100644 --- a/libnetwork/osl/interface_linux.go +++ b/libnetwork/osl/interface_linux.go @@ -14,6 +14,31 @@ import ( "github.com/vishvananda/netns" ) +// newInterface creates a new interface in the given namespace using the +// provided options. +func newInterface(ns *Namespace, srcName, dstPrefix string, options ...IfaceOption) (*Interface, error) { + i := &Interface{ + srcName: srcName, + dstName: dstPrefix, + ns: ns, + } + for _, opt := range options { + if opt != nil { + // TODO(thaJeztah): use multi-error instead of returning early. + if err := opt(i); err != nil { + return nil, err + } + } + } + if i.master != "" { + i.dstMaster = ns.findDst(i.master, true) + if i.dstMaster == "" { + return nil, fmt.Errorf("could not find an appropriate master %q for %q", i.master, i.srcName) + } + } + return i, nil +} + // Interface represents the settings and identity of a network device. // It is used as a return type for Network.Link, and it is common practice // for the caller to use this information when moving interface SrcName from @@ -88,53 +113,8 @@ func (i *Interface) Routes() []*net.IPNet { // Remove an interface from the sandbox by renaming to original name // and moving it out of the sandbox. func (i *Interface) Remove() error { - i.ns.Lock() - isDefault := i.ns.isDefault - nlh := i.ns.nlHandle - i.ns.Unlock() - - // Find the network interface identified by the DstName attribute. - iface, err := nlh.LinkByName(i.DstName()) - if err != nil { - return err - } - - // Down the interface before configuring - if err := nlh.LinkSetDown(iface); err != nil { - return err - } - - err = nlh.LinkSetName(iface, i.SrcName()) - if err != nil { - log.G(context.TODO()).Debugf("LinkSetName failed for interface %s: %v", i.SrcName(), err) - return err - } - - // if it is a bridge just delete it. - if i.Bridge() { - if err := nlh.LinkDel(iface); err != nil { - return fmt.Errorf("failed deleting bridge %q: %v", i.SrcName(), err) - } - } else if !isDefault { - // Move the network interface to caller namespace. - if err := nlh.LinkSetNsFd(iface, ns.ParseHandlerInt()); err != nil { - log.G(context.TODO()).Debugf("LinkSetNsPid failed for interface %s: %v", i.SrcName(), err) - return err - } - } - - i.ns.Lock() - for index, intf := range i.ns.iFaces { - if intf == i { - i.ns.iFaces = append(i.ns.iFaces[:index], i.ns.iFaces[index+1:]...) - break - } - } - i.ns.Unlock() - - i.ns.checkLoV6() - - return nil + nameSpace := i.ns + return nameSpace.RemoveInterface(i) } // Statistics returns the sandbox's side veth interface statistics. @@ -160,8 +140,8 @@ func (i *Interface) Statistics() (*types.InterfaceStatistics, error) { } func (n *Namespace) findDst(srcName string, isBridge bool) string { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() for _, i := range n.iFaces { // The master should match the srcname of the interface and the @@ -180,24 +160,12 @@ func (n *Namespace) findDst(srcName string, isBridge bool) string { // to only provide a prefix for DstName. The AddInterface api will auto-generate // an appropriate suffix for the DstName to disambiguate. func (n *Namespace) AddInterface(srcName, dstPrefix string, options ...IfaceOption) error { - i := &Interface{ - srcName: srcName, - dstName: dstPrefix, - ns: n, - } - if err := i.processInterfaceOptions(options...); err != nil { + i, err := newInterface(n, srcName, dstPrefix, options...) + if err != nil { return err } - if i.master != "" { - i.dstMaster = n.findDst(i.master, true) - if i.dstMaster == "" { - return fmt.Errorf("could not find an appropriate master %q for %q", - i.master, i.srcName) - } - } - - n.Lock() + n.mu.Lock() if n.isDefault { i.dstName = i.srcName } else { @@ -209,7 +177,7 @@ func (n *Namespace) AddInterface(srcName, dstPrefix string, options ...IfaceOpti isDefault := n.isDefault nlh := n.nlHandle nlhHost := ns.NlHandle() - n.Unlock() + n.mu.Unlock() // If it is a bridge interface we have to create the bridge inside // the namespace so don't try to lookup the interface using srcName @@ -285,15 +253,69 @@ func (n *Namespace) AddInterface(srcName, dstPrefix string, options ...IfaceOpti return fmt.Errorf("error setting interface %q routes to %q: %v", iface.Attrs().Name, i.Routes(), err) } - n.Lock() + n.mu.Lock() n.iFaces = append(n.iFaces, i) - n.Unlock() + n.mu.Unlock() n.checkLoV6() return nil } +// RemoveInterface removes an interface from the namespace by renaming to +// original name and moving it out of the sandbox. +func (n *Namespace) RemoveInterface(i *Interface) error { + n.mu.Lock() + isDefault := n.isDefault + nlh := n.nlHandle + n.mu.Unlock() + + // Find the network interface identified by the DstName attribute. + iface, err := nlh.LinkByName(i.DstName()) + if err != nil { + return err + } + + // Down the interface before configuring + if err := nlh.LinkSetDown(iface); err != nil { + return err + } + + // TODO(aker): Why are we doing this? This would fail if the initial interface set up failed before the "dest interface" was moved into its own namespace; see https://github.com/moby/moby/pull/46315/commits/108595c2fe852a5264b78e96f9e63cda284990a6#r1331253578 + err = nlh.LinkSetName(iface, i.SrcName()) + if err != nil { + log.G(context.TODO()).Debugf("LinkSetName failed for interface %s: %v", i.SrcName(), err) + return err + } + + // if it is a bridge just delete it. + if i.Bridge() { + if err := nlh.LinkDel(iface); err != nil { + return fmt.Errorf("failed deleting bridge %q: %v", i.SrcName(), err) + } + } else if !isDefault { + // Move the network interface to caller namespace. + // TODO(aker): What's this really doing? There are no calls to LinkDel in this package: is this code really used? (Interface.Remove() has 3 callers); see https://github.com/moby/moby/pull/46315/commits/108595c2fe852a5264b78e96f9e63cda284990a6#r1331265335 + if err := nlh.LinkSetNsFd(iface, ns.ParseHandlerInt()); err != nil { + log.G(context.TODO()).Debugf("LinkSetNsFd failed for interface %s: %v", i.SrcName(), err) + return err + } + } + + n.mu.Lock() + for index, intf := range i.ns.iFaces { + if intf == i { + i.ns.iFaces = append(i.ns.iFaces[:index], i.ns.iFaces[index+1:]...) + break + } + } + n.mu.Unlock() + + // TODO(aker): This function will disable IPv6 on lo interface if the removed interface was the last one offering IPv6 connectivity. That's a weird behavior, and shouldn't be hiding this deep down in this function. + n.checkLoV6() + return nil +} + func configureInterface(nlh *netlink.Handle, iface netlink.Link, i *Interface) error { ifaceName := iface.Attrs().Name ifaceConfigurators := []struct { diff --git a/libnetwork/osl/namespace_linux.go b/libnetwork/osl/namespace_linux.go index 13e46caafc..73270662b7 100644 --- a/libnetwork/osl/namespace_linux.go +++ b/libnetwork/osl/namespace_linux.go @@ -285,16 +285,16 @@ func createNetworkNamespace(path string, osCreate bool) error { } func unmountNamespaceFile(path string) { - if _, err := os.Stat(path); err == nil { - if err := syscall.Unmount(path, syscall.MNT_DETACH); err != nil && !errors.Is(err, unix.EINVAL) { - log.G(context.TODO()).WithError(err).Error("Error unmounting namespace file") - } + if _, err := os.Stat(path); err != nil { + // ignore when we cannot stat the path + return + } + if err := syscall.Unmount(path, syscall.MNT_DETACH); err != nil && !errors.Is(err, unix.EINVAL) { + log.G(context.TODO()).WithError(err).Error("Error unmounting namespace file") } } -func createNamespaceFile(path string) (err error) { - var f *os.File - +func createNamespaceFile(path string) error { once.Do(createBasePath) // Remove it from garbage collection list if present removeFromGarbagePaths(path) @@ -304,13 +304,16 @@ func createNamespaceFile(path string) (err error) { // wait for garbage collection to complete if it is in progress // before trying to create the file. + // + // TODO(aker): This garbage-collection was for a kernel bug in kernels 3.18-4.0.1: is this still needed on current kernels (and on kernel 3.10)? see https://github.com/moby/moby/pull/46315/commits/c0a6beba8e61d4019e1806d5241ba22007072ca2#r1331327103 gpmWg.Wait() - if f, err = os.Create(path); err == nil { - f.Close() + f, err := os.Create(path) + if err != nil { + return err } - - return err + _ = f.Close() + return nil } // Namespace represents a network sandbox. It represents a Linux network @@ -328,7 +331,7 @@ type Namespace struct { isDefault bool nlHandle *netlink.Handle loV6Enabled bool - sync.Mutex + mu sync.Mutex } // Interfaces returns the collection of Interface previously added with the AddInterface @@ -450,8 +453,8 @@ func (n *Namespace) InvokeFunc(f func()) error { } func (n *Namespace) nsPath() string { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() return n.path } @@ -478,24 +481,13 @@ func (n *Namespace) Destroy() error { } // Restore restores the network namespace. -func (n *Namespace) Restore(ifsopt map[Iface][]IfaceOption, routes []*types.StaticRoute, gw net.IP, gw6 net.IP) error { +func (n *Namespace) Restore(interfaces map[Iface][]IfaceOption, routes []*types.StaticRoute, gw net.IP, gw6 net.IP) error { // restore interfaces - for name, opts := range ifsopt { - i := &Interface{ - srcName: name.SrcName, - dstName: name.DstPrefix, - ns: n, - } - if err := i.processInterfaceOptions(opts...); err != nil { + for iface, opts := range interfaces { + i, err := newInterface(n, iface.SrcName, iface.DstPrefix, opts...) + if err != nil { return err } - if i.master != "" { - i.dstMaster = n.findDst(i.master, true) - if i.dstMaster == "" { - return fmt.Errorf("could not find an appropriate master %q for %q", - i.master, i.srcName) - } - } if n.isDefault { i.dstName = i.srcName } else { @@ -506,76 +498,60 @@ func (n *Namespace) Restore(ifsopt map[Iface][]IfaceOption, routes []*types.Stat // due to the docker network connect/disconnect, so the dstName should // restore from the namespace for _, link := range links { - addrs, err := n.nlHandle.AddrList(link, netlink.FAMILY_V4) - if err != nil { - return err - } ifaceName := link.Attrs().Name - if strings.HasPrefix(ifaceName, "vxlan") { - if i.dstName == "vxlan" { - i.dstName = ifaceName - break - } + if i.dstName == "vxlan" && strings.HasPrefix(ifaceName, "vxlan") { + i.dstName = ifaceName + break } // find the interface name by ip if i.address != nil { - for _, addr := range addrs { + addresses, err := n.nlHandle.AddrList(link, netlink.FAMILY_V4) + if err != nil { + return err + } + for _, addr := range addresses { if addr.IPNet.String() == i.address.String() { i.dstName = ifaceName break } - continue } if i.dstName == ifaceName { break } } // This is to find the interface name of the pair in overlay sandbox - if strings.HasPrefix(ifaceName, "veth") { - if i.master != "" && i.dstName == "veth" { - i.dstName = ifaceName - } + if i.master != "" && i.dstName == "veth" && strings.HasPrefix(ifaceName, "veth") { + i.dstName = ifaceName } } var index int - indexStr := strings.TrimPrefix(i.dstName, name.DstPrefix) - if indexStr != "" { - index, err = strconv.Atoi(indexStr) + if idx := strings.TrimPrefix(i.dstName, iface.DstPrefix); idx != "" { + index, err = strconv.Atoi(idx) if err != nil { - return err + return fmt.Errorf("failed to restore interface in network namespace %q: invalid dstName for interface: %s: %v", n.path, i.dstName, err) } } index++ - n.Lock() - if index > n.nextIfIndex[name.DstPrefix] { - n.nextIfIndex[name.DstPrefix] = index + n.mu.Lock() + if index > n.nextIfIndex[iface.DstPrefix] { + n.nextIfIndex[iface.DstPrefix] = index } n.iFaces = append(n.iFaces, i) - n.Unlock() + n.mu.Unlock() } } - // restore routes - for _, r := range routes { - n.Lock() - n.staticRoutes = append(n.staticRoutes, r) - n.Unlock() - } - - // restore gateway + // restore routes and gateways + n.mu.Lock() + n.staticRoutes = append(n.staticRoutes, routes...) if len(gw) > 0 { - n.Lock() n.gw = gw - n.Unlock() } - if len(gw6) > 0 { - n.Lock() n.gwv6 = gw6 - n.Unlock() } - + n.mu.Unlock() return nil } @@ -586,7 +562,7 @@ func (n *Namespace) checkLoV6() { action = "disable" ) - n.Lock() + n.mu.Lock() for _, iface := range n.iFaces { if iface.AddressIPv6() != nil { enable = true @@ -594,7 +570,7 @@ func (n *Namespace) checkLoV6() { break } } - n.Unlock() + n.mu.Unlock() if n.loV6Enabled == enable { return diff --git a/libnetwork/osl/neigh_linux.go b/libnetwork/osl/neigh_linux.go index 8fa0f2b10a..94a3141c67 100644 --- a/libnetwork/osl/neigh_linux.go +++ b/libnetwork/osl/neigh_linux.go @@ -32,8 +32,8 @@ type neigh struct { } func (n *Namespace) findNeighbor(dstIP net.IP, dstMac net.HardwareAddr) *neigh { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() for _, nh := range n.neighbors { if nh.dstIP.Equal(dstIP) && bytes.Equal(nh.dstMac, dstMac) { @@ -51,9 +51,9 @@ func (n *Namespace) DeleteNeighbor(dstIP net.IP, dstMac net.HardwareAddr) error return NeighborSearchError{dstIP, dstMac, false} } - n.Lock() + n.mu.Lock() nlh := n.nlHandle - n.Unlock() + n.mu.Unlock() var linkIndex int if nh.linkDst != "" { @@ -96,14 +96,14 @@ func (n *Namespace) DeleteNeighbor(dstIP net.IP, dstMac net.HardwareAddr) error } } - n.Lock() + n.mu.Lock() for i, neighbor := range n.neighbors { if neighbor.dstIP.Equal(dstIP) && bytes.Equal(neighbor.dstMac, dstMac) { n.neighbors = append(n.neighbors[:i], n.neighbors[i+1:]...) break } } - n.Unlock() + n.mu.Unlock() log.G(context.TODO()).Debugf("Neighbor entry deleted for IP %v, mac %v", dstIP, dstMac) return nil @@ -142,9 +142,9 @@ func (n *Namespace) AddNeighbor(dstIP net.IP, dstMac net.HardwareAddr, force boo } } - n.Lock() + n.mu.Lock() nlh := n.nlHandle - n.Unlock() + n.mu.Unlock() if nh.linkDst != "" { iface, err = nlh.LinkByName(nh.linkDst) @@ -176,9 +176,9 @@ func (n *Namespace) AddNeighbor(dstIP net.IP, dstMac net.HardwareAddr, force boo return nil } - n.Lock() + n.mu.Lock() n.neighbors = append(n.neighbors, nh) - n.Unlock() + n.mu.Unlock() log.G(context.TODO()).Debugf("Neighbor entry added for IP:%v, mac:%v on ifc:%s", dstIP, dstMac, nh.linkName) return nil diff --git a/libnetwork/osl/options_linux.go b/libnetwork/osl/options_linux.go index 74e2a38d9d..bc617c7b0a 100644 --- a/libnetwork/osl/options_linux.go +++ b/libnetwork/osl/options_linux.go @@ -24,18 +24,6 @@ func WithFamily(family int) NeighOption { } } -func (i *Interface) processInterfaceOptions(options ...IfaceOption) error { - for _, opt := range options { - if opt != nil { - // TODO(thaJeztah): use multi-error instead of returning early. - if err := opt(i); err != nil { - return err - } - } - } - return nil -} - // WithIsBridge sets whether the interface is a bridge. func WithIsBridge(isBridge bool) IfaceOption { return func(i *Interface) error { diff --git a/libnetwork/osl/route_linux.go b/libnetwork/osl/route_linux.go index ab0e228066..18ef4ef1d7 100644 --- a/libnetwork/osl/route_linux.go +++ b/libnetwork/osl/route_linux.go @@ -10,16 +10,16 @@ import ( // Gateway returns the IPv4 gateway for the sandbox. func (n *Namespace) Gateway() net.IP { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() return n.gw } // GatewayIPv6 returns the IPv6 gateway for the sandbox. func (n *Namespace) GatewayIPv6() net.IP { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() return n.gwv6 } @@ -28,8 +28,8 @@ func (n *Namespace) GatewayIPv6() net.IP { // directly connected routes are stored on the particular interface they // refer to. func (n *Namespace) StaticRoutes() []*types.StaticRoute { - n.Lock() - defer n.Unlock() + n.mu.Lock() + defer n.mu.Unlock() routes := make([]*types.StaticRoute, len(n.staticRoutes)) for i, route := range n.staticRoutes { @@ -40,48 +40,37 @@ func (n *Namespace) StaticRoutes() []*types.StaticRoute { return routes } -func (n *Namespace) setGateway(gw net.IP) { - n.Lock() - n.gw = gw - n.Unlock() -} - -func (n *Namespace) setGatewayIPv6(gwv6 net.IP) { - n.Lock() - n.gwv6 = gwv6 - n.Unlock() -} - -// SetGateway sets the default IPv4 gateway for the sandbox. +// SetGateway sets the default IPv4 gateway for the sandbox. It is a no-op +// if the given gateway is empty. func (n *Namespace) SetGateway(gw net.IP) error { - // Silently return if the gateway is empty if len(gw) == 0 { return nil } - err := n.programGateway(gw, true) - if err == nil { - n.setGateway(gw) + if err := n.programGateway(gw, true); err != nil { + return err } - - return err + n.mu.Lock() + n.gw = gw + n.mu.Unlock() + return nil } // UnsetGateway the previously set default IPv4 gateway in the sandbox. +// It is a no-op if no gateway was set. func (n *Namespace) UnsetGateway() error { gw := n.Gateway() - - // Silently return if the gateway is empty if len(gw) == 0 { return nil } - err := n.programGateway(gw, false) - if err == nil { - n.setGateway(net.IP{}) + if err := n.programGateway(gw, false); err != nil { + return err } - - return err + n.mu.Lock() + n.gw = net.IP{} + n.mu.Unlock() + return nil } func (n *Namespace) programGateway(gw net.IP, isAdd bool) error { @@ -99,7 +88,7 @@ func (n *Namespace) programGateway(gw net.IP, isAdd bool) error { } if linkIndex == 0 { - return fmt.Errorf("Direct route for the gateway %s could not be found", gw) + return fmt.Errorf("direct route for the gateway %s could not be found", gw) } if isAdd { @@ -118,7 +107,7 @@ func (n *Namespace) programGateway(gw net.IP, isAdd bool) error { } // Program a route in to the namespace routing table. -func (n *Namespace) programRoute(path string, dest *net.IPNet, nh net.IP) error { +func (n *Namespace) programRoute(dest *net.IPNet, nh net.IP) error { gwRoutes, err := n.nlHandle.RouteGet(nh) if err != nil { return fmt.Errorf("route for the next hop %s could not be found: %v", nh, err) @@ -133,7 +122,7 @@ func (n *Namespace) programRoute(path string, dest *net.IPNet, nh net.IP) error } // Delete a route from the namespace routing table. -func (n *Namespace) removeRoute(path string, dest *net.IPNet, nh net.IP) error { +func (n *Namespace) removeRoute(dest *net.IPNet, nh net.IP) error { gwRoutes, err := n.nlHandle.RouteGet(nh) if err != nil { return fmt.Errorf("route for the next hop could not be found: %v", err) @@ -147,67 +136,70 @@ func (n *Namespace) removeRoute(path string, dest *net.IPNet, nh net.IP) error { }) } -// SetGatewayIPv6 sets the default IPv6 gateway for the sandbox. +// SetGatewayIPv6 sets the default IPv6 gateway for the sandbox. It is a no-op +// if the given gateway is empty. func (n *Namespace) SetGatewayIPv6(gwv6 net.IP) error { - // Silently return if the gateway is empty if len(gwv6) == 0 { return nil } - err := n.programGateway(gwv6, true) - if err == nil { - n.setGatewayIPv6(gwv6) + if err := n.programGateway(gwv6, true); err != nil { + return err } - return err + n.mu.Lock() + n.gwv6 = gwv6 + n.mu.Unlock() + return nil } // UnsetGatewayIPv6 unsets the previously set default IPv6 gateway in the sandbox. +// It is a no-op if no gateway was set. func (n *Namespace) UnsetGatewayIPv6() error { gwv6 := n.GatewayIPv6() - - // Silently return if the gateway is empty if len(gwv6) == 0 { return nil } - err := n.programGateway(gwv6, false) - if err == nil { - n.Lock() - n.gwv6 = net.IP{} - n.Unlock() + if err := n.programGateway(gwv6, false); err != nil { + return err } - return err + n.mu.Lock() + n.gwv6 = net.IP{} + n.mu.Unlock() + return nil } // AddStaticRoute adds a static route to the sandbox. func (n *Namespace) AddStaticRoute(r *types.StaticRoute) error { - err := n.programRoute(n.nsPath(), r.Destination, r.NextHop) - if err == nil { - n.Lock() - n.staticRoutes = append(n.staticRoutes, r) - n.Unlock() + if err := n.programRoute(r.Destination, r.NextHop); err != nil { + return err } - return err + + n.mu.Lock() + n.staticRoutes = append(n.staticRoutes, r) + n.mu.Unlock() + return nil } // RemoveStaticRoute removes a static route from the sandbox. func (n *Namespace) RemoveStaticRoute(r *types.StaticRoute) error { - err := n.removeRoute(n.nsPath(), r.Destination, r.NextHop) - if err == nil { - n.Lock() - lastIndex := len(n.staticRoutes) - 1 - for i, v := range n.staticRoutes { - if v == r { - // Overwrite the route we're removing with the last element - n.staticRoutes[i] = n.staticRoutes[lastIndex] - // Shorten the slice to trim the extra element - n.staticRoutes = n.staticRoutes[:lastIndex] - break - } - } - n.Unlock() + if err := n.removeRoute(r.Destination, r.NextHop); err != nil { + return err } - return err + + n.mu.Lock() + lastIndex := len(n.staticRoutes) - 1 + for i, v := range n.staticRoutes { + if v == r { + // Overwrite the route we're removing with the last element + n.staticRoutes[i] = n.staticRoutes[lastIndex] + // Shorten the slice to trim the extra element + n.staticRoutes = n.staticRoutes[:lastIndex] + break + } + } + n.mu.Unlock() + return nil }