Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 27 additions & 20 deletions libpod/oci_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,15 @@ func bindPorts(ports []types.PortMapping) ([]*os.File, error) {
var files []*os.File
sctpWarning := true
for _, port := range ports {
isV6 := net.ParseIP(port.HostIP).To4() == nil
if port.HostIP == "" {
isV6 = false
}
protocols := strings.SplitSeq(port.Protocol, ",")
for protocol := range protocols {
for i := uint16(0); i < port.Range; i++ {
isV6 := false
if port.HostIP != "" {
isV6 = net.ParseIP(port.HostIP).To4() == nil
}
Comment on lines +38 to +41
Copy link
Member

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.

f, err := bindPort(protocol, port.HostIP, port.HostPort+i, isV6, &sctpWarning)
if err != nil {
// close all open ports in case of early error so we do not
// rely garbage collector to close them
Comment on lines -44 to -45
Copy link
Member

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

for _, f := range files {
f.Close()
}
Expand All @@ -65,19 +63,23 @@ func bindPort(protocol, hostIP string, port uint16, isV6 bool, sctpWarning *bool
addr *net.UDPAddr
err error
)
if isV6 {
addr, err = net.ResolveUDPAddr("udp6", fmt.Sprintf("[%s]:%d", hostIP, port))

proto := "udp"
if hostIP != "" {
if isV6 {
proto = "udp6"
addr, err = net.ResolveUDPAddr(proto, fmt.Sprintf("[%s]:%d", hostIP, port))
} else {
proto = "udp4"
addr, err = net.ResolveUDPAddr(proto, fmt.Sprintf("%s:%d", hostIP, port))
}
} else {
addr, err = net.ResolveUDPAddr("udp4", fmt.Sprintf("%s:%d", hostIP, port))
addr, err = net.ResolveUDPAddr(proto, fmt.Sprintf(":%d", port))
}
if err != nil {
return nil, fmt.Errorf("cannot resolve the UDP address: %w", err)
}

proto := "udp4"
if isV6 {
proto = "udp6"
}
server, err := net.ListenUDP(proto, addr)
if err != nil {
return nil, fmt.Errorf("cannot listen on the UDP port: %w", err)
Expand All @@ -98,19 +100,24 @@ func bindPort(protocol, hostIP string, port uint16, isV6 bool, sctpWarning *bool
addr *net.TCPAddr
err error
)
if isV6 {
addr, err = net.ResolveTCPAddr("tcp6", fmt.Sprintf("[%s]:%d", hostIP, port))

// dual-stack bind by default unless hostIP is specified
proto := "tcp"
if hostIP != "" {
if isV6 {
proto = "tcp6"
addr, err = net.ResolveTCPAddr(proto, fmt.Sprintf("[%s]:%d", hostIP, port))
} else {
proto = "tcp4"
addr, err = net.ResolveTCPAddr(proto, fmt.Sprintf("%s:%d", hostIP, port))
}
} else {
addr, err = net.ResolveTCPAddr("tcp4", fmt.Sprintf("%s:%d", hostIP, port))
addr, err = net.ResolveTCPAddr(proto, fmt.Sprintf(":%d", port))
}
if err != nil {
return nil, fmt.Errorf("cannot resolve the TCP address: %w", err)
}

proto := "tcp4"
if isV6 {
proto = "tcp6"
}
server, err := net.ListenTCP(proto, addr)
if err != nil {
return nil, fmt.Errorf("cannot listen on the TCP port: %w", err)
Expand Down
44 changes: 44 additions & 0 deletions test/system/500-networking.bats
Original file line number Diff line number Diff line change
Expand Up @@ -1122,4 +1122,48 @@ EOF
is "$output" ".*${pasta_iface}.*"
}

# Refer https://github.com/containers/netavark/issues/1338
# bats test_tags=ci:parallel
@test "podman run - dual-stack port binding" {
Copy link
Member

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

skip_if_rootless

myport=$(random_free_port)
cname="c-$(safename)"
teststring=$(random_string 30)

run_podman run -d --name $cname -p $myport:$myport \
$IMAGE nc -l -n -v -p $myport
cid="$output"

wait_for_output "listening on .*:$myport .*" $cid

echo "$teststring" > /dev/tcp/127.0.0.1/$myport

run_podman logs $cid
assert "$output" =~ "listening on \[::\]:$myport" "nc listening on IPv6 wildcard (dual-stack)"
assert "$output" =~ "$teststring" "IPv4 connection received"

run_podman rm -f -t0 $cid
}

# 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"
Copy link
Member

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.


myport=$(random_free_port)
cname1="c1-$(safename)"

run_podman run -d --name $cname1 -p $myport:8080 $IMAGE sleep inf
cid1="$output"

run_podman 126 run --rm -p 0.0.0.0:$myport:8080 $IMAGE true
assert "$output" =~ "address already in use" "explicit IPv4 wildcard should conflict with dual-stack"

run_podman 126 run --rm -p [::]:$myport:8080 $IMAGE true
assert "$output" =~ "address already in use" "explicit IPv6 wildcard should conflict with dual-stack"

run_podman rm -f -t0 $cid1
}

# vim: filetype=sh