close
Skip to content

Fix IPv6 Port Forwarding for the Bridge Driver#2604

Merged
arkodg merged 1 commit intomoby:masterfrom
arkodg:fix-port-forwarding
Dec 15, 2020
Merged

Fix IPv6 Port Forwarding for the Bridge Driver#2604
arkodg merged 1 commit intomoby:masterfrom
arkodg:fix-port-forwarding

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Dec 14, 2020

  1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
    ContainerPort) based on the input and system parameters
  2. Update the userland proxy as well as dummy proxy (inside port mapper) to
    specifically listen to either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta arko.dasgupta@docker.com

@arkodg arkodg requested a review from thaJeztah December 14, 2020 00:04
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Dec 14, 2020

cc @bboehmke

Copy link
Copy Markdown
Contributor

@bboehmke bboehmke left a comment

Choose a reason for hiding this comment

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

The implementation looks far better and cleaner as my try.

Comment thread drivers/bridge/port_mapping.go Outdated
pb, err := n.allocatePortsInternal(ep.extConnConfig.PortBindings, ep.addr.IP, defHostIP, ulPxyEnabled)
if err != nil {
return nil, err
var containerIPv6 net.IP = nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it necessary to set containerIPv6 explicit to nil?

var containerIPv6 net.IP should already be nil by default

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ack, thanks

}

// IPv6 port binding excluding user land proxy
if n.driver.config.EnableIP6Tables && ep.addrv6 != nil {
Copy link
Copy Markdown
Contributor

@bboehmke bboehmke Dec 14, 2020

Choose a reason for hiding this comment

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

The EnableIP6Tables check was removed completely.

Is there anything that prevents a creation of ip6tables rules in AppendForwardingTableEntry of the portmapper if EnableIP6Tables is not set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If

n.portMapperV6.SetIptablesChain(natChain, n.getNetworkBridgeName())

is skipped, pm.chain will be nil and will skip the iptables programming
if pm.chain == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh I see. That is for sure far cleaner than before.

@arkodg arkodg changed the title Fix Port Forwarding for the Bridge Driver Fix IPv6 Port Forwarding for the Bridge Driver Dec 15, 2020
@arkodg arkodg force-pushed the fix-port-forwarding branch 2 times, most recently from 75f4191 to f0cba0e Compare December 15, 2020 02:30
1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
ContainerPort) based on the input and system parameters
2. Update the userland proxy as well as dummy proxy (inside port mapper) to
specifically listen on either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
@arkodg arkodg force-pushed the fix-port-forwarding branch from f0cba0e to 28576a4 Compare December 15, 2020 02:46
@bboehmke
Copy link
Copy Markdown
Contributor

LGTM

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Dec 15, 2020

thanks @bboehmke can you please formally approve the PR, so I can merge it and vendor it into moby/moby

@arkodg arkodg merged commit fa125a3 into moby:master Dec 15, 2020
@arkodg arkodg deleted the fix-port-forwarding branch December 15, 2020 16:25
@bboehmke bboehmke mentioned this pull request Dec 15, 2020
3 tasks
arkodg pushed a commit to arkodg/moby that referenced this pull request Dec 15, 2020
Brings in moby/libnetwork#2604

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Dec 23, 2020
Brings in moby/libnetwork#2604

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
Upstream-commit: 78eafdd947db80832c6d0985c81cb56944a23739
Component: engine
Comment thread cmd/proxy/sctp_proxy.go
listener, err := sctp.ListenSCTP("sctp", frontendAddr)
// detect version of hostIP to bind only to correct version
ipVersion := ipv4
if frontendAddr.IPAddrs[0].IP.To4() == nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we currently don't support IPv4-mapped IPv6 addresses today, we build out a separate ipv4 and ipv6 stack

@vin01
Copy link
Copy Markdown

vin01 commented Jan 5, 2021

1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
   ContainerPort) based on the input and system parameters

2. Update the userland proxy as well as dummy proxy (inside port mapper) to
   specifically listen to either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta arko.dasgupta@docker.com

This looks like a breaking change since until now docker-proxy has been binding to both v4 and v6 addresses if both are available on the system which I believe made sense.

@fbezdeka
Copy link
Copy Markdown

fbezdeka commented Jan 5, 2021

According to the Changelog [1] this is the only network related change in 20.10.2.
Now my IPv6 port forwarding is completely broken.
Downgrading to 20.10.1 fixed it.

There are no docker-proxy processes started. 20.10.1 starts a process for each port-forwarding.

Example:

sudo docker run \
    --hostname gitlab \
    --env 'GITLAB_PORT=443' \
    --env 'GITLAB_HTTPS=true' \
    --publish [<ip>]:2222:22 \
    --publish [<ip>]:8080:80 \
    --publish [<ip>]:8443:443 \
    --name gitlab \
    --restart always \
    --volume /srv/gitlab/config:/etc/gitlab:Z \
    --volume /srv/gitlab/logs:/var/log/gitlab:Z \
    --volume /srv/gitlab/data:/var/opt/gitlab:Z \
    gitlab/gitlab-ce:latest

[1] https://docs.docker.com/engine/release-notes/

@pokotiX
Copy link
Copy Markdown

pokotiX commented Jan 5, 2021

According to the Changelog [1] this is the only network related change in 20.10.2.
Now my IPv6 port forwarding is completely broken.
Downgrading to 20.10.1 fixed it.

There are no docker-proxy processes started. 20.10.1 starts a process for each port-forwarding.

Example:

sudo docker run \
    --hostname gitlab \
    --env 'GITLAB_PORT=443' \
    --env 'GITLAB_HTTPS=true' \
    --publish [<ip>]:2222:22 \
    --publish [<ip>]:8080:80 \
    --publish [<ip>]:8443:443 \
    --name gitlab \
    --restart always \
    --volume /srv/gitlab/config:/etc/gitlab:Z \
    --volume /srv/gitlab/logs:/var/log/gitlab:Z \
    --volume /srv/gitlab/data:/var/opt/gitlab:Z \
    gitlab/gitlab-ce:latest

[1] https://docs.docker.com/engine/release-notes/

I confirm, I have the same problem

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jan 5, 2021

@pokotiV @fbezdeka @vin01 can you please raise a separate issue to discuss this
thanks

@terencehonles
Copy link
Copy Markdown

@pokotiV @fbezdeka @vin01 can you please raise a separate issue to discuss this
thanks

@arkodg done in #2607 (I believe the title captures the problem that everyone was seeing (it does mine))

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants