libnetwork/drvregistry: split up the registries

There is no benefit to having a single registry for both IPAM drivers
and network drivers. IPAM drivers are registered in a separate namespace
from network drivers, have separate registration methods, separate
accessor methods and do not interact with network drivers within a
DrvRegistry in any way. The only point of commonality is

    interface { GetPluginGetter() plugingetter.PluginGetter }

which is only used by the respective remote drivers and therefore should
be outside of the scope of a driver registry.

Create new, separate registry types for network drivers and IPAM
drivers, respectively. These types are "legacy-free". Neither type has
GetPluginGetter methods. The IPAMs registry does not have an
IPAMDefaultAddressSpaces method as that information can be queried
directly from the driver using its GetDefaultAddressSpaces method.
The Networks registry does not have an AddDriver method as that method
is a trivial wrapper around calling one of its arguments with its other
arguments.

Refactor DrvRegistry in terms of the new IPAMs and Networks registries
so that existing code in libnetwork and Swarmkit will continue to work.

Signed-off-by: Cory Snider <csnider@mirantis.com>

libnetwork/driverapi/driverapi.go

@@ -156,12 +156,16 @@ type JoinInfo interface {
 	AddTableEntry(tableName string, key string, value []byte) error
 }
 
+// Registerer provides a way for network drivers to be dynamically registered.
+type Registerer interface {
+	RegisterDriver(name string, driver Driver, capability Capability) error
+}
+
 // DriverCallback provides a Callback interface for Drivers into LibNetwork
 type DriverCallback interface {
+	Registerer
 	// GetPluginGetter returns the pluginv2 getter.
 	GetPluginGetter() plugingetter.PluginGetter
-	// RegisterDriver provides a way for Remote drivers to dynamically register new NetworkType and associate with a driver instance
-	RegisterDriver(name string, driver Driver, capability Capability) error
 }
 
 // Capability represents the high level capabilities of the drivers which libnetwork can make use of

libnetwork/drvregistry/drvregistry.go

@@ -1,155 +1,58 @@
 package drvregistry
 
 import (
-	"errors"
 	"fmt"
-	"strings"
-	"sync"
 
 	"github.com/docker/docker/libnetwork/driverapi"
 	"github.com/docker/docker/libnetwork/ipamapi"
-	"github.com/docker/docker/libnetwork/types"
 	"github.com/docker/docker/pkg/plugingetter"
 )
 
-type driverData struct {
-	driver     driverapi.Driver
-	capability driverapi.Capability
-}
-
-type ipamData struct {
-	driver     ipamapi.Ipam
-	capability *ipamapi.Capability
-	// default address spaces are provided by ipam driver at registration time
-	defaultLocalAddressSpace, defaultGlobalAddressSpace string
-}
-
-type driverTable map[string]*driverData
-type ipamTable map[string]*ipamData
-
 // DrvRegistry holds the registry of all network drivers and IPAM drivers that it knows about.
 type DrvRegistry struct {
-	sync.Mutex
-	drivers      driverTable
-	ipamDrivers  ipamTable
-	dfn          DriverNotifyFunc
+	Networks
+	IPAMs
 	pluginGetter plugingetter.PluginGetter
 }
 
-// Functors definition
+var _ driverapi.DriverCallback = (*DrvRegistry)(nil)
+var _ ipamapi.Callback = (*DrvRegistry)(nil)
 
 // InitFunc defines the driver initialization function signature.
 type InitFunc func(driverapi.DriverCallback, map[string]interface{}) error
 
-// IPAMWalkFunc defines the IPAM driver table walker function signature.
-type IPAMWalkFunc func(name string, driver ipamapi.Ipam, cap *ipamapi.Capability) bool
-
-// DriverWalkFunc defines the network driver table walker function signature.
-type DriverWalkFunc func(name string, driver driverapi.Driver, capability driverapi.Capability) bool
-
-// DriverNotifyFunc defines the notify function signature when a new network driver gets registered.
-type DriverNotifyFunc func(name string, driver driverapi.Driver, capability driverapi.Capability) error
-
 // Placeholder is a type for function arguments which need to be present for Swarmkit
 // to compile, but for which the only acceptable value is nil.
 type Placeholder *struct{}
 
