- Notifications
You must be signed in to change notification settings - Fork2.9k
libpod: bind both IPv4 and IPv6 addresses with dual-stack wildcard#27311
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow ourrelease note process to remove it. Instructions for interacting with me using PR comments are availablehere. If you have questions or suggestions related to my behavior, please file an issue against thekubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR isNOT APPROVED This pull-request has been approved by:danishprakash The full list of commands accepted by this bot can be foundhere. Needs approval from an approver in each of these files: Approvers can indicate their approval by writing |
libpod/oci_util.go Outdated
| f6,err:=bindPort(protocol,"",port.HostPort+i,true,&sctpWarning) | ||
| iferr!=nil { | ||
| logrus.Warnf("Failed to bind IPv6 for port %d, continuing with IPv4 only: %v",port.HostPort+i,err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| logrus.Warnf("Failed to bind IPv6 for port %d, continuing with IPv4 only: %v",port.HostPort+i,err) | |
| logrus.Warnf("failed to bind IPv6 for port %d, continuing with IPv4 only: %v",port.HostPort+i,err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, logrus statements are generally expected to start upper case while actual errors should be lower case. Though there is nor formal policy.
libpod/oci_util.go Outdated
| } | ||
| f6,err:=bindPort(protocol,"",port.HostPort+i,true,&sctpWarning) | ||
| iferr!=nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be erred and not warn ? Some users might be expecting bind onv6 explicitly ?
mheon commentedOct 17, 2025
I would lean towards retaining current behavior for binding ports and just changing the bind to the port to be IPv4 only - but, if we do want to do this, the Podman 6 merge window opens next week, giving us the opportunity to make breaking changes like this. |
Luap99 commentedOct 20, 2025
I disagree, binding v4 only is a bug as we do not correctly prevent another v6 application on the host from taking the port. |
Luap99 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't like to have two sockets for v6 and v4, that is just an extra cpu/memory hit. The way it should be:
- no HostIP: bind dual stack, e.g. tcp proto
- ipv4 HostIP (including
0.0.0.0): bind v4, e.g. tcp4 - ipv6 HostIP (including
::): bind v6 only, e.g. tcp
Now there are several other problems with port bindings that I would love to fix for v6, not for this PR though.
- There is the bug of forcing
0.0.0.0in parseSplitPort() when passing ports on the cli which is not right then and should be removed. Additionally there are many cases where 0.0.0.0 is added as hostip in inspect and other display commands such as podman port. That isn't really right and needs to be figured out. - Port binding happens does not happen inside the network interface setup where it should be ideally. The problem is that doesn't work due PostConfigureNetNS and the complication that ports must be bound before we start conmon but the netns must be configured afterwards so there isn't a way to achieve this right now.
- Using the golang std library API to bind the ports is problematic because it calls listen() which we really should not. That way connections that do not get redirected via firewall get buffered by the kernel and as such hang, i.e. we see that when the firewall rules get flushed. It would be best to just bind the ports.
mheon commentedOct 20, 2025
Second and third points sound like there would be benefits to offloading the port binding to Conmon - just give it a list of things to listen on, instead of passing an arbitrary number of FDs for ports |
Luap99 commentedOct 20, 2025
That doesn't work, the network setup without a userns (PostConfigureNetNS) happens before conmon is even started but ports have to be bound before the network setup. That is what I meant point 2 is basically unfixable because of these two different order that I tried to remove int he past but doesn't work due the somewhat broken checkpoint/restore support. |
3f90ff3 to6555004Comparedanishprakash commentedOct 24, 2025
By default, podman will now bind dual-stack unless or until an IP is specified; in that case, podman parses and determines the protocol (as was the case before) and uses either [tcp|udp]4 or 6. The problem with 0.0.0.0 still lies, perhaps can be discussed in another PR given the impact on compat that would have. Though netavark currently fails to properly set up nftable rules with rootless containers: |
Currently podman only binds IPv4 wildcard address. But sincegolang net.Listen("tcp",) listens on all IPv4 and IPv6 addresses,we (intentionally) handle both 4 and 6 here. But the downside was that youcould still explicitly request an IPv6 (::) wildcard along with adual-stack("") wildcard causing netavark to leak nftable rules.This change handles both IPv4(0.0.0.0) and IPv6(::) when no address ispassed, and throws EADDRINUSE if an explicit IPv4 or IPv6 is requestedon top of dual-stack wildcard.Signed-off-by: Danish Prakash <contact@danishpraka.sh>6555004 tob76d3b7Compare| isV6:=false | ||
| ifport.HostIP!="" { | ||
| isV6=net.ParseIP(port.HostIP).To4()==nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why was this moved, this is doing the same check now inside a potentially big loop doing unnecessary work over and over again.
| // close all open ports in case of early error so we do not | ||
| // rely garbage collector to close them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this comment should not be deleted
| # Refer https://github.com/containers/netavark/issues/1338 | ||
| # bats test_tags=ci:parallel | ||
| @test"podman run - dual-stack port binding" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That doesn't seem to actually test anything new over existing port forwarding tests. I see nothing here actually testing we bind a dual stack socket. OVerall I don't see why this test would be needed at all when we have the conflict test below
| # Refer https://github.com/containers/netavark/issues/1338 | ||
| # bats test_tags=ci:parallel | ||
| @test"podman run - dual-stack conflicts with explicit wildcards" { | ||
| skip_if_rootless"port reservation only works as root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well that seems a bit misleading, while the port reversion is only used as root I would expect rootless to fail in the exact same way, no? So I see no reason to skip this test rootless.
Opening this pull request to continue the discussion fromcontainers/netavark#1338
Currently podman only binds IPv4 wildcard address. But since golang net.Listen("tcp",) listens on all IPv4 and IPv6 addresses, we (intentionally) handle both 4 and 6 here. But the downside was that you could still explicitly request an IPv6 (::) wildcard along with a dual-stack("") wildcard causing netavark to leak nftable rules. This change handles both IPv4(0.0.0.0) and IPv6(::) when no address is passed, and throws EADDRINUSE if an explicit IPv4 or IPv6 is requested on top of dual-stack wildcard.
cc/@Luap99