Merge pull request #46174 from thaJeztah/libnetwork_osl_cleanups

libnetwork/osl: remove redundant locks, and assorted cleanups

libnetwork/osl/interface_linux.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"sync"
 	"syscall"
 	"time"
 
@@ -15,6 +14,11 @@ import (
 	"github.com/vishvananda/netns"
 )
 
+// nwIface 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
+// host namespace to DstName in a different net namespace with the appropriate
+// network settings.
 type nwIface struct {
 	srcName     string
 	dstName     string
@@ -27,107 +31,72 @@ type nwIface struct {
 	routes      []*net.IPNet
 	bridge      bool
 	ns          *networkNamespace
-	sync.Mutex
 }
 
+// SrcName returns the name of the interface in the origin network namespace.
 func (i *nwIface) SrcName() string {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.srcName
 }
 
+// DstName returns the name that will be assigned to the interface once
+// moved inside a network namespace. When the caller passes in a DstName,
+// it is only expected to pass a prefix. The name will be modified with an
+// auto-generated suffix.
 func (i *nwIface) DstName() string {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.dstName
 }
 
 func (i *nwIface) DstMaster() string {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.dstMaster
 }
 
+// Bridge returns true if the interface is a bridge.
 func (i *nwIface) Bridge() bool {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.bridge
 }
 
+// Master returns the srcname of the master interface for this interface.
 func (i *nwIface) Master() string {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.master
 }
 
 func (i *nwIface) MacAddress() net.HardwareAddr {
-	i.Lock()
-	defer i.Unlock()
-
 	return types.GetMacCopy(i.mac)
 }
 
+// Address returns the IPv4 address for the interface.
 func (i *nwIface) Address() *net.IPNet {
-	i.Lock()
-	defer i.Unlock()
-
 	return types.GetIPNetCopy(i.address)
 }
 
+// AddressIPv6 returns the IPv6 address for the interface.
 func (i *nwIface) AddressIPv6() *net.IPNet {
-	i.Lock()
-	defer i.Unlock()
-
 	return types.GetIPNetCopy(i.addressIPv6)
 }
 
+// LinkLocalAddresses returns the link-local IP addresses assigned to the
+// interface.
 func (i *nwIface) LinkLocalAddresses() []*net.IPNet {
-	i.Lock()
-	defer i.Unlock()
-
 	return i.llAddrs
 }
 
+// Routes returns IP routes for the interface.
 func (i *nwIface) Routes() []*net.IPNet {
-	i.Lock()
-	defer i.Unlock()
-
 	routes := make([]*net.IPNet, len(i.routes))
 	for index, route := range i.routes {
-		r := types.GetIPNetCopy(route)
-		routes[index] = r
+		routes[index] = types.GetIPNetCopy(route)
 	}
 
 	return routes
 }
 
-func (n *networkNamespace) Interfaces() []Interface {
-	n.Lock()
-	defer n.Unlock()
-
-	ifaces := make([]Interface, len(n.iFaces))
-
-	for i, iface := range n.iFaces {
-		ifaces[i] = iface
-	}
-
-	return ifaces
-}
-
+// Remove an interface from the sandbox by renaming to original name
+// and moving it out of the sandbox.
 func (i *nwIface) Remove() error {
-	i.Lock()
-	n := i.ns
-	i.Unlock()
-
-	n.Lock()
-	isDefault := n.isDefault
-	nlh := n.nlHandle
-	n.Unlock()
+	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())
@@ -159,29 +128,25 @@ func (i *nwIface) Remove() error {
 		}
 	}
 
-	n.Lock()
-	for index, intf := range n.iFaces {
+	i.ns.Lock()
+	for index, intf := range i.ns.iFaces {
 		if intf == i {
-			n.iFaces = append(n.iFaces[:index], n.iFaces[index+1:]...)
+			i.ns.iFaces = append(i.ns.iFaces[:index], i.ns.iFaces[index+1:]...)
 			break
 		}
 	}
-	n.Unlock()
+	i.ns.Unlock()
 
-	n.checkLoV6()
+	i.ns.checkLoV6()
 
 	return nil
 }
 
