Skip to content

Allow user to specify container's link-local addresses#23415

Merged
crosbymichael merged 1 commit intomoby:masterfrom
aboch:ll
Jun 14, 2016
Merged

Allow user to specify container's link-local addresses#23415
crosbymichael merged 1 commit intomoby:masterfrom
aboch:ll

Conversation

@aboch
Copy link
Contributor

@aboch aboch commented Jun 9, 2016 •
edited
Loading

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 moby/libnetwork#1228 and docker-archive-public/docker.engine-api#269. But want this PR open to start docker changes review process.

Signed-off-by: Alessandro Boch [email protected]

@justincormack
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

justincormack commented Jun 9, 2016

@aboch ok sounds reasonable.

@justincormack
Copy link
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
Copy link
Contributor Author

aboch commented Jun 9, 2016

Thanks @justincormack, just re-pushed with manual vendor

@aboch
Copy link
Contributor Author

aboch commented Jun 9, 2016

need rebase

@justincormack justincormack added this to the 1.12.0 milestone Jun 10, 2016
@cpuguy83
Copy link
Member

cpuguy83 commented Jun 10, 2016

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

@aboch
Copy link
Contributor Author

aboch commented Jun 10, 2016

Thanks @cpuguy83 , updated

@mbdas
Copy link

mbdas commented Jun 10, 2016 •
edited
Loading

👍
This feature is just what we were looking for!

@shmurthy62
Copy link

shmurthy62 commented Jun 10, 2016

This is what we are looking for as well.

@mavenugo
Copy link
Contributor

mavenugo commented Jun 14, 2016

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

icecrime
icecrime reviewed Jun 14, 2016
View reviewed changes
container/container.go Outdated
Copy link
Contributor

@icecrime icecrime Jun 14, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@aboch aboch Jun 14, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@aboch aboch Jun 14, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@icecrime icecrime Jun 14, 2016

Choose a reason for hiding this comment

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

Works for me, thanks.

@icecrime
Copy link
Contributor

icecrime commented Jun 14, 2016

LGTM

icecrime
icecrime reviewed Jun 14, 2016
View reviewed changes
docs/reference/run.md Outdated
Copy link
Contributor

@icecrime icecrime Jun 14, 2016

Choose a reason for hiding this comment

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

There a 1 space offset on the 'bridge' line below :-(

Copy link
Contributor Author

@aboch aboch Jun 14, 2016

Choose a reason for hiding this comment

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

give me a sec

Copy link
Contributor Author

@aboch aboch Jun 14, 2016

Choose a reason for hiding this comment

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

should be ok now

@crosbymichael
Copy link
Contributor

crosbymichael commented Jun 14, 2016

LGTM

@crosbymichael crosbymichael merged commit c97fdbe into moby:master Jun 14, 2016
@albers albers mentioned this pull request Jun 21, 2016
Merged
@sdurrheimer sdurrheimer mentioned this pull request Jun 23, 2016
Merged
This was referenced Jun 24, 2016
Closed
Closed
@aboch aboch deleted the ll branch December 1, 2016 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

1.12.0

Development

Successfully merging this pull request may close these issues.

9 participants

@aboch @justincormack @cpuguy83 @mbdas @shmurthy62 @mavenugo @icecrime @crosbymichael @GordonTheTurtle