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

Conversation

@selansen

selansen commented Feb 23, 2018

This is continuation of PR #36054
. I updated all review comments and re-vendor libnetwork. While doing so , I accidentally deleted my commits. Hence I was not able to update into PR 36054.

Description
When user creates a network without specifying a --subnet, docker will pick a subnet for the network from the static set 172.[17-31].0.0/16 and 192.168.[0-240].0/20 for the local scope networks and from the static set 10.[0-255].[0-255].0/24 for the global scope networks.
For different reasons, several users have asked to be able to control these defaults.
This PR brings in a change to allow users to control the default address pools at daemon boot.

As an example,
dockerd --default-address-pools base=10.10.0.0/16,size=24
would allow user to set the 256 pools 10.10.[0-255].0/24 as default for the local scope networks.

Multiple --default-address-pools can be specified.
To specify the option in the config json file:

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

@codecov

This comment has been minimized.

codecov bot commented Feb 24, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@d4e48af). Click here to learn what that means.
The diff coverage is 45.45%.

@@            Coverage Diff            @@
##             master   #36396   +/-   ##
=========================================
  Coverage          ?   35.09%           
=========================================
  Files             ?      615           
  Lines             ?    45745           
  Branches          ?        0           
=========================================
  Hits              ?    16053           
  Misses            ?    27582           
  Partials          ?     2110
@selansen

This comment has been minimized.

selansen commented Feb 24, 2018

@thaJeztah , @dnephin , @vdemeester , Pls use this for #36054.
Final Revendor-ing is done. Libnetwork code is merged into upstream with all required changes.

