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 container's link-local addresses #23415

Merged
merged 1 commit into from Jun 14, 2016

Conversation

Projects
None yet
9 participants
@aboch
Contributor

aboch commented Jun 9, 2016

Allows docker containers to fit in deployments which make use of link-local IPs to segregate and expose local host services.

Follows usage example:

~$ docker network create nw1
c5be8936aecc7b276f22a9f3f2f441ef1c33811417d56f044ce6ac2a7a0649dc
~$ docker network create nw2
d05ddc77d6132193988dc0f01beb5fb6dbe2e5119c8595ecce4dda4373fb2e9a
~$ 
~$ docker run -d --name c0 --link-local-ip 169.254.1.1 --net nw1 alpine top
23631c5fc3349176b2dd3edc92b5a442751406edc9cacc5f1ebaf5367da1955f
~$ docker network connect --link-local-ip 169.254.2.2 --link-local-ip fe80::169:254:2:2 nw2 c0
~$ 
~$ docker exec c0 ip addr show eth0 | grep 169
    inet 169.254.1.1/16 scope global eth0
~$ docker exec c0 ip addr show eth1 | grep 169
    inet 169.254.2.2/16 scope global eth1
    inet6 fe80::169:254:2:2/64 scope link 
~$ 
~$ docker stop c0 && docker start c0
c0
c0
~$ docker exec c0 ip addr show eth0 | grep 169
    inet 169.254.1.1/16 scope global eth0
~$ docker exec c0 ip addr show eth1 | grep 169
    inet 169.254.2.2/16 scope global eth1
    inet6 fe80::169:254:2:2/64 scope link 
~$ 
~$ docker run --link-local-ip 192.168.4.4 --net nw1 alpine ifconfig
docker: Error response from daemon: invalid link local IP address: 192.168.4.4.

Link-local IPs are special IPs which belong to a well known subnet and are purely managed by the operator, usually dependent on the architecture where they are deployed. Therefore they are not managed by docker (IPAM driver). Libnetwork will honor the request, only check they are effectively of link-local type, and program them on the container's interface.

Note: Won't build as it depends on docker/libnetwork#1228 and docker/engine-api#269. But want this PR open to start docker changes review process.

Signed-off-by: Alessandro Boch aboch@docker.com

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jun 9, 2016

Contributor

Why have a special option rather than recognising the link local address ranges in --ip and --ip6? (Aside from the issue that I just noticed that those only allow for single addresses not lists, which especially for ipv6 is a bit of an issue).

Contributor

justincormack commented Jun 9, 2016

Why have a special option rather than recognising the link local address ranges in --ip and --ip6? (Aside from the issue that I just noticed that those only allow for single addresses not lists, which especially for ipv6 is a bit of an issue).

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Jun 9, 2016

Contributor

@justincormack

The regular IP addresses are accepted only on user-defined networks with configured subnets, and are maintained by the IPAM driver which ensures their uniqueness. The restriction does not need to apply to link-local addresses.

Even though as you said it could just be an implementation detail to separate the twos, we want to clearly separate link-local addresses from the regular --ip and --ip6 at UX level.

Contributor

aboch commented Jun 9, 2016

@justincormack

The regular IP addresses are accepted only on user-defined networks with configured subnets, and are maintained by the IPAM driver which ensures their uniqueness. The restriction does not need to apply to link-local addresses.

Even though as you said it could just be an implementation detail to separate the twos, we want to clearly separate link-local addresses from the regular --ip and --ip6 at UX level.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jun 9, 2016

Contributor

@aboch ok sounds reasonable.

Contributor

justincormack commented Jun 9, 2016

@aboch ok sounds reasonable.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jun 9, 2016

Contributor

You can vendor in the changes to libnetwork and engine-api if you want to check the CI here.

Contributor

justincormack commented Jun 9, 2016

You can vendor in the changes to libnetwork and engine-api if you want to check the CI here.

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Jun 9, 2016

Contributor

Thanks @justincormack, just re-pushed with manual vendor

Contributor

aboch commented Jun 9, 2016

