diff --git a/daemon/networkdriver/bridge/driver.go b/daemon/networkdriver/bridge/driver.go index 8c5db9f843..8da7a0457a 100644 --- a/daemon/networkdriver/bridge/driver.go +++ b/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/daemon/networkdriver/bridge/driver_test.go b/daemon/networkdriver/bridge/driver_test.go new file mode 100644 index 0000000000..f8ddd4c64e --- /dev/null +++ b/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/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index e29959a245..880cfb2986 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/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/daemon/networkdriver/portmapper/mapper_test.go b/daemon/networkdriver/portmapper/mapper_test.go index 4c09f3c651..881ea028f3 100644 --- a/daemon/networkdriver/portmapper/mapper_test.go +++ b/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 {