// NetworkConfig stores the daemon-wide networking configurations
type NetworkConfig struct {
// Default address pools for docker networks
DefaultAddressPools []*ipamutils.NetworkToSplit `json:"default-node-local-address-pools,omitempty"`

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

It looks like this is identical in both unix and windows. Can this go into CommonConfig instead?

This comment has been minimized.

@selansen

selansen Feb 26, 2018

Currently we dont support it on windows.

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

I see you adding this same field in config_windows.go. Should it be removed then?

This comment has been minimized.

@selansen

selansen Feb 26, 2018

My CI for windows p *windowsRS1 * failed due to build issue. so I had to
modify to fix CI failure.

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

Either way there is a problem here.

Either the code needs to be changed so it's not referenced on windows, or this field needs to be moved into a non-platform specific file. It does not make sense to dupliacte it.

func (p *PoolsOpt) Value() []*types.NetworkToSplit {
var pd []*types.NetworkToSplit
pd = append(pd, *p.values...)
return pd

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

Why not return *p.values ? Might also be good to check for nil.

This comment has been minimized.

@selansen

selansen Feb 26, 2018

make sense. will modify

// PoolsOpt is a Value type for parsing the default address pools definitions
type PoolsOpt struct {
values *[]*types.NetworkToSplit

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

I don't think this needs to be a pointer. The PoolsOpt receiver is already a pointer, so you should be able to p.values = append(p.values, ...)

//c.Assert(out, checker.Contains, "175.30.0.0/16")
c.Assert(out.IPAM.Config[0].Subnet, checker.Equals, "175.30.0.0/16")
// Create a bridge network and verify its subnet is the second default pool

This comment has been minimized.

@dnephin

dnephin Feb 26, 2018

Member

It looks like these are separate cases? Can you split this into separate Test functions?

This comment has been minimized.

@selansen

selansen Feb 26, 2018

It is same test case. We are testing it by creating 2 different network and make sure they fall into same user defined subnet. thats all.

@selansen

This comment has been minimized.

selansen commented Feb 27, 2018

@dnephin , Addressed all the comments that we discussed yesterday. Pls take a look at it when you have time.

@dnephin

I guess the tests will fail now. I see why you were using a pointer to a slice.

@@ -15,7 +15,7 @@ type BridgeConfig struct {
// to the docker daemon when you launch it with say: `dockerd -e windows`
type Config struct {
CommonConfig
NetworkConfig

This comment has been minimized.

@dnephin

dnephin Feb 27, 2018

Member

Still not sure why this is being added on windows if you say it isn't used, but at least now the struct isn't duplicated.

This comment has been minimized.

@selansen

selansen Feb 27, 2018

missed this one. will remove it.

@@ -44,4 +44,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(opts.NewPoolsOpt(conf.NetworkConfig.DefaultAddressPools), "default-node-local-address-pools", "Set the default address pools for node specific local networks")

This comment has been minimized.

@dnephin

dnephin Feb 27, 2018

Member

You can remove "Set the". I see we have it for one other flag, but it reads better without it.

This comment has been minimized.

@selansen

selansen Feb 27, 2018

corrected it.

func (p *PoolsOpt) String() string {
pools := []string{}
for _, pool := range p.values {
repr := fmt.Sprintf(" %s %s", pool.Base, pool.Size)

This comment has been minimized.

@dnephin

dnephin Feb 27, 2018

Member

Why the leading space in the format? There's already a space added by strings.Join()

This comment has been minimized.

@selansen

selansen Feb 27, 2018

Corrected it.

// NetworkConfig stores the daemon-wide networking configurations
type NetworkConfig struct {
// Default address pools for docker networks
DefaultAddressPools []*ipamutils.NetworkToSplit `json:"default-node-local-address-pools,omitempty"`

This comment has been minimized.

@dnephin

dnephin Feb 27, 2018

Member

Now I see why you were using a pointer to a slice.

The type here can be PoolsOpt instead of []*ipamutils.NetworkToSplit.

That way you don't need to accept any value in NewPoolsOpt, and to get the value out you can use: DefaultAddressPools.Value()

@thaJeztah thaJeztah added this to backlog in maintainers-session Mar 1, 2018

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Mar 6, 2018

Yup, tests look to be failing;

20:44:12 ----------------------------------------------------------------------
20:44:12 FAIL: docker_api_network_test.go:182: DockerDaemonSuite.TestAPIDaemonNetworkPools
20:44:12 
20:44:12 [d4ac7cc665224] waiting for daemon to start
20:44:12 [d4ac7cc665224] daemon started
20:44:12 
20:44:12 docker_api_network_test.go:199:
20:44:12     //c.Assert(out, checker.Contains, "175.30.0.0/16")
20:44:12     c.Assert(out.IPAM.Config[0].Subnet, checker.Equals, "175.30.0.0/16")
20:44:12 ... obtained string = "172.18.0.0/16"
20:44:12 ... expected string = "175.30.0.0/16"
20:44:12 
20:44:12 [d4ac7cc665224] exiting daemon
@selansen

This comment has been minimized.

selansen commented Mar 6, 2018

@selansen

This comment has been minimized.

selansen commented Mar 14, 2018

@dnephin , All your suggestions have been taken care. Both manual and integration tests are working fine.
@thaJeztah Failures are not due to this changes. I have committed Flaky test issue on separate PR.
Also , libnetwork changes are already merged into moby when you picked up libnetwork for latest fixes.

@dnephin

Thanks for making these changes

LGTM

@StrategistXIV

This comment has been minimized.

StrategistXIV commented Mar 20, 2018

Looks great! I really can't wait to use it in my setup :)

@vdemeester Could you review this as well?

@selansen

This comment has been minimized.

selansen commented Mar 20, 2018

@vdemeester @thaJeztah , I think we have gone through multiple review cycle on this.
Could pls give final blessing and merge it ?

@vdemeester

LGTM 🐸

@Vanuan

This comment has been minimized.

Vanuan commented Mar 21, 2018

Looks like there is a conflict

@selansen

This comment has been minimized.

selansen commented Mar 22, 2018

@thaJeztah , I just did rebase. before it brings another conflict , pls merge it.

@selansen

This comment has been minimized.

selansen commented May 1, 2018

I am going to work on CLI part which needs to be updated.
Let me know where should I follow up with documentation.

@selansen

This comment has been minimized.

selansen commented May 1, 2018

@thaJeztah , updated docker/cli#818.
Hope we are good to merge this PR.

@cpuguy83 cpuguy83 merged commit 82d9185 into moby:master May 3, 2018

7 of 8 checks passed

codecov/patch 45.45% of diff hit (target 50%)
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 40483 has succeeded
Details
janky Jenkins build Docker-PRs 49221 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 9664 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 20621 has succeeded
Details
z Jenkins build Docker-PRs-s390x 9581 has succeeded
Details
@TimWolla

This comment has been minimized.

Contributor

TimWolla commented May 3, 2018

🙌

Is this expected to be included with Docker CE 18.05? I notice that a RC was released 7 days ago and don't know whether new features are shipped after the RC release.

@Vanuan

This comment has been minimized.

Vanuan commented May 3, 2018

🙌 🎆 🎉

@Vanuan

This comment has been minimized.

Vanuan commented May 3, 2018

@TimWolla I think we're still waiting for docker/cli#818 to be merged

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented May 3, 2018

@BretFisher

This comment has been minimized.

BretFisher commented May 3, 2018

Thanks so much, team!

@selansen

This comment has been minimized.

selansen commented May 3, 2018

Thanks to everyone for helping me to push it into upstream!!!!

@dkfi

This comment has been minimized.

dkfi commented May 25, 2018

Is there a way to get notified when this feature will be released?

@vdemeester

This comment has been minimized.

Member

vdemeester commented May 25, 2018

@dkfi should be in 18.06 release 😉

@ToonSpin

This comment has been minimized.

ToonSpin commented Jul 28, 2018

Note that Docker remembers its networks, and this setting is only applied to new networks. If, like me, you are using docker-compose but are still finding routes that do not conform to your settings in the JSON configuration, then executing docker network ls and then docker network rm NETWORK on any offending networks should fix things.

@rca

This comment has been minimized.

rca commented Aug 6, 2018

@mavenugo in the thread above you mention, "given that you identified my earlier comment on this topic." I'm looking to customize the IPAM for swarm network creation without having to specify the subnet for every stack I deploy. Is there, by chance, some trick that can be used until this is implemented for swarm networks as well.

Thank you!

@selansen

This comment has been minimized.

selansen commented Aug 6, 2018

We are working on overlay network support at this moment. New feature will be available in the next release.
only viable option is using "--subnet" while creating network. Looks like you are using the option already

@rca

This comment has been minimized.

rca commented Aug 7, 2018

Yep, that's how I've been getting around this, and I'm looking to get out of the subnet management business. ;)

Thanks for the response, @selansen !

@selansen

This comment has been minimized.

selansen commented Aug 7, 2018

Hopefully soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment