Skip to content

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

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

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

Conversation

@akerouanton
Copy link
Copy Markdown
Member

@akerouanton akerouanton commented Jan 20, 2025

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
Copy link
Copy Markdown
Contributor

vvoland commented Jan 20, 2025

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

Copy link
Copy Markdown
Contributor

@vvoland vvoland left a comment

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)

@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 <[email protected]>
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