libnet: pass store as an arg to netdrivers

Before that change, we were passing the datastore to network drivers
through a `map[string]interface{}`. Then, each driver that needed the
store would cast the datastore to the correct type.

This was not a good design, as it was not clear which drivers were using
the store and which were not. Not all unit tests were passing the store,
leading to logs about uninitialized store being written.

This change makes the store a parameter of the `RegisterX` functions.
All unit tests are now passing a valid datastore to the drivers. A new
testutil func is added for that purpose.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This commit is contained in:
Albin Kerouanton
2024-12-20 15:29:22 +01:00
parent 40f58b3492
commit e5bf6d8ba0
20 changed files with 135 additions and 134 deletions

View File

@@ -0,0 +1,20 @@
package storeutils
import (
"testing"
"github.com/docker/docker/libnetwork/datastore"
"gotest.tools/v3/assert"
)
// NewTempStore creates a new temporary libnetwork store for testing purposes.
// The store is created in a temporary directory that is cleaned up when the
// test finishes.
func NewTempStore(t *testing.T) *datastore.Store {
t.Helper()
ds, err := datastore.New(t.TempDir(), "libnetwork")
assert.NilError(t, err)
return ds
}

View File

@@ -133,7 +133,7 @@ func New(cfgOptions ...config.Option) (*Controller, error) {
return nil, err
}
if err := registerNetworkDrivers(&c.drvRegistry, c.makeDriverConfig); err != nil {
if err := registerNetworkDrivers(&c.drvRegistry, c.store, c.makeDriverConfig); err != nil {
return nil, err
}
@@ -333,9 +333,7 @@ func (c *Controller) makeDriverConfig(ntype string) map[string]interface{} {
return nil
}
cfg := map[string]interface{}{
netlabel.LocalKVClient: c.store,
}
cfg := map[string]interface{}{}
for _, label := range c.cfg.Labels {
key, val, _ := strings.Cut(label, "=")
if !strings.HasPrefix(key, netlabel.DriverPrefix+"."+ntype) {

View File

@@ -167,15 +167,16 @@ const (
)
// New constructs a new bridge driver
func newDriver() *driver {
func newDriver(store *datastore.Store) *driver {
return &driver{
store: store,
networks: map[string]*bridgeNetwork{},
}
}
// Register registers a new instance of bridge driver.
func Register(r driverapi.Registerer, config map[string]interface{}) error {
d := newDriver()
func Register(r driverapi.Registerer, store *datastore.Store, config map[string]interface{}) error {
d := newDriver(store)
if err := d.configure(config); err != nil {
return err
}
@@ -570,7 +571,7 @@ func (d *driver) configure(option map[string]interface{}) error {
d.config = config
d.Unlock()
return d.initStore(option)
return d.initStore()
}
func setupHashNetIpset(name string, family uint8) error {

View File

@@ -13,6 +13,7 @@ import (
"github.com/docker/docker/internal/nlwrap"
"github.com/docker/docker/internal/testutils/netnsutils"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/internal/netiputil"
"github.com/docker/docker/libnetwork/ipamapi"
@@ -243,7 +244,7 @@ func getIPv6Data(t *testing.T) []driverapi.IPAMData {
func TestCreateFullOptions(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
config := &configuration{
EnableIPForwarding: true,
@@ -298,7 +299,7 @@ func TestCreateFullOptions(t *testing.T) {
func TestCreateNoConfig(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
netconfig := &networkConfiguration{BridgeName: DefaultBridgeName, EnableIPv4: true}
genericOption := make(map[string]interface{})
@@ -311,7 +312,7 @@ func TestCreateNoConfig(t *testing.T) {
func TestCreateFullOptionsLabels(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
config := &configuration{
EnableIPForwarding: true,
@@ -422,7 +423,7 @@ func TestCreateFullOptionsLabels(t *testing.T) {
func TestCreate(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -448,7 +449,7 @@ func TestCreate(t *testing.T) {
func TestCreateFail(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -466,7 +467,7 @@ func TestCreateFail(t *testing.T) {
func TestCreateMultipleNetworks(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
config := &configuration{
EnableIPTables: true,
@@ -669,7 +670,7 @@ func TestQueryEndpointInfoHairpin(t *testing.T) {
func testQueryEndpointInfo(t *testing.T, ulPxyEnabled bool) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
portallocator.Get().ReleaseAll()
var proxyBinary string
@@ -782,7 +783,7 @@ func getPortMapping() []types.PortBinding {
func TestLinkContainers(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
iptable := iptables.GetIptable(iptables.IPv4)
config := &configuration{
@@ -1091,7 +1092,7 @@ func TestValidateFixedCIDRV6(t *testing.T) {
func TestSetDefaultGw(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -1199,7 +1200,7 @@ func TestCleanupIptableRules(t *testing.T) {
func TestCreateWithExistingBridge(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -1271,7 +1272,7 @@ func TestCreateParallel(t *testing.T) {
c := netnsutils.SetupTestOSContextEx(t)
defer c.Cleanup(t)
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
portallocator.Get().ReleaseAll()
if err := d.configure(nil); err != nil {

View File

@@ -10,7 +10,6 @@ import (
"github.com/containerd/log"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/types"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
@@ -25,23 +24,15 @@ const (
bridgeEndpointPrefix = "bridge-endpoint"
)
func (d *driver) initStore(option map[string]interface{}) error {
if data, ok := option[netlabel.LocalKVClient]; ok {
var ok bool
d.store, ok = data.(*datastore.Store)
if !ok {
return types.InternalErrorf("incorrect data in datastore configuration: %v", data)
}
func (d *driver) initStore() error {
err := d.populateNetworks()
if err != nil {
return err
}
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
return nil

View File

@@ -6,13 +6,14 @@ import (
"github.com/docker/docker/internal/nlwrap"
"github.com/docker/docker/internal/testutils/netnsutils"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/netlabel"
)
func TestLinkCreate(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -108,7 +109,7 @@ func TestLinkCreate(t *testing.T) {
func TestLinkCreateTwo(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -147,7 +148,7 @@ func TestLinkCreateTwo(t *testing.T) {
func TestLinkCreateNoEnableIPv6(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)
@@ -183,7 +184,7 @@ func TestLinkCreateNoEnableIPv6(t *testing.T) {
func TestLinkDelete(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
if err := d.configure(nil); err != nil {
t.Fatalf("Failed to setup driver config: %v", err)

View File

@@ -14,6 +14,7 @@ import (
"github.com/containerd/log"
"github.com/docker/docker/internal/testutils/netnsutils"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/iptables"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/ns"
@@ -27,7 +28,7 @@ import (
func TestPortMappingConfig(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
config := &configuration{
EnableIPTables: true,
@@ -111,7 +112,7 @@ func TestPortMappingV6Config(t *testing.T) {
t.Fatalf("Could not bring loopback iface up: %v", err)
}
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
config := &configuration{
EnableIPTables: true,
@@ -828,7 +829,7 @@ func TestAddPortMappings(t *testing.T) {
GwModeIPv4: tc.gwMode4,
GwModeIPv6: tc.gwMode6,
},
driver: newDriver(),
driver: newDriver(storeutils.NewTempStore(t)),
}
genericOption := map[string]interface{}{
netlabel.GenericData: &configuration{

View File

@@ -8,6 +8,7 @@ import (
"github.com/docker/docker/internal/nlwrap"
"github.com/docker/docker/internal/testutils/netnsutils"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/iptables"
"github.com/docker/docker/libnetwork/netlabel"
@@ -191,7 +192,7 @@ func assertBridgeConfig(config *networkConfiguration, br *bridgeInterface, d *dr
// Regression test for https://github.com/moby/moby/issues/46445
func TestSetupIP6TablesWithHostIPv4(t *testing.T) {
defer netnsutils.SetupTestOSContext(t)()
d := newDriver()
d := newDriver(storeutils.NewTempStore(t))
dc := &configuration{
EnableIPTables: true,
EnableIP6Tables: true,
@@ -364,7 +365,7 @@ func TestOutgoingNATRules(t *testing.T) {
EnableIP6Tables: tc.enableIP6Tables,
}
r := &testRegisterer{t: t}
if err := Register(r, map[string]interface{}{netlabel.GenericData: dc}); err != nil {
if err := Register(r, storeutils.NewTempStore(t), map[string]interface{}{netlabel.GenericData: dc}); err != nil {
t.Fatal(err)
}
if r.d == nil {

View File

@@ -63,11 +63,12 @@ type network struct {
}
// Register initializes and registers the libnetwork ipvlan driver.
func Register(r driverapi.Registerer, config map[string]interface{}) error {
func Register(r driverapi.Registerer, store *datastore.Store, config map[string]interface{}) error {
d := &driver{
store: store,
networks: networkTable{},
}
if err := d.initStore(config); err != nil {
if err := d.initStore(); err != nil {
return err
}
return r.RegisterDriver(NetworkType, d, driverapi.Capability{

View File

@@ -10,7 +10,6 @@ import (
"github.com/containerd/log"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/types"
)
@@ -41,22 +40,14 @@ type ipSubnet struct {
}
// initStore drivers are responsible for caching their own persistent state
func (d *driver) initStore(option map[string]interface{}) error {
if data, ok := option[netlabel.LocalKVClient]; ok {
var ok bool
d.store, ok = data.(*datastore.Store)
if !ok {
return types.InternalErrorf("incorrect data in datastore configuration: %v", data)
}
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
func (d *driver) initStore() error {
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
return nil

View File

@@ -5,6 +5,7 @@ package ipvlan
import (
"testing"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
)
@@ -31,25 +32,25 @@ func (dt *driverTester) RegisterDriver(name string, drv driverapi.Driver, cap dr
}
func TestIpvlanRegister(t *testing.T) {
if err := Register(&driverTester{t: t}, nil); err != nil {
if err := Register(&driverTester{t: t}, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}
}
func TestIpvlanNilConfig(t *testing.T) {
dt := &driverTester{t: t}
if err := Register(dt, nil); err != nil {
if err := Register(dt, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}
if err := dt.d.initStore(nil); err != nil {
if err := dt.d.initStore(); err != nil {
t.Fatal(err)
}
}
func TestIpvlanType(t *testing.T) {
dt := &driverTester{t: t}
if err := Register(dt, nil); err != nil {
if err := Register(dt, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}

View File

@@ -57,11 +57,12 @@ type network struct {
}
// Register initializes and registers the libnetwork macvlan driver
func Register(r driverapi.Registerer, config map[string]interface{}) error {
func Register(r driverapi.Registerer, store *datastore.Store, _ map[string]interface{}) error {
d := &driver{
store: store,
networks: networkTable{},
}
if err := d.initStore(config); err != nil {
if err := d.initStore(); err != nil {
return err
}
return r.RegisterDriver(NetworkType, d, driverapi.Capability{

View File

@@ -10,7 +10,6 @@ import (
"github.com/containerd/log"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/types"
)
@@ -40,22 +39,14 @@ type ipSubnet struct {
}
// initStore drivers are responsible for caching their own persistent state
func (d *driver) initStore(option map[string]interface{}) error {
if data, ok := option[netlabel.LocalKVClient]; ok {
var ok bool
d.store, ok = data.(*datastore.Store)
if !ok {
return types.InternalErrorf("incorrect data in datastore configuration: %v", data)
}
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
func (d *driver) initStore() error {
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
return nil

View File

@@ -5,6 +5,7 @@ package macvlan
import (
"testing"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
)
@@ -31,25 +32,25 @@ func (dt *driverTester) RegisterDriver(name string, drv driverapi.Driver, cap dr
}
func TestMacvlanRegister(t *testing.T) {
if err := Register(&driverTester{t: t}, nil); err != nil {
if err := Register(&driverTester{t: t}, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}
}
func TestMacvlanNilConfig(t *testing.T) {
dt := &driverTester{t: t}
if err := Register(dt, nil); err != nil {
if err := Register(dt, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}
if err := dt.d.initStore(nil); err != nil {
if err := dt.d.initStore(); err != nil {
t.Fatal(err)
}
}
func TestMacvlanType(t *testing.T) {
dt := &driverTester{t: t}
if err := Register(dt, nil); err != nil {
if err := Register(dt, storeutils.NewTempStore(t), nil); err != nil {
t.Fatal(err)
}

View File

@@ -128,20 +128,26 @@ func IsBuiltinLocalDriver(networkType string) bool {
return ok
}
// New constructs a new bridge driver
func newDriver(networkType string) *driver {
return &driver{name: networkType, networks: map[string]*hnsNetwork{}}
func newDriver(networkType string, store *datastore.Store) (*driver, error) {
d := &driver{
name: networkType,
store: store,
networks: map[string]*hnsNetwork{},
}
err := d.initStore()
if err != nil {
return nil, fmt.Errorf("failed to initialize %q driver: %w", networkType, err)
}
return d, nil
}
// GetInit returns an initializer for the given network type
func RegisterBuiltinLocalDrivers(r driverapi.Registerer, driverConfig func(string) map[string]interface{}) error {
// RegisterBuiltinLocalDrivers registers the builtin local drivers.
func RegisterBuiltinLocalDrivers(r driverapi.Registerer, store *datastore.Store) error {
for networkType := range builtinLocalDrivers {
d := newDriver(networkType)
err := d.initStore(driverConfig(networkType))
d, err := newDriver(networkType, store)
if err != nil {
return fmt.Errorf("failed to initialize %q driver: %w", networkType, err)
return err
}
err = r.RegisterDriver(networkType, d, driverapi.Capability{
DataScope: scope.Local,
ConnectivityScope: scope.Local,

View File

@@ -10,7 +10,6 @@ import (
"github.com/containerd/log"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/types"
)
@@ -19,23 +18,15 @@ const (
windowsEndpointPrefix = "windows-endpoint"
)
func (d *driver) initStore(option map[string]interface{}) error {
if data, ok := option[netlabel.LocalKVClient]; ok {
var ok bool
d.store, ok = data.(*datastore.Store)
if !ok {
return types.InternalErrorf("incorrect data in datastore configuration: %v", data)
}
func (d *driver) initStore() error {
err := d.populateNetworks()
if err != nil {
return err
}
err := d.populateNetworks()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
err = d.populateEndpoints()
if err != nil {
return err
}
return nil

View File

@@ -7,13 +7,16 @@ import (
"net"
"testing"
"github.com/docker/docker/internal/testutils/storeutils"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/netlabel"
"github.com/docker/docker/libnetwork/types"
"gotest.tools/v3/assert"
)
func testNetwork(networkType string, t *testing.T) {
d := newDriver(networkType)
d, err := newDriver(networkType, storeutils.NewTempStore(t))
assert.NilError(t, err)
bnw, _ := types.ParseCIDR("172.16.0.0/24")
br, _ := types.ParseCIDR("172.16.0.1/16")
@@ -30,7 +33,7 @@ func testNetwork(networkType string, t *testing.T) {
},
}
err := d.CreateNetwork("dummy", netOption, nil, ipdList, nil)
err = d.CreateNetwork("dummy", netOption, nil, ipdList, nil)
if err != nil {
t.Fatalf("Failed to create bridge: %v", err)
}

View File

@@ -3,6 +3,7 @@ package libnetwork
import (
"fmt"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/drivers/bridge"
"github.com/docker/docker/libnetwork/drivers/host"
@@ -12,23 +13,25 @@ import (
"github.com/docker/docker/libnetwork/drivers/overlay"
)
func registerNetworkDrivers(r driverapi.Registerer, driverConfig func(string) map[string]interface{}) error {
noConfig := func(fn func(driverapi.Registerer) error) func(driverapi.Registerer, map[string]interface{}) error {
return func(r driverapi.Registerer, _ map[string]interface{}) error { return fn(r) }
}
func registerNetworkDrivers(r driverapi.Registerer, store *datastore.Store, driverConfig func(string) map[string]interface{}) error {
for _, nr := range []struct {
ntype string
register func(driverapi.Registerer, map[string]interface{}) error
register func(driverapi.Registerer, *datastore.Store, map[string]interface{}) error
}{
{ntype: bridge.NetworkType, register: bridge.Register},
{ntype: host.NetworkType, register: noConfig(host.Register)},
{ntype: host.NetworkType, register: func(r driverapi.Registerer, _ *datastore.Store, _ map[string]interface{}) error {
return host.Register(r)
}},
{ntype: ipvlan.NetworkType, register: ipvlan.Register},
{ntype: macvlan.NetworkType, register: macvlan.Register},
{ntype: null.NetworkType, register: noConfig(null.Register)},
{ntype: overlay.NetworkType, register: overlay.Register},
{ntype: null.NetworkType, register: func(r driverapi.Registerer, _ *datastore.Store, _ map[string]interface{}) error {
return null.Register(r)
}},
{ntype: overlay.NetworkType, register: func(r driverapi.Registerer, _ *datastore.Store, config map[string]interface{}) error {
return overlay.Register(r, config)
}},
} {
if err := nr.register(r, driverConfig(nr.ntype)); err != nil {
if err := nr.register(r, store, driverConfig(nr.ntype)); err != nil {
return fmt.Errorf("failed to register %q driver: %w", nr.ntype, err)
}
}

View File

@@ -3,13 +3,14 @@ package libnetwork
import (
"fmt"
"github.com/docker/docker/libnetwork/datastore"
"github.com/docker/docker/libnetwork/driverapi"
"github.com/docker/docker/libnetwork/drivers/null"
"github.com/docker/docker/libnetwork/drivers/windows"
"github.com/docker/docker/libnetwork/drivers/windows/overlay"
)
func registerNetworkDrivers(r driverapi.Registerer, driverConfig func(string) map[string]interface{}) error {
func registerNetworkDrivers(r driverapi.Registerer, store *datastore.Store, _ func(string) map[string]interface{}) error {
for _, nr := range []struct {
ntype string
register func(driverapi.Registerer) error
@@ -22,5 +23,5 @@ func registerNetworkDrivers(r driverapi.Registerer, driverConfig func(string) ma
}
}
return windows.RegisterBuiltinLocalDrivers(r, driverConfig)
return windows.RegisterBuiltinLocalDrivers(r, store)
}

View File

@@ -57,9 +57,6 @@ const (
// HostIPv6 is the Source-IPv6 Address used to SNAT IPv6 container traffic
HostIPv6 = Prefix + ".host_ipv6"
// LocalKVClient constants represents the local kv store client
LocalKVClient = DriverPrivatePrefix + "localkv_client"
// NoProxy6To4 disables proxying from an IPv6 host port to an IPv4-only
// container, when the default binding address is 0.0.0.0. This label
// is intended for internal use, it may be removed in a future release.