Merge pull request #24337 from Luap99/expose-ports-ps

ps: fix display of exposed ports

cmd/podman/containers/ps.go

@@ -1,6 +1,7 @@
 package containers
 
 import (
+	"cmp"
 	"errors"
 	"fmt"
 	"os"
@@ -491,40 +492,84 @@ func portsToString(ports []types.PortMapping, exposedPorts map[uint16][]string)
 	if len(ports) == 0 && len(exposedPorts) == 0 {
 		return ""
 	}
+	portMap := make(map[string]struct{})
+
 	sb := &strings.Builder{}
 	for _, port := range ports {
 		hostIP := port.HostIP
 		if hostIP == "" {
 			hostIP = "0.0.0.0"
 		}
-		protocols := strings.Split(port.Protocol, ",")
-		for _, protocol := range protocols {
-			if port.Range > 1 {
-				fmt.Fprintf(sb, "%s:%d-%d->%d-%d/%s, ",
-					hostIP, port.HostPort, port.HostPort+port.Range-1,
-					port.ContainerPort, port.ContainerPort+port.Range-1, protocol)
-			} else {
-				fmt.Fprintf(sb, "%s:%d->%d/%s, ",
-					hostIP, port.HostPort,
-					port.ContainerPort, protocol)
+		if port.Range > 1 {
+			fmt.Fprintf(sb, "%s:%d-%d->%d-%d/%s, ",
+				hostIP, port.HostPort, port.HostPort+port.Range-1,
+				port.ContainerPort, port.ContainerPort+port.Range-1, port.Protocol)
+			for i := range port.Range {
+				portMap[fmt.Sprintf("%d/%s", port.ContainerPort+i, port.Protocol)] = struct{}{}
 			}
+		} else {
+			fmt.Fprintf(sb, "%s:%d->%d/%s, ",
+				hostIP, port.HostPort,
+				port.ContainerPort, port.Protocol)
+			portMap[fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol)] = struct{}{}
 		}
 	}
 
-	// iterating a map is not deterministic so let's convert slice first and sort by port to make it deterministic
-	sortedPorts := make([]uint16, 0, len(exposedPorts))
-	for port := range exposedPorts {
-		sortedPorts = append(sortedPorts, port)
-	}
-	slices.Sort(sortedPorts)
-	for _, port := range sortedPorts {
-		for _, protocol := range exposedPorts[port] {
-			// exposed ports do not have a host part and are just written as "NUM/PROTO"
-			fmt.Fprintf(sb, "%d/%s, ", port, protocol)
+	// iterating a map is not deterministic so let's convert slice first and sort by protocol and port to make it deterministic
+	sortedPorts := make([]exposedPort, 0, len(exposedPorts))
+	for port, protocols := range exposedPorts {
+		for _, proto := range protocols {
+			sortedPorts = append(sortedPorts, exposedPort{num: port, protocol: proto})
 		}
 	}
+	slices.SortFunc(sortedPorts, func(a, b exposedPort) int {
+		protoCmp := cmp.Compare(a.protocol, b.protocol)
+		if protoCmp != 0 {
+			return protoCmp
+		}
+		return cmp.Compare(a.num, b.num)
+	})
+
+	var prevPort *exposedPort
+	for _, port := range sortedPorts {
+		// only if it was not published already so we do not have duplicates
+		if _, ok := portMap[fmt.Sprintf("%d/%s", port.num, port.protocol)]; ok {
+			continue
+		}
+
+		if prevPort != nil {
+			// if the prevPort is one below us we know it is a range, do not print it and just increase the range by one
+			if prevPort.protocol == port.protocol && prevPort.num == port.num-prevPort.portRange-1 {
+				prevPort.portRange++
+				continue
+			}
+			// the new port is not a range with the previous one so print it
+			printExposedPort(prevPort, sb)
+		}
+		prevPort = &port
+	}
+	// do not forget to print the last port
+	if prevPort != nil {
+		printExposedPort(prevPort, sb)
+	}
 
 	display := sb.String()
 	// make sure to trim the last ", " of the string
 	return display[:len(display)-2]
 }
+
+type exposedPort struct {
+	num      uint16
+	protocol string
+	// portRange is 0 indexed
+	portRange uint16
+}
+
+func printExposedPort(port *exposedPort, sb *strings.Builder) {
+	// exposed ports do not have a host part and are just written as "NUM[-RANGE]/PROTO"
+	if port.portRange > 0 {
+		fmt.Fprintf(sb, "%d-%d/%s, ", port.num, port.num+port.portRange, port.protocol)
+	} else {
+		fmt.Fprintf(sb, "%d/%s, ", port.num, port.protocol)
+	}
+}

cmd/podman/containers/ps_test.go

@@ -0,0 +1,196 @@
+package containers
+
+import (
+	"testing"
+
+	"github.com/containers/common/libnetwork/types"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_portsToString(t *testing.T) {
+	tests := []struct {
+		name         string
+		ports        []types.PortMapping
+		exposedPorts map[uint16][]string
+		want         string
+	}{
+		{
+			name: "no ports",
+			want: "",
+		},
+		{
+			name:         "no ports empty map/slice",
+			ports:        []types.PortMapping{},
+			exposedPorts: map[uint16][]string{},
+			want:         "",
+		},
+		{
+			name: "single published port",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         1,
+				},
+			},
+			want: "0.0.0.0:8080->80/tcp",
+		},
+		{
+			name: "published port with host ip",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					HostIP:        "127.0.0.1",
+					Protocol:      "tcp",
+					Range:         1,
+				},
+			},
+			want: "127.0.0.1:8080->80/tcp",
+		},
+		{
+			name: "published port with same exposed port",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         1,
+				},
+			},
+			exposedPorts: map[uint16][]string{
+				80: {"tcp"},
+			},
+			want: "0.0.0.0:8080->80/tcp",
+		},
+		{
+			name: "published port and exposed port with different protocol",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         1,
+				},
+			},
+			exposedPorts: map[uint16][]string{
+				80: {"udp"},
+			},
+			want: "0.0.0.0:8080->80/tcp, 80/udp",
+		},
+		{
+			name: "published port range",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         3,
+				},
+			},
+			want: "0.0.0.0:8080-8082->80-82/tcp",
+		},
+		{
+			name: "published port range and exposed port in that range",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         3,
+				},
+			},
+			exposedPorts: map[uint16][]string{
+				81: {"tcp"},
+			},
+			want: "0.0.0.0:8080-8082->80-82/tcp",
+		},
+		{
+			name: "two published ports",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         3,
+				},
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "udp",
+					Range:         1,
+				},
+			},
+			want: "0.0.0.0:8080-8082->80-82/tcp, 0.0.0.0:8080->80/udp",
+		},
+		{
+			name: "exposed port",
+			exposedPorts: map[uint16][]string{
+				80: {"tcp"},
+			},
+			want: "80/tcp",
+		},
+		{
+			name: "exposed port multiple protocols",
+			exposedPorts: map[uint16][]string{
+				80: {"tcp", "udp"},
+			},
+			want: "80/tcp, 80/udp",
+		},
+		{
+			name: "exposed port range",
+			exposedPorts: map[uint16][]string{
+				80: {"tcp"},
+				81: {"tcp"},
+				82: {"tcp"},
+			},
+			want: "80-82/tcp",
+		},
+		{
+			name: "exposed port range with different protocols",
+			exposedPorts: map[uint16][]string{
+				80: {"tcp", "udp"},
+				81: {"tcp", "sctp"},
+				82: {"tcp", "udp"},
+			},
+			want: "81/sctp, 80-82/tcp, 80/udp, 82/udp",
+		},
+		{
+			name: "multiple exposed port ranges",
+			exposedPorts: map[uint16][]string{
+				80: {"tcp"},
+				81: {"tcp"},
+				82: {"tcp"},
+				// 83 missing to split the range
+				84: {"tcp"},
+				85: {"tcp"},
+				86: {"tcp"},
+			},
+			want: "80-82/tcp, 84-86/tcp",
+		},
+		{
+			name: "published port range partially overlaps with exposed port range",
+			ports: []types.PortMapping{
+				{
+					ContainerPort: 80,
+					HostPort:      8080,
+					Protocol:      "tcp",
+					Range:         3,
+				},
+			},
+			exposedPorts: map[uint16][]string{
+				82: {"tcp"},
+				83: {"tcp"},
+				84: {"tcp"},
+			},
+			want: "0.0.0.0:8080-8082->80-82/tcp, 83-84/tcp",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := portsToString(tt.ports, tt.exposedPorts)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

test/e2e/run_networking_test.go

@@ -377,8 +377,18 @@ var _ = Describe("Podman run networking", func() {
 	})
 
 	It("podman run --expose port range", func() {
+		session := podmanTest.Podman([]string{"run", "-d", "--expose", "1000-9999", ALPINE})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+
+		session = podmanTest.Podman([]string{"ps", "-a", "--format", "{{.Ports}}"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+		// This must use Equal() to ensure we do not see anything extra
+		Expect(session.OutputToString()).To(Equal("1000-9999/tcp"))
+
 		name := "testctr"
-		session := podmanTest.Podman([]string{"run", "-d", "--expose", "222-223", "-P", "--name", name, ALPINE, "sleep", "100"})
+		session = podmanTest.Podman([]string{"run", "-d", "--expose", "222-223", "-P", "--name", name, ALPINE, "sleep", "100"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 		inspectOut := podmanTest.InspectContainer(name)
@@ -396,12 +406,20 @@ var _ = Describe("Podman run networking", func() {
 		name := "testctr"
 		session := podmanTest.Podman([]string{"create", "-t", "--expose", "80", "-p", "80", "--name", name, ALPINE, "/bin/sh"})
 		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
 		inspectOut := podmanTest.InspectContainer(name)
 		Expect(inspectOut).To(HaveLen(1))
 		Expect(inspectOut[0].NetworkSettings.Ports).To(HaveLen(1))
 		Expect(inspectOut[0].NetworkSettings.Ports["80/tcp"]).To(HaveLen(1))
-		Expect(inspectOut[0].NetworkSettings.Ports["80/tcp"][0].HostPort).To(Not(Equal("80")))
+		hostPort := inspectOut[0].NetworkSettings.Ports["80/tcp"][0].HostPort
+		Expect(hostPort).To(Not(Equal("80")))
 		Expect(inspectOut[0].NetworkSettings.Ports["80/tcp"][0]).To(HaveField("HostIP", "0.0.0.0"))
+
+		session = podmanTest.Podman([]string{"ps", "-a", "--format", "{{.Ports}}"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+		// This must use Equal() to ensure we do not see the extra ", 80/tcp" from the exposed port
+		Expect(session.OutputToString()).To(Equal("0.0.0.0:" + hostPort + "->80/tcp"))
 	})
 
 	It("podman run --publish-all with EXPOSE port ranges in Dockerfile", func() {
@@ -417,6 +435,7 @@ EXPOSE 2004-2005/tcp`, ALPINE)
 		// Verify that the buildah is just passing through the EXPOSE keys
 		inspect := podmanTest.Podman([]string{"inspect", imageName})
 		inspect.WaitWithDefaultTimeout()
+		Expect(inspect).Should(ExitCleanly())
 		image := inspect.InspectImageJSON()
 		Expect(image).To(HaveLen(1))
 		Expect(image[0].Config.ExposedPorts).To(HaveLen(3))
@@ -424,9 +443,20 @@ EXPOSE 2004-2005/tcp`, ALPINE)
 		Expect(image[0].Config.ExposedPorts).To(HaveKey("2001-2003/tcp"))
 		Expect(image[0].Config.ExposedPorts).To(HaveKey("2004-2005/tcp"))
 
-		containerName := "testcontainer"
-		session := podmanTest.Podman([]string{"create", "--publish-all", "--name", containerName, imageName, "true"})
+		session := podmanTest.Podman([]string{"create", imageName})
 		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+
+		session = podmanTest.Podman([]string{"ps", "-a", "--format", "{{.Ports}}"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
+		// This must use Equal() to ensure we do not see anything extra
+		Expect(session.OutputToString()).To(Equal("2001-2005/tcp"))
+
+		containerName := "testcontainer"
+		session = podmanTest.Podman([]string{"create", "--publish-all", "--name", containerName, imageName})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())
 		inspectOut := podmanTest.InspectContainer(containerName)
 		Expect(inspectOut).To(HaveLen(1))