From e32e96048a049705918ddf6e97b8c3c1a6964517 Mon Sep 17 00:00:00 2001 From: selansen Date: Tue, 28 Aug 2018 18:13:48 -0400 Subject: [PATCH] Default Address Pool Code Refactor We have two params that have been passed around as part of swarm init config. As part of refactoring the code, we created NetworkConfig struct and added address pool and subnet mask length as part of the new struct Older PR review comment has been addressed in this commit. Signed-off-by: selansen --- manager/allocator/allocator.go | 21 +++++------ manager/allocator/allocator_linux_test.go | 2 +- manager/allocator/allocator_test.go | 12 +++---- .../allocator/cnmallocator/drivers_ipam.go | 15 ++++---- .../cnmallocator/networkallocator.go | 14 ++++++-- .../cnmallocator/networkallocator_test.go | 2 +- manager/allocator/network.go | 10 +++++- manager/controlapi/cluster.go | 13 +++++++ manager/manager.go | 35 +++++++------------ node/node.go | 12 +++---- 10 files changed, 73 insertions(+), 63 deletions(-) diff --git a/manager/allocator/allocator.go b/manager/allocator/allocator.go index a6f0289a02..e8b7e88c47 100644 --- a/manager/allocator/allocator.go +++ b/manager/allocator/allocator.go @@ -1,12 +1,12 @@ package allocator import ( - "net" "sync" "github.com/docker/docker/pkg/plugingetter" "github.com/docker/go-events" "github.com/docker/swarmkit/api" + "github.com/docker/swarmkit/manager/allocator/cnmallocator" "github.com/docker/swarmkit/manager/state" "github.com/docker/swarmkit/manager/state/store" "golang.org/x/net/context" @@ -34,12 +34,8 @@ type Allocator struct { // pluginGetter provides access to docker's plugin inventory. pluginGetter plugingetter.PluginGetter - // DefaultAddrPool specifies default subnet pool for global scope networks - defaultAddrPool []*net.IPNet - - // SubnetSize specifies the subnet size of the networks created from - // the default subnet pool - subnetSize int + // networkConfig stores network related config for the cluster + networkConfig *cnmallocator.NetworkConfig } // taskBallot controls how the voting for task allocation is @@ -73,17 +69,16 @@ type allocActor struct { // New returns a new instance of Allocator for use during allocation // stage of the manager. -func New(store *store.MemoryStore, pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize int) (*Allocator, error) { +func New(store *store.MemoryStore, pg plugingetter.PluginGetter, netConfig *cnmallocator.NetworkConfig) (*Allocator, error) { a := &Allocator{ store: store, taskBallot: &taskBallot{ votes: make(map[string][]string), }, - stopChan: make(chan struct{}), - doneChan: make(chan struct{}), - pluginGetter: pg, - defaultAddrPool: defaultAddrPool, - subnetSize: subnetSize, + stopChan: make(chan struct{}), + doneChan: make(chan struct{}), + pluginGetter: pg, + networkConfig: netConfig, } return a, nil diff --git a/manager/allocator/allocator_linux_test.go b/manager/allocator/allocator_linux_test.go index 9c4894ca3e..b64832eba1 100644 --- a/manager/allocator/allocator_linux_test.go +++ b/manager/allocator/allocator_linux_test.go @@ -16,7 +16,7 @@ func TestIPAMNotNil(t *testing.T) { assert.NotNil(t, s) defer s.Close() - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) diff --git a/manager/allocator/allocator_test.go b/manager/allocator/allocator_test.go index ac90550fa8..585885f860 100644 --- a/manager/allocator/allocator_test.go +++ b/manager/allocator/allocator_test.go @@ -27,7 +27,7 @@ func TestAllocator(t *testing.T) { assert.NotNil(t, s) defer s.Close() - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) @@ -668,7 +668,7 @@ func TestNoDuplicateIPs(t *testing.T) { return nil })) - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) @@ -800,7 +800,7 @@ func TestAllocatorRestoreForDuplicateIPs(t *testing.T) { return true } - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) // Start allocator @@ -950,7 +950,7 @@ func TestAllocatorRestartNoEndpointSpec(t *testing.T) { return true } - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) // Start allocator @@ -1154,7 +1154,7 @@ func TestAllocatorRestoreForUnallocatedNetwork(t *testing.T) { return true } - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) // Start allocator @@ -1181,7 +1181,7 @@ func TestNodeAllocator(t *testing.T) { assert.NotNil(t, s) defer s.Close() - a, err := New(s, nil, nil, 0) + a, err := New(s, nil, nil) assert.NoError(t, err) assert.NotNil(t, a) diff --git a/manager/allocator/cnmallocator/drivers_ipam.go b/manager/allocator/cnmallocator/drivers_ipam.go index d0daccc75a..39bce0615c 100644 --- a/manager/allocator/cnmallocator/drivers_ipam.go +++ b/manager/allocator/cnmallocator/drivers_ipam.go @@ -1,7 +1,6 @@ package cnmallocator import ( - "net" "strconv" "strings" @@ -14,7 +13,7 @@ import ( "github.com/sirupsen/logrus" ) -func initIPAMDrivers(r *drvregistry.DrvRegistry, defaultAddrPool []*net.IPNet, subnetSize int) error { +func initIPAMDrivers(r *drvregistry.DrvRegistry, netConfig *NetworkConfig) error { var addressPool []*ipamutils.NetworkToSplit var str strings.Builder str.WriteString("Subnetlist - ") @@ -22,16 +21,16 @@ func initIPAMDrivers(r *drvregistry.DrvRegistry, defaultAddrPool []*net.IPNet, s // from the info. We will be using it to call Libnetwork API // We also need to log new address pool info whenever swarm init // happens with default address pool option - if defaultAddrPool != nil { - for _, p := range defaultAddrPool { + if netConfig != nil { + for _, p := range netConfig.DefaultAddrPool { addressPool = append(addressPool, &ipamutils.NetworkToSplit{ - Base: p.String(), - Size: subnetSize, + Base: p, + Size: int(netConfig.SubnetSize), }) - str.WriteString(p.String() + ",") + str.WriteString(p + ",") } str.WriteString(": Size ") - str.WriteString(strconv.Itoa(subnetSize)) + str.WriteString(strconv.Itoa(int(netConfig.SubnetSize))) } if err := ipamutils.ConfigGlobalScopeDefaultNetworks(addressPool); err != nil { return err diff --git a/manager/allocator/cnmallocator/networkallocator.go b/manager/allocator/cnmallocator/networkallocator.go index 22229d5095..7a786406e1 100644 --- a/manager/allocator/cnmallocator/networkallocator.go +++ b/manager/allocator/cnmallocator/networkallocator.go @@ -86,8 +86,18 @@ type initializer struct { ntype string } +// NetworkConfig is used to store network related cluster config in the Manager. +type NetworkConfig struct { + // DefaultAddrPool specifies default subnet pool for global scope networks + DefaultAddrPool []string + + // SubnetSize specifies the subnet size of the networks created from + // the default subnet pool + SubnetSize uint32 +} + // New returns a new NetworkAllocator handle -func New(pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize int) (networkallocator.NetworkAllocator, error) { +func New(pg plugingetter.PluginGetter, netConfig *NetworkConfig) (networkallocator.NetworkAllocator, error) { na := &cnmNetworkAllocator{ networks: make(map[string]*network), services: make(map[string]struct{}), @@ -106,7 +116,7 @@ func New(pg plugingetter.PluginGetter, defaultAddrPool []*net.IPNet, subnetSize return nil, err } - if err = initIPAMDrivers(reg, defaultAddrPool, subnetSize); err != nil { + if err = initIPAMDrivers(reg, netConfig); err != nil { return nil, err } diff --git a/manager/allocator/cnmallocator/networkallocator_test.go b/manager/allocator/cnmallocator/networkallocator_test.go index 17d87af74f..00d2e47f8d 100644 --- a/manager/allocator/cnmallocator/networkallocator_test.go +++ b/manager/allocator/cnmallocator/networkallocator_test.go @@ -13,7 +13,7 @@ import ( ) func newNetworkAllocator(t *testing.T) networkallocator.NetworkAllocator { - na, err := New(nil, nil, 0) + na, err := New(nil, nil) assert.NoError(t, err) assert.NotNil(t, na) return na diff --git a/manager/allocator/network.go b/manager/allocator/network.go index f264c78951..9136928311 100644 --- a/manager/allocator/network.go +++ b/manager/allocator/network.go @@ -68,7 +68,15 @@ type networkContext struct { } func (a *Allocator) doNetworkInit(ctx context.Context) (err error) { - na, err := cnmallocator.New(a.pluginGetter, a.defaultAddrPool, a.subnetSize) + var netConfig *cnmallocator.NetworkConfig + if a.networkConfig != nil && a.networkConfig.DefaultAddrPool != nil { + netConfig = &cnmallocator.NetworkConfig{ + DefaultAddrPool: a.networkConfig.DefaultAddrPool, + SubnetSize: a.networkConfig.SubnetSize, + } + } + + na, err := cnmallocator.New(a.pluginGetter, netConfig) if err != nil { return err } diff --git a/manager/controlapi/cluster.go b/manager/controlapi/cluster.go index 7b34807294..ad24804484 100644 --- a/manager/controlapi/cluster.go +++ b/manager/controlapi/cluster.go @@ -19,6 +19,13 @@ const ( // expiredCertGrace is the amount of time to keep a node in the // blacklist beyond its certificate expiration timestamp. expiredCertGrace = 24 * time.Hour * 7 + // inbuilt default subnet size + inbuiltSubnetSize = 24 +) + +var ( + // inbuilt default address pool + inbuiltDefaultAddressPool = []string{"10.0.0.0/8"} ) func validateClusterSpec(spec *api.ClusterSpec) error { @@ -268,6 +275,12 @@ func redactClusters(clusters []*api.Cluster) []*api.Cluster { DefaultAddressPool: cluster.DefaultAddressPool, SubnetSize: cluster.SubnetSize, } + if newCluster.DefaultAddressPool == nil { + // This is just for CLI display. Set the inbuilt default pool for + // user reference. + newCluster.DefaultAddressPool = inbuiltDefaultAddressPool + newCluster.SubnetSize = inbuiltSubnetSize + } redactedClusters = append(redactedClusters, newCluster) } diff --git a/manager/manager.go b/manager/manager.go index c8bafb77aa..106c8e2b58 100644 --- a/manager/manager.go +++ b/manager/manager.go @@ -20,6 +20,7 @@ import ( "github.com/docker/swarmkit/identity" "github.com/docker/swarmkit/log" "github.com/docker/swarmkit/manager/allocator" + "github.com/docker/swarmkit/manager/allocator/cnmallocator" "github.com/docker/swarmkit/manager/allocator/networkallocator" "github.com/docker/swarmkit/manager/controlapi" "github.com/docker/swarmkit/manager/dispatcher" @@ -127,12 +128,8 @@ type Config struct { // FIPS setting. FIPS bool - // DefaultAddrPool specifies default subnet pool for global scope networks - DefaultAddrPool []*net.IPNet - - // SubnetSize specifies the subnet size of the networks created from - // the default subnet pool - SubnetSize int + // NetworkConfig stores network related config for the cluster + NetworkConfig *cnmallocator.NetworkConfig } // Manager is the cluster manager for Swarm. @@ -947,14 +944,10 @@ func (m *Manager) becomeLeader(ctx context.Context) { nil, 0) - var defaultAddrPool []string - for _, p := range m.config.DefaultAddrPool { - defaultAddrPool = append(defaultAddrPool, p.String()) - } // If defaultAddrPool is valid we update cluster object with new value - if defaultAddrPool != nil { - clusterObj.DefaultAddressPool = defaultAddrPool - clusterObj.SubnetSize = uint32(m.config.SubnetSize) + if m.config.NetworkConfig != nil && m.config.NetworkConfig.DefaultAddrPool != nil { + clusterObj.DefaultAddressPool = m.config.NetworkConfig.DefaultAddrPool + clusterObj.SubnetSize = m.config.NetworkConfig.SubnetSize } err := store.CreateCluster(tx, clusterObj) @@ -1003,24 +996,20 @@ func (m *Manager) becomeLeader(ctx context.Context) { // If DefaultAddrPool is null, Read from store and check if // DefaultAddrPool info is stored in cluster object - if m.config.DefaultAddrPool == nil { + if m.config.NetworkConfig == nil || m.config.NetworkConfig.DefaultAddrPool == nil { var cluster *api.Cluster s.View(func(tx store.ReadTx) { cluster = store.GetCluster(tx, clusterID) }) if cluster.DefaultAddressPool != nil { for _, address := range cluster.DefaultAddressPool { - _, b, err := net.ParseCIDR(address) - if err != nil { - log.G(ctx).WithError(err).Error("Default Address Pool reading failed for cluster object %s", address) - } - m.config.DefaultAddrPool = append(m.config.DefaultAddrPool, b) + m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, address) } + m.config.NetworkConfig.SubnetSize = cluster.SubnetSize } - m.config.SubnetSize = int(cluster.SubnetSize) } - m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.DefaultAddrPool, m.config.SubnetSize) + m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.NetworkConfig) if err != nil { log.G(ctx).WithError(err).Error("failed to create allocator") // TODO(stevvooe): It doesn't seem correct here to fail @@ -1144,7 +1133,7 @@ func defaultClusterObject( rootCA *ca.RootCA, fips bool, defaultAddressPool []string, - subnetSize int) *api.Cluster { + subnetSize uint32) *api.Cluster { var caKey []byte if rcaSigner, err := rootCA.Signer(); err == nil { caKey = rcaSigner.Key @@ -1178,7 +1167,7 @@ func defaultClusterObject( UnlockKeys: initialUnlockKeys, FIPS: fips, DefaultAddressPool: defaultAddressPool, - SubnetSize: uint32(subnetSize), + SubnetSize: subnetSize, } } diff --git a/node/node.go b/node/node.go index 843c738995..a3a762f380 100644 --- a/node/node.go +++ b/node/node.go @@ -28,6 +28,7 @@ import ( "github.com/docker/swarmkit/ioutils" "github.com/docker/swarmkit/log" "github.com/docker/swarmkit/manager" + "github.com/docker/swarmkit/manager/allocator/cnmallocator" "github.com/docker/swarmkit/manager/encryption" "github.com/docker/swarmkit/remotes" "github.com/docker/swarmkit/xnet" @@ -105,12 +106,8 @@ type Config struct { // for connections to the remote API (including the raft service). AdvertiseRemoteAPI string - // DefaultAddrPool specifies default subnet pool for global scope networks - DefaultAddrPool []*net.IPNet - - // SubnetSize specifies the subnet size of the networks created from - // the default subnet pool - SubnetSize int + // NetworkConfig stores network related config for the cluster + NetworkConfig *cnmallocator.NetworkConfig // Executor specifies the executor to use for the agent. Executor exec.Executor @@ -1002,8 +999,7 @@ func (n *Node) runManager(ctx context.Context, securityConfig *ca.SecurityConfig PluginGetter: n.config.PluginGetter, RootCAPaths: rootPaths, FIPS: n.config.FIPS, - DefaultAddrPool: n.config.DefaultAddrPool, - SubnetSize: n.config.SubnetSize, + NetworkConfig: n.config.NetworkConfig, }) if err != nil { return false, err