Make ErrPortAlreadyAllocated an error interface with a few extras,

adjust tests to fit.

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

daemon/networkdriver/bridge/driver.go

@@ -11,6 +11,7 @@ import (
 	"github.com/docker/libcontainer/netlink"
 	"github.com/dotcloud/docker/daemon/networkdriver"
 	"github.com/dotcloud/docker/daemon/networkdriver/ipallocator"
+	"github.com/dotcloud/docker/daemon/networkdriver/portallocator"
 	"github.com/dotcloud/docker/daemon/networkdriver/portmapper"
 	"github.com/dotcloud/docker/engine"
 	"github.com/dotcloud/docker/pkg/iptables"
@@ -413,15 +414,22 @@ func AllocatePort(job *engine.Job) engine.Status {
 			break
 		}
 
-		// There is no point in immediately retrying to map an explicitely
-		// chosen port.
-		if hostPort != 0 {
-			job.Logf("Failed to bind %s for container address %s", host.String(), container.String())
+		switch allocerr := err.(type) {
+		case portallocator.ErrPortAlreadyAllocated:
+			// There is no point in immediately retrying to map an explicitly
+			// chosen port.
+			if hostPort != 0 {
+				job.Logf("Failed to bind %s for container address %s: %s", allocerr.IPPort(), container.String(), allocerr.Error())
+				break
+			}
+
+			// Automatically chosen 'free' port failed to bind: move on the next.
+			job.Logf("Failed to bind %s for container address %s. Trying another port.", allocerr.IPPort(), container.String())
+		default:
+			// some other error during mapping
+			job.Logf("Received an unexpected error during port allocation: %s", err.Error())
 			break
 		}
-
-		// Automatically chosen 'free' port failed to bind: move on the next.
-		job.Logf("Failed to bind %s for container address %s. Trying another port.", host.String(), container.String())
 	}
 
 	if err != nil {

daemon/networkdriver/portallocator/portallocator.go

@@ -2,6 +2,7 @@ package portallocator
 
 import (
 	"errors"
+	"fmt"
 	"net"
 	"sync"
 )
@@ -18,9 +19,8 @@ const (
 )
 
 var (
-	ErrAllPortsAllocated    = errors.New("all ports are allocated")
-	ErrPortAlreadyAllocated = errors.New("port has already been allocated")
-	ErrUnknownProtocol      = errors.New("unknown protocol")
+	ErrAllPortsAllocated = errors.New("all ports are allocated")
+	ErrUnknownProtocol   = errors.New("unknown protocol")
 )
 
 var (
@@ -30,6 +30,34 @@ var (
 	globalMap = ipMapping{}
 )
 
+type ErrPortAlreadyAllocated struct {
+	ip   string
+	port int
+}
+
+func NewErrPortAlreadyAllocated(ip string, port int) ErrPortAlreadyAllocated {
+	return ErrPortAlreadyAllocated{
+		ip:   ip,
+		port: port,
+	}
+}
+
+func (e ErrPortAlreadyAllocated) IP() string {
+	return e.ip
+}
+
+func (e ErrPortAlreadyAllocated) Port() int {
+	return e.port
+}
+
+func (e ErrPortAlreadyAllocated) IPPort() string {
+	return fmt.Sprintf("%s:%d", e.ip, e.port)
+}
+
+func (e ErrPortAlreadyAllocated) Error() string {
+	return fmt.Sprintf("Bind for %s:%d failed: port is already allocated", e.ip, e.port)
+}
+
 func RequestPort(ip net.IP, proto string, port int) (int, error) {
 	mutex.Lock()
 	defer mutex.Unlock()
@@ -47,7 +75,7 @@ func RequestPort(ip net.IP, proto string, port int) (int, error) {
 			mapping[proto][port] = true
 			return port, nil
 		} else {
-			return 0, ErrPortAlreadyAllocated
+			return 0, NewErrPortAlreadyAllocated(ip.String(), port)
 		}
 	} else {
 		port, err := findPort(ip, proto)

daemon/networkdriver/portallocator/portallocator_test.go

@@ -83,8 +83,11 @@ func TestReleaseUnreadledPort(t *testing.T) {
 	}
 
 	port, err = RequestPort(defaultIP, "tcp", 5000)
-	if err != ErrPortAlreadyAllocated {
-		t.Fatalf("Expected error %s got %s", ErrPortAlreadyAllocated, err)
+
+	switch err.(type) {
+	case ErrPortAlreadyAllocated:
+	default:
+		t.Fatalf("Expected port allocation error got %s", err)
 	}
 }
 

daemon/networkdriver/portmapper/mapper.go

@@ -33,19 +33,6 @@ var (
 	ErrPortNotMapped             = errors.New("port is not mapped")
 )
 
-type genericAddr struct {
-	IP   net.IP
-	Port int
-}
-
-func (g *genericAddr) Network() string {
-	return ""
-}
-
-func (g *genericAddr) String() string {
-	return fmt.Sprintf("%s:%d", g.IP.String(), g.Port)
-}
-
 func SetIptablesChain(c *iptables.Chain) {
 	chain = c
 }
@@ -65,7 +52,7 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) {
 	case *net.TCPAddr:
 		proto = "tcp"
 		if allocatedHostPort, err = portallocator.RequestPort(hostIP, proto, hostPort); err != nil {
-			return &net.TCPAddr{IP: hostIP, Port: hostPort}, err
+			return nil, err
 		}
 		m = &mapping{
 			proto:     proto,
@@ -75,7 +62,7 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) {
 	case *net.UDPAddr:
 		proto = "udp"
 		if allocatedHostPort, err = portallocator.RequestPort(hostIP, proto, hostPort); err != nil {
-			return &net.UDPAddr{IP: hostIP, Port: hostPort}, err
+			return nil, err
 		}
 		m = &mapping{
 			proto:     proto,
@@ -83,8 +70,8 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) {
 			container: container,
 		}
 	default:
-		// Always return a proper net.Addr for proper reporting.
-		return &genericAddr{IP: hostIP, Port: hostPort}, ErrUnknownBackendAddressType
+		err = ErrUnknownBackendAddressType
+		return nil, err
 	}
 
 	// When binding fails:
@@ -111,7 +98,7 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) {
 
 	p, err := newProxy(m.host, m.container)
 	if err != nil {
-		// need to undo the iptables rules before we reutrn
+		// need to undo the iptables rules before we return
 		forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort)
 		return m.host, err
 	}

daemon/networkdriver/portmapper/mapper_test.go

@@ -55,25 +55,16 @@ func TestMapPorts(t *testing.T) {
 			dstAddr1.String(), dstAddr1.Network(), host.String(), host.Network())
 	}
 
-	if host, err := Map(srcAddr1, dstIp1, 80); err == nil {
+	if _, err := Map(srcAddr1, dstIp1, 80); err == nil {
 		t.Fatalf("Port is in use - mapping should have failed")
-	} else if !addrEqual(dstAddr1, host) {
-		t.Fatalf("Incorrect mapping result: expected %s:%s, got %s:%s",
-			dstAddr1.String(), dstAddr1.Network(), host.String(), host.Network())
 	}
 
-	if host, err := Map(srcAddr2, dstIp1, 80); err == nil {
+	if _, err := Map(srcAddr2, dstIp1, 80); err == nil {
 		t.Fatalf("Port is in use - mapping should have failed")
-	} else if !addrEqual(dstAddr1, host) {
-		t.Fatalf("Incorrect mapping result: expected %s:%s, got %s:%s",
-			dstAddr1.String(), dstAddr1.Network(), host.String(), host.Network())
 	}
 
-	if host, err := Map(srcAddr2, dstIp2, 80); err != nil {
+	if _, err := Map(srcAddr2, dstIp2, 80); err != nil {
 		t.Fatalf("Failed to allocate port: %s", err)
-	} else if !addrEqual(dstAddr2, host) {
-		t.Fatalf("Incorrect mapping result: expected %s:%s, got %s:%s",
-			dstAddr1.String(), dstAddr1.Network(), host.String(), host.Network())
 	}
 
 	if Unmap(dstAddr1) != nil {