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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 25 additions & 3 deletions vendor/src/github.com/docker/go-connections/nat/nat.go
View file
Open in desktop
Expand Up @@ -146,9 +146,31 @@ func ParsePortSpecs(ports []string) (map[Port]struct{}, map[Port][]PortBinding,
rawPort = fmt.Sprintf(":%s", rawPort)
}

parts, err := PartParser(portSpecTemplate, rawPort)
if err != nil {
return nil, nil, err
parts := make(map[string]string)
if rawPort != "" && rawPort[0] == '[' {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to see if we do really need the square brackets.

The -p value can be:

  2. :
  3. ::

In all the cases, I think, we need to
a) If no : are present in value, then ContainerPort=value, exit
b) else split value by : into N elements, ContainerPort=, HostPort=<(N-1)th element>
if len(elements) == 3, then parse the IPv4 host address from the first element, exit
else if len(elements) > 3 join the first N-2 elements in one string and parse the IPv6 from it, exit
else exit

Copy link
Member

Choose a reason for hiding this comment

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

I saw this PR #20842

So wondering if we can make use of the standard net functions here, e.g. net. SplitHostPort()

Copy link
Author

Choose a reason for hiding this comment

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

I am trying to see if we do really need the square brackets.

Even if we don’t need them, I think it makes sense to use them. Using square brackets for separating IPv6 addresses from ports is what all other programs do, so there’s a strong argument for consistency.

So wondering if we can make use of the standard net functions here, e.g. net. SplitHostPort()

I don’t see what using net.SplitHostPort() would buy us in this case. I don’t think it makes the code any shorter or handles any more cases than what we handle currently. If you disagree, can you provide a code snippet to illustrate what you mean please?

Copy link

Choose a reason for hiding this comment

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

Even if we don’t need them, I think it makes sense to use them. Using square brackets for separating IPv6 addresses from ports is what all other programs do, so there’s a strong argument for consistency.

Sticking to ipv6 literal notation is very good idea. It's too easy to get lost in plethora of colons without square brackets (not to mention that the number of colons in ipv6 can vary).
https://www.ietf.org/rfc/rfc2732.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. It's mainly for readability. Thanks for the pointer to the ipv6 literal.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t see what using net.SplitHostPort() would buy us in this case. I don’t think it makes the code any shorter or handles any more cases than what we handle currently.

I didn't delve into it deeply; but thought to mention it, in case that code was better tested and/or handled corner case that we didn't think of.

ipEnd := strings.Index(rawPort, "]")
if ipEnd == -1 {
return nil, nil, fmt.Errorf("IP address starts with \"[\", but corresponding \"]\" was not found")
}
parts["ip"] = rawPort[1:ipEnd]
if ipEnd >= len(rawPort)-2 {
return nil, nil, fmt.Errorf("port spec too short (expected at least 2 characters after \"]\"): %q", rawPort)
}
if rawPort[ipEnd+1] != ':' {
return nil, nil, fmt.Errorf("\"]\" must be followed by \":\": %q", rawPort)
}
nonIpParts := strings.Split(rawPort[ipEnd+2:], ":")
if len(nonIpParts) != 2 {
return nil, nil, fmt.Errorf("got %d parts, wanted 2: %q", len(nonIpParts), rawPort[ipEnd+2:])
}
parts["hostPort"] = nonIpParts[0]
parts["containerPort"] = nonIpParts[1]
} else {
var err error
parts, err = PartParser(portSpecTemplate, rawPort)
if err != nil {
return nil, nil, err
}
}

var (
Expand Down
14 changes: 9 additions & 5 deletions vendor/src/github.com/docker/libnetwork/portmapper/mapper.go
View file
Open in desktop
Expand Up @@ -128,16 +128,20 @@ func (pm *PortMapper) MapRange(container net.Addr, hostIP net.IP, hostPortStart,
}

containerIP, containerPort := getIPAndPort(m.container)
if err := pm.forward(iptables.Append, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil {
return nil, err
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.

}

cleanup := func() error {
// need to undo the iptables rules before we return
m.userlandProxy.Stop()
pm.forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort)
if err := pm.Allocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil {
return err
if hostIP.To4() != nil {
pm.forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort)
if err := pm.Allocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil {
return err
}
}

return nil
Expand Down