Listen on mapped host ports before mapping more ports

Because we set SO_REUSEADDR on sockets for host ports, if there
are port mappings for INADDR_ANY (the default) as well as for
specific host ports - bind() cannot be used to detect clashes.

That means, for example, on daemon startup, if the port allocator
returns the first port in its ephemeral range for a specific host
adddress, and the next port mapping is for 0.0.0.0 - the same port
is returned and both bind() calls succeed. Then, the container
fails to start later when listen() spots the problem and it's too
late to find another port.

So, bind and listen to each set of ports as they're allocated
instead of just binding.

Signed-off-by: Rob Murray <rob.murray@docker.com>

integration/networking/port_mapping_linux_test.go

@@ -1282,3 +1282,25 @@ func TestSkipRawRules(t *testing.T) {
 		})
 	}
 }
+
+// Regression test for https://github.com/docker/compose/issues/12846
+func TestMixAnyWithSpecificHostAddrs(t *testing.T) {
+	ctx := setupTest(t)
+	// Start a new daemon, so the port allocator will start with new/empty ephemeral port ranges,
+	// making a clash more likely.
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Stop(t)
+	c := d.NewClientT(t)
+	defer c.Close()
+
+	ctrId := container.Run(ctx, t, c,
+		container.WithExposedPorts("80/tcp", "81/tcp", "82/tcp"),
+		container.WithPortMap(nat.PortMap{
+			"81/tcp": {{}},
+			"82/tcp": {{}},
+			"80/tcp": {{HostIP: "127.0.0.1"}},
+		}),
+	)
+	defer c.ContainerRemove(ctx, ctrId, containertypes.RemoveOptions{Force: true})
+}

libnetwork/drivers/bridge/port_mapping_linux.go

