Skip to content

Revert "libnet/d/bridge: port mappings: filter by input iface" - #49310

Merged
akerouanton merged 1 commit into
moby:masterfrom
akerouanton:revert-48721
Jan 20, 2025
Merged

Revert "libnet/d/bridge: port mappings: filter by input iface"#49310
akerouanton merged 1 commit into
moby:masterfrom
akerouanton:revert-48721

Conversation

@akerouanton

@akerouanton akerouanton commented Jan 20, 2025

Copy link
Copy Markdown
Member

Revert:

- What I did

This reverts commit 433b1f9.

#48721 prevents ports mapped on the host to be accessed by containers using the host's IP addresses:

# UDP listen on 6767
$ docker run --rm -it  -p <host-ip>:6767:6767/udp busybox nc -u -lp 6767

# Send "asdf" to the above -- this fails
$ docker run --rm -it  busybox sh -c 'echo asdf | nc -w1 -u <host-ip>  6767'

Another use-case that might break is load-balancing clusters with floating IPs moved around, and routes to backend nodes provisioned on LB nodes as /32 (eg. IPVS + keepalived).

We need to rework that fix to only filter out remote connections destined to loopback addresses.

@vvoland

vvoland commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

For context, this commit broke UDP networking between containers via the host mapped ports:

# UDP listen on 6767
$ docker run --rm -it  -p <host-ip>:6767:6767/udp busybox nc -u -lp 6767

# Send "asdf" to the above
$ docker run --rm -it  busybox sh -c 'echo asdf | nc -w1 -u <host-ip>  6767'

With the reverted commit, the above didn't work.
It did work, when a port wouldn't be explicitly mapped to the host IP:

-docker run --rm -it  -p <host-ip>:6767:6767/udp busybox nc -u -lp 6767
+docker run --rm -it  -p 6767:6767/udp busybox nc -u -lp 6767

@vvoland vvoland left a comment

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.

I confirm it fixes the regression on my side so LGTM (once the CI is green)

This reverts commit 433b1f9.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@thaJeztah thaJeztah added this to the 28.0.0 milestone Jan 20, 2025
@thaJeztah

Copy link
Copy Markdown
Member

After this one's merged, we should probably re-open #45610 (added that in the first comment)

@akerouanton
akerouanton merged commit 0fecf05 into moby:master Jan 20, 2025
@akerouanton
akerouanton deleted the revert-48721 branch January 20, 2025 14:51
akerouanton added a commit to akerouanton/docker that referenced this pull request Jan 22, 2025
This is a regression test for moby#49310.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
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.

4 participants