Allow passing allocation subnet to `docker run` #7436

phemmer · 2014-08-06T06:00:03Z

This is an extension of PR #5829. This PR allows passing a range of addresses to docker run for the IP allocator to pick an address from.

Example usage:

docker run --ip 172.42.44.10 myimage
docker run --ip 172.42.44.10/32 myimage
docker run --ip 172.42.44.0/24 myimage

This obsoletes #5829 as you can pass a specific IP in addition to a subnet/range.

I haven't added anything in the way of documentation. I wanted to put this out in its current form and make requested changes first.

The purpose of this PR is for multi-tenant solutions, where you have multiple products or customers running on a docker host, and you want to be able to add firewall rules per customer. With this, when starting a customer's container, you could pass a subnet, and ensure that their container gets an IP that will be properly matched by a firewall rule or routing policy.

Related: #2801 #6743

…er containers. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)

…creates, to avoid reusing the static IPs Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)

phemmer · 2014-08-06T06:01:14Z

daemon/networkdriver/ipallocator/allocator.go

+		rangeFirstInt      = networkFirstInt
+		rangeLastInt       = networkLastInt
+		subnetAllocatedPos = allocated.last
+		pos                = subnetAllocatedPos - (rangeFirstInt - baseInt) // relative to the start of available addresses


I changed a lot of the variable names as they were extremely confusing to work with. Some of them were integers, some were IPNet, IP, etc.
I can change the names to something else if desired. Just not the originals please :-)

LK4D4 · 2014-08-06T06:07:26Z

Hi, @phemmer . I have similar PR but daemon-wide for multihost installations #6101, it needs some care about docs. There is already similar code for ranges, maybe we can collaborate with our code bases.

phemmer · 2014-08-06T06:25:17Z

@LK4D4 I think they serve different goals, and can co-exist.
The purpose of this PR is for multi-tenant solutions, where you have multiple products or customers running on a docker host, and you want to be able to add firewall rules per customer. With this, when starting a customer's container, you could pass a subnet, and ensure that their container gets an IP that will be properly matched by a firewall rule or routing policy.

updated description to include this

LK4D4 · 2014-08-06T06:41:19Z

@phemmer, Yeah, but one of us will have pretty difficult rebase :D

SvenDowideit · 2014-08-08T02:48:12Z

can you please add documentation in cli.md, the man pages and the networking document.

phemmer · 2014-08-09T16:45:10Z

@ashahab-altiscale that's exactly as it's supposed to behave. This feature allows you to change the IPs docker picks from, not change the IP and subnet of the container. You told it it could pick from anywhere between 172.17.0.0 to 172.17.255.255, and it did.

phemmer · 2014-08-09T18:59:34Z

@SvenDowideit Documentation added.

However the biggest uncertainty I have is the name of the parameter. Right now it's --ip, however unless people read the --help output, and understand that it's meant to be taken literally, it might be confused as for setting the IP & subnet of the container.
Some alternate names I had throught of were --ip-pool and --ip-range. But --ip is also nice and simple, and technically accurate as the name implies nothing about the subnet.

thaJeztah · 2014-08-09T19:09:36Z

Just chiming in that, indeed, --ip might be confusing, especially since the same option for the server has a different meaning. From the same document;

--ip=IP_ADDRESS — see Binding container ports

Not sure what the best name would be though.

ashahab-altiscale · 2014-08-09T19:38:38Z

Yes I realized it right after posting the comment. Great feature!

@ashahab-altiscale https://github.com/ashahab-altiscale that's exactly as
it's supposed to behave. This feature allows you to change the IPs docker
picks from, not change the IP and subnet of the container. You told it it
could pick from anywhere between 172.17.0.0 to 172.17.255.255, and it did.

—
Reply to this email directly or view it on GitHub
#7436 (comment).

jamtur01 · 2014-08-10T17:21:20Z

docs/sources/articles/networking.md

@@ -613,6 +617,20 @@ Docker do all of the configuration:
 At this point your container should be able to perform networking
 operations as usual.

