Rework port parsing to support --expose and -P

As part of this, make a major change to the type we use to
represent port mappings in SpecGen (from using existing OCICNI
structs to using our own custom one). This struct has the
advantage of supporting ranges, massively reducing traffic over
the wire for Podman commands using them (for example, the
`podman run -p 5000-6000` command will now send only one struct
instead of 1000). This struct also allows us to easily validate
which ports are in use, and which are not, which is necessary for
--expose.

Once we have parsed the ports from the new struct, we can produce
an accurate map including all currently requested ports, and use
that to determine what ports need to be exposed (some requested
exposed ports may already be included in a mapping from --publish
and will be ignored) and what open ports on the host we can map
them to.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

7 files changed, 404 insertions, 21 deletions

diff --git a/pkg/domain/entities/types.go b/pkg/domain/entities/types.go
index 9fbe04c9a..21ab025de 100644
--- a/pkg/domain/entities/types.go
+++ b/pkg/domain/entities/types.go
@@ -8,7 +8,6 @@ import (
 	"github.com/containers/libpod/libpod/events"
 	"github.com/containers/libpod/pkg/specgen"
 	"github.com/containers/storage/pkg/archive"
-	"github.com/cri-o/ocicni/pkg/ocicni"
 )
 
 type Container struct {
@@ -40,7 +39,7 @@ type NetOptions struct {
 	DNSServers         []net.IP
 	Network            specgen.Namespace
 	NoHosts            bool
-	PublishPorts       []ocicni.PortMapping
+	PublishPorts       []specgen.PortMapping
 	StaticIP           *net.IP
 	StaticMAC          *net.HardwareAddr
 }
diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
index 14836035d..be54b60d2 100644
--- a/pkg/specgen/generate/container_create.go
+++ b/pkg/specgen/generate/container_create.go
@@ -96,7 +96,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 		return nil, err
 	}
 
-	opts, err := createContainerOptions(rt, s, pod, finalVolumes)
+	opts, err := createContainerOptions(ctx, rt, s, pod, finalVolumes, newImage)
 	if err != nil {
 		return nil, err
 	}
@@ -115,7 +115,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 	return rt.NewContainer(ctx, runtimeSpec, options...)
 }
 
-func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *libpod.Pod, volumes []*specgen.NamedVolume) ([]libpod.CtrCreateOption, error) {
+func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGenerator, pod *libpod.Pod, volumes []*specgen.NamedVolume, img *image.Image) ([]libpod.CtrCreateOption, error) {
 	var options []libpod.CtrCreateOption
 	var err error
 
@@ -134,7 +134,7 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l
 		options = append(options, rt.WithPod(pod))
 	}
 	destinations := []string{}
-	//	// Take all mount and named volume destinations.
+	// Take all mount and named volume destinations.
 	for _, mount := range s.Mounts {
 		destinations = append(destinations, mount.Destination)
 	}
@@ -188,7 +188,7 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l
 	options = append(options, libpod.WithPrivileged(s.Privileged))
 
 	// Get namespace related options
-	namespaceOptions, err := GenerateNamespaceOptions(s, rt, pod)
+	namespaceOptions, err := GenerateNamespaceOptions(ctx, s, rt, pod, img)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go
index 5c065cdda..96c65b551 100644
--- a/pkg/specgen/generate/namespaces.go
+++ b/pkg/specgen/generate/namespaces.go
@@ -1,12 +1,14 @@
 package generate
 
 import (
+	"context"
 	"os"
 	"strings"
 
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/libpod/define"
+	"github.com/containers/libpod/libpod/image"
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/libpod/pkg/specgen"
 	"github.com/containers/libpod/pkg/util"
@@ -76,7 +78,7 @@ func GetDefaultNamespaceMode(nsType string, cfg *config.Config, pod *libpod.Pod)
 // joining a pod.
 // TODO: Consider grouping options that are not directly attached to a namespace
 // elsewhere.
-func GenerateNamespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod.Pod) ([]libpod.CtrCreateOption, error) {
+func GenerateNamespaceOptions(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod.Pod, img *image.Image) ([]libpod.CtrCreateOption, error) {
 	toReturn := []libpod.CtrCreateOption{}
 
 	// If pod is not nil, get infra container.
@@ -204,7 +206,6 @@ func GenerateNamespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod
 	}
 
 	// Net
-	// TODO image ports
 	// TODO validate CNINetworks, StaticIP, StaticIPv6 are only set if we
 	// are in bridge mode.
 	postConfigureNetNS := !s.UserNS.IsHost()
@@ -221,9 +222,17 @@ func GenerateNamespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod
 		}
 		toReturn = append(toReturn, libpod.WithNetNSFrom(netCtr))
 	case specgen.Slirp:
