From d478e13639980aba8af3a25cb9cbfefb19eaf1f9 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 24 Jan 2023 14:26:03 -0500 Subject: [PATCH] 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 --- libnetwork/controller.go | 2 +- libnetwork/drivers_ipam.go | 16 ++++++------- libnetwork/drvregistry/drvregistry_test.go | 12 +++++----- libnetwork/ipamapi/contract.go | 17 +++++++++----- libnetwork/ipams/builtin/builtin.go | 25 +++------------------ libnetwork/ipams/builtin/builtin_unix.go | 23 +++++++++++++++++-- libnetwork/ipams/builtin/builtin_windows.go | 24 +++++++++++++++----- libnetwork/ipams/null/null.go | 11 +++++++-- libnetwork/ipams/remote/remote.go | 10 +++++++-- libnetwork/ipams/windowsipam/windowsipam.go | 8 +++---- 10 files changed, 90 insertions(+), 58 deletions(-) diff --git a/libnetwork/controller.go b/libnetwork/controller.go index 1baece920d..fdc62d8ffc 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -149,7 +149,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) { } } - if err = initIPAMDrivers(drvRegistry, nil, nil, c.cfg.DefaultAddressPool); err != nil { + if err = initIPAMDrivers(drvRegistry, c.cfg.PluginGetter, c.cfg.DefaultAddressPool); err != nil { return nil, err } diff --git a/libnetwork/drivers_ipam.go b/libnetwork/drivers_ipam.go index 66bab7ce1d..1c468902b3 100644 --- a/libnetwork/drivers_ipam.go +++ b/libnetwork/drivers_ipam.go @@ -1,30 +1,30 @@ package libnetwork import ( - "github.com/docker/docker/libnetwork/drvregistry" "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" "github.com/docker/docker/libnetwork/ipamutils" + "github.com/docker/docker/pkg/plugingetter" ) -func initIPAMDrivers(r *drvregistry.DrvRegistry, lDs, gDs interface{}, addressPool []*ipamutils.NetworkToSplit) error { +func initIPAMDrivers(r ipamapi.Registerer, pg plugingetter.PluginGetter, addressPool []*ipamutils.NetworkToSplit) error { // TODO: pass address pools as arguments to builtinIpam.Init instead of // indirectly through global mutable state. Swarmkit references that // function so changing its signature breaks the build. if err := builtinIpam.SetDefaultIPAddressPool(addressPool); err != nil { return err } - for _, fn := range [](func(ipamapi.Callback, interface{}, interface{}) error){ - builtinIpam.Init, - remoteIpam.Init, - nullIpam.Init, + + for _, fn := range [](func(ipamapi.Registerer) error){ + builtinIpam.Register, + nullIpam.Register, } { - if err := fn(r, lDs, gDs); err != nil { + if err := fn(r); err != nil { return err } } - return nil + return remoteIpam.Register(r, pg) } diff --git a/libnetwork/drvregistry/drvregistry_test.go b/libnetwork/drvregistry/drvregistry_test.go index ead157ef5e..f224d4e95e 100644 --- a/libnetwork/drvregistry/drvregistry_test.go +++ b/libnetwork/drvregistry/drvregistry_test.go @@ -99,20 +99,20 @@ func getNew(t *testing.T) *DrvRegistry { t.Fatal(err) } - err = initIPAMDrivers(reg, nil, nil) + err = initIPAMDrivers(reg) if err != nil { t.Fatal(err) } return reg } -func initIPAMDrivers(r *DrvRegistry, lDs, gDs interface{}) error { +func initIPAMDrivers(r *DrvRegistry) error { for _, fn := range [](func(ipamapi.Callback, interface{}, interface{}) error){ - builtinIpam.Init, - remoteIpam.Init, - nullIpam.Init, + builtinIpam.Init, //nolint:staticcheck + remoteIpam.Init, //nolint:staticcheck + nullIpam.Init, //nolint:staticcheck } { - if err := fn(r, lDs, gDs); err != nil { + if err := fn(r, nil, nil); err != nil { return err } } diff --git a/libnetwork/ipamapi/contract.go b/libnetwork/ipamapi/contract.go index 5e75bdfbd4..36f1b5a9c4 100644 --- a/libnetwork/ipamapi/contract.go +++ b/libnetwork/ipamapi/contract.go @@ -21,14 +21,21 @@ const ( RequestAddressType = "RequestAddressType" ) -// Callback provides a Callback interface for registering an IPAM instance into LibNetwork +// Registerer provides a callback interface for registering IPAM instances into libnetwork. +type Registerer interface { + // RegisterIpamDriver provides a way for drivers to dynamically register with libnetwork + RegisterIpamDriver(name string, driver Ipam) error + // RegisterIpamDriverWithCapabilities provides a way for drivers to dynamically register with libnetwork and specify capabilities + RegisterIpamDriverWithCapabilities(name string, driver Ipam, capability *Capability) error +} + +// Callback is a legacy interface for registering an IPAM instance into LibNetwork. +// +// The narrower [Registerer] interface is preferred for new code. type Callback interface { + Registerer // GetPluginGetter returns the pluginv2 getter. GetPluginGetter() plugingetter.PluginGetter - // RegisterIpamDriver provides a way for Remote drivers to dynamically register with libnetwork - RegisterIpamDriver(name string, driver Ipam) error - // RegisterIpamDriverWithCapabilities provides a way for Remote drivers to dynamically register with libnetwork and specify capabilities - RegisterIpamDriverWithCapabilities(name string, driver Ipam, capability *Capability) error } // Well-known errors returned by IPAM diff --git a/libnetwork/ipams/builtin/builtin.go b/libnetwork/ipams/builtin/builtin.go index 2529105b21..b72bdd07b2 100644 --- a/libnetwork/ipams/builtin/builtin.go +++ b/libnetwork/ipams/builtin/builtin.go @@ -1,10 +1,8 @@ package builtin import ( - "errors" "net" - "github.com/docker/docker/libnetwork/datastore" "github.com/docker/docker/libnetwork/ipam" "github.com/docker/docker/libnetwork/ipamapi" "github.com/docker/docker/libnetwork/ipamutils" @@ -15,25 +13,8 @@ var ( defaultAddressPool []*net.IPNet ) -// initBuiltin registers the built-in ipam service with libnetwork -func initBuiltin(ic ipamapi.Callback, l, g interface{}) error { - var ( - ok bool - localDs, globalDs datastore.DataStore - ) - - if l != nil { - if localDs, ok = l.(datastore.DataStore); !ok { - return errors.New("incorrect local datastore passed to built-in ipam init") - } - } - - if g != nil { - if globalDs, ok = g.(datastore.DataStore); !ok { - return errors.New("incorrect global datastore passed to built-in ipam init") - } - } - +// registerBuiltin registers the built-in ipam driver with libnetwork. +func registerBuiltin(ic ipamapi.Registerer) error { var localAddressPool []*net.IPNet if len(defaultAddressPool) > 0 { localAddressPool = append([]*net.IPNet(nil), defaultAddressPool...) @@ -41,7 +22,7 @@ func initBuiltin(ic ipamapi.Callback, l, g interface{}) error { localAddressPool = ipamutils.GetLocalScopeDefaultNetworks() } - a, err := ipam.NewAllocator(localDs, globalDs, localAddressPool, ipamutils.GetGlobalScopeDefaultNetworks()) + a, err := ipam.NewAllocator(nil, nil, localAddressPool, ipamutils.GetGlobalScopeDefaultNetworks()) if err != nil { return err } diff --git a/libnetwork/ipams/builtin/builtin_unix.go b/libnetwork/ipams/builtin/builtin_unix.go index 03a7bfbd53..8cf37499dc 100644 --- a/libnetwork/ipams/builtin/builtin_unix.go +++ b/libnetwork/ipams/builtin/builtin_unix.go @@ -3,9 +3,28 @@ package builtin -import "github.com/docker/docker/libnetwork/ipamapi" +import ( + "errors" + + "github.com/docker/docker/libnetwork/ipamapi" +) // Init registers the built-in ipam service with libnetwork +// +// Deprecated: use [Register]. func Init(ic ipamapi.Callback, l, g interface{}) error { - return initBuiltin(ic, l, g) + if l != nil { + return errors.New("non-nil local datastore passed to built-in ipam init") + } + + if g != nil { + return errors.New("non-nil global datastore passed to built-in ipam init") + } + + return Register(ic) +} + +// Register registers the built-in ipam service with libnetwork. +func Register(r ipamapi.Registerer) error { + return registerBuiltin(r) } diff --git a/libnetwork/ipams/builtin/builtin_windows.go b/libnetwork/ipams/builtin/builtin_windows.go index 497e57cfc0..84d2e4ae89 100644 --- a/libnetwork/ipams/builtin/builtin_windows.go +++ b/libnetwork/ipams/builtin/builtin_windows.go @@ -4,18 +4,32 @@ package builtin import ( + "errors" + "github.com/docker/docker/libnetwork/ipamapi" "github.com/docker/docker/libnetwork/ipams/windowsipam" ) -// Init registers the built-in ipam service with libnetwork +// Init registers the built-in ipam services with libnetwork. +// +// Deprecated: use [Register]. func Init(ic ipamapi.Callback, l, g interface{}) error { - initFunc := windowsipam.GetInit(windowsipam.DefaultIPAM) + if l != nil { + return errors.New("non-nil local datastore passed to built-in ipam init") + } - err := initBuiltin(ic, l, g) - if err != nil { + if g != nil { + return errors.New("non-nil global datastore passed to built-in ipam init") + } + + return Register(ic) +} + +// Register registers the built-in ipam services with libnetwork. +func Register(r ipamapi.Registerer) error { + if err := registerBuiltin(r); err != nil { return err } - return initFunc(ic, l, g) + return windowsipam.Register(windowsipam.DefaultIPAM, r) } diff --git a/libnetwork/ipams/null/null.go b/libnetwork/ipams/null/null.go index 3d8a028944..91201075dc 100644 --- a/libnetwork/ipams/null/null.go +++ b/libnetwork/ipams/null/null.go @@ -69,7 +69,14 @@ func (a *allocator) IsBuiltIn() bool { return true } -// Init registers a remote ipam when its plugin is activated +// Init registers the null ipam driver with ic. +// +// Deprecated: use [Register]. func Init(ic ipamapi.Callback, l, g interface{}) error { - return ic.RegisterIpamDriver(ipamapi.NullIPAM, &allocator{}) + return Register(ic) +} + +// Register registers the null ipam driver with r. +func Register(r ipamapi.Registerer) error { + return r.RegisterIpamDriver(ipamapi.NullIPAM, &allocator{}) } diff --git a/libnetwork/ipams/remote/remote.go b/libnetwork/ipams/remote/remote.go index 4b8c7c5014..9ce06c55c3 100644 --- a/libnetwork/ipams/remote/remote.go +++ b/libnetwork/ipams/remote/remote.go @@ -30,9 +30,15 @@ func newAllocator(name string, client *plugins.Client) ipamapi.Ipam { return a } -// Init registers a remote ipam when its plugin is activated +// Init registers a remote ipam when its plugin is activated. +// +// Deprecated: use [Register]. func Init(cb ipamapi.Callback, l, g interface{}) error { + return Register(cb, cb.GetPluginGetter()) +} +// Register registers a remote ipam when its plugin is activated. +func Register(cb ipamapi.Registerer, pg plugingetter.PluginGetter) error { newPluginHandler := func(name string, client *plugins.Client) { a := newAllocator(name, client) if cps, err := a.(*allocator).getCapabilities(); err == nil { @@ -50,7 +56,7 @@ func Init(cb ipamapi.Callback, l, g interface{}) error { // Unit test code is unaware of a true PluginStore. So we fall back to v1 plugins. handleFunc := plugins.Handle - if pg := cb.GetPluginGetter(); pg != nil { + if pg != nil { handleFunc = pg.Handle activePlugins := pg.GetAllManagedPluginsByCap(ipamapi.PluginEndpointType) for _, ap := range activePlugins { diff --git a/libnetwork/ipams/windowsipam/windowsipam.go b/libnetwork/ipams/windowsipam/windowsipam.go index cdca945026..24455f71b6 100644 --- a/libnetwork/ipams/windowsipam/windowsipam.go +++ b/libnetwork/ipams/windowsipam/windowsipam.go @@ -24,11 +24,9 @@ var ( type allocator struct { } -// GetInit registers the built-in ipam service with libnetwork -func GetInit(ipamName string) func(ic ipamapi.Callback, l, g interface{}) error { - return func(ic ipamapi.Callback, l, g interface{}) error { - return ic.RegisterIpamDriver(ipamName, &allocator{}) - } +// Register registers the built-in ipam service with libnetwork +func Register(ipamName string, r ipamapi.Registerer) error { + return r.RegisterIpamDriver(ipamName, &allocator{}) } func (a *allocator) GetDefaultAddressSpaces() (string, string, error) {