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

Proof of concept: support IPv6 addresses in -p #20315

Closed
wants to merge 1 commit into from

Conversation

stapelberg
Copy link

see issue #11518

I’m aware that I’m changing vendored code, but this way the change is easy for people to play with. Once the discussion reaches agreement, I can open pull requests for the individual repositories.

When running the patched docker daemon, I can use:

# docker run -p 80:80 nginx:1 &
# netstat -lnpt
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name
tcp6       0      0 :::80                   :::*                    LISTEN      752/docker-proxy

And now, with an IPv6 address:

# docker run -p [::1]:80:80 nginx:1 &
# netstat -lnpt
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name
tcp6       0      0 ::1:80                  :::*                    LISTEN      790/docker-proxy

Any feedback welcome.

@GordonTheTurtle GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Feb 14, 2016
@emsi
Copy link

emsi commented Feb 15, 2016

Looks simple and does the trick. :)
Works for me. 👍

@thaJeztah
Copy link
Member

ping @mavenugo (for libnetwork), @calavera (for go-connection)

@thaJeztah
Copy link
Member

Also can you make sure your commits are signed-off, @stapelberg, otherwise we cannot review/merge you PR

@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 16, 2016
@stapelberg
Copy link
Author

@thaJeztah Signed it off now.

@thaJeztah
Copy link
Member

Quite some weird (probably unrelated) failures on WindowsTP4 https://jenkins.dockerproject.org/job/Docker-PRs-WoW-TP4/1047/console. I'll re-trigger that job

@andreaskoch
Copy link

The change does work for me. I can now bind a docker container to a specific IPv6 address.

But there is one difference to binding specific IPv4 addresses. When I bind a container port to a specific IPv4 address - the container will see the correct remote address for all HTTP requests. When I do the same with an IPv6 address the remote address for all requests is always the IP of the docker bridge (172.17.0.1).

Example

If you bind the nginx container to a specific IPv6 adress:

docker run -ti --rm -p [2a03:b0c0:3:d0::137:5002]:80:80 nginx:latest

You will see 172.17.0.1 as the source of the request:

172.17.0.1 - - [18/Feb/2016:21:03:57 +0000] "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36" "-"

If you do the same with an IPv4 address:

docker run -ti --rm -p 46.101.115.244:80:80 nginx:latest

You will see the correct remote IP address and not the IP of the docker bridge as the source of the request:

84.164.250.233 - - [18/Feb/2016:21:08:28 +0000] "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36" "-"

Do you have an idea how to fix that?

@stapelberg
Copy link
Author

@andreaskoch That is an orthogonal issue to what is being addressed in this pull request. (Sorry, don’t have a link to the tracking issue handy.)

@icecrime
Copy link
Contributor

Ping @mavenugo @aboch for review!

Also @stapelberg, please note that some of the changes here belong in another repository, and we won't be able to merge this PR as it is:

08:30:02 ---> Making bundle: validate-vendor (in bundles/1.11.0-dev/validate-vendor)
08:33:48 The result of ./hack/vendor.sh differs
08:33:48 
08:33:48  M vendor/src/github.com/docker/go-connections/nat/nat.go
08:33:48  M vendor/src/github.com/docker/libnetwork/portmapper/mapper.go

@icecrime icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 29, 2016
@stapelberg
Copy link
Author

@icecrime I know, I just wanted some feedback before properly splitting it up :).