-// New returns a new driver registry handle.
+// New returns a new legacy driver registry.
+//
+// Deprecated: use the separate [Networks] and [IPAMs] registries.
 func New(lDs, gDs Placeholder, dfn DriverNotifyFunc, ifn Placeholder, pg plugingetter.PluginGetter) (*DrvRegistry, error) {
-	r := &DrvRegistry{
-		drivers:      make(driverTable),
-		ipamDrivers:  make(ipamTable),
-		dfn:          dfn,
+	return &DrvRegistry{
+		Networks:     Networks{Notify: dfn},
 		pluginGetter: pg,
-	}
-
-	return r, nil
+	}, nil
 }
 
 // AddDriver adds a network driver to the registry.
-func (r *DrvRegistry) AddDriver(ntype string, fn InitFunc, config map[string]interface{}) error {
+//
+// Deprecated: call fn(r, config) directly.
+func (r *DrvRegistry) AddDriver(_ string, fn InitFunc, config map[string]interface{}) error {
 	return fn(r, config)
 }
 
-// WalkIPAMs walks the IPAM drivers registered in the registry and invokes the passed walk function and each one of them.
-func (r *DrvRegistry) WalkIPAMs(ifn IPAMWalkFunc) {
-	type ipamVal struct {
-		name string
-		data *ipamData
-	}
-
-	r.Lock()
-	ivl := make([]ipamVal, 0, len(r.ipamDrivers))
-	for k, v := range r.ipamDrivers {
-		ivl = append(ivl, ipamVal{name: k, data: v})
-	}
-	r.Unlock()
-
-	for _, iv := range ivl {
-		if ifn(iv.name, iv.data.driver, iv.data.capability) {
-			break
-		}
-	}
-}
-
-// WalkDrivers walks the network drivers registered in the registry and invokes the passed walk function and each one of them.
-func (r *DrvRegistry) WalkDrivers(dfn DriverWalkFunc) {
-	type driverVal struct {
-		name string
-		data *driverData
-	}
-
-	r.Lock()
-	dvl := make([]driverVal, 0, len(r.drivers))
-	for k, v := range r.drivers {
-		dvl = append(dvl, driverVal{name: k, data: v})
-	}
-	r.Unlock()
-
-	for _, dv := range dvl {
-		if dfn(dv.name, dv.data.driver, dv.data.capability) {
-			break
-		}
-	}
-}
-
-// Driver returns the actual network driver instance and its capability  which registered with the passed name.
-func (r *DrvRegistry) Driver(name string) (driverapi.Driver, *driverapi.Capability) {
-	r.Lock()
-	defer r.Unlock()
-
-	d, ok := r.drivers[name]
-	if !ok {
-		return nil, nil
-	}
-
-	return d.driver, &d.capability
-}
-
-// IPAM returns the actual IPAM driver instance and its capability which registered with the passed name.
-func (r *DrvRegistry) IPAM(name string) (ipamapi.Ipam, *ipamapi.Capability) {
-	r.Lock()
-	defer r.Unlock()
-
-	i, ok := r.ipamDrivers[name]
-	if !ok {
-		return nil, nil
-	}
-
-	return i.driver, i.capability
-}
-
 // IPAMDefaultAddressSpaces returns the default address space strings for the passed IPAM driver name.
+//
+// Deprecated: call GetDefaultAddressSpaces() on the IPAM driver.
 func (r *DrvRegistry) IPAMDefaultAddressSpaces(name string) (string, string, error) {
-	r.Lock()
-	defer r.Unlock()
+	d, _ := r.IPAM(name)
 
-	i, ok := r.ipamDrivers[name]
-	if !ok {
+	if d == nil {
 		return "", "", fmt.Errorf("ipam %s not found", name)
 	}
 
-	return i.defaultLocalAddressSpace, i.defaultGlobalAddressSpace, nil
+	return d.GetDefaultAddressSpaces()
 }
 
 // GetPluginGetter returns the plugingetter
@@ -157,65 +60,12 @@ func (r *DrvRegistry) GetPluginGetter() plugingetter.PluginGetter {
 	return r.pluginGetter
 }
 
-// RegisterDriver registers the network driver when it gets discovered.
-func (r *DrvRegistry) RegisterDriver(ntype string, driver driverapi.Driver, capability driverapi.Capability) error {
-	if strings.TrimSpace(ntype) == "" {
-		return errors.New("network type string cannot be empty")
+// Driver returns the network driver instance registered under name, and its capability.
+func (r *DrvRegistry) Driver(name string) (driverapi.Driver, *driverapi.Capability) {
+	d, c := r.Networks.Driver(name)
+
+	if c == (driverapi.Capability{}) {
+		return d, nil
 	}
-
-	r.Lock()
-	dd, ok := r.drivers[ntype]
-	r.Unlock()
-
-	if ok && dd.driver.IsBuiltIn() {
-		return driverapi.ErrActiveRegistration(ntype)
-	}
-
-	if r.dfn != nil {
-		if err := r.dfn(ntype, driver, capability); err != nil {
-			return err
-		}
-	}
-
-	dData := &driverData{driver, capability}
-
-	r.Lock()
-	r.drivers[ntype] = dData
-	r.Unlock()
-
-	return nil
-}
-
-func (r *DrvRegistry) registerIpamDriver(name string, driver ipamapi.Ipam, caps *ipamapi.Capability) error {
-	if strings.TrimSpace(name) == "" {
-		return errors.New("ipam driver name string cannot be empty")
-	}
-
-	r.Lock()
-	dd, ok := r.ipamDrivers[name]
-	r.Unlock()
-	if ok && dd.driver.IsBuiltIn() {
-		return types.ForbiddenErrorf("ipam driver %q already registered", name)
-	}
-
-	locAS, glbAS, err := driver.GetDefaultAddressSpaces()
-	if err != nil {
-		return types.InternalErrorf("ipam driver %q failed to return default address spaces: %v", name, err)
-	}
-
-	r.Lock()
-	r.ipamDrivers[name] = &ipamData{driver: driver, defaultLocalAddressSpace: locAS, defaultGlobalAddressSpace: glbAS, capability: caps}
-	r.Unlock()
-
-	return nil
-}
-
-// RegisterIpamDriver registers the IPAM driver discovered with default capabilities.
-func (r *DrvRegistry) RegisterIpamDriver(name string, driver ipamapi.Ipam) error {
-	return r.registerIpamDriver(name, driver, &ipamapi.Capability{})
-}
-
-// RegisterIpamDriverWithCapabilities registers the IPAM driver discovered with specified capabilities.
-func (r *DrvRegistry) RegisterIpamDriverWithCapabilities(name string, driver ipamapi.Ipam, caps *ipamapi.Capability) error {
-	return r.registerIpamDriver(name, driver, caps)
+	return d, &c
 }

libnetwork/drvregistry/drvregistry_test.go

@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/docker/docker/libnetwork/datastore"
-	"github.com/docker/docker/libnetwork/discoverapi"
 	"github.com/docker/docker/libnetwork/driverapi"
 	"github.com/docker/docker/libnetwork/ipamapi"
 	builtinIpam "github.com/docker/docker/libnetwork/ipams/builtin"
@@ -18,48 +17,16 @@ import (
 
 const mockDriverName = "mock-driver"
 
-type mockDriver struct{}
+type mockDriver struct {
+	driverapi.Driver
+}
+
+var mockDriverCaps = driverapi.Capability{DataScope: datastore.LocalScope}
 
 var md = mockDriver{}
 
 func mockDriverInit(reg driverapi.DriverCallback, opt map[string]interface{}) error {
-	return reg.RegisterDriver(mockDriverName, &md, driverapi.Capability{DataScope: datastore.LocalScope})
-}
-
-func (m *mockDriver) CreateNetwork(nid string, options map[string]interface{}, nInfo driverapi.NetworkInfo, ipV4Data, ipV6Data []driverapi.IPAMData) error {
-	return nil
-}
-
-func (m *mockDriver) DeleteNetwork(nid string) error {
-	return nil
-}
-
-func (m *mockDriver) CreateEndpoint(nid, eid string, ifInfo driverapi.InterfaceInfo, options map[string]interface{}) error {
-	return nil
-}
-
-func (m *mockDriver) DeleteEndpoint(nid, eid string) error {
-	return nil
-}
-
-func (m *mockDriver) EndpointOperInfo(nid, eid string) (map[string]interface{}, error) {
-	return nil, nil
-}
-
-func (m *mockDriver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo, options map[string]interface{}) error {
-	return nil
-}
-
-func (m *mockDriver) Leave(nid, eid string) error {
-	return nil
-}
-
-func (m *mockDriver) DiscoverNew(dType discoverapi.DiscoveryType, data interface{}) error {
-	return nil
-}
-
-func (m *mockDriver) DiscoverDelete(dType discoverapi.DiscoveryType, data interface{}) error {
-	return nil
+	return reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
 }
 
 func (m *mockDriver) Type() string {
@@ -70,29 +37,6 @@ func (m *mockDriver) IsBuiltIn() bool {
 	return true
 }
 
-func (m *mockDriver) ProgramExternalConnectivity(nid, eid string, options map[string]interface{}) error {
-	return nil
-}
-
-func (m *mockDriver) RevokeExternalConnectivity(nid, eid string) error {
-	return nil
-}
-
-func (m *mockDriver) NetworkAllocate(id string, option map[string]string, ipV4Data, ipV6Data []driverapi.IPAMData) (map[string]string, error) {
-	return nil, nil
-}
-
-func (m *mockDriver) NetworkFree(id string) error {
-	return nil
-}
-
-func (m *mockDriver) EventNotify(etype driverapi.EventType, nid, tableName, key string, value []byte) {
-}
-
-func (m *mockDriver) DecodeTableEntry(tablename string, key string, value []byte) (string, map[string]string) {
-	return "", nil
-}
-
 func getNew(t *testing.T) *DrvRegistry {
 	reg, err := New(nil, nil, nil, nil, nil)
 	if err != nil {

libnetwork/drvregistry/ipams.go

@@ -0,0 +1,84 @@
+package drvregistry
+
+import (
+	"errors"
+	"strings"
+	"sync"
+
+	"github.com/docker/docker/libnetwork/ipamapi"
+	"github.com/docker/docker/libnetwork/types"
+)
+
+type ipamDriver struct {
+	driver     ipamapi.Ipam
+	capability *ipamapi.Capability
+}
+
+// IPAMs is a registry of IPAM drivers. The zero value is an empty IPAM driver
+// registry, ready to use.
+type IPAMs struct {
+	mu      sync.Mutex
+	drivers map[string]ipamDriver
+}
+
+var _ ipamapi.Registerer = (*IPAMs)(nil)
+
+// IPAM returns the actual IPAM driver instance and its capability which registered with the passed name.
+func (ir *IPAMs) IPAM(name string) (ipamapi.Ipam, *ipamapi.Capability) {
+	ir.mu.Lock()
+	defer ir.mu.Unlock()
+
+	d := ir.drivers[name]
+	return d.driver, d.capability
+}
+
+// RegisterIpamDriverWithCapabilities registers the IPAM driver discovered with specified capabilities.
+func (ir *IPAMs) RegisterIpamDriverWithCapabilities(name string, driver ipamapi.Ipam, caps *ipamapi.Capability) error {
+	if strings.TrimSpace(name) == "" {
+		return errors.New("ipam driver name string cannot be empty")
+	}
+
+	ir.mu.Lock()
+	defer ir.mu.Unlock()
+
+	dd, ok := ir.drivers[name]
+	if ok && dd.driver.IsBuiltIn() {
+		return types.ForbiddenErrorf("ipam driver %q already registered", name)
+	}
+
+	if ir.drivers == nil {
+		ir.drivers = make(map[string]ipamDriver)
+	}
+	ir.drivers[name] = ipamDriver{driver: driver, capability: caps}
+
+	return nil
+}
+
+// RegisterIpamDriver registers the IPAM driver discovered with default capabilities.
+func (ir *IPAMs) RegisterIpamDriver(name string, driver ipamapi.Ipam) error {
+	return ir.RegisterIpamDriverWithCapabilities(name, driver, &ipamapi.Capability{})
+}
+
+// IPAMWalkFunc defines the IPAM driver table walker function signature.
+type IPAMWalkFunc func(name string, driver ipamapi.Ipam, cap *ipamapi.Capability) bool
+
+// WalkIPAMs walks the IPAM drivers registered in the registry and invokes the passed walk function and each one of them.
+func (ir *IPAMs) WalkIPAMs(ifn IPAMWalkFunc) {
+	type ipamVal struct {
+		name string
+		data ipamDriver
+	}
+
+	ir.mu.Lock()
+	ivl := make([]ipamVal, 0, len(ir.drivers))
+	for k, v := range ir.drivers {
+		ivl = append(ivl, ipamVal{name: k, data: v})
+	}
+	ir.mu.Unlock()
+
+	for _, iv := range ivl {
+		if ifn(iv.name, iv.data.driver, iv.data.capability) {
+			break
+		}
+	}
+}

libnetwork/drvregistry/ipams_test.go

@@ -0,0 +1,51 @@
+package drvregistry
+
+import (
+	"runtime"
+	"sort"
+	"testing"
+
+	"github.com/docker/docker/libnetwork/ipamapi"
+	builtinIpam "github.com/docker/docker/libnetwork/ipams/builtin"
+	nullIpam "github.com/docker/docker/libnetwork/ipams/null"
+	remoteIpam "github.com/docker/docker/libnetwork/ipams/remote"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func getNewIPAMs(t *testing.T) *IPAMs {
+	r := &IPAMs{}
+
+	assert.Assert(t, builtinIpam.Register(r))
+	assert.Assert(t, remoteIpam.Register(r, nil))
+	assert.Assert(t, nullIpam.Register(r))
+
+	return r
+}
+
+func TestIPAMs(t *testing.T) {
+	t.Run("IPAM", func(t *testing.T) {
+		reg := getNewIPAMs(t)
+
+		i, cap := reg.IPAM("default")
+		assert.Check(t, i != nil)
+		assert.Check(t, cap != nil)
+	})
+
+	t.Run("WalkIPAMs", func(t *testing.T) {
+		reg := getNewIPAMs(t)
+
+		ipams := make([]string, 0, 2)
+		reg.WalkIPAMs(func(name string, driver ipamapi.Ipam, cap *ipamapi.Capability) bool {
+			ipams = append(ipams, name)
+			return false
+		})
+
+		sort.Strings(ipams)
+		expected := []string{"default", "null"}
+		if runtime.GOOS == "windows" {
+			expected = append(expected, "windows")
+		}
+		assert.Check(t, is.DeepEqual(ipams, expected))
+	})
+}

libnetwork/drvregistry/networks.go

@@ -0,0 +1,93 @@
+package drvregistry
+
+import (
+	"errors"
+	"strings"
+	"sync"
+
+	"github.com/docker/docker/libnetwork/driverapi"
+)
+
+// DriverWalkFunc defines the network driver table walker function signature.
+type DriverWalkFunc func(name string, driver driverapi.Driver, capability driverapi.Capability) bool
+
+// DriverNotifyFunc defines the notify function signature when a new network driver gets registered.
+type DriverNotifyFunc func(name string, driver driverapi.Driver, capability driverapi.Capability) error
+
+type driverData struct {
+	driver     driverapi.Driver
+	capability driverapi.Capability
+}
+
+// Networks is a registry of network drivers. The zero value is an empty network
+// driver registry, ready to use.
+type Networks struct {
+	// Notify is called whenever a network driver is registered.
+	Notify DriverNotifyFunc
+
+	mu      sync.Mutex
+	drivers map[string]driverData
+}
+
+var _ driverapi.Registerer = (*Networks)(nil)
+
+// WalkDrivers walks the network drivers registered in the registry and invokes the passed walk function and each one of them.
+func (nr *Networks) WalkDrivers(dfn DriverWalkFunc) {
+	type driverVal struct {
+		name string
+		data driverData
+	}
+
+	nr.mu.Lock()
+	dvl := make([]driverVal, 0, len(nr.drivers))
+	for k, v := range nr.drivers {
+		dvl = append(dvl, driverVal{name: k, data: v})
+	}
+	nr.mu.Unlock()
+
+	for _, dv := range dvl {
+		if dfn(dv.name, dv.data.driver, dv.data.capability) {
+			break
+		}
+	}
+}
+
+// Driver returns the network driver instance registered under name, and its capability.
+func (nr *Networks) Driver(name string) (driverapi.Driver, driverapi.Capability) {
+	nr.mu.Lock()
+	defer nr.mu.Unlock()
+
+	d := nr.drivers[name]
+	return d.driver, d.capability
+}
+
+// RegisterDriver registers the network driver with nr.
+func (nr *Networks) RegisterDriver(ntype string, driver driverapi.Driver, capability driverapi.Capability) error {
+	if strings.TrimSpace(ntype) == "" {
+		return errors.New("network type string cannot be empty")
+	}
+
+	nr.mu.Lock()
+	dd, ok := nr.drivers[ntype]
+	nr.mu.Unlock()
+
+	if ok && dd.driver.IsBuiltIn() {
+		return driverapi.ErrActiveRegistration(ntype)
+	}
+
+	if nr.Notify != nil {
+		if err := nr.Notify(ntype, driver, capability); err != nil {
+			return err
+		}
+	}
+
+	nr.mu.Lock()
+	defer nr.mu.Unlock()
+
+	if nr.drivers == nil {
+		nr.drivers = make(map[string]driverData)
+	}
+	nr.drivers[ntype] = driverData{driver: driver, capability: capability}
+
+	return nil
+}

libnetwork/drvregistry/networks_test.go

@@ -0,0 +1,51 @@
+package drvregistry
+
+import (
+	"testing"
+
+	"github.com/docker/docker/libnetwork/driverapi"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestNetworks(t *testing.T) {
+	t.Run("RegisterDriver", func(t *testing.T) {
+		var reg Networks
+		err := reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
+		assert.NilError(t, err)
+	})
+
+	t.Run("RegisterDuplicateDriver", func(t *testing.T) {
+		var reg Networks
+		err := reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
+		assert.NilError(t, err)
+
+		// Try adding the same driver
+		err = reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
+		assert.Check(t, is.ErrorContains(err, ""))
+	})
+
+	t.Run("Driver", func(t *testing.T) {
+		var reg Networks
+		err := reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
+		assert.NilError(t, err)
+
+		d, cap := reg.Driver(mockDriverName)
+		assert.Check(t, d != nil)
+		assert.Check(t, is.DeepEqual(cap, mockDriverCaps))
+	})
+
+	t.Run("WalkDrivers", func(t *testing.T) {
+		var reg Networks
+		err := reg.RegisterDriver(mockDriverName, &md, mockDriverCaps)
+		assert.NilError(t, err)
+
+		var driverName string
+		reg.WalkDrivers(func(name string, driver driverapi.Driver, capability driverapi.Capability) bool {
+			driverName = name
+			return false
+		})
+
+		assert.Check(t, is.Equal(driverName, mockDriverName))
+	})
+}