From 187971736cb9cc3b80554d5e0c78538b41901adf Mon Sep 17 00:00:00 2001 From: Arnaud Porterie Date: Thu, 19 Jun 2014 23:33:51 +0200 Subject: [PATCH 1/4] Restrict portallocator to Docker allocated ports Port allocation status is stored in a global map: a port detected in use will remain as such for the lifetime of the daemon. Change the behavior to only mark as allocated ports which are claimed by Docker itself (which we can trust to properly remove from the allocation map once released). Ports allocated by other applications will always be retried to account for the eventually of the port having been released. Docker-DCO-1.1-Signed-off-by: Arnaud Porterie (github: icecrime) Upstream-commit: dafddf461eeabd792d9ced6caced75ad6961c1d7 Component: engine --- .../daemon/networkdriver/bridge/driver.go | 78 ++++++------- .../networkdriver/bridge/driver_test.go | 106 ++++++++++++++++++ .../daemon/networkdriver/portmapper/mapper.go | 84 +++++++++++--- .../networkdriver/portmapper/mapper_test.go | 24 +++- 4 files changed, 227 insertions(+), 65 deletions(-) create mode 100644 components/engine/daemon/networkdriver/bridge/driver_test.go diff --git a/components/engine/daemon/networkdriver/bridge/driver.go b/components/engine/daemon/networkdriver/bridge/driver.go index 8c5db9f843..8da7a0457a 100644 --- a/components/engine/daemon/networkdriver/bridge/driver.go +++ b/components/engine/daemon/networkdriver/bridge/driver.go @@ -11,7 +11,6 @@ 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" @@ -354,9 +353,6 @@ func Release(job *engine.Job) engine.Status { var ( id = job.Args[0] containerInterface = currentInterfaces.Get(id) - ip net.IP - port int - proto string ) if containerInterface == nil { @@ -367,22 +363,6 @@ func Release(job *engine.Job) engine.Status { if err := portmapper.Unmap(nat); err != nil { log.Printf("Unable to unmap port %s: %s", nat, err) } - - // this is host mappings - switch a := nat.(type) { - case *net.TCPAddr: - proto = "tcp" - ip = a.IP - port = a.Port - case *net.UDPAddr: - proto = "udp" - ip = a.IP - port = a.Port - } - - if err := portallocator.ReleasePort(ip, proto, port); err != nil { - log.Printf("Unable to release port %s", nat) - } } if err := ipallocator.ReleaseIP(bridgeNetwork, &containerInterface.IP); err != nil { @@ -399,7 +379,7 @@ func AllocatePort(job *engine.Job) engine.Status { ip = defaultBindingIP id = job.Args[0] hostIP = job.Getenv("HostIP") - origHostPort = job.GetenvInt("HostPort") + hostPort = job.GetenvInt("HostPort") containerPort = job.GetenvInt("ContainerPort") proto = job.Getenv("Proto") network = currentInterfaces.Get(id) @@ -409,11 +389,16 @@ func AllocatePort(job *engine.Job) engine.Status { ip = net.ParseIP(hostIP) } - var ( - hostPort int - container net.Addr - host net.Addr - ) + // host ip, proto, and host port + var container net.Addr + switch proto { + case "tcp": + container = &net.TCPAddr{IP: network.IP, Port: containerPort} + case "udp": + container = &net.UDPAddr{IP: network.IP, Port: containerPort} + default: + return job.Errorf("unsupported address type %s", proto) + } /* Try up to 10 times to get a port that's not already allocated. @@ -421,27 +406,22 @@ func AllocatePort(job *engine.Job) engine.Status { In the event of failure to bind, return the error that portmapper.Map yields. */ + + var host net.Addr for i := 0; i < 10; i++ { - // host ip, proto, and host port - hostPort, err = portallocator.RequestPort(ip, proto, origHostPort) - - if err != nil { - return job.Error(err) - } - - if proto == "tcp" { - host = &net.TCPAddr{IP: ip, Port: hostPort} - container = &net.TCPAddr{IP: network.IP, Port: containerPort} - } else { - host = &net.UDPAddr{IP: ip, Port: hostPort} - container = &net.UDPAddr{IP: network.IP, Port: containerPort} - } - - if err = portmapper.Map(container, ip, hostPort); err == nil { + if host, err = portmapper.Map(container, ip, hostPort); err == nil { break } - job.Logf("Failed to bind %s:%d for container address %s:%d. Trying another port.", ip.String(), hostPort, network.IP.String(), containerPort) + // 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()) + 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 { @@ -451,12 +431,18 @@ func AllocatePort(job *engine.Job) engine.Status { network.PortMappings = append(network.PortMappings, host) out := engine.Env{} - out.Set("HostIP", ip.String()) - out.SetInt("HostPort", hostPort) - + switch netAddr := host.(type) { + case *net.TCPAddr: + out.Set("HostIP", netAddr.IP.String()) + out.SetInt("HostPort", netAddr.Port) + case *net.UDPAddr: + out.Set("HostIP", netAddr.IP.String()) + out.SetInt("HostPort", netAddr.Port) + } if _, err := out.WriteTo(job.Stdout); err != nil { return job.Error(err) } + return engine.StatusOK } diff --git a/components/engine/daemon/networkdriver/bridge/driver_test.go b/components/engine/daemon/networkdriver/bridge/driver_test.go new file mode 100644 index 0000000000..f8ddd4c64e --- /dev/null +++ b/components/engine/daemon/networkdriver/bridge/driver_test.go @@ -0,0 +1,106 @@ +package bridge + +import ( + "fmt" + "net" + "strconv" + "testing" + + "github.com/dotcloud/docker/engine" +) + +func findFreePort(t *testing.T) int { + l, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatal("Failed to find a free port") + } + defer l.Close() + + result, err := net.ResolveTCPAddr("tcp", l.Addr().String()) + if err != nil { + t.Fatal("Failed to resolve address to identify free port") + } + return result.Port +} + +func newPortAllocationJob(eng *engine.Engine, port int) (job *engine.Job) { + strPort := strconv.Itoa(port) + + job = eng.Job("allocate_port", "container_id") + job.Setenv("HostIP", "127.0.0.1") + job.Setenv("HostPort", strPort) + job.Setenv("Proto", "tcp") + job.Setenv("ContainerPort", strPort) + return +} + +func TestAllocatePortDetection(t *testing.T) { + eng := engine.New() + eng.Logging = false + + freePort := findFreePort(t) + + // Init driver + job := eng.Job("initdriver") + if res := InitDriver(job); res != engine.StatusOK { + t.Fatal("Failed to initialize network driver") + } + + // Allocate interface + job = eng.Job("allocate_interface", "container_id") + if res := Allocate(job); res != engine.StatusOK { + t.Fatal("Failed to allocate network interface") + } + + // Allocate same port twice, expect failure on second call + job = newPortAllocationJob(eng, freePort) + if res := AllocatePort(job); res != engine.StatusOK { + t.Fatal("Failed to find a free port to allocate") + } + if res := AllocatePort(job); res == engine.StatusOK { + t.Fatal("Duplicate port allocation granted by AllocatePort") + } +} + +func TestAllocatePortReclaim(t *testing.T) { + eng := engine.New() + eng.Logging = false + + freePort := findFreePort(t) + + // Init driver + job := eng.Job("initdriver") + if res := InitDriver(job); res != engine.StatusOK { + t.Fatal("Failed to initialize network driver") + } + + // Allocate interface + job = eng.Job("allocate_interface", "container_id") + if res := Allocate(job); res != engine.StatusOK { + t.Fatal("Failed to allocate network interface") + } + + // Occupy port + listenAddr := fmt.Sprintf(":%d", freePort) + tcpListenAddr, err := net.ResolveTCPAddr("tcp", listenAddr) + if err != nil { + t.Fatalf("Failed to resolve TCP address '%s'", listenAddr) + } + + l, err := net.ListenTCP("tcp", tcpListenAddr) + if err != nil { + t.Fatalf("Fail to listen on port %d", freePort) + } + + // Allocate port, expect failure + job = newPortAllocationJob(eng, freePort) + if res := AllocatePort(job); res == engine.StatusOK { + t.Fatal("Successfully allocated currently used port") + } + + // Reclaim port, retry allocation + l.Close() + if res := AllocatePort(job); res != engine.StatusOK { + t.Fatal("Failed to allocate previously reclaimed port") + } +} diff --git a/components/engine/daemon/networkdriver/portmapper/mapper.go b/components/engine/daemon/networkdriver/portmapper/mapper.go index e29959a245..880cfb2986 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper.go +++ b/components/engine/daemon/networkdriver/portmapper/mapper.go @@ -3,10 +3,12 @@ package portmapper import ( "errors" "fmt" - "github.com/dotcloud/docker/pkg/iptables" - "github.com/dotcloud/docker/pkg/proxy" "net" "sync" + + "github.com/dotcloud/docker/daemon/networkdriver/portallocator" + "github.com/dotcloud/docker/pkg/iptables" + "github.com/dotcloud/docker/pkg/proxy" ) type mapping struct { @@ -31,47 +33,87 @@ 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 } -func Map(container net.Addr, hostIP net.IP, hostPort int) error { +func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) { lock.Lock() defer lock.Unlock() - var m *mapping + var ( + m *mapping + err error + proto string + allocatedHostPort int + ) + switch container.(type) { case *net.TCPAddr: + proto = "tcp" + if allocatedHostPort, err = portallocator.RequestPort(hostIP, proto, hostPort); err != nil { + return &net.TCPAddr{IP: hostIP, Port: hostPort}, err + } m = &mapping{ - proto: "tcp", - host: &net.TCPAddr{IP: hostIP, Port: hostPort}, + proto: proto, + host: &net.TCPAddr{IP: hostIP, Port: allocatedHostPort}, container: container, } case *net.UDPAddr: + proto = "udp" + if allocatedHostPort, err = portallocator.RequestPort(hostIP, proto, hostPort); err != nil { + return &net.UDPAddr{IP: hostIP, Port: hostPort}, err + } m = &mapping{ - proto: "udp", - host: &net.UDPAddr{IP: hostIP, Port: hostPort}, + proto: proto, + host: &net.UDPAddr{IP: hostIP, Port: allocatedHostPort}, container: container, } default: - return ErrUnknownBackendAddressType + // Always return a proper net.Addr for proper reporting. + return &genericAddr{IP: hostIP, Port: hostPort}, ErrUnknownBackendAddressType } + // When binding fails: + // - for a specifically requested port: it might be locked by some other + // process, so we want to allow for an ulterior retry + // - for an automatically allocated port: it falls in the Docker range of + // ports, so we'll just remember it as used and try the next free one + defer func() { + if err != nil && hostPort != 0 { + portallocator.ReleasePort(hostIP, proto, allocatedHostPort) + } + }() + key := getKey(m.host) if _, exists := currentMappings[key]; exists { - return ErrPortMappedForIP + err = ErrPortMappedForIP + return m.host, err } containerIP, containerPort := getIPAndPort(m.container) - if err := forward(iptables.Add, m.proto, hostIP, hostPort, containerIP.String(), containerPort); err != nil { - return err + if err := forward(iptables.Add, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil { + return m.host, err } p, err := newProxy(m.host, m.container) if err != nil { // need to undo the iptables rules before we reutrn - forward(iptables.Delete, m.proto, hostIP, hostPort, containerIP.String(), containerPort) - return err + forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) + return m.host, err } m.userlandProxy = p @@ -79,7 +121,7 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) error { go p.Run() - return nil + return m.host, nil } func Unmap(host net.Addr) error { @@ -100,6 +142,18 @@ func Unmap(host net.Addr) error { if err := forward(iptables.Delete, data.proto, hostIP, hostPort, containerIP.String(), containerPort); err != nil { return err } + + switch a := host.(type) { + case *net.TCPAddr: + if err := portallocator.ReleasePort(a.IP, "tcp", a.Port); err != nil { + return err + } + case *net.UDPAddr: + if err := portallocator.ReleasePort(a.IP, "udp", a.Port); err != nil { + return err + } + } + return nil } diff --git a/components/engine/daemon/networkdriver/portmapper/mapper_test.go b/components/engine/daemon/networkdriver/portmapper/mapper_test.go index 4c09f3c651..881ea028f3 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper_test.go +++ b/components/engine/daemon/networkdriver/portmapper/mapper_test.go @@ -44,20 +44,36 @@ func TestMapPorts(t *testing.T) { srcAddr1 := &net.TCPAddr{Port: 1080, IP: net.ParseIP("172.16.0.1")} srcAddr2 := &net.TCPAddr{Port: 1080, IP: net.ParseIP("172.16.0.2")} - if err := Map(srcAddr1, dstIp1, 80); err != nil { + addrEqual := func(addr1, addr2 net.Addr) bool { + return (addr1.Network() == addr2.Network()) && (addr1.String() == addr2.String()) + } + + if host, err := Map(srcAddr1, dstIp1, 80); err != nil { t.Fatalf("Failed to allocate port: %s", err) + } 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 Map(srcAddr1, dstIp1, 80) == nil { + if host, 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 Map(srcAddr2, dstIp1, 80) == nil { + if host, 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 err := Map(srcAddr2, dstIp2, 80); err != nil { + if host, 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 { From 52ddc2736f28db38350d7c6e2ffa1cd69f1e8fc9 Mon Sep 17 00:00:00 2001 From: Erik Hollensbe Date: Wed, 25 Jun 2014 13:17:20 -0700 Subject: [PATCH 2/4] Make ErrPortAlreadyAllocated an error interface with a few extras, adjust tests to fit. Docker-DCO-1.1-Signed-off-by: Erik Hollensbe (github: erikh) Upstream-commit: ffd68badc0a3d70fbd063903702355a387621b10 Component: engine --- .../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/components/engine/daemon/networkdriver/bridge/driver.go b/components/engine/daemon/networkdriver/bridge/driver.go index 8da7a0457a..16439d2aca 100644 --- a/components/engine/daemon/networkdriver/bridge/driver.go +++ b/components/engine/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/components/engine/daemon/networkdriver/portallocator/portallocator.go b/components/engine/daemon/networkdriver/portallocator/portallocator.go index 251ab94473..3b01e28a30 100644 --- a/components/engine/daemon/networkdriver/portallocator/portallocator.go +++ b/components/engine/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/components/engine/daemon/networkdriver/portallocator/portallocator_test.go b/components/engine/daemon/networkdriver/portallocator/portallocator_test.go index 5a4765ddd4..9869c332e9 100644 --- a/components/engine/daemon/networkdriver/portallocator/portallocator_test.go +++ b/components/engine/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/components/engine/daemon/networkdriver/portmapper/mapper.go b/components/engine/daemon/networkdriver/portmapper/mapper.go index 880cfb2986..d4469cd6d0 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper.go +++ b/components/engine/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/components/engine/daemon/networkdriver/portmapper/mapper_test.go b/components/engine/daemon/networkdriver/portmapper/mapper_test.go index 881ea028f3..cae74ebb8b 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper_test.go +++ b/components/engine/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 { From 4ad46ffe312dbd0d76f5d2c9d7cc483689e40f01 Mon Sep 17 00:00:00 2001 From: Erik Hollensbe Date: Thu, 26 Jun 2014 00:09:19 -0700 Subject: [PATCH 3/4] Use last allocated port logic in port allocator Docker-DCO-1.1-Signed-off-by: Erik Hollensbe (github: erikh) Upstream-commit: e77729c2e081c5ce55aa1d4f316b7c7703e5de96 Component: engine --- .../daemon/networkdriver/bridge/driver.go | 17 ++++--- .../portallocator/portallocator.go | 49 ++++++++++++------- .../daemon/networkdriver/portmapper/mapper.go | 24 ++++----- 3 files changed, 49 insertions(+), 41 deletions(-) diff --git a/components/engine/daemon/networkdriver/bridge/driver.go b/components/engine/daemon/networkdriver/bridge/driver.go index 16439d2aca..a843da0499 100644 --- a/components/engine/daemon/networkdriver/bridge/driver.go +++ b/components/engine/daemon/networkdriver/bridge/driver.go @@ -20,7 +20,8 @@ import ( ) const ( - DefaultNetworkBridge = "docker0" + DefaultNetworkBridge = "docker0" + MaxAllocatedPortAttempts = 10 ) // Network interface represents the networking stack of a container @@ -401,15 +402,15 @@ func AllocatePort(job *engine.Job) engine.Status { return job.Errorf("unsupported address type %s", proto) } - /* - Try up to 10 times to get a port that's not already allocated. - - In the event of failure to bind, return the error that portmapper.Map - yields. - */ + // + // Try up to 10 times to get a port that's not already allocated. + // + // In the event of failure to bind, return the error that portmapper.Map + // yields. + // var host net.Addr - for i := 0; i < 10; i++ { + for i := 0; i < MaxAllocatedPortAttempts; i++ { if host, err = portmapper.Map(container, ip, hostPort); err == nil { break } diff --git a/components/engine/daemon/networkdriver/portallocator/portallocator.go b/components/engine/daemon/networkdriver/portallocator/portallocator.go index 3b01e28a30..c722ba98ba 100644 --- a/components/engine/daemon/networkdriver/portallocator/portallocator.go +++ b/components/engine/daemon/networkdriver/portallocator/portallocator.go @@ -7,9 +7,13 @@ import ( "sync" ) +type portMap struct { + p map[int]struct{} + last int +} + type ( - portMap map[int]bool - protocolMap map[string]portMap + protocolMap map[string]*portMap ipMapping map[string]protocolMap ) @@ -71,8 +75,8 @@ func RequestPort(ip net.IP, proto string, port int) (int, error) { mapping := getOrCreate(ip) if port > 0 { - if !mapping[proto][port] { - mapping[proto][port] = true + if _, ok := mapping[proto].p[port]; !ok { + mapping[proto].p[port] = struct{}{} return port, nil } else { return 0, NewErrPortAlreadyAllocated(ip.String(), port) @@ -94,8 +98,8 @@ func ReleasePort(ip net.IP, proto string, port int) error { ip = getDefault(ip) - mapping := getOrCreate(ip) - delete(mapping[proto], port) + mapping := getOrCreate(ip)[proto] + delete(mapping.p, port) return nil } @@ -114,8 +118,8 @@ func getOrCreate(ip net.IP) protocolMap { if _, ok := globalMap[ipstr]; !ok { globalMap[ipstr] = protocolMap{ - "tcp": portMap{}, - "udp": portMap{}, + "tcp": &portMap{p: map[int]struct{}{}, last: 0}, + "udp": &portMap{p: map[int]struct{}{}, last: 0}, } } @@ -123,21 +127,28 @@ func getOrCreate(ip net.IP) protocolMap { } func findPort(ip net.IP, proto string) (int, error) { - port := BeginPortRange + mapping := getOrCreate(ip)[proto] - mapping := getOrCreate(ip) - - for mapping[proto][port] { - port++ - - if port > EndPortRange { - return 0, ErrAllPortsAllocated - } + if mapping.last == 0 { + mapping.p[BeginPortRange] = struct{}{} + mapping.last = BeginPortRange + return BeginPortRange, nil } - mapping[proto][port] = true + for port := mapping.last + 1; port != mapping.last; port++ { + if port > EndPortRange { + port = BeginPortRange + } - return port, nil + if _, ok := mapping.p[port]; !ok { + mapping.p[port] = struct{}{} + mapping.last = port + return port, nil + } + + } + + return 0, ErrAllPortsAllocated } func getDefault(ip net.IP) net.IP { diff --git a/components/engine/daemon/networkdriver/portmapper/mapper.go b/components/engine/daemon/networkdriver/portmapper/mapper.go index d4469cd6d0..1bd332271f 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper.go +++ b/components/engine/daemon/networkdriver/portmapper/mapper.go @@ -48,6 +48,13 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) { allocatedHostPort int ) + // release the port on any error during return. + defer func() { + if err != nil { + portallocator.ReleasePort(hostIP, proto, allocatedHostPort) + } + }() + switch container.(type) { case *net.TCPAddr: proto = "tcp" @@ -74,33 +81,22 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (net.Addr, error) { return nil, err } - // When binding fails: - // - for a specifically requested port: it might be locked by some other - // process, so we want to allow for an ulterior retry - // - for an automatically allocated port: it falls in the Docker range of - // ports, so we'll just remember it as used and try the next free one - defer func() { - if err != nil && hostPort != 0 { - portallocator.ReleasePort(hostIP, proto, allocatedHostPort) - } - }() - key := getKey(m.host) if _, exists := currentMappings[key]; exists { err = ErrPortMappedForIP - return m.host, err + return nil, err } containerIP, containerPort := getIPAndPort(m.container) if err := forward(iptables.Add, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort); err != nil { - return m.host, err + return nil, err } p, err := newProxy(m.host, m.container) if err != nil { // need to undo the iptables rules before we return forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) - return m.host, err + return nil, err } m.userlandProxy = p From 229de7479e90939f3649d2a62012411c7db27b6c Mon Sep 17 00:00:00 2001 From: Erik Hollensbe Date: Fri, 27 Jun 2014 15:57:20 -0700 Subject: [PATCH 4/4] portmapper: unit tests for remap problem Docker-DCO-1.1-Signed-off-by: Erik Hollensbe (github: erikh) Upstream-commit: 2cdcfc0c57e375955d3ed7c8e1487f9d4751b255 Component: engine --- .../networkdriver/portmapper/mapper_test.go | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/components/engine/daemon/networkdriver/portmapper/mapper_test.go b/components/engine/daemon/networkdriver/portmapper/mapper_test.go index cae74ebb8b..6affdc5445 100644 --- a/components/engine/daemon/networkdriver/portmapper/mapper_test.go +++ b/components/engine/daemon/networkdriver/portmapper/mapper_test.go @@ -1,6 +1,7 @@ package portmapper import ( + "github.com/dotcloud/docker/daemon/networkdriver/portallocator" "github.com/dotcloud/docker/pkg/iptables" "github.com/dotcloud/docker/pkg/proxy" "net" @@ -112,3 +113,40 @@ func TestGetUDPIPAndPort(t *testing.T) { t.Fatalf("expected port %d got %d", ep, port) } } + +func TestMapAllPortsSingleInterface(t *testing.T) { + dstIp1 := net.ParseIP("0.0.0.0") + srcAddr1 := &net.TCPAddr{Port: 1080, IP: net.ParseIP("172.16.0.1")} + + hosts := []net.Addr{} + var host net.Addr + var err error + + defer func() { + for _, val := range hosts { + Unmap(val) + } + }() + + for i := 0; i < 10; i++ { + for i := portallocator.BeginPortRange; i < portallocator.EndPortRange; i++ { + if host, err = Map(srcAddr1, dstIp1, 0); err != nil { + t.Fatal(err) + } + + hosts = append(hosts, host) + } + + if _, err := Map(srcAddr1, dstIp1, portallocator.BeginPortRange); err == nil { + t.Fatal("Port %d should be bound but is not", portallocator.BeginPortRange) + } + + for _, val := range hosts { + if err := Unmap(val); err != nil { + t.Fatal(err) + } + } + + hosts = []net.Addr{} + } +}