-		toReturn = append(toReturn, libpod.WithNetNS(s.PortMappings, postConfigureNetNS, "slirp4netns", nil))
+		portMappings, err := createPortMappings(ctx, s, img)
+		if err != nil {
+			return nil, err
+		}
+		toReturn = append(toReturn, libpod.WithNetNS(portMappings, postConfigureNetNS, "slirp4netns", nil))
 	case specgen.Bridge:
-		toReturn = append(toReturn, libpod.WithNetNS(s.PortMappings, postConfigureNetNS, "bridge", s.CNINetworks))
+		portMappings, err := createPortMappings(ctx, s, img)
+		if err != nil {
+			return nil, err
+		}
+		toReturn = append(toReturn, libpod.WithNetNS(portMappings, postConfigureNetNS, "bridge", s.CNINetworks))
 	}
 
 	if s.UseImageHosts {
@@ -428,7 +437,7 @@ func specConfigureNamespaces(s *specgen.SpecGenerator, g *generate.Generator, rt
 	if g.Config.Annotations == nil {
 		g.Config.Annotations = make(map[string]string)
 	}
-	if s.PublishImagePorts {
+	if s.PublishExposedPorts {
 		g.Config.Annotations[libpod.InspectAnnotationPublishAll] = libpod.InspectResponseTrue
 	} else {
 		g.Config.Annotations[libpod.InspectAnnotationPublishAll] = libpod.InspectResponseFalse
diff --git a/pkg/specgen/generate/pod_create.go b/pkg/specgen/generate/pod_create.go
index babfba9bc..df5775f8b 100644
--- a/pkg/specgen/generate/pod_create.go
+++ b/pkg/specgen/generate/pod_create.go
@@ -83,7 +83,11 @@ func createPodOptions(p *specgen.PodSpecGenerator) ([]libpod.PodCreateOption, er
 		options = append(options, libpod.WithPodUseImageHosts())
 	}
 	if len(p.PortMappings) > 0 {
-		options = append(options, libpod.WithInfraContainerPorts(p.PortMappings))
+		ports, _, _, err := parsePortMapping(p.PortMappings)
+		if err != nil {
+			return nil, err
+		}
+		options = append(options, libpod.WithInfraContainerPorts(ports))
 	}
 	options = append(options, libpod.WithPodCgroups())
 	return options, nil
diff --git a/pkg/specgen/generate/ports.go b/pkg/specgen/generate/ports.go
new file mode 100644
index 000000000..91c8e68d1
--- /dev/null
+++ b/pkg/specgen/generate/ports.go
@@ -0,0 +1,333 @@
+package generate
+
+import (
+	"context"
+	"net"
+	"strconv"
+	"strings"
+
+	"github.com/containers/libpod/libpod/image"
+	"github.com/containers/libpod/pkg/specgen"
+	"github.com/cri-o/ocicni/pkg/ocicni"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+const (
+	protoTCP  = "tcp"
+	protoUDP  = "udp"
+	protoSCTP = "sctp"
+)
+
+// Parse port maps to OCICNI port mappings.
+// Returns a set of OCICNI port mappings, and maps of utilized container and
+// host ports.
+func parsePortMapping(portMappings []specgen.PortMapping) ([]ocicni.PortMapping, map[string]map[string]map[uint16]uint16, map[string]map[string]map[uint16]uint16, error) {
+	// First, we need to validate the ports passed in the specgen, and then
+	// convert them into CNI port mappings.
+	finalMappings := []ocicni.PortMapping{}
+
+	// To validate, we need two maps: one for host ports, one for container
+	// ports.
+	// Each is a map of protocol to map of IP address to map of port to
+	// port (for hostPortValidate, it's host port to container port;
+	// for containerPortValidate, container port to host port.
+	// These will ensure no collisions.
+	hostPortValidate := make(map[string]map[string]map[uint16]uint16)
+	containerPortValidate := make(map[string]map[string]map[uint16]uint16)
+
+	// Initialize the first level of maps (we can't really guess keys for
+	// the rest).
+	for _, proto := range []string{protoTCP, protoUDP, protoSCTP} {
+		hostPortValidate[proto] = make(map[string]map[uint16]uint16)
+		containerPortValidate[proto] = make(map[string]map[uint16]uint16)
+	}
+
+	// Iterate through all port mappings, generating OCICNI PortMapping
+	// structs and validating there is no overlap.
+	for _, port := range portMappings {
+		// First, check proto
+		protocols, err := checkProtocol(port.Protocol, true)
+		if err != nil {
+			return nil, nil, nil, err
+		}
+
+		// Validate host IP
+		hostIP := port.HostIP
+		if hostIP == "" {
+			hostIP = "0.0.0.0"
+		}
+		if ip := net.ParseIP(hostIP); ip == nil {
+			return nil, nil, nil, errors.Errorf("invalid IP address %s in port mapping", port.HostIP)
+		}
+
+		// Validate port numbers and range.
+		len := port.Range
+		if len == 0 {
+			len = 1
+		}
+		containerPort := port.ContainerPort
+		if containerPort == 0 {
+			return nil, nil, nil, errors.Errorf("container port number must be non-0")
+		}
+		hostPort := port.HostPort
+		if hostPort == 0 {
+			hostPort = containerPort
+		}
+		if uint32(len-1)+uint32(containerPort) > 65535 {
+			return nil, nil, nil, errors.Errorf("container port range exceeds maximum allowable port number")
+		}
+		if uint32(len-1)+uint32(hostPort) > 65536 {
+			return nil, nil, nil, errors.Errorf("host port range exceeds maximum allowable port number")
+		}
+
+		// Iterate through ports, populating maps to check for conflicts
+		// and generating CNI port mappings.
+		for _, p := range protocols {
+			hostIPMap := hostPortValidate[p]
+			ctrIPMap := containerPortValidate[p]
+
+			hostPortMap, ok := hostIPMap[hostIP]
+			if !ok {
+				hostPortMap = make(map[uint16]uint16)
+				hostIPMap[hostIP] = hostPortMap
+			}
+			ctrPortMap, ok := ctrIPMap[hostIP]
+			if !ok {
+				ctrPortMap = make(map[uint16]uint16)
+				ctrIPMap[hostIP] = ctrPortMap
+			}
+
+			// Iterate through all port numbers in the requested
+			// range.
+			var index uint16
+			for index = 0; index < len; index++ {
+				cPort := containerPort + index
+				hPort := hostPort + index
+
+				if cPort == 0 || hPort == 0 {
+					return nil, nil, nil, errors.Errorf("host and container ports cannot be 0")
+				}
+
+				testCPort := ctrPortMap[cPort]
+				if testCPort != 0 && testCPort != hPort {
+					// This is an attempt to redefine a port
+					return nil, nil, nil, errors.Errorf("conflicting port mappings for container port %d (protocol %s)", cPort, p)
+				}
+				ctrPortMap[cPort] = hPort
+
+				testHPort := hostPortMap[hPort]
+				if testHPort != 0 && testHPort != cPort {
+					return nil, nil, nil, errors.Errorf("conflicting port mappings for host port %d (protocol %s)", hPort, p)
+				}
+				hostPortMap[hPort] = cPort
+
+				// If we have an exact duplicate, just continue
+				if testCPort == hPort && testHPort == cPort {
+					continue
+				}
+
+				// We appear to be clear. Make an OCICNI port
+				// struct.
+				// Don't use hostIP - we want to preserve the
+				// empty string hostIP by default for compat.
+				cniPort := ocicni.PortMapping{
+					HostPort:      int32(hPort),
+					ContainerPort: int32(cPort),
+					Protocol:      p,
+					HostIP:        port.HostIP,
+				}
+				finalMappings = append(finalMappings, cniPort)
+			}
+		}
+	}
+
+	return finalMappings, containerPortValidate, hostPortValidate, nil
+}
+
+// Make final port mappings for the container
+func createPortMappings(ctx context.Context, s *specgen.SpecGenerator, img *image.Image) ([]ocicni.PortMapping, error) {
+	finalMappings, containerPortValidate, hostPortValidate, err := parsePortMapping(s.PortMappings)
+	if err != nil {
+		return nil, err
+	}
+
+	// If not publishing exposed ports, or if we are publishing and there is
+	// nothing to publish - then just return the port mappings we've made so
+	// far.
+	if !s.PublishExposedPorts || (len(s.Expose) == 0 && img == nil) {
+		return finalMappings, nil
+	}
+
+	logrus.Debugf("Adding exposed ports")
+
+	// We need to merge s.Expose into image exposed ports
+	expose := make(map[uint16]string)
+	for k, v := range s.Expose {
+		expose[k] = v
+	}
+	if img != nil {
+		inspect, err := img.InspectNoSize(ctx)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error inspecting image to get exposed ports")
+		}
+		for imgExpose := range inspect.Config.ExposedPorts {
+			// Expose format is portNumber[/protocol]
+			splitExpose := strings.SplitN(imgExpose, "/", 2)
+			num, err := strconv.Atoi(splitExpose[0])
+			if err != nil {
+				return nil, errors.Wrapf(err, "unable to convert image EXPOSE statement %q to port number", imgExpose)
+			}
+			if num > 65535 || num < 1 {
+				return nil, errors.Errorf("%d from image EXPOSE statement %q is not a valid port number", num, imgExpose)
+			}
+			// No need to validate protocol, we'll do it below.
+			if len(splitExpose) == 1 {
+				expose[uint16(num)] = "tcp"
+			} else {
+				expose[uint16(num)] = splitExpose[1]
+			}
+		}
+	}
+
+	// There's been a request to expose some ports. Let's do that.
+	// Start by figuring out what needs to be exposed.
+	// This is a map of container port number to protocols to expose.
+	toExpose := make(map[uint16][]string)
+	for port, proto := range expose {
+		// Validate protocol first
+		protocols, err := checkProtocol(proto, false)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error validating protocols for exposed port %d", port)
+		}
+
+		if port == 0 {
+			return nil, errors.Errorf("cannot expose 0 as it is not a valid port number")
+		}
+
+		// Check to see if the port is already present in existing
+		// mappings.
+		for _, p := range protocols {
+			ctrPortMap, ok := containerPortValidate[p]["0.0.0.0"]
+			if !ok {
+				ctrPortMap = make(map[uint16]uint16)
+				containerPortValidate[p]["0.0.0.0"] = ctrPortMap
+			}
+
+			if portNum := ctrPortMap[port]; portNum == 0 {
+				// We want to expose this port for this protocol
+				exposeProto, ok := toExpose[port]
+				if !ok {
+					exposeProto = []string{}
+				}
+				exposeProto = append(exposeProto, p)
+				toExpose[port] = exposeProto
+			}
+		}
+	}
+
+	// We now have a final list of ports that we want exposed.
+	// Let's find empty, unallocated host ports for them.
+	for port, protocols := range toExpose {
+		for _, p := range protocols {
+			// Find an open port on the host.
+			// I see a faint possibility that this will infinite
+			// loop trying to find a valid open port, so I've
+			// included a max-tries counter.
+			hostPort := 0
+			tries := 15
+			for hostPort == 0 && tries > 0 {
+				// We can't select a specific protocol, which is
+				// unfortunate for the UDP case.
+				candidate, err := getRandomPort()
+				if err != nil {
+					return nil, err
+				}
+
+				// Check if the host port is already bound
+				hostPortMap, ok := hostPortValidate[p]["0.0.0.0"]
+				if !ok {
+					hostPortMap = make(map[uint16]uint16)
+					hostPortValidate[p]["0.0.0.0"] = hostPortMap
+				}
+
+				if checkPort := hostPortMap[uint16(candidate)]; checkPort != 0 {
+					// Host port is already allocated, try again
+					tries--
+					continue
+				}
+
+				hostPortMap[uint16(candidate)] = port
+				hostPort = candidate
+				logrus.Debugf("Mapping exposed port %d/%s to host port %d", port, p, hostPort)
+
+				// Make a CNI port mapping
+				cniPort := ocicni.PortMapping{
+					HostPort:      int32(candidate),
+					ContainerPort: int32(port),
+					Protocol:      p,
+					HostIP:        "",
+				}
+				finalMappings = append(finalMappings, cniPort)
+			}
+			if tries == 0 && hostPort == 0 {
+				// We failed to find an open port.
+				return nil, errors.Errorf("failed to find an open port to expose container port %d on the host", port)
+			}
+		}
+	}
+
+	return finalMappings, nil
+}
+
+// Check a string to ensure it is a comma-separated set of valid protocols
+func checkProtocol(protocol string, allowSCTP bool) ([]string, error) {
+	protocols := make(map[string]struct{})
+	splitProto := strings.Split(protocol, ",")
+	// Don't error on duplicates - just deduplicate
+	for _, p := range splitProto {
+		switch p {
+		case protoTCP, "":
+			protocols[protoTCP] = struct{}{}
+		case protoUDP:
+			protocols[protoUDP] = struct{}{}
+		case protoSCTP:
+			if !allowSCTP {
+				return nil, errors.Errorf("protocol SCTP is not allowed for exposed ports")
+			}
+			protocols[protoSCTP] = struct{}{}
+		default:
+			return nil, errors.Errorf("unrecognized protocol %q in port mapping", p)
+		}
+	}
+
+	finalProto := []string{}
+	for p := range protocols {
+		finalProto = append(finalProto, p)
+	}
+
+	// This shouldn't be possible, but check anyways
+	if len(finalProto) == 0 {
+		return nil, errors.Errorf("no valid protocols specified for port mapping")
+	}
+
+	return finalProto, nil
+}
+
+// Find a random, open port on the host
+func getRandomPort() (int, error) {
+	l, err := net.Listen("tcp", ":0")
+	if err != nil {
+		return 0, errors.Wrapf(err, "unable to get free TCP port")
+	}
+	defer l.Close()
+	_, randomPort, err := net.SplitHostPort(l.Addr().String())
+	if err != nil {
+		return 0, errors.Wrapf(err, "unable to determine free port")
+	}
+	rp, err := strconv.Atoi(randomPort)
+	if err != nil {
+		return 0, errors.Wrapf(err, "unable to convert random port to int")
+	}
+	return rp, nil
+}
diff --git a/pkg/specgen/podspecgen.go b/pkg/specgen/podspecgen.go
index 3f830014d..682f3f215 100644
--- a/pkg/specgen/podspecgen.go
+++ b/pkg/specgen/podspecgen.go
@@ -2,8 +2,6 @@ package specgen
 
 import (
 	"net"
-
-	"github.com/cri-o/ocicni/pkg/ocicni"
 )
 
 // PodBasicConfig contains basic configuration options for pods.
@@ -79,7 +77,7 @@ type PodNetworkConfig struct {
 	// container, this will forward the ports to the entire pod.
 	// Only available if NetNS is set to Bridge or Slirp.
 	// Optional.
-	PortMappings []ocicni.PortMapping `json:"portmappings,omitempty"`
+	PortMappings []PortMapping `json:"portmappings,omitempty"`
 	// CNINetworks is a list of CNI networks that the infra container will
 	// join. As, by default, containers share their network with the infra
 	// container, these networks will effectively be joined by the
diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go
index 20c8f8800..4f1c4fde1 100644
--- a/pkg/specgen/specgen.go
+++ b/pkg/specgen/specgen.go
@@ -6,7 +6,6 @@ import (
 
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/storage"
-	"github.com/cri-o/ocicni/pkg/ocicni"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 )
 
@@ -306,11 +305,23 @@ type ContainerNetworkConfig struct {
 	// PortBindings is a set of ports to map into the container.
 	// Only available if NetNS is set to bridge or slirp.
 	// Optional.
-	PortMappings []ocicni.PortMapping `json:"portmappings,omitempty"`
-	// PublishImagePorts will publish ports specified in the image to random
-	// ports outside.
-	// Requires Image to be set.
-	PublishImagePorts bool `json:"publish_image_ports,omitempty"`
+	PortMappings []PortMapping `json:"portmappings,omitempty"`
+	// PublishExposedPorts will publish ports specified in the image to
+	// random unused ports (guaranteed to be above 1024) on the host.
+	// This is based on ports set in Expose below, and any ports specified
+	// by the Image (if one is given).
+	// Only available if NetNS is set to Bridge or Slirp.
+	PublishExposedPorts bool `json:"publish_image_ports,omitempty"`
+	// Expose is a number of ports that will be forwarded to the container
+	// if PublishExposedPorts is set.
+	// Expose is a map of uint16 (port number) to a string representing
+	// protocol. Allowed protocols are "tcp", "udp", and "sctp", or some
+	// combination of the three separated by commas.
+	// If protocol is set to "" we will assume TCP.
+	// Only available if NetNS is set to Bridge or Slirp, and
+	// PublishExposedPorts is set.
+	// Optional.
+	Expose map[uint16]string `json:"expose,omitempty"`
 	// CNINetworks is a list of CNI networks to join the container to.
 	// If this list is empty, the default CNI network will be joined
 	// instead. If at least one entry is present, we will not join the
@@ -410,6 +421,35 @@ type NamedVolume struct {
 	Options []string
 }
 
+// PortMapping is one or more ports that will be mapped into the container.
+type PortMapping struct {
+	// HostIP is the IP that we will bind to on the host.
+	// If unset, assumed to be 0.0.0.0 (all interfaces).
+	HostIP string `json:"host_ip,omitempty"`
+	// ContainerPort is the port number that will be exposed from the
+	// container.
+	// Mandatory.
+	ContainerPort uint16 `json:"container_port"`
+	// HostPort is the port number that will be forwarded from the host into
+	// the container.
+	// If omitted, will be assumed to be identical to
+	HostPort uint16 `json:"host_port,omitempty"`
+	// Range is the number of ports that will be forwarded, starting at
+	// HostPort and ContainerPort and counting up.
+	// This is 1-indexed, so 1 is assumed to be a single port (only the
+	// Hostport:Containerport mapping will be added), 2 is two ports (both
+	// Hostport:Containerport and Hostport+1:Containerport+1), etc.
+	// If unset, assumed to be 1 (a single port).
+	// Both hostport + range and containerport + range must be less than
+	// 65536.
+	Range uint16 `json:"range,omitempty"`
+	// Protocol is the protocol forward.
+	// Must be either "tcp", "udp", and "sctp", or some combination of these
+	// separated by commas.
+	// If unset, assumed to be TCP.
+	Protocol string `json:"protocol,omitempty"`
+}
+
 // NewSpecGenerator returns a SpecGenerator struct given one of two mandatory inputs
 func NewSpecGenerator(arg string, rootfs bool) *SpecGenerator {
 	csc := ContainerStorageConfig{}