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

fix port forwarding with ipv6.disable=1 #2635

Merged
merged 1 commit into from
May 25, 2021

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Apr 26, 2021

Make docker run -p 80:80 functional again on environments with kernel boot parameter ipv6.disable=1.

Fix moby/moby#42288
fixes #2629 Network issue with IPv6 following update to version 20.10.6


Tested on Ubuntu 21.04.

Previously, docker run -p 80:80 with ipv6.disable=1 was failing with an error:
docker: Error response from daemon: driver failed programming external connectivity on endpoint naughty_moore (038e9ed4b5ea77e1c52462d6d04ad001fbad9beb185a6511aadc217c8a271608): Error starting userland proxy: listen tcp6 [::]:80: socket: address family not supported by protocol.

@@ -50,6 +51,12 @@ func (n *bridgeNetwork) allocatePortsInternal(bindings []types.PortBinding, cont
bs = append(bs, bIPv4)
}

// skip adding v6 addr when the kernel was booted with `ipv6.disable=1`
// https://github.com/moby/moby/issues/42288
if !IsV6Listenable() {
Copy link
Member

Choose a reason for hiding this comment

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

The only case we need to fix here (I think...) is when 0.0.0.0 is passed.
If someone specifies a v6 address and we can't do that I don't think we should ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

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 understand what was fixed.
I think the check for IsV6Listenable() (or whwatever its called now) should go into https://github.com/moby/libnetwork/pull/2635/files#diff-5e6a1e1b6a7570d4362c2cfd293b0f05b409b6c9b1c8157b9832f427f6e0cf56L109 explicitly where we are checking if the bind address is 0.0.0.0

Copy link
Member Author

Choose a reason for hiding this comment

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

isV6Binding := c.HostIP != nil && c.HostIP.To4() == nil is set to false when the bind address is 0.0.0.0, and set to true when the bind address is set to IPv6 explicitly, so the issue was fixed IIUC.

Test matrix:

  • docker run -p 80:80: listens on 0.0.0.0:80, without an error
  • docker run -p 0.0.0.0:80:80: listens on 0.0.0.0:80, without an error
  • docker run -p [::]:80:80: raises an error

Copy link
Member

Choose a reason for hiding this comment

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

My point is we are already checking for 0.0.0.0 when validating the bind address, why not add the check when we do that?

Copy link
Member

Choose a reason for hiding this comment

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

It's also a bit less confusing because it is checking net.IPv4zero specifically.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that, but couldn't make it work

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be a stickler but I don't understand the check you have in place and how it works.
Meanwhile: https://play.golang.org/p/jtQGBQ4yzWd

Copy link
Member

Choose a reason for hiding this comment

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

It should be able to implicitBinding := hostIP != nil && hostIp.Equal(net.IPv4zero)

Copy link
Member

Choose a reason for hiding this comment

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

@tianon helped me unbreak my brain here.
This seems like it's ok because we are checking if it is a v4 binding and only continuing if it is a v6-only binding which makes sense. It may also be that we actually have a nil host IP...

Make `docker run -p 80:80` functional again on environments with kernel boot parameter `ipv6.disable=1`.

Fix moby/moby issue 42288

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Member Author

@moby/libnetwork @thaJeztah @tiborvass PTAL

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Maybe a unit test for this?

Probably need to hijack the v6ListenableOnce in the test.

@arkodg
Copy link
Contributor

arkodg commented May 3, 2021

might be a better place to reuse some of the validation above and then check IsV6Listenable

@AkihiroSuda
Copy link
Member Author

Help wanted for testing.

Anyway, would it be possible to release v20.10.7 to provide the quick fix, and revisit the testing plan later?
cc @thaJeztah @tiborvass

@thaJeztah
Copy link
Member

@AkihiroSuda could you have a look at @arkodg 's suggestion? If we're able to move the check inside that function, we can probably add a unit test for it at least

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request May 14, 2021
docker-engine 20.10.6 broke container port forwarding for hosts without IPv6
support:

docker: Error response from daemon: driver failed programming external
connectivity on endpoint naughty_moore
(038e9ed4b5ea77e1c52462d6d04ad001fbad9beb185a6511aadc217c8a271608): Error
starting userland proxy: listen tcp6 [::]:80: socket: address family not
supported by protocol.

Add a libnetwork patch from an upstream pull request to fix this, after
adjusting the patch to apply to docker-engine (which has libnetwork vendored
under vendor/github.com/docker/libnetwork):

- moby/libnetwork#2635,
- moby/moby#42322

Signed-off-by: Peter Korsgaard <[email protected]>
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request May 17, 2021
docker-engine 20.10.6 broke container port forwarding for hosts without IPv6
support:

docker: Error response from daemon: driver failed programming external
connectivity on endpoint naughty_moore
(038e9ed4b5ea77e1c52462d6d04ad001fbad9beb185a6511aadc217c8a271608): Error
starting userland proxy: listen tcp6 [::]:80: socket: address family not
supported by protocol.

Add a libnetwork patch from an upstream pull request to fix this, after
adjusting the patch to apply to docker-engine (which has libnetwork vendored
under vendor/github.com/docker/libnetwork):

- moby/libnetwork#2635,
- moby/moby#42322

Signed-off-by: Peter Korsgaard <[email protected]>
(cherry picked from commit 2fd3390)
Signed-off-by: Peter Korsgaard <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM to get the fix in, but we should have a look at cleaning up in a follow up

@thaJeztah
Copy link
Member

@cpuguy83 @arkodg ok with you to merge this?

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Did some manual validation here to at least check the logic.

@thaJeztah
Copy link
Member

let me bring this one in and open a vendor PR in moby

@thaJeztah thaJeztah merged commit 64b7a45 into moby:master May 25, 2021
akerouanton added a commit to akerouanton/docker that referenced this pull request Nov 14, 2021
Since moby/libnetwork#2635 has been merged, allocatePortsInternal()
checks if IPv6 is enabled by calling IsV6Listenable(). This function
calls `net.Listen("tcp6", "[::1]:0")` and returns false when
net.Listen() fails.

TestPortMappingV6Config() starts by setting up a new net ns to run into
it. The loopback interface is not bring up in this net ns, thus
net.Listen() fails and IsV6Listenable() returns false. This change takes
care of bringing loopback iface up right after moving to the new net ns.

This test has been reported has flaky on s390x in moby#42468. For some
reason, this test seems to be consistently green on the CI (on amd64
arch) and when running `hack/test/unit` locally. However it consistently
fails when running `TESTFLAGS='-shuffle on' hack/test/unit` locally.

Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton added a commit to akerouanton/docker that referenced this pull request Nov 16, 2021
Since moby/libnetwork#2635 has been merged, allocatePortsInternal()
checks if IPv6 is enabled by calling IsV6Listenable(). This function
calls `net.Listen("tcp6", "[::1]:0")` and returns false when
net.Listen() fails.

TestPortMappingV6Config() starts by setting up a new net ns to run into
it. The loopback interface is not bring up in this net ns, thus
net.Listen() fails and IsV6Listenable() returns false. This change takes
care of bringing loopback iface up right after moving to the new net ns.

This test has been reported has flaky on s390x in moby#42468. For some
reason, this test seems to be consistently green on the CI (on amd64
arch) and when running `hack/test/unit` locally. However it consistently
fails when running `TESTFLAGS='-shuffle on' hack/test/unit` locally.

Signed-off-by: Albin Kerouanton <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Nov 19, 2021
Since moby/libnetwork#2635 has been merged, allocatePortsInternal()
checks if IPv6 is enabled by calling IsV6Listenable(). This function
calls `net.Listen("tcp6", "[::1]:0")` and returns false when
net.Listen() fails.

TestPortMappingV6Config() starts by setting up a new net ns to run into
it. The loopback interface is not bring up in this net ns, thus
net.Listen() fails and IsV6Listenable() returns false. This change takes
care of bringing loopback iface up right after moving to the new net ns.

This test has been reported has flaky on s390x in #42468. For some
reason, this test seems to be consistently green on the CI (on amd64
arch) and when running `hack/test/unit` locally. However it consistently
fails when running `TESTFLAGS='-shuffle on' hack/test/unit` locally.

Signed-off-by: Albin Kerouanton <[email protected]>
Upstream-commit: c721bad8ccddeb353e71d4b4b26ad878d1ae8bee
Component: engine
evol262 pushed a commit to evol262/moby that referenced this pull request Jan 12, 2022
Since moby/libnetwork#2635 has been merged, allocatePortsInternal()
checks if IPv6 is enabled by calling IsV6Listenable(). This function
calls `net.Listen("tcp6", "[::1]:0")` and returns false when
net.Listen() fails.

TestPortMappingV6Config() starts by setting up a new net ns to run into
it. The loopback interface is not bring up in this net ns, thus
net.Listen() fails and IsV6Listenable() returns false. This change takes
care of bringing loopback iface up right after moving to the new net ns.

This test has been reported has flaky on s390x in moby#42468. For some
reason, this test seems to be consistently green on the CI (on amd64
arch) and when running `hack/test/unit` locally. However it consistently
fails when running `TESTFLAGS='-shuffle on' hack/test/unit` locally.

Signed-off-by: Albin Kerouanton <[email protected]>
ulbricht-inr pushed a commit to InnoRoute/buildroot that referenced this pull request Apr 23, 2022
docker-engine 20.10.6 broke container port forwarding for hosts without IPv6
support:

docker: Error response from daemon: driver failed programming external
connectivity on endpoint naughty_moore
(038e9ed4b5ea77e1c52462d6d04ad001fbad9beb185a6511aadc217c8a271608): Error
starting userland proxy: listen tcp6 [::]:80: socket: address family not
supported by protocol.

Add a libnetwork patch from an upstream pull request to fix this, after
adjusting the patch to apply to docker-engine (which has libnetwork vendored
under vendor/github.com/docker/libnetwork):

- moby/libnetwork#2635,
- moby/moby#42322

Signed-off-by: Peter Korsgaard <[email protected]>
(cherry picked from commit 2fd3390)
Signed-off-by: Peter Korsgaard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants