Fix ps port output

When defining multiple ports (same src/dst) like `-p 80:80 -p 443:443`
then podman will not show the complete output on `podman ps` (only
`0.0.0.0:80->80/tcp` in the example). This also applies to port ranges.

This patch refactors the port loop by pre-checking for ranges and
displaying them correctly to the end user.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>

2 files changed, 150 insertions, 43 deletions

diff --git a/cmd/podman/containers/ps.go b/cmd/podman/containers/ps.go
index 41d309f51..446b46471 100644
--- a/cmd/podman/containers/ps.go
+++ b/cmd/podman/containers/ps.go
@@ -371,12 +371,6 @@ func (l psReporter) CreatedHuman() string {
 // portsToString converts the ports used to a string of the from "port1, port2"
 // and also groups a continuous list of ports into a readable format.
 func portsToString(ports []ocicni.PortMapping) string {
-	type portGroup struct {
-		first int32
-		last  int32
-	}
-	portDisplay := []string{}
-
 	if len(ports) == 0 {
 		return ""
 	}
@@ -385,41 +379,124 @@ func portsToString(ports []ocicni.PortMapping) string {
 		return comparePorts(ports[i], ports[j])
 	})
 
-	// portGroupMap is used for grouping continuous ports.
-	portGroupMap := make(map[string]*portGroup)
-	var groupKeyList []string
+	portGroups := [][]ocicni.PortMapping{}
+	currentGroup := []ocicni.PortMapping{}
+	for i, v := range ports {
+		var prevPort, nextPort *int32
+		if i > 0 {
+			prevPort = &ports[i-1].ContainerPort
+		}
+		if i+1 < len(ports) {
+			nextPort = &ports[i+1].ContainerPort
+		}
 
-	for _, v := range ports {
+		port := v.ContainerPort
 
-		hostIP := v.HostIP
-		if hostIP == "" {
-			hostIP = "0.0.0.0"
+		// Helper functions
+		addToCurrentGroup := func(x ocicni.PortMapping) {
+			currentGroup = append(currentGroup, x)
 		}
-		// If hostPort and containerPort are not same, consider as individual port.
-		if v.ContainerPort != v.HostPort {
-			portDisplay = append(portDisplay, fmt.Sprintf("%s:%d->%d/%s", hostIP, v.HostPort, v.ContainerPort, v.Protocol))
-			continue
+
+		addToPortGroup := func(x ocicni.PortMapping) {
+			portGroups = append(portGroups, []ocicni.PortMapping{x})
+		}
+
+		finishCurrentGroup := func() {
+			portGroups = append(portGroups, currentGroup)
+			currentGroup = []ocicni.PortMapping{}
 		}
 
-		portMapKey := fmt.Sprintf("%s/%s", hostIP, v.Protocol)
+		// Single entry slice
+		if prevPort == nil && nextPort == nil {
+			addToPortGroup(v)
+		}
+
+		// Start of the slice with len > 0
+		if prevPort == nil && nextPort != nil {
+			isGroup := *nextPort-1 == port
+
+			if isGroup {
+				// Start with a group
+				addToCurrentGroup(v)
+			} else {
+				// Start with single item
+				addToPortGroup(v)
+			}
 
-		portgroup, ok := portGroupMap[portMapKey]
-		if !ok {
-			portGroupMap[portMapKey] = &portGroup{first: v.ContainerPort, last: v.ContainerPort}
-			// This list is required to traverse portGroupMap.
-			groupKeyList = append(groupKeyList, portMapKey)
 			continue
 		}
 
-		if portgroup.last == (v.ContainerPort - 1) {
-			portgroup.last = v.ContainerPort
+		// Middle of the slice with len > 0
+		if prevPort != nil && nextPort != nil {
+			currentIsGroup := *prevPort+1 == port
+			nextIsGroup := *nextPort-1 == port
+
+			if currentIsGroup {
+				// Maybe in the middle of a group
+				addToCurrentGroup(v)
+
+				if !nextIsGroup {
+					// End of a group
+					finishCurrentGroup()
+				}
+			} else if nextIsGroup {
+				// Start of a new group
+				addToCurrentGroup(v)
+			} else {
+				// No group at all
+				addToPortGroup(v)
+			}
+
 			continue
 		}
+
+		// End of the slice with len > 0
+		if prevPort != nil && nextPort == nil {
+			isGroup := *prevPort+1 == port
+
+			if isGroup {
+				// End group
+				addToCurrentGroup(v)
+				finishCurrentGroup()
+			} else {
+				// End single item
+				addToPortGroup(v)
+			}
+		}
 	}
-	// For each portMapKey, format group list and append to output string.
-	for _, portKey := range groupKeyList {
-		group := portGroupMap[portKey]
-		portDisplay = append(portDisplay, formatGroup(portKey, group.first, group.last))
+
+	portDisplay := []string{}
+	for _, group := range portGroups {
+		if len(group) == 0 {
+			// Usually should not happen, but better do not crash.
+			continue
+		}
+
+		first := group[0]
+
+		hostIP := first.HostIP
+		if hostIP == "" {
+			hostIP = "0.0.0.0"
+		}
+
+		// Single mappings
+		if len(group) == 1 {
+			portDisplay = append(portDisplay,
+				fmt.Sprintf(
+					"%s:%d->%d/%s",
+					hostIP, first.HostPort, first.ContainerPort, first.Protocol,
+				),
+			)
+			continue
+		}
+
+		// Group mappings
+		last := group[len(group)-1]
+		portDisplay = append(portDisplay, formatGroup(
+			fmt.Sprintf("%s/%s", hostIP, first.Protocol),
+			first.HostPort, last.HostPort,
+			first.ContainerPort, last.ContainerPort,
+		))
 	}
 	return strings.Join(portDisplay, ", ")
 }
@@ -440,9 +517,10 @@ func comparePorts(i, j ocicni.PortMapping) bool {
 	return i.Protocol < j.Protocol
 }
 
-// formatGroup returns the group as <IP:startPort:lastPort->startPort:lastPort/Proto>
-// e.g 0.0.0.0:1000-1006->1000-1006/tcp.
-func formatGroup(key string, start, last int32) string {
+// formatGroup returns the group in the format:
+// <IP:firstHost:lastHost->firstCtr:lastCtr/Proto>
+// e.g 0.0.0.0:1000-1006->2000-2006/tcp.
+func formatGroup(key string, firstHost, lastHost, firstCtr, lastCtr int32) string {
 	parts := strings.Split(key, "/")
 	groupType := parts[0]
 	var ip string
@@ -450,12 +528,16 @@ func formatGroup(key string, start, last int32) string {
 		ip = parts[0]
 		groupType = parts[1]
 	}
-	group := strconv.Itoa(int(start))
-	if start != last {
-		group = fmt.Sprintf("%s-%d", group, last)
-	}
-	if ip != "" {
-		group = fmt.Sprintf("%s:%s->%s", ip, group, group)
+
+	group := func(first, last int32) string {
+		group := strconv.Itoa(int(first))
+		if first != last {
+			group = fmt.Sprintf("%s-%d", group, last)
+		}
+		return group
 	}
-	return fmt.Sprintf("%s/%s", group, groupType)
+	hostGroup := group(firstHost, lastHost)
+	ctrGroup := group(firstCtr, lastCtr)
+
+	return fmt.Sprintf("%s:%s->%s/%s", ip, hostGroup, ctrGroup, groupType)
 }
diff --git a/test/e2e/ps_test.go b/test/e2e/ps_test.go
index 48ef566ce..c65738993 100644
--- a/test/e2e/ps_test.go
+++ b/test/e2e/ps_test.go
@@ -411,18 +411,43 @@ var _ = Describe("Podman ps", func() {
 		Expect(output).To(ContainSubstring(podName))
 	})
 
-	It("podman ps test with port range", func() {
-		session := podmanTest.RunTopContainer("")
+	It("podman ps test with single port range", func() {
+		session := podmanTest.Podman([]string{"run", "-dt", "-p", "2000-2006:2000-2006", ALPINE, "top"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
-		session = podmanTest.Podman([]string{"run", "-dt", "-p", "2000-2006:2000-2006", ALPINE, "top"})
+		session = podmanTest.Podman([]string{"ps", "--format", "{{.Ports}}"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.OutputToString()).To(ContainSubstring("0.0.0.0:2000-2006"))
+	})
+
+	It("podman ps test with invalid port range", func() {
+		session := podmanTest.Podman([]string{
+			"run", "-p", "1000-2000:2000-3000", "-p", "1999-2999:3001-4001", ALPINE,
+		})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(125))
+		Expect(session.ErrorToString()).To(ContainSubstring("conflicting port mappings for host port 1999"))
+	})
+
+	It("podman ps test with multiple port range", func() {
+		session := podmanTest.Podman([]string{
+			"run", "-dt",
+			"-p", "3000-3001:3000-3001",
+			"-p", "3100-3102:4000-4002",
+			"-p", "30080:30080",
+			"-p", "30443:30443",
+			"-p", "8000:8080",
+			ALPINE, "top"},
+		)
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
 		session = podmanTest.Podman([]string{"ps", "--format", "{{.Ports}}"})
 		session.WaitWithDefaultTimeout()
-		Expect(session.OutputToString()).To(ContainSubstring("0.0.0.0:2000-2006"))
+		Expect(session.OutputToString()).To(ContainSubstring(
+			"0.0.0.0:3000-3001->3000-3001/tcp, 0.0.0.0:3100-3102->4000-4002/tcp, 0.0.0.0:8000->8080/tcp, 0.0.0.0:30080->30080/tcp, 0.0.0.0:30443->30443/tcp",
+		))
 	})
 
 	It("podman ps sync flag", func() {