Thanks @justincormack, just re-pushed with manual vendor

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Jun 9, 2016

Contributor

need rebase

Contributor

aboch commented Jun 9, 2016

need rebase

@justincormack justincormack added this to the 1.12.0 milestone Jun 10, 2016

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 10, 2016

Contributor

The test that verifies the length of the help message failed.

Contributor

cpuguy83 commented Jun 10, 2016

The test that verifies the length of the help message failed.

@aboch

This comment has been minimized.

Show comment
Hide comment
@aboch

aboch Jun 10, 2016

Contributor

Thanks @cpuguy83 , updated

Contributor

aboch commented Jun 10, 2016

Thanks @cpuguy83 , updated

@mbdas

This comment has been minimized.

Show comment
Hide comment
@mbdas

mbdas Jun 10, 2016

👍
This feature is just what we were looking for!

mbdas commented Jun 10, 2016

👍
This feature is just what we were looking for!

@shmurthy62

This comment has been minimized.

Show comment
Hide comment
@shmurthy62

shmurthy62 Jun 10, 2016

This is what we are looking for as well.

shmurthy62 commented Jun 10, 2016

This is what we are looking for as well.

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Jun 14, 2016

Contributor

@aboch can you pls remove the manual vendor-in and rebase to the master ?

Contributor

mavenugo commented Jun 14, 2016

@aboch can you pls remove the manual vendor-in and rebase to the master ?

if ipam != nil && (ipam.IPv4Address != "" || ipam.IPv6Address != "" || len(ipam.LinkLocalIPs) > 0) {
var ipList []net.IP
for _, ips := range ipam.LinkLocalIPs {
if ip := net.ParseIP(ips); ip != nil {

This comment has been minimized.

@icecrime

icecrime Jun 14, 2016

Contributor

Is it ok to silently ignore failures to parse here? I haven't tested, but it seems from reading the code like I could just put anything in --link-local-ip and not get any errors.

@icecrime

icecrime Jun 14, 2016

Contributor

Is it ok to silently ignore failures to parse here? I haven't tested, but it seems from reading the code like I could just put anything in --link-local-ip and not get any errors.

This comment has been minimized.

@aboch

aboch Jun 14, 2016

Contributor

That's true. This is the same behavior we have today for the other ip addresses.

@aboch

aboch Jun 14, 2016

Contributor

That's true. This is the same behavior we have today for the other ip addresses.

This comment has been minimized.

@aboch

aboch Jun 14, 2016

Contributor

I am ok to validate the user specified IP addresses, but it should be done once for all the IPs and probably via a utility function that can be reused and provide a consistent error message. If you are ok with it, I am suggesting to address it as a following bug fix.

@aboch

aboch Jun 14, 2016

Contributor

I am ok to validate the user specified IP addresses, but it should be done once for all the IPs and probably via a utility function that can be reused and provide a consistent error message. If you are ok with it, I am suggesting to address it as a following bug fix.

This comment has been minimized.

@icecrime

icecrime Jun 14, 2016

Contributor

Works for me, thanks.

@icecrime

icecrime Jun 14, 2016

Contributor

Works for me, thanks.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jun 14, 2016

Contributor

LGTM

Contributor

icecrime commented Jun 14, 2016

LGTM

Show outdated Hide outdated docs/reference/run.md
Allow user to specify container's link-local addresses
Signed-off-by: Alessandro Boch <aboch@docker.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 14, 2016

Contributor

LGTM

Contributor

crosbymichael commented Jun 14, 2016

LGTM

@crosbymichael crosbymichael merged commit c97fdbe into moby:master Jun 14, 2016

4 of 8 checks passed

documentation fail
Details
experimental Jenkins build Docker-PRs-experimental 19990 has failed
Details
userns Jenkins build Docker-PRs-userns 10945 is running
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 3273 is running
Details
docker/dco-signed All commits signed
Details
gccgo Jenkins build Docker-PRs-gccgo 6856 has succeeded
Details
janky Jenkins build Docker-PRs 28786 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 27374 has succeeded
Details

@aboch aboch deleted the aboch:ll branch Dec 1, 2016

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