Browse code

libnet: un-plumb datastores from IPAM inits

The datastore arguments to the IPAM driver Init() functions are always
nil, even in Swarmkit. The only IPAM driver which consumed the
datastores was builtin; all others (null, remote, windowsipam) simply
ignored it. As the signatures of the IPAM driver init functions cannot
be changed without breaking the Swarmkit build, they have to be left
with the same signatures for the time being. Assert that nil datastores
are always passed into the builtin IPAM driver's init function so that
there is no ambiguity the datastores are no longer respected.

Add new Register functions for the IPAM drivers which are free from the
legacy baggage of the Init functions. (The legacy Init functions can be
removed once Swarmkit is migrated to using the Register functions.) As
the remote IPAM driver is the only one which depends on a PluginGetter,
pass it in explicitly as an argument to Register. The other IPAM drivers
should not be forced to depend on a GetPluginGetter() method they do not
use (Interface Segregation Principle).

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

Cory Snider authored on 2023/01/25 04:26:03
Showing 10 changed files
... ...
@@ -149,7 +149,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) {
149 149
 		}
150 150
 	}
151 151
 
152
-	if err = initIPAMDrivers(drvRegistry, nil, nil, c.cfg.DefaultAddressPool); err != nil {
152
+	if err = initIPAMDrivers(drvRegistry, c.cfg.PluginGetter, c.cfg.DefaultAddressPool); err != nil {
153 153
 		return nil, err
154 154
 	}
155 155
 
... ...
@@ -1,30 +1,30 @@
1 1
 package libnetwork
2 2
 
3 3
 import (
4
-	"github.com/docker/docker/libnetwork/drvregistry"
5 4
 	"github.com/docker/docker/libnetwork/ipamapi"
6 5
 	builtinIpam "github.com/docker/docker/libnetwork/ipams/builtin"
7 6
 	nullIpam "github.com/docker/docker/libnetwork/ipams/null"
8 7
 	remoteIpam "github.com/docker/docker/libnetwork/ipams/remote"
9 8
 	"github.com/docker/docker/libnetwork/ipamutils"
9
+	"github.com/docker/docker/pkg/plugingetter"
10 10
 )
11 11
 
12
-func initIPAMDrivers(r *drvregistry.DrvRegistry, lDs, gDs interface{}, addressPool []*ipamutils.NetworkToSplit) error {
12
+func initIPAMDrivers(r ipamapi.Registerer, pg plugingetter.PluginGetter, addressPool []*ipamutils.NetworkToSplit) error {
13 13
 	// TODO: pass address pools as arguments to builtinIpam.Init instead of
14 14
 	// indirectly through global mutable state. Swarmkit references that
15 15
 	// function so changing its signature breaks the build.
16 16
 	if err := builtinIpam.SetDefaultIPAddressPool(addressPool); err != nil {
17 17
 		return err
18 18
 	}
19
-	for _, fn := range [](func(ipamapi.Callback, interface{}, interface{}) error){
20
-		builtinIpam.Init,
21
-		remoteIpam.Init,
22
-		nullIpam.Init,
19
+
20
+	for _, fn := range [](func(ipamapi.Registerer) error){
21
+		builtinIpam.Register,
22
+		nullIpam.Register,
23 23
 	} {
24
-		if err := fn(r, lDs, gDs); err != nil {
24
+		if err := fn(r); err != nil {
25 25
 			return err
26 26
 		}
27 27
 	}
28 28
 
29
-	return nil
29
+	return remoteIpam.Register(r, pg)
30 30
 }
... ...
@@ -99,20 +99,20 @@ func getNew(t *testing.T) *DrvRegistry {
99 99
 		t.Fatal(err)
100 100
 	}
101 101
 
102
-	err = initIPAMDrivers(reg, nil, nil)
102
+	err = initIPAMDrivers(reg)
103 103
 	if err != nil {
104 104
 		t.Fatal(err)
105 105
 	}
106 106
 	return reg
107 107
 }
108 108
 
109
-func initIPAMDrivers(r *DrvRegistry, lDs, gDs interface{}) error {
109
+func initIPAMDrivers(r *DrvRegistry) error {
110 110
 	for _, fn := range [](func(ipamapi.Callback, interface{}, interface{}) error){
111
-		builtinIpam.Init,
112
-		remoteIpam.Init,
113
-		nullIpam.Init,
111
+		builtinIpam.Init, //nolint:staticcheck
112
+		remoteIpam.Init,  //nolint:staticcheck
113
+		nullIpam.Init,    //nolint:staticcheck
114 114
 	} {
115
-		if err := fn(r, lDs, gDs); err != nil {
115
+		if err := fn(r, nil, nil); err != nil {
116 116
 			return err
117 117
 		}
118 118
 	}
... ...
@@ -21,14 +21,21 @@ const (
21 21
 	RequestAddressType = "RequestAddressType"
22 22
 )
23 23
 
24
-// Callback provides a Callback interface for registering an IPAM instance into LibNetwork
24
+// Registerer provides a callback interface for registering IPAM instances into libnetwork.
25
+type Registerer interface {
26
+	// RegisterIpamDriver provides a way for drivers to dynamically register with libnetwork
27
+	RegisterIpamDriver(name string, driver Ipam) error
28
+	// RegisterIpamDriverWithCapabilities provides a way for drivers to dynamically register with libnetwork and specify capabilities
29
+	RegisterIpamDriverWithCapabilities(name string, driver Ipam, capability *Capability) error
30
+}
31
+
32
+// Callback is a legacy interface for registering an IPAM instance into LibNetwork.
33
+//
34
+// The narrower [Registerer] interface is preferred for new code.
25 35
 type Callback interface {
36
+	Registerer
26 37
 	// GetPluginGetter returns the pluginv2 getter.
27 38
 	GetPluginGetter() plugingetter.PluginGetter
28
-	// RegisterIpamDriver provides a way for Remote drivers to dynamically register with libnetwork
29
-	RegisterIpamDriver(name string, driver Ipam) error
30
-	// RegisterIpamDriverWithCapabilities provides a way for Remote drivers to dynamically register with libnetwork and specify capabilities
31
-	RegisterIpamDriverWithCapabilities(name string, driver Ipam, capability *Capability) error
32 39
 }
33 40
 
34 41
 // Well-known errors returned by IPAM
... ...
@@ -1,10 +1,8 @@
1 1
 package builtin
2 2
 
3 3
 import (
4
-	"errors"
5 4
 	"net"
6 5
 
7
-	"github.com/docker/docker/libnetwork/datastore"
8 6
 	"github.com/docker/docker/libnetwork/ipam"
9 7
 	"github.com/docker/docker/libnetwork/ipamapi"
10 8
 	"github.com/docker/docker/libnetwork/ipamutils"
... ...
@@ -15,25 +13,8 @@ var (
15 15
 	defaultAddressPool []*net.IPNet
16 16
 )
17 17
 
18
-// initBuiltin registers the built-in ipam service with libnetwork
19
-func initBuiltin(ic ipamapi.Callback, l, g interface{}) error {
20
-	var (
21
-		ok                bool
22
-		localDs, globalDs datastore.DataStore
23
-	)
24
-
25
-	if l != nil {
26
-		if localDs, ok = l.(datastore.DataStore); !ok {
27
-			return errors.New("incorrect local datastore passed to built-in ipam init")
28
-		}
29
-	}
30
-
31
-	if g != nil {
32
-		if globalDs, ok = g.(datastore.DataStore); !ok {
33
-			return errors.New("incorrect global datastore passed to built-in ipam init")
34
-		}
35
-	}
36
-
18
+// registerBuiltin registers the built-in ipam driver with libnetwork.
19
+func registerBuiltin(ic ipamapi.Registerer) error {
37 20
 	var localAddressPool []*net.IPNet
38 21
 	if len(defaultAddressPool) > 0 {
39 22
 		localAddressPool = append([]*net.IPNet(nil), defaultAddressPool...)
... ...
@@ -41,7 +22,7 @@ func initBuiltin(ic ipamapi.Callback, l, g interface{}) error {
41 41
 		localAddressPool = ipamutils.GetLocalScopeDefaultNetworks()
42 42
 	}
43 43
 
44
-	a, err := ipam.NewAllocator(localDs, globalDs, localAddressPool, ipamutils.GetGlobalScopeDefaultNetworks())
44
+	a, err := ipam.NewAllocator(nil, nil, localAddressPool, ipamutils.GetGlobalScopeDefaultNetworks())
45 45
 	if err != nil {
46 46
 		return err
47 47
 	}
... ...
@@ -3,9 +3,28 @@
3 3
 
4 4
 package builtin
5 5
 
6
-import "github.com/docker/docker/libnetwork/ipamapi"
6
+import (
7
+	"errors"
8
+
9
+	"github.com/docker/docker/libnetwork/ipamapi"
10
+)
7 11
 
8 12
 // Init registers the built-in ipam service with libnetwork
13
+//
14
+// Deprecated: use [Register].
9 15
 func Init(ic ipamapi.Callback, l, g interface{}) error {
10
-	return initBuiltin(ic, l, g)
16
+	if l != nil {
17
+		return errors.New("non-nil local datastore passed to built-in ipam init")
18
+	}
19
+
20
+	if g != nil {
21
+		return errors.New("non-nil global datastore passed to built-in ipam init")
22
+	}
23
+
24
+	return Register(ic)
25
+}
26
+
27
+// Register registers the built-in ipam service with libnetwork.
28
+func Register(r ipamapi.Registerer) error {
29
+	return registerBuiltin(r)
11 30
 }
... ...
@@ -4,18 +4,32 @@
4 4
 package builtin
5 5
 
6 6
 import (
7
+	"errors"
8
+
7 9
 	"github.com/docker/docker/libnetwork/ipamapi"
8 10
 	"github.com/docker/docker/libnetwork/ipams/windowsipam"
9 11
 )
10 12
 
11
-// Init registers the built-in ipam service with libnetwork
13
+// Init registers the built-in ipam services with libnetwork.
14
+//
15
+// Deprecated: use [Register].
12 16
 func Init(ic ipamapi.Callback, l, g interface{}) error {
13
-	initFunc := windowsipam.GetInit(windowsipam.DefaultIPAM)
17
+	if l != nil {
18
+		return errors.New("non-nil local datastore passed to built-in ipam init")
19
+	}
20
+
21
+	if g != nil {
22
+		return errors.New("non-nil global datastore passed to built-in ipam init")
23
+	}
24
+
25
+	return Register(ic)
26
+}
14 27
 
15
-	err := initBuiltin(ic, l, g)
16
-	if err != nil {
28
+// Register registers the built-in ipam services with libnetwork.
29
+func Register(r ipamapi.Registerer) error {
30
+	if err := registerBuiltin(r); err != nil {
17 31
 		return err
18 32
 	}
19 33
 
20
-	return initFunc(ic, l, g)
34
+	return windowsipam.Register(windowsipam.DefaultIPAM, r)
21 35
 }
... ...
@@ -69,7 +69,14 @@ func (a *allocator) IsBuiltIn() bool {
69 69
 	return true
70 70
 }
71 71
 
72
-// Init registers a remote ipam when its plugin is activated
72
+// Init registers the null ipam driver with ic.
73
+//
74
+// Deprecated: use [Register].
73 75
 func Init(ic ipamapi.Callback, l, g interface{}) error {
74
-	return ic.RegisterIpamDriver(ipamapi.NullIPAM, &allocator{})
76
+	return Register(ic)
77
+}
78
+
79
+// Register registers the null ipam driver with r.
80
+func Register(r ipamapi.Registerer) error {
81
+	return r.RegisterIpamDriver(ipamapi.NullIPAM, &allocator{})
75 82
 }
... ...
@@ -30,9 +30,15 @@ func newAllocator(name string, client *plugins.Client) ipamapi.Ipam {
30 30
 	return a
31 31
 }
32 32
 
33
-// Init registers a remote ipam when its plugin is activated
33
+// Init registers a remote ipam when its plugin is activated.
34
+//
35
+// Deprecated: use [Register].
34 36
 func Init(cb ipamapi.Callback, l, g interface{}) error {
37
+	return Register(cb, cb.GetPluginGetter())
38
+}
35 39
 
40
+// Register registers a remote ipam when its plugin is activated.
41
+func Register(cb ipamapi.Registerer, pg plugingetter.PluginGetter) error {
36 42
 	newPluginHandler := func(name string, client *plugins.Client) {
37 43
 		a := newAllocator(name, client)
38 44
 		if cps, err := a.(*allocator).getCapabilities(); err == nil {
... ...
@@ -50,7 +56,7 @@ func Init(cb ipamapi.Callback, l, g interface{}) error {
50 50
 
51 51
 	// Unit test code is unaware of a true PluginStore. So we fall back to v1 plugins.
52 52
 	handleFunc := plugins.Handle
53
-	if pg := cb.GetPluginGetter(); pg != nil {
53
+	if pg != nil {
54 54
 		handleFunc = pg.Handle
55 55
 		activePlugins := pg.GetAllManagedPluginsByCap(ipamapi.PluginEndpointType)
56 56
 		for _, ap := range activePlugins {
... ...
@@ -24,11 +24,9 @@ var (
24 24
 type allocator struct {
25 25
 }
26 26
 
27
-// GetInit registers the built-in ipam service with libnetwork
28
-func GetInit(ipamName string) func(ic ipamapi.Callback, l, g interface{}) error {
29
-	return func(ic ipamapi.Callback, l, g interface{}) error {
30
-		return ic.RegisterIpamDriver(ipamName, &allocator{})
31
-	}
27
+// Register registers the built-in ipam service with libnetwork
28
+func Register(ipamName string, r ipamapi.Registerer) error {
29
+	return r.RegisterIpamDriver(ipamName, &allocator{})
32 30
 }
33 31
 
34 32
 func (a *allocator) GetDefaultAddressSpaces() (string, string, error) {