-// Returns the sandbox's side veth interface statistics
+// Statistics returns the sandbox's side veth interface statistics.
 func (i *nwIface) Statistics() (*types.InterfaceStatistics, error) {
-	i.Lock()
-	n := i.ns
-	i.Unlock()
-
-	l, err := n.nlHandle.LinkByName(i.DstName())
+	l, err := i.ns.nlHandle.LinkByName(i.DstName())
 	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve the statistics for %s in netns %s: %v", i.DstName(), n.path, err)
+		return nil, fmt.Errorf("failed to retrieve the statistics for %s in netns %s: %v", i.DstName(), i.ns.path, err)
 	}
 
 	stats := l.Attrs().Statistics
@@ -215,7 +180,11 @@ func (n *networkNamespace) findDst(srcName string, isBridge bool) string {
 }
 
 func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...IfaceOption) error {
-	i := &nwIface{srcName: srcName, dstName: dstPrefix, ns: n}
+	i := &nwIface{
+		srcName: srcName,
+		dstName: dstPrefix,
+		ns:      n,
+	}
 	i.processInterfaceOptions(options...)
 
 	if i.master != "" {
@@ -243,12 +212,11 @@ func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...If
 	// 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
 	if i.bridge {
-		link := &netlink.Bridge{
+		if err := nlh.LinkAdd(&netlink.Bridge{
 			LinkAttrs: netlink.LinkAttrs{
 				Name: i.srcName,
 			},
-		}
-		if err := nlh.LinkAdd(link); err != nil {
+		}); err != nil {
 			return fmt.Errorf("failed to create bridge %q: %v", i.srcName, err)
 		}
 	} else {

libnetwork/osl/namespace_linux.go

@@ -46,23 +46,6 @@ var (
 	prefix           = defaultPrefix
 )
 
-// The networkNamespace type is the linux implementation of the Sandbox
-// interface. It represents a linux network namespace, and moves an interface
-// into it when called on method AddInterface or sets the gateway etc.
-type networkNamespace struct {
-	path         string
-	iFaces       []*nwIface
-	gw           net.IP
-	gwv6         net.IP
-	staticRoutes []*types.StaticRoute
-	neighbors    []*neigh
-	nextIfIndex  map[string]int
-	isDefault    bool
-	nlHandle     *netlink.Handle
-	loV6Enabled  bool
-	sync.Mutex
-}
-
 // SetBasePath sets the base url prefix for the ns path
 func SetBasePath(path string) {
 	prefix = path
@@ -242,14 +225,6 @@ func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error) {
 	return n, nil
 }
 
-func (n *networkNamespace) InterfaceOptions() IfaceOptionSetter {
-	return n
-}
-
-func (n *networkNamespace) NeighborOptions() NeighborOptionSetter {
-	return n
-}
-
 func mountNetworkNamespace(basePath string, lnPath string) error {
 	return syscall.Mount(basePath, lnPath, "bind", syscall.MS_BIND, "")
 }
@@ -338,6 +313,39 @@ func createNamespaceFile(path string) (err error) {
 	return err
 }
 
+// The networkNamespace type is the linux implementation of the Sandbox
+// interface. It represents a linux network namespace, and moves an interface
+// into it when called on method AddInterface or sets the gateway etc.
+type networkNamespace struct {
+	path         string
+	iFaces       []*nwIface
+	gw           net.IP
+	gwv6         net.IP
+	staticRoutes []*types.StaticRoute
+	neighbors    []*neigh
+	nextIfIndex  map[string]int
+	isDefault    bool
+	nlHandle     *netlink.Handle
+	loV6Enabled  bool
+	sync.Mutex
+}
+
+func (n *networkNamespace) Interfaces() []Interface {
+	ifaces := make([]Interface, len(n.iFaces))
+	for i, iface := range n.iFaces {
+		ifaces[i] = iface
+	}
+	return ifaces
+}
+
+func (n *networkNamespace) InterfaceOptions() IfaceOptionSetter {
+	return n
+}
+
+func (n *networkNamespace) NeighborOptions() NeighborOptionSetter {
+	return n
+}
+
 func (n *networkNamespace) loopbackUp() error {
 	iface, err := n.nlHandle.LinkByName("lo")
 	if err != nil {
@@ -474,7 +482,11 @@ func (n *networkNamespace) Destroy() error {
 func (n *networkNamespace) Restore(ifsopt map[Iface][]IfaceOption, routes []*types.StaticRoute, gw net.IP, gw6 net.IP) error {
 	// restore interfaces
 	for name, opts := range ifsopt {
-		i := &nwIface{srcName: name.SrcName, dstName: name.DstPrefix, ns: n}
+		i := &nwIface{
+			srcName: name.SrcName,
+			dstName: name.DstPrefix,
+			ns:      n,
+		}
 		i.processInterfaceOptions(opts...)
 		if i.master != "" {
 			i.dstMaster = n.findDst(i.master, true)
@@ -594,6 +606,26 @@ func (n *networkNamespace) checkLoV6() {
 	n.loV6Enabled = enable
 }
 
+// ApplyOSTweaks applies linux configs on the sandbox
+func (n *networkNamespace) ApplyOSTweaks(types []SandboxType) {
+	for _, t := range types {
+		switch t {
+		case SandboxTypeLoadBalancer, SandboxTypeIngress:
+			kernel.ApplyOSTweaks(map[string]*kernel.OSValue{
+				// disables any special handling on port reuse of existing IPVS connection table entries
+				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L32
+				"net.ipv4.vs.conn_reuse_mode": {Value: "0", CheckFn: nil},
+				// expires connection from the IPVS connection table when the backend is not available
+				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L133
+				"net.ipv4.vs.expire_nodest_conn": {Value: "1", CheckFn: nil},
+				// expires persistent connections to destination servers with weights set to 0
+				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L151
+				"net.ipv4.vs.expire_quiescent_template": {Value: "1", CheckFn: nil},
+			})
+		}
+	}
+}
+
 func setIPv6(nspath, iface string, enable bool) error {
 	errCh := make(chan error, 1)
 	go func() {
@@ -659,23 +691,3 @@ func setIPv6(nspath, iface string, enable bool) error {
 	}()
 	return <-errCh
 }
-
-// ApplyOSTweaks applies linux configs on the sandbox
-func (n *networkNamespace) ApplyOSTweaks(types []SandboxType) {
-	for _, t := range types {
-		switch t {
-		case SandboxTypeLoadBalancer, SandboxTypeIngress:
-			kernel.ApplyOSTweaks(map[string]*kernel.OSValue{
-				// disables any special handling on port reuse of existing IPVS connection table entries
-				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L32
-				"net.ipv4.vs.conn_reuse_mode": {Value: "0", CheckFn: nil},
-				// expires connection from the IPVS connection table when the backend is not available
-				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L133
-				"net.ipv4.vs.expire_nodest_conn": {Value: "1", CheckFn: nil},
-				// expires persistent connections to destination servers with weights set to 0
-				// more info: https://github.com/torvalds/linux/blame/v5.15/Documentation/networking/ipvs-sysctl.rst#L151
-				"net.ipv4.vs.expire_quiescent_template": {Value: "1", CheckFn: nil},
-			})
-		}
-	}
-}

libnetwork/osl/sandbox.go

@@ -156,8 +156,6 @@ type Info interface {
 	// directly connected routes are stored on the particular interface they
 	// refer to.
 	StaticRoutes() []*types.StaticRoute
-
-	// TODO: Add ip tables etc.
 }
 
 // Interface represents the settings and identity of a network device. It is

libnetwork/osl/sandbox_linux_test.go

@@ -37,6 +37,7 @@ func generateRandomName(prefix string, size int) (string, error) {
 }
 
 func newKey(t *testing.T) (string, error) {
+	t.Helper()
 	name, err := generateRandomName("netns", 12)
 	if err != nil {
 		return "", err
@@ -55,67 +56,68 @@ func newKey(t *testing.T) (string, error) {
 	return name, nil
 }
 
-func newInfo(hnd *netlink.Handle, t *testing.T) (Sandbox, error) {
-	veth := &netlink.Veth{
+func newInfo(t *testing.T, hnd *netlink.Handle) (Sandbox, error) {
+	t.Helper()
+	err := hnd.LinkAdd(&netlink.Veth{
 		LinkAttrs: netlink.LinkAttrs{Name: vethName1, TxQLen: 0},
 		PeerName:  vethName2,
-	}
-	if err := hnd.LinkAdd(veth); err != nil {
+	})
+	if err != nil {
 		return nil, err
 	}
 
-	// Store the sandbox side pipe interface
-	// This is needed for cleanup on DeleteEndpoint()
-	intf1 := &nwIface{}
-	intf1.srcName = vethName2
-	intf1.dstName = sboxIfaceName
-
 	ip4, addr, err := net.ParseCIDR("192.168.1.100/24")
 	if err != nil {
 		return nil, err
 	}
-	intf1.address = addr
-	intf1.address.IP = ip4
+	addr.IP = ip4
 
 	ip6, addrv6, err := net.ParseCIDR("fe80::2/64")
 	if err != nil {
 		return nil, err
 	}
-	intf1.addressIPv6 = addrv6
-	intf1.addressIPv6.IP = ip6
+	addrv6.IP = ip6
 
 	_, route, err := net.ParseCIDR("192.168.2.1/32")
 	if err != nil {
 		return nil, err
 	}
 
-	intf1.routes = []*net.IPNet{route}
+	// Store the sandbox side pipe interface
+	// This is needed for cleanup on DeleteEndpoint()
+	intf1 := &nwIface{
+		srcName:     vethName2,
+		dstName:     sboxIfaceName,
+		address:     addr,
+		addressIPv6: addrv6,
+		routes:      []*net.IPNet{route},
+	}
 
-	intf2 := &nwIface{}
-	intf2.srcName = "testbridge"
-	intf2.dstName = sboxIfaceName
-	intf2.bridge = true
+	intf2 := &nwIface{
+		srcName: "testbridge",
+		dstName: sboxIfaceName,
+		bridge:  true,
+	}
 
-	veth = &netlink.Veth{
+	err = hnd.LinkAdd(&netlink.Veth{
 		LinkAttrs: netlink.LinkAttrs{Name: vethName3, TxQLen: 0},
 		PeerName:  vethName4,
-	}
-
-	if err := hnd.LinkAdd(veth); err != nil {
+	})
+	if err != nil {
 		return nil, err
 	}
 
-	intf3 := &nwIface{}
-	intf3.srcName = vethName4
-	intf3.dstName = sboxIfaceName
-	intf3.master = "testbridge"
-
-	info := &networkNamespace{iFaces: []*nwIface{intf1, intf2, intf3}}
-
-	info.gw = net.ParseIP("192.168.1.1")
-	info.gwv6 = net.ParseIP("fe80::1")
+	intf3 := &nwIface{
+		srcName: vethName4,
+		dstName: sboxIfaceName,
+		master:  "testbridge",
+	}
 
-	return info, nil
+	return &networkNamespace{
+		iFaces: []*nwIface{intf1, intf2, intf3},
+		gw:     net.ParseIP("192.168.1.1"),
+		gwv6:   net.ParseIP("fe80::1"),
+	}, nil
 }
 
 func verifySandbox(t *testing.T, s Sandbox, ifaceSuffixes []string) {
@@ -400,7 +402,7 @@ func TestSandboxCreate(t *testing.T) {
 		t.Fatalf("s.Key() returned %s. Expected %s", s.Key(), key)
 	}
 
-	tbox, err := newInfo(ns.NlHandle(), t)
+	tbox, err := newInfo(t, ns.NlHandle())
 	if err != nil {
 		t.Fatalf("Failed to generate new sandbox info: %v", err)
 	}
@@ -499,7 +501,7 @@ func TestAddRemoveInterface(t *testing.T) {
 		t.Fatalf("s.Key() returned %s. Expected %s", s.Key(), key)
 	}
 
-	tbox, err := newInfo(ns.NlHandle(), t)
+	tbox, err := newInfo(t, ns.NlHandle())
 	if err != nil {
 		t.Fatalf("Failed to generate new sandbox info: %v", err)
 	}