if hostIP.To4() != nil {
if err := pm.forward(iptables.Append, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When you submit to libnetwork, remember to also add the .To4() check around the pm.forward() in the following defer statement at line 138.

Copy link
Contributor

Choose a reason for hiding this comment

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

In libnetwork your change to skip the iptables programming for v6 host IP, needed because we have no ip6tables package, may be ok, but would this work when daemon is started with --userland-proxy=false ? Can you give it a try ?

Copy link
Author

Choose a reason for hiding this comment

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

When you submit to libnetwork, remember to also add the .To4() check around the pm.forward() in the following defer statement at line 138.

Done.

In libnetwork your change to skip the iptables programming for v6 host IP, needed because we have no ip6tables package, may be ok, but would this work when daemon is started with --userland-proxy=false ? Can you give it a try ?

I’m not quite sure I get what you mean.

When starting docker with --userland-proxy=false, IPv6 connections don’t work at all:

$ docker run -p 80:80 nginx:1 &
$ curl -v -4 http://localhost:80/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 80 (#0)
> GET / HTTP/1.1
> Host: localhost
> User-Agent: curl/7.46.0
> Accept: */*
> 
< HTTP/1.1 200 OK
[…]
$ curl -v -6 http://localhost:80/
*   Trying ::1...
* Connected to localhost (::1) port 80 (#0)
> GET / HTTP/1.1
> Host: localhost
> User-Agent: curl/7.46.0
> Accept: */*
> 
[timeout]

I was under the impression that IPv6 requires the userland proxy.

In any case, my patch doesn’t change the outcome of this test.

If the test was not what you had in mind, might I ask you to check out this pull request and run the test yourself please?

Copy link

Choose a reason for hiding this comment

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

Using docker with IPv6 is completely different. One has to start docker daemon it with --fixed-cidr-v6= and access each container via it's own ipv6. There's enough addresses to do so :)

Copy link
Contributor

Choose a reason for hiding this comment

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

When starting docker with --userland-proxy=false, IPv6 connections don’t work at all: [..]

Yes that's the point I wanted to make. It will only work if the daemon was started with --userland-proxy=false. If we allow this to go in, it needs to be documented. Be aware there is a PR to have the proxy off by default. But as long as it is documented that user has to enable it for this feature, then we are good.

I was under the impression that IPv6 requires the userland proxy.

That is not strictly true. If you have a routable IPv6 for your container, you can always route up to the container (Sure you may need to set some routes: IPv6 with Docker ).

Also a general question, why do you need the port translation for IPv6 ?

Using docker with IPv6 is completely different. One has to start docker daemon it with --fixed-cidr-v6= and access each container via it's own ipv6.

Not strictly true either. You can now create your bridge network witth

docker network create --subnet <IPv6 pool> <network name>

Copy link

Choose a reason for hiding this comment

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

Also a general question, why do you need the port translation for IPv6 ?

The idea is to have the ability to bind to ipv6 interface only. It's possible with ipv4 but not with ipv6.

@thaJeztah
Copy link
Member

ping @stapelberg can you have a look at the review comments?

fixes issue moby#11518

Signed-off-by: Michael Stapelberg <stapelberg@google.com>
@stapelberg
Copy link
Author

ping @stapelberg can you have a look at the review comments?

Done. What’s the process for vendored code? When should I open up pull requests for docker/go-connections and docker/libnetwork? Should they reference this issue? How do I get the vendored code merged into the docker repository once the PRs were accepted?

@thaJeztah
Copy link
Member

What’s the process for vendored code? When should I open up pull requests for docker/go-connections and docker/libnetwork? Should they reference this issue? How do I get the vendored code merged into the docker repository once the PRs were accepted?

Yes, you can open PR's for those now if you think the changes in those libraries are "stable"; once we move to code review, those PR's can be merged, and you can update make/vendor.sh and run it, to get the "vendor" CI to pass

@thaJeztah
Copy link
Member

ping @aboch do you think we can move this to code review?

@thaJeztah
Copy link
Member

oh! just noticed all changes here are in the libnetwork code; can you reopen this PR in the libnetwork repository?

@thaJeztah thaJeztah closed this Mar 31, 2016
@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 31, 2016
@lenovouser
Copy link

@stapelberg can you resubmit this to docker/libnetwork?

stapelberg added a commit to stapelberg/libnetwork that referenced this pull request Jul 30, 2016
split out of moby/moby#20315
in order to fix moby/moby#11518

Signed-off-by: Michael Stapelberg <stapelberg@google.com>
stapelberg added a commit to stapelberg/go-connections that referenced this pull request Jul 30, 2016
split out of moby/moby#20315
in order to fix moby/moby#11518

Signed-off-by: Michael Stapelberg <stapelberg@google.com>
stapelberg added a commit to stapelberg/go-connections that referenced this pull request Jul 30, 2016
split out of moby/moby#20315
in order to fix moby/moby#11518

Signed-off-by: Michael Stapelberg <stapelberg@google.com>
@stapelberg
Copy link
Author

Done.

@thaJeztah
Copy link
Member

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

Successfully merging this pull request may close these issues.

None yet

10 participants