@@ -15,6 +15,7 @@ import (
 	"syscall"
 
 	"github.com/containerd/log"
+	"github.com/docker/docker/libnetwork/drivers/bridge/internal/firewaller"
 	"github.com/docker/docker/libnetwork/drivers/bridge/internal/rlkclient"
 	"github.com/docker/docker/libnetwork/netutils"
 	"github.com/docker/docker/libnetwork/portallocator"
@@ -103,7 +104,7 @@ func (n *bridgeNetwork) addPortMappings(
 
 	defer func() {
 		if retErr != nil {
-			if err := n.releasePortBindings(bindings); err != nil {
+			if err := releasePortBindings(bindings, n.firewallerNetwork); err != nil {
 				log.G(ctx).Warnf("Release port bindings: %s", err.Error())
 			}
 		}
@@ -167,7 +168,7 @@ func (n *bridgeNetwork) addPortMappings(
 		}
 
 		// Allocate and bind a host port.
-		newB, err := bindHostPorts(ctx, toBind, proxyPath)
+		newB, err := bindHostPorts(ctx, toBind, proxyPath, pdc, n.firewallerNetwork)
 		if err != nil {
 			return nil, err
 		}
@@ -177,66 +178,12 @@ func (n *bridgeNetwork) addPortMappings(
 		toBind = toBind[:0]
 	}
 
-	for i := range bindings {
-		b := bindings[i]
-		if pdc != nil && b.HostPort != 0 {
-			var err error
-			hip, ok := netip.AddrFromSlice(b.HostIP)
-			if !ok {
-				return nil, fmt.Errorf("invalid host IP address in %s", b)
-			}
-			chip, ok := netip.AddrFromSlice(b.childHostIP)
-			if !ok {
-				return nil, fmt.Errorf("invalid child host IP address %s in %s", b.childHostIP, b)
-			}
-			bindings[i].portDriverRemove, err = pdc.AddPort(ctx, b.Proto.String(), hip, chip, int(b.HostPort))
-			if err != nil {
-				var pErr *rlkclient.ProtocolUnsupportedError
-				if errors.As(err, &pErr) {
-					log.G(ctx).WithFields(log.Fields{
-						"error": pErr,
-					}).Warnf("discarding request for %q", net.JoinHostPort(hip.String(), strconv.Itoa(int(b.HostPort))))
-					bindings[i].rootlesskitUnsupported = true
-					continue
-				}
-				return nil, err
-			}
-		}
-	}
-
-	if err := n.firewallerNetwork.AddPorts(ctx, mergeChildHostIPs(bindings)); err != nil {
-		return nil, err
-	}
-
-	// Now the iptables rules are set up, it's safe to start the userland proxy.
-	// (If it was started before the iptables rules were created, it may have
-	// accepted a connection, then become unreachable due to NAT rules sending
-	// packets directly to the container.)
-	// If not starting the proxy, nothing will ever accept a connection on the
-	// socket. But, listen anyway so that the binding shows up in "netstat -at".
-	somaxconn := 0
+	// Start userland proxy processes.
 	if proxyPath != "" {
-		somaxconn = -1 // silently capped to "/proc/sys/net/core/somaxconn"
-	}
-	for i := range bindings {
-		if bindings[i].boundSocket == nil || bindings[i].rootlesskitUnsupported {
-			continue
-		}
-		if bindings[i].Proto == types.TCP {
-			rc, err := bindings[i].boundSocket.SyscallConn()
-			if err != nil {
-				return nil, fmt.Errorf("raw conn not available on TCP socket: %w", err)
+		for i := range bindings {
+			if bindings[i].boundSocket == nil || bindings[i].rootlesskitUnsupported || bindings[i].stopProxy != nil {
+				continue
 			}
-			if errC := rc.Control(func(fd uintptr) {
-				err = syscall.Listen(int(fd), somaxconn)
-			}); errC != nil {
-				return nil, fmt.Errorf("failed to Control TCP socket: %w", err)
-			}
-			if err != nil {
-				return nil, fmt.Errorf("failed to listen on TCP socket: %w", err)
-			}
-		}
-		if proxyPath != "" {
 			var err error
 			bindings[i].stopProxy, err = startProxy(
 				bindings[i].childPortBinding(), proxyPath, bindings[i].boundSocket,
@@ -468,6 +415,8 @@ func bindHostPorts(
 	ctx context.Context,
 	cfg []portBindingReq,
 	proxyPath string,
+	pdc portDriverClient,
+	fwn firewaller.Network,
 ) ([]portBinding, error) {
 	if len(cfg) == 0 {
 		return nil, nil
@@ -486,16 +435,19 @@ func bindHostPorts(
 	var err error
 	for i := 0; i < maxAllocatePortAttempts; i++ {
 		var b []portBinding
-		b, err = attemptBindHostPorts(ctx, cfg, proto.String(), hostPort, hostPortEnd, proxyPath)
+		b, err = attemptBindHostPorts(ctx, cfg, proto.String(), hostPort, hostPortEnd, proxyPath, pdc, fwn)
 		if err == nil {
 			return b, nil
 		}
 		// There is no point in immediately retrying to map an explicitly chosen port.
 		if hostPort != 0 && hostPort == hostPortEnd {
-			log.G(ctx).Warnf("Failed to allocate and map port: %s", err)
+			log.G(ctx).WithError(err).Warnf("Failed to allocate and map port")
 			break
 		}
-		log.G(ctx).Warnf("Failed to allocate and map port: %s, retry: %d", err, i+1)
+		log.G(ctx).WithFields(log.Fields{
+			"error":   err,
+			"attempt": i + 1,
+		}).Warn("Failed to allocate and map port")
 	}
 	return nil, err
 }
@@ -517,6 +469,8 @@ func attemptBindHostPorts(
 	proto string,
 	hostPortStart, hostPortEnd uint16,
 	proxyPath string,
+	pdc portDriverClient,
+	fwn firewaller.Network,
 ) (_ []portBinding, retErr error) {
 	var err error
 	var port int
@@ -546,20 +500,8 @@ func attemptBindHostPorts(
 	res := make([]portBinding, 0, len(cfg))
 	defer func() {
 		if retErr != nil {
-			for _, pb := range res {
-				if pb.boundSocket != nil {
-					if err := pb.boundSocket.Close(); err != nil {
-						log.G(ctx).Warnf("Failed to close port binding for %s: %s", pb, err)
-					}
-				}
-				// TODO(robmry) - this is only needed because the userland proxy may have
-				//  been started for SCTP. If a bound socket is passed to the proxy after
-				//  iptables rules have been configured (as it is for TCP/UDP), remove this.
-				if pb.stopProxy != nil {
-					if err := pb.stopProxy(); err != nil {
-						log.G(ctx).Warnf("Failed to stop proxy for %s: %s", pb, err)
-					}
-				}
+			if err := releasePortBindings(res, fwn); err != nil {
+				log.G(ctx).WithError(err).Warn("Failed to release port bindings")
 			}
 		}
 	}()
@@ -588,11 +530,10 @@ func attemptBindHostPorts(
 					//  to the userland proxy, because the proxy is not able to convert the
 					//  file descriptor into an sctp.SCTPListener (fd is an unexported member
 					//  of the struct, and ListenSCTP is the only constructor).
-					//  So, it is possible for the proxy to start listening and accept
+					//  If that changes, remove this.
+					//  Until then, it is possible for the proxy to start listening and accept
 					//  connections before iptables rules are created that would bypass
 					//  the proxy for external connections.
-					//  Remove this and pb.stopProxy() from the cleanup function above if
-					//  this is fixed.
 					pb, err = startSCTPProxy(c, port, proxyPath)
 				}
 			default:
@@ -604,6 +545,24 @@ func attemptBindHostPorts(
 		}
 		res = append(res, pb)
 	}
+
+	if err := configPortDriver(ctx, res, pdc); err != nil {
+		return nil, err
+	}
+	if err := fwn.AddPorts(ctx, mergeChildHostIPs(res)); err != nil {
+		return nil, err
+	}
+	// Now the firewall rules are set up, it's safe to listen on the socket. (Listening
+	// earlier could result in dropped connections if the proxy becomes unreachable due
+	// to NAT rules sending packets directly to the container.)
+	//
+	// If not starting the proxy, nothing will ever accept a connection on the
+	// socket. Listen here anyway because SO_REUSEADDR is set, so bind() won't notice
+	// the problem if a port's bound to both INADDR_ANY and a specific address. (Also
+	// so the binding shows up in "netstat -at".)
+	if err := tcpListenBoundPorts(res, proxyPath); err != nil {
+		return nil, err
+	}
 	return res, nil
 }
 
@@ -732,6 +691,64 @@ func startSCTPProxy(cfg portBindingReq, port int, proxyPath string) (_ portBindi
 	return pb, nil
 }
 
+// configPortDriver passes the port binding's details to rootlesskit, and updates the
+// port binding with callbacks to remove the rootlesskit config (or marks the binding as
+// unsupported by rootlesskit).
+func configPortDriver(ctx context.Context, pbs []portBinding, pdc portDriverClient) error {
+	for i := range pbs {
+		b := pbs[i]
+		if pdc != nil && b.HostPort != 0 {
+			var err error
+			hip, ok := netip.AddrFromSlice(b.HostIP)
+			if !ok {
+				return fmt.Errorf("invalid host IP address in %s", b)
+			}
+			chip, ok := netip.AddrFromSlice(b.childHostIP)
+			if !ok {
+				return fmt.Errorf("invalid child host IP address %s in %s", b.childHostIP, b)
+			}
+			pbs[i].portDriverRemove, err = pdc.AddPort(ctx, b.Proto.String(), hip, chip, int(b.HostPort))
+			if err != nil {
+				var pErr *rlkclient.ProtocolUnsupportedError
+				if errors.As(err, &pErr) {
+					log.G(ctx).WithFields(log.Fields{
+						"error": pErr,
+					}).Warnf("discarding request for %q", net.JoinHostPort(hip.String(), strconv.Itoa(int(b.HostPort))))
+					pbs[i].rootlesskitUnsupported = true
+					continue
+				}
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func tcpListenBoundPorts(pbs []portBinding, proxyPath string) error {
+	somaxconn := 0
+	if proxyPath != "" {
+		somaxconn = -1 // silently capped to "/proc/sys/net/core/somaxconn"
+	}
+	for i := range pbs {
+		if pbs[i].boundSocket == nil || pbs[i].rootlesskitUnsupported || pbs[i].Proto != types.TCP {
+			continue
+		}
+		rc, err := pbs[i].boundSocket.SyscallConn()
+		if err != nil {
+			return fmt.Errorf("raw conn not available on TCP socket: %w", err)
+		}
+		if errC := rc.Control(func(fd uintptr) {
+			err = syscall.Listen(int(fd), somaxconn)
+		}); errC != nil {
+			return fmt.Errorf("failed to Control TCP socket: %w", err)
+		}
+		if err != nil {
+			return fmt.Errorf("failed to listen on TCP socket: %w", err)
+		}
+	}
+	return nil
+}
+
 // releasePorts attempts to release all port bindings, does not stop on failure
 func (n *bridgeNetwork) releasePorts(ep *bridgeEndpoint) error {
 	n.Lock()
@@ -739,10 +756,10 @@ func (n *bridgeNetwork) releasePorts(ep *bridgeEndpoint) error {
 	ep.portMapping = nil
 	n.Unlock()
 
-	return n.releasePortBindings(pbs)
+	return releasePortBindings(pbs, n.firewallerNetwork)
 }
 
-func (n *bridgeNetwork) releasePortBindings(pbs []portBinding) error {
+func releasePortBindings(pbs []portBinding, fwn firewaller.Network) error {
 	var errs []error
 	for _, pb := range pbs {
 		if pb.boundSocket != nil {
@@ -761,7 +778,7 @@ func (n *bridgeNetwork) releasePortBindings(pbs []portBinding) error {
 			}
 		}
 	}
-	if err := n.firewallerNetwork.DelPorts(context.TODO(), mergeChildHostIPs(pbs)); err != nil {
+	if err := fwn.DelPorts(context.TODO(), mergeChildHostIPs(pbs)); err != nil {
 		errs = append(errs, err)
 	}
 	for _, pb := range pbs {

libnetwork/drivers/bridge/port_mapping_linux_test.go

@@ -263,7 +263,7 @@ func TestBindHostPortsError(t *testing.T) {
 			},
 		},
 	}
-	pbs, err := bindHostPorts(context.Background(), cfg, "")
+	pbs, err := bindHostPorts(context.Background(), cfg, "", nil, nil)
 	assert.Check(t, is.Error(err, "port binding mismatch 80/tcp:8080-8080, 80/tcp:8080-8081"))
 	assert.Check(t, is.Nil(pbs))
 }