+As mentioned briefly above, `docker run` accepts a `--ip` flag which
+limits the IP pool docker will use to pick the container's IP address


phemmer · 2014-08-10T17:29:31Z

@jamtur01 docker/Docker fixed

thaJeztah · 2014-08-10T17:43:57Z

@phemmer has the decision on naming this --ip been finalised yet?

phemmer · 2014-08-10T17:46:06Z

@thaJeztah I do not know. if that discussion has been continued somewhere besides this PR, I am unaware of it.

thaJeztah · 2014-08-10T17:53:13Z

@phemmer ok, thnx. Think it might be slightly confusing and inconsistent (see my prior comment). But IANTM, just my personal view :). Apart from that, looks like a nice addition.

phemmer · 2014-08-10T18:06:21Z

Well to throw in my $0.02 on the conflict with the daemon's --ip argument:

I think the daemon --ip argument is misnamed. Without reading any --help output or documentation, I would expect such an argument to set the IP of docker (the bridge addr). I think a more appropriate name would be --bind-ip or similar.

Also do we want to go down the route where no argument can share a name between the daemon and docker run unless the action is similar? The --ip case aside, I think that approach is bad.

thaJeztah · 2014-08-10T18:37:23Z

Re-reading the option again, --ip is probably ok. My confusion was mainly caused by the =IP_RANGE. From which I gathered that only a range could be specified, but re-reading the example, I noticed that a single address is allowed as well (a single address is also a range – albeit very limited 😄).

Maybe someone has ideas to make that clearer. IP_ADDRESS | RANGE , IP_RANGE[/SIZE]. Don't have a good suggestion here.

RE: "..no argument can share a name.."
No! Sorry I gave that impression, but in this case one option allows a single ip-address, the other a range. Which supports which may become hard to remember. And I 100% agree that the other option is confusing.

Please don't let my comment block merging of this PR, nothing that cannot be improved on later on!

* commit 'a27bebc': Fixed ipallocator to record static IPs along with the dynamic IPs it creates, to avoid reusing the static IPs Adding '--ip' option to 'docker run' to let users manage IPs for docker containers. Conflicts: daemon/networkdriver/ipallocator/allocator.go runconfig/config_test.go runconfig/parse.go Docker-DCO-1.1-Signed-off-by: Patrick Hemmer <patrick.hemmer@gmail.com> (github: phemmer)

Docker-DCO-1.1-Signed-off-by: Patrick Hemmer <patrick.hemmer@gmail.com> (github: phemmer)

DwayneBull · 2014-08-20T20:28:52Z

I'm new to github, does the "all is well" status mean this will be merged to master soon?

thaJeztah · 2014-08-20T20:43:58Z

@DwayneBull no, "all well" means that all unit-tests and integration-tests passed correctly.

In other words, the changes in this pull request didn't break anything that was working before. However, that doesn't mean that this PR will be merged soon, or even ever. The decision to merge PRs depends on the maintainers agreeing that the PR should be merged.

Just bookmark this page to keep track on progress

DwayneBull · 2014-08-20T21:04:13Z

Ah I see. Sorry I don't know how this all works. Thank you for the information.

ashahab-altiscale · 2014-09-04T18:16:41Z

Is this under review for merge to master? I find this to be very useful.

DwayneBull · 2014-09-23T11:34:34Z

Given that #6101 was merged, is this still valid?

LK4D4 · 2014-09-23T11:58:46Z

@DwayneBull Yup, this is per-container option.

DwayneBull · 2014-09-23T12:56:02Z

@LK4D4 So ip allocation would look like this?
(container --ip) subset of (deamon --fixed-cidr) subset of (deamon --bip)

LK4D4 · 2014-09-23T13:04:30Z

@DwayneBull Yes indeed

ashahab-altiscale · 2014-09-23T16:05:14Z

@phemmer , @LK4D4 , what is needed for this to be merged?

phemmer · 2014-09-23T16:07:43Z

