From 3109fc9537682a7b9ee566f3674484b1b2296707 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 5 Sep 2014 18:59:31 -0700 Subject: [PATCH 1/3] Add Test for port allocation bug (port already in use by other programs than docker) Signed-off-by: Tibor Vass --- integration-cli/docker_cli_run_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index e781f3782..1dc0e41b0 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -4,6 +4,7 @@ import ( "bufio" "fmt" "io/ioutil" + "net" "os" "os/exec" "path" @@ -1875,3 +1876,21 @@ func TestRunDeallocatePortOnMissingIptablesRule(t *testing.T) { deleteAllContainers() logDone("run - port should be deallocated even on iptables error") } + +func TestRunPortInUse(t *testing.T) { + port := "1234" + l, err := net.Listen("tcp", ":"+port) + if err != nil { + t.Fatal(err) + } + defer l.Close() + cmd := exec.Command(dockerBinary, "run", "-p", port+":80", "busybox", "true") + out, _, err := runCommandWithOutput(cmd) + if err == nil { + t.Fatalf("Host port %s already in use, has been allocated by docker: %q", port, out) + } + + deleteAllContainers() + + logDone("run - port in use") +} From 3b6a29b81a5280187b3d03c91950cf93f7e263ec Mon Sep 17 00:00:00 2001 From: Erik Hollensbe Date: Sun, 7 Sep 2014 12:33:51 -0700 Subject: [PATCH 2/3] Fix an issue where already allocated ports would not trigger an error. Docker-DCO-1.1-Signed-off-by: Erik Hollensbe (github: erikh) --- daemon/networkdriver/portmapper/mapper.go | 21 ++++++++- daemon/networkdriver/portmapper/proxy.go | 55 +++++++++++++++++++---- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index 7e8e266bb..774b58059 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/daemon/networkdriver/portmapper/mapper.go @@ -100,11 +100,28 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (host net.Addr, err er m.userlandProxy = proxy currentMappings[key] = m - if err := proxy.Start(); err != nil { + cleanup := func() error { // need to undo the iptables rules before we return forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) + proxy.Stop() + forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) + m.userlandProxy = nil + delete(currentMappings, key) + if err := portallocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil { + return err + } - return nil, err + return nil + } + + if err := proxy.Start(); err != nil { + if err := cleanup(); err != nil { + return nil, fmt.Errorf("Error during port allocation cleanup: %v", err) + } + + if err == ErrPortMappingFailure { + return nil, portallocator.NewErrPortAlreadyAllocated(hostIP.String(), allocatedHostPort) + } } return m.host, nil diff --git a/daemon/networkdriver/portmapper/proxy.go b/daemon/networkdriver/portmapper/proxy.go index b24723727..67cd9710b 100644 --- a/daemon/networkdriver/portmapper/proxy.go +++ b/daemon/networkdriver/portmapper/proxy.go @@ -1,6 +1,7 @@ package portmapper import ( + "errors" "flag" "log" "net" @@ -9,11 +10,14 @@ import ( "os/signal" "strconv" "syscall" + "time" "github.com/docker/docker/pkg/proxy" "github.com/docker/docker/reexec" ) +var ErrPortMappingFailure = errors.New("Failure Mapping Port") + const userlandProxyCommandName = "docker-proxy" func init() { @@ -37,9 +41,12 @@ func execProxy() { p, err := proxy.NewProxy(host, container) if err != nil { - log.Fatal(err) + os.Stdout.WriteString("1\n") + os.Exit(1) } + os.Stdout.WriteString("0\n") + go handleStopSignals(p) // Run will block until the proxy stops @@ -96,10 +103,8 @@ func NewProxyCommand(proto string, hostIP net.IP, hostPort int, containerIP net. return &proxyCommand{ cmd: &exec.Cmd{ - Path: reexec.Self(), - Args: args, - Stdout: os.Stdout, - Stderr: os.Stderr, + Path: reexec.Self(), + Args: args, SysProcAttr: &syscall.SysProcAttr{ Pdeathsig: syscall.SIGTERM, // send a sigterm to the proxy if the daemon process dies }, @@ -108,12 +113,44 @@ func NewProxyCommand(proto string, hostIP net.IP, hostPort int, containerIP net. } func (p *proxyCommand) Start() error { - return p.cmd.Start() + stdout, err := p.cmd.StdoutPipe() + if err != nil { + return err + } + if err := p.cmd.Start(); err != nil { + return err + } + + errchan := make(chan error) + after := time.After(1 * time.Second) + go func() { + buf := make([]byte, 2) + stdout.Read(buf) + + if string(buf) != "0\n" { + errchan <- ErrPortMappingFailure + } else { + errchan <- nil + } + }() + + var readErr error + + select { + case readErr = <-errchan: + case <-after: + readErr = ErrPortMappingFailure + } + + return readErr } func (p *proxyCommand) Stop() error { - err := p.cmd.Process.Signal(os.Interrupt) - p.cmd.Wait() + if p.cmd.Process != nil { + err := p.cmd.Process.Signal(os.Interrupt) + p.cmd.Wait() + return err + } - return err + return nil } From 41e9e93e27ccd637d9490412622529bdc7d7b8ff Mon Sep 17 00:00:00 2001 From: Alexandr Morozov Date: Mon, 8 Sep 2014 14:22:50 +0400 Subject: [PATCH 3/3] Fix my own comments from #7927 Signed-off-by: Alexandr Morozov --- daemon/networkdriver/portmapper/mapper.go | 14 ++----- daemon/networkdriver/portmapper/proxy.go | 45 ++++++++++++----------- integration-cli/docker_cli_run_test.go | 8 ++-- 3 files changed, 31 insertions(+), 36 deletions(-) diff --git a/daemon/networkdriver/portmapper/mapper.go b/daemon/networkdriver/portmapper/mapper.go index 774b58059..24ca0d892 100644 --- a/daemon/networkdriver/portmapper/mapper.go +++ b/daemon/networkdriver/portmapper/mapper.go @@ -97,16 +97,10 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (host net.Addr, err er return nil, err } - m.userlandProxy = proxy - currentMappings[key] = m - cleanup := func() error { // need to undo the iptables rules before we return - forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) proxy.Stop() forward(iptables.Delete, m.proto, hostIP, allocatedHostPort, containerIP.String(), containerPort) - m.userlandProxy = nil - delete(currentMappings, key) if err := portallocator.ReleasePort(hostIP, m.proto, allocatedHostPort); err != nil { return err } @@ -118,12 +112,10 @@ func Map(container net.Addr, hostIP net.IP, hostPort int) (host net.Addr, err er if err := cleanup(); err != nil { return nil, fmt.Errorf("Error during port allocation cleanup: %v", err) } - - if err == ErrPortMappingFailure { - return nil, portallocator.NewErrPortAlreadyAllocated(hostIP.String(), allocatedHostPort) - } + return nil, err } - + m.userlandProxy = proxy + currentMappings[key] = m return m.host, nil } diff --git a/daemon/networkdriver/portmapper/proxy.go b/daemon/networkdriver/portmapper/proxy.go index 67cd9710b..1e8e0c398 100644 --- a/daemon/networkdriver/portmapper/proxy.go +++ b/daemon/networkdriver/portmapper/proxy.go @@ -1,8 +1,9 @@ package portmapper import ( - "errors" "flag" + "fmt" + "io/ioutil" "log" "net" "os" @@ -16,8 +17,6 @@ import ( "github.com/docker/docker/reexec" ) -var ErrPortMappingFailure = errors.New("Failure Mapping Port") - const userlandProxyCommandName = "docker-proxy" func init() { @@ -42,12 +41,11 @@ func execProxy() { p, err := proxy.NewProxy(host, container) if err != nil { os.Stdout.WriteString("1\n") + fmt.Fprint(os.Stderr, err) os.Exit(1) } - - os.Stdout.WriteString("0\n") - go handleStopSignals(p) + os.Stdout.WriteString("0\n") // Run will block until the proxy stops p.Run() @@ -117,40 +115,43 @@ func (p *proxyCommand) Start() error { if err != nil { return err } + defer stdout.Close() + stderr, err := p.cmd.StderrPipe() + if err != nil { + return err + } + defer stderr.Close() if err := p.cmd.Start(); err != nil { return err } - errchan := make(chan error) - after := time.After(1 * time.Second) + errchan := make(chan error, 1) go func() { buf := make([]byte, 2) stdout.Read(buf) if string(buf) != "0\n" { - errchan <- ErrPortMappingFailure - } else { - errchan <- nil + errStr, _ := ioutil.ReadAll(stderr) + errchan <- fmt.Errorf("Error starting userland proxy: %s", errStr) + return } + errchan <- nil }() - var readErr error - select { - case readErr = <-errchan: - case <-after: - readErr = ErrPortMappingFailure + case err := <-errchan: + return err + case <-time.After(1 * time.Second): + return fmt.Errorf("Timed out proxy starting the userland proxy") } - - return readErr } func (p *proxyCommand) Stop() error { if p.cmd.Process != nil { - err := p.cmd.Process.Signal(os.Interrupt) - p.cmd.Wait() - return err + if err := p.cmd.Process.Signal(os.Interrupt); err != nil { + return err + } + return p.cmd.Wait() } - return nil } diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 1dc0e41b0..a8632a6f8 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -1887,10 +1887,12 @@ func TestRunPortInUse(t *testing.T) { cmd := exec.Command(dockerBinary, "run", "-p", port+":80", "busybox", "true") out, _, err := runCommandWithOutput(cmd) if err == nil { - t.Fatalf("Host port %s already in use, has been allocated by docker: %q", port, out) + t.Fatalf("Binding on used port must fail") + } + if !strings.Contains(out, "address already in use") { + t.Fatalf("Out must be about \"address already in use\", got %s", out) } deleteAllContainers() - - logDone("run - port in use") + logDone("run - fail if port already in use") }