From ffd68badc0a3d70fbd063903702355a387621b10 Mon Sep 17 00:00:00 2001 From: Erik Hollensbe Date: Wed, 25 Jun 2014 13:17:20 -0700 Subject: [PATCH] Make ErrPortAlreadyAllocated an error interface with a few extras, adjust tests to fit. Docker-DCO-1.1-Signed-off-by: Erik Hollensbe (github: erikh) --- daemon/networkdriver/bridge/driver.go | 22 ++++++++---- .../portallocator/portallocator.go | 36 ++++++++++++++++--- .../portallocator/portallocator_test.go | 7 ++-- daemon/networkdriver/portmapper/mapper.go | 23 +++--------- .../networkdriver/portmapper/mapper_test.go | 15 ++------ 5 files changed, 60 insertions(+), 43 deletions(-) diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index 8da7a0457a..16439d2aca 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/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 { diff --git a/daemon/networkdriver/portallocator/portallocator.go b/daemon/networkdriver/portallocator/portallocator.go index 251ab94473..3b01e28a30 100644 --- a/daemon/networkdriver/portallocator/portallocator.go +++ b/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) diff --git a/daemon/networkdriver/portallocator/portallocator_test.go b/daemon/networkdriver/portallocator/portallocator_test.go index 5a4765ddd4..9869c332e9 100644 --- a/daemon/networkdriver/portallocator/portallocator_test.go +++ b/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) } } diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index 880cfb2986..d4469cd6d0 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/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 } diff --git a/daemon/networkdriver/portmapper/mapper_test.go b/daemon/networkdriver/portmapper/mapper_test.go index 881ea028f3..cae74ebb8b 100644 --- a/daemon/networkdriver/portmapper/mapper_test.go +++ b/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 {