For a project maintainer to take interest in it.
I need to rebase it, but I don't want to keep rebasing it if it's going to be months before it's merged. So once it gets interest, I'll start keeping it maintained.

pesho · 2014-10-21T01:31:06Z

Very much interested in this one. Any maintainer interested in reviewing it? cc @shykes

I think the daemon --ip argument is misnamed.

+1, --ip makes perfect sense as a run argument in this case (unlike the daemon argument)

LK4D4 · 2014-10-21T16:36:29Z

daemon/networkdriver/bridge/driver.go

+			if ipAddr == nil {
+				return job.Error(fmt.Errorf("Invalid IP: %s", requestedIP))
+			}
+			if ipAddr.To4 != nil {


To4 is function which can't be nil. Heh, this is like python :/

Sure it can.

http://golang.org/pkg/net/#IP.To4

If ip is not an IPv4 address, To4 returns nil.

You compare function with nil, not its result.

LK4D4 · 2014-10-21T16:44:02Z

I like this very much. But now it needs rewriting because of merging --fixed-cidr :(

shykes · 2014-11-01T00:56:49Z

Sorry for late review.

All the maintainers agree that this is needed, in some form. There should be a way to configure different network ranges for different containers. The "multi-tenant product" use case you describe is totally valid.
There is a general concern with mixing 2 different types of configuration in the same API. It's not unique to this PR, but reviewing this triggered a long conversation about that problem. Basically, we believe that infrastructure-specific config, like "allocate this IP range", or "use this storage driver", or "mount this directory from the host", should not be exposed over the docker API, because they violate separation of concerns. The Docker API should be used to consume infrastructure - not define it.
In the case of this specific PR, your use case would be enabled by wrapping the docker client, rather than hooking it. Generally we want to encourage hooking instead of wrapping, because you can easily compose hooks, but you cannot compose wrappers.

Because of this, we are suggesting the following:

Don't expose this feature in the API. So, I'm afraid we're going to close this PR. Again, really sorry for coming in so late, I know you've put considerable effort in reacting to code review. FYI we are working on a better design process, so that you can get a design proposal approved first, and then send implementation, which should avoid such waste of time.
Instead, we really really want to make this feature available via hooks. It just so happens that we are starting work on plugins for Docker. We are going to use this use case as the 1st use case for plugins. This will be a joint effort between @erikh (who is handling various network-related improvements atm), @dmcg (who contributed the communication layer we'll use for plugins), @ibuildthecloud (wants plugins to contribute another network feature), @jfrazelle @crosbymichael @dmp42 (doing this review with me), @cpuguy83 (playing with docker+libchan).

TLDR: we're closing this but a shitload of people want to implement it in a different way, and build something very important for the project in the process. You are welcome to join the fun :)

vishh added 2 commits May 16, 2014 17:36

Adding '--ip' option to 'docker run' to let users manage IPs for dock…

e557970

…er containers. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)

Fixed ipallocator to record static IPs along with the dynamic IPs it …

a27bebc

…creates, to avoid reusing the static IPs Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)

phemmer reviewed Aug 6, 2014
View reviewed changes

jamtur01 added the /project/doc label Aug 10, 2014

jamtur01 reviewed Aug 10, 2014
View reviewed changes

phemmer added 3 commits August 14, 2014 00:26

allow passing a subnet for the IP allocator to use

3048415

Docker-DCO-1.1-Signed-off-by: Patrick Hemmer <patrick.hemmer@gmail.com> (github: phemmer)

add documentation for docker run --ip

53f7592

Docker-DCO-1.1-Signed-off-by: Patrick Hemmer <patrick.hemmer@gmail.com> (github: phemmer)

tristanz mentioned this pull request Aug 18, 2014

Extend run config to let users specify an IP #5829

Closed

stuartpb mentioned this pull request Aug 26, 2014

Deploying all apps at startup plushu/plushu-deploy-app-local-container#1

Closed

kenshin54 mentioned this pull request Sep 12, 2014

set up app container networking before containers start weaveworks/weave#47

Closed

LK4D4 reviewed Oct 21, 2014
View reviewed changes

shykes closed this Nov 1, 2014

shykes mentioned this pull request Nov 1, 2014

Adding custom bridge support to docker run #6704

Closed

phemmer mentioned this pull request Nov 7, 2014

Assign IP statically or via DHCP to a container #4952

Closed

dmcg mentioned this pull request Nov 26, 2014

Find a way to stop spamming GitHub user @dmcg #9354

Closed

This was referenced Jan 8, 2015

Feature: Assigning IP and Gateway before the container running #9972

Closed

Feature: Assigning IP and Gateway before the container running #9971

Closed

fmzhen mentioned this pull request Apr 2, 2015

Is there more details about Docker networking plugin #12030

Closed

mavenugo mentioned this pull request Oct 14, 2015

Allow users to specify an IP for their container #6743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing allocation subnet to `docker run` #7436

Allow passing allocation subnet to `docker run` #7436

phemmer commented Aug 6, 2014

phemmer Aug 6, 2014

LK4D4 commented Aug 6, 2014

phemmer commented Aug 6, 2014

LK4D4 commented Aug 6, 2014

SvenDowideit commented Aug 8, 2014

phemmer commented Aug 9, 2014

phemmer commented Aug 9, 2014

thaJeztah commented Aug 9, 2014

ashahab-altiscale commented Aug 9, 2014

jamtur01 Aug 10, 2014

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

DwayneBull commented Aug 20, 2014

thaJeztah commented Aug 20, 2014

DwayneBull commented Aug 20, 2014

ashahab-altiscale commented Sep 4, 2014

DwayneBull commented Sep 23, 2014

LK4D4 commented Sep 23, 2014

DwayneBull commented Sep 23, 2014

LK4D4 commented Sep 23, 2014

ashahab-altiscale commented Sep 23, 2014

phemmer commented Sep 23, 2014

pesho commented Oct 21, 2014

LK4D4 Oct 21, 2014

phemmer Oct 21, 2014

LK4D4 Oct 21, 2014

phemmer Oct 21, 2014

LK4D4 commented Oct 21, 2014

shykes commented Nov 1, 2014

Allow passing allocation subnet to docker run #7436

Allow passing allocation subnet to docker run #7436

Conversation

phemmer commented Aug 6, 2014

phemmer Aug 6, 2014

Choose a reason for hiding this comment

LK4D4 commented Aug 6, 2014

phemmer commented Aug 6, 2014

LK4D4 commented Aug 6, 2014

SvenDowideit commented Aug 8, 2014

phemmer commented Aug 9, 2014

phemmer commented Aug 9, 2014

thaJeztah commented Aug 9, 2014

ashahab-altiscale commented Aug 9, 2014

jamtur01 Aug 10, 2014

Choose a reason for hiding this comment

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

phemmer commented Aug 10, 2014

thaJeztah commented Aug 10, 2014

DwayneBull commented Aug 20, 2014

thaJeztah commented Aug 20, 2014

DwayneBull commented Aug 20, 2014

ashahab-altiscale commented Sep 4, 2014

DwayneBull commented Sep 23, 2014

LK4D4 commented Sep 23, 2014

DwayneBull commented Sep 23, 2014

LK4D4 commented Sep 23, 2014

ashahab-altiscale commented Sep 23, 2014

phemmer commented Sep 23, 2014

pesho commented Oct 21, 2014

LK4D4 Oct 21, 2014

Choose a reason for hiding this comment

phemmer Oct 21, 2014

Choose a reason for hiding this comment

LK4D4 Oct 21, 2014

Choose a reason for hiding this comment

phemmer Oct 21, 2014

Choose a reason for hiding this comment

LK4D4 commented Oct 21, 2014

shykes commented Nov 1, 2014

Allow passing allocation subnet to `docker run` #7436

Allow passing allocation subnet to `docker run` #7436