Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user to specify default address pools for docker networks #36396

Merged
merged 1 commit into from May 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/dockerd/config_unix.go
Expand Up @@ -18,6 +18,7 @@ func installConfigFlags(conf *config.Config, flags *pflag.FlagSet) {
installUnixConfigFlags(conf, flags)

conf.Ulimits = make(map[string]*units.Ulimit)
conf.NetworkConfig.DefaultAddressPools = opts.PoolsOpt{}

// Set default value for `--default-shm-size`
conf.ShmSize = opts.MemBytes(config.DefaultShmSize)
Expand All @@ -44,4 +45,6 @@ func installConfigFlags(conf *config.Config, flags *pflag.FlagSet) {
flags.Var(&conf.ShmSize, "default-shm-size", "Default shm size for containers")
flags.BoolVar(&conf.NoNewPrivileges, "no-new-privileges", false, "Set no-new-privileges by default for new containers")
flags.StringVar(&conf.IpcMode, "default-ipc-mode", config.DefaultIpcMode, `Default mode for containers ipc ("shareable" | "private")`)
flags.Var(&conf.NetworkConfig.DefaultAddressPools, "default-address-pool", "Default address pools for node specific local networks")

}
7 changes: 7 additions & 0 deletions daemon/config/config.go
Expand Up @@ -71,6 +71,12 @@ type commonBridgeConfig struct {
FixedCIDR string `json:"fixed-cidr,omitempty"`
}

// NetworkConfig stores the daemon-wide networking configurations
type NetworkConfig struct {
// Default address pools for docker networks
DefaultAddressPools opts.PoolsOpt `json:"default-address-pools,omitempty"`
}

// CommonTLSOptions defines TLS configuration for the daemon server.
// It includes json tags to deserialize configuration from a file
// using the same names that the flags in the command line use.
Expand Down Expand Up @@ -173,6 +179,7 @@ type CommonConfig struct {

LogConfig
BridgeConfig // bridgeConfig holds bridge network specific configuration.
NetworkConfig
registry.ServiceOptions

sync.Mutex
Expand Down
1 change: 0 additions & 1 deletion daemon/config/config_unix.go
Expand Up @@ -23,7 +23,6 @@ type Config struct {

// These fields are common to all unix platforms.
CommonUnixConfig

// Fields below here are platform specific.
CgroupParent string `json:"cgroup-parent,omitempty"`
EnableSelinuxSupport bool `json:"selinux-enabled,omitempty"`
Expand Down
4 changes: 4 additions & 0 deletions daemon/daemon.go
Expand Up @@ -1223,6 +1223,10 @@ func (daemon *Daemon) networkOptions(dconfig *config.Config, pg plugingetter.Plu
options = append(options, nwconfig.OptionLabels(dconfig.Labels))
options = append(options, driverOptions(dconfig)...)

if len(dconfig.NetworkConfig.DefaultAddressPools.Value()) > 0 {
options = append(options, nwconfig.OptionDefaultAddressPoolConfig(dconfig.NetworkConfig.DefaultAddressPools.Value()))
}

if daemon.configStore != nil && daemon.configStore.LiveRestoreEnabled && len(activeSandboxes) != 0 {
options = append(options, nwconfig.OptionActiveSandboxes(activeSandboxes))
}
Expand Down
3 changes: 3 additions & 0 deletions daemon/daemon_unix.go
Expand Up @@ -833,6 +833,9 @@ func (daemon *Daemon) initNetworkController(config *config.Config, activeSandbox
if err = n.Delete(); err != nil {
return nil, fmt.Errorf("could not delete the default bridge network: %v", err)
}
if len(config.NetworkConfig.DefaultAddressPools.Value()) > 0 && !daemon.configStore.LiveRestoreEnabled {
removeDefaultBridgeInterface()
}
}

if !config.DisableBridge {
Expand Down
179 changes: 179 additions & 0 deletions integration/network/service_test.go
Expand Up @@ -9,10 +9,189 @@ import (
swarmtypes "github.com/docker/docker/api/types/swarm"
"github.com/docker/docker/client"
"github.com/docker/docker/integration/internal/swarm"
"github.com/docker/docker/internal/test/daemon"
"github.com/gotestyourself/gotestyourself/assert"
"github.com/gotestyourself/gotestyourself/icmd"
"github.com/gotestyourself/gotestyourself/poll"
)

// delInterface removes given network interface
func delInterface(t *testing.T, ifName string) {
icmd.RunCommand("ip", "link", "delete", ifName).Assert(t, icmd.Success)
icmd.RunCommand("iptables", "-t", "nat", "--flush").Assert(t, icmd.Success)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These flushes are pretty generic. Can we instead delete the rules that are needed?
Alternatively maybe start the tests in a custom network namespace to prevent leaking to other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of the key test for this feature is to delete docker0 network and see if it comes up with new IP Subnetwork range. So we need this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's flushing the whole table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helper function and it is used only in test API. when we delete interface we clear NAT rules associated to have clear start for testing purpose. I did some code reading and looks like already same function is added in helper.go. Pls take a look at it

func DeleteInterface(t *testing.T, ifName string) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works I guess it doesn't matter 🤷‍♂️

icmd.RunCommand("iptables", "--flush").Assert(t, icmd.Success)
}

func TestDaemonRestartWithLiveRestore(t *testing.T) {
d := daemon.New(t)
defer d.Stop(t)
d.Start(t)
d.Restart(t, "--live-restore=true",
"--default-address-pool", "base=175.30.0.0/16,size=16",
"--default-address-pool", "base=175.33.0.0/16,size=24")

// Verify bridge network's subnet
cli, err := d.NewClient()
assert.Assert(t, err)
defer cli.Close()
out, err := cli.NetworkInspect(context.Background(), "bridge", types.NetworkInspectOptions{})
assert.NilError(t, err)
// Make sure docker0 doesn't get override with new IP in live restore case
assert.Equal(t, out.IPAM.Config[0].Subnet, "172.18.0.0/16")
}

func TestDaemonDefaultNetworkPools(t *testing.T) {
// Remove docker0 bridge and the start daemon defining the predefined address pools
defaultNetworkBridge := "docker0"
delInterface(t, defaultNetworkBridge)
d := daemon.New(t)
defer d.Stop(t)
d.Start(t,
"--default-address-pool", "base=175.30.0.0/16,size=16",
"--default-address-pool", "base=175.33.0.0/16,size=24")

// Verify bridge network's subnet
cli, err := d.NewClient()
assert.Assert(t, err)
defer cli.Close()
out, err := cli.NetworkInspect(context.Background(), "bridge", types.NetworkInspectOptions{})
assert.NilError(t, err)
assert.Equal(t, out.IPAM.Config[0].Subnet, "175.30.0.0/16")

// Create a bridge network and verify its subnet is the second default pool
name := "elango"
networkCreate := types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out, err = cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
assert.Equal(t, out.IPAM.Config[0].Subnet, "175.33.0.0/24")

// Create a bridge network and verify its subnet is the third default pool
name = "saanvi"
networkCreate = types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out, err = cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
assert.Equal(t, out.IPAM.Config[0].Subnet, "175.33.1.0/24")
delInterface(t, defaultNetworkBridge)

}

func TestDaemonRestartWithExistingNetwork(t *testing.T) {
defaultNetworkBridge := "docker0"
d := daemon.New(t)
d.Start(t)
defer d.Stop(t)
// Verify bridge network's subnet
cli, err := d.NewClient()
assert.Assert(t, err)
defer cli.Close()

// Create a bridge network
name := "elango"
networkCreate := types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out, err := cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
networkip := out.IPAM.Config[0].Subnet

// Restart daemon with default address pool option
d.Restart(t,
"--default-address-pool", "base=175.30.0.0/16,size=16",
"--default-address-pool", "base=175.33.0.0/16,size=24")

out1, err := cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
assert.Equal(t, out1.IPAM.Config[0].Subnet, networkip)
delInterface(t, defaultNetworkBridge)
}

func TestDaemonRestartWithExistingNetworkWithDefaultPoolRange(t *testing.T) {
defaultNetworkBridge := "docker0"
d := daemon.New(t)
d.Start(t)
defer d.Stop(t)
// Verify bridge network's subnet
cli, err := d.NewClient()
assert.Assert(t, err)
defer cli.Close()

// Create a bridge network
name := "elango"
networkCreate := types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out, err := cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
networkip := out.IPAM.Config[0].Subnet

// Create a bridge network
name = "sthira"
networkCreate = types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out, err = cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)
networkip2 := out.IPAM.Config[0].Subnet

// Restart daemon with default address pool option
d.Restart(t,
"--default-address-pool", "base=175.18.0.0/16,size=16",
"--default-address-pool", "base=175.19.0.0/16,size=24")

// Create a bridge network
name = "saanvi"
networkCreate = types.NetworkCreate{
CheckDuplicate: false,
}
networkCreate.Driver = "bridge"
_, err = cli.NetworkCreate(context.Background(), name, networkCreate)
assert.NilError(t, err)
out1, err := cli.NetworkInspect(context.Background(), name, types.NetworkInspectOptions{})
assert.NilError(t, err)

assert.Check(t, (out1.IPAM.Config[0].Subnet != networkip))
assert.Check(t, (out1.IPAM.Config[0].Subnet != networkip2))
delInterface(t, defaultNetworkBridge)
}

func TestDaemonWithBipAndDefaultNetworkPool(t *testing.T) {
defaultNetworkBridge := "docker0"
d := daemon.New(t)
defer d.Stop(t)
d.Start(t, "--bip=172.60.0.1/16",
"--default-address-pool", "base=175.30.0.0/16,size=16",
"--default-address-pool", "base=175.33.0.0/16,size=24")

// Verify bridge network's subnet
cli, err := d.NewClient()
assert.Assert(t, err)
defer cli.Close()
out, err := cli.NetworkInspect(context.Background(), "bridge", types.NetworkInspectOptions{})
assert.NilError(t, err)
// Make sure BIP IP doesn't get override with new default address pool .
assert.Equal(t, out.IPAM.Config[0].Subnet, "172.60.0.1/16")
delInterface(t, defaultNetworkBridge)
}

func TestServiceWithPredefinedNetwork(t *testing.T) {
defer setupTest(t)()
d := swarm.NewSwarm(t, testEnv)
Expand Down
84 changes: 84 additions & 0 deletions opts/address_pools.go
@@ -0,0 +1,84 @@
package opts

import (
"encoding/csv"
"encoding/json"
"fmt"
"strconv"
"strings"

types "github.com/docker/libnetwork/ipamutils"
)

// PoolsOpt is a Value type for parsing the default address pools definitions
type PoolsOpt struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we really want type PoolsOpt []*types.NetworkToSplit, though I'm not sure why that would be a pointer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@selansen could you have a look at this one? (also why we need a pointer)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had discussion with Brian over phone on this. when we expand this feature to add global scope we will be adding more attribute to this structure and hence keeping struct as it is.

values []*types.NetworkToSplit
}

// UnmarshalJSON fills values structure info from JSON input
func (p *PoolsOpt) UnmarshalJSON(raw []byte) error {
return json.Unmarshal(raw, &(p.values))
}

// Set predefined pools
func (p *PoolsOpt) Set(value string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a unit test.

Copy link
Contributor Author

@selansen selansen Apr 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have newly added 6 more integration tests now to cover all combination.
Will check if any Set functions have unit test and add accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added unit test also for Set function

csvReader := csv.NewReader(strings.NewReader(value))
fields, err := csvReader.Read()
if err != nil {
return err
}

poolsDef := types.NetworkToSplit{}

for _, field := range fields {
parts := strings.SplitN(field, "=", 2)
if len(parts) != 2 {
return fmt.Errorf("invalid field '%s' must be a key=value pair", field)
}

key := strings.ToLower(parts[0])
value := strings.ToLower(parts[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker (should check what we do for other options), but we should consider removing these, and be strict (i.e. only lowercase if that's what the option is named)


switch key {
case "base":
poolsDef.Base = value
case "size":
size, err := strconv.Atoi(value)
if err != nil {
return fmt.Errorf("invalid size value: %q (must be integer): %v", value, err)
}
poolsDef.Size = size
default:
return fmt.Errorf("unexpected key '%s' in '%s'", key, field)
}
}

p.values = append(p.values, &poolsDef)

return nil
}

// Type returns the type of this option
func (p *PoolsOpt) Type() string {
return "pool-options"
}

// String returns a string repr of this option
func (p *PoolsOpt) String() string {
pools := []string{}
for _, pool := range p.values {
repr := fmt.Sprintf("%s %d", pool.Base, pool.Size)
pools = append(pools, repr)
}
return strings.Join(pools, ", ")
}

// Value returns the mounts
func (p *PoolsOpt) Value() []*types.NetworkToSplit {
return p.values
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I just checked: what's needed to make default-address-pools (plural) work inside daemon.json, but --default-address-pool as flag, is to implement the NamedOption interface;

diff --git a/opts/address_pools.go b/opts/address_pools.go
index 1e995c6779..d2cffc0ee5 100644
--- a/opts/address_pools.go
+++ b/opts/address_pools.go
@@ -60,7 +60,7 @@ func (p *PoolsOpt) Set(value string) error {
 
 // Type returns the type of this option
 func (p *PoolsOpt) Type() string {
-       return "default-address-pool"
+       return "pool-options"
 }
 
 // String returns a string repr of this option
@@ -77,3 +77,10 @@ func (p *PoolsOpt) String() string {
 func (p *PoolsOpt) Value() []*types.NetworkToSplit {
        return p.values
 }
+
+// Name returns the flag name of this option
+func (p *PoolsOpt) Name() string {
+       return "default-address-pools"
+}
+
+var _ NamedOption = &PoolsOpt{}

(last line is optional, and only to verify we implement the NamedOption interface)

Adding the above, and both of these will work:

As flag (singular)

dockerd --default-address-pool base=10.10.0.0/16,size=24

In daemon.json (plural):

{"default-address-pools":[{"base":"172.80.0.0/16","size":24},{"base":"172.90.0.0/16","size":24}]}


// Name returns the flag name of this option
func (p *PoolsOpt) Name() string {
return "default-address-pools"
}
20 changes: 20 additions & 0 deletions opts/address_pools_test.go
@@ -0,0 +1,20 @@
package opts // import "github.com/docker/docker/opts"

import (
"testing"
)

func TestAddressPoolOpt(t *testing.T) {
poolopt := &PoolsOpt{}
var addresspool = "base=175.30.0.0/16,size=16"
var invalidAddresspoolString = "base=175.30.0.0/16,size=16, base=175.33.0.0/16,size=24"

if err := poolopt.Set(addresspool); err != nil {
t.Fatal(err)
}

if err := poolopt.Set(invalidAddresspoolString); err == nil {
t.Fatal(err)
}

}