Merge pull request #21092 from WeiZhang555/fix-21064-detach-keys

Client print error when specify wrong detach keys

api/client/attach.go

@@ -3,6 +3,7 @@ package client
 import (
 	"fmt"
 	"io"
+	"net/http/httputil"
 
 	"golang.org/x/net/context"
 
@@ -66,9 +67,12 @@ func (cli *DockerCli) CmdAttach(args ...string) error {
 		defer signal.StopCatch(sigc)
 	}
 
-	resp, err := cli.client.ContainerAttach(context.Background(), options)
-	if err != nil {
-		return err
+	resp, errAttach := cli.client.ContainerAttach(context.Background(), options)
+	if errAttach != nil && errAttach != httputil.ErrPersistEOF {
+		// ContainerAttach returns an ErrPersistEOF (connection closed)
+		// means server met an error and put it in Hijacked connection
+		// keep the error and read detailed error message from hijacked connection later
+		return errAttach
 	}
 	defer resp.Close()
 
@@ -90,6 +94,10 @@ func (cli *DockerCli) CmdAttach(args ...string) error {
 		return err
 	}
 
+	if errAttach != nil {
+		return errAttach
+	}
+
 	_, status, err := getExitCode(cli, options.ContainerID)
 	if err != nil {
 		return err

api/client/run.go

@@ -3,6 +3,7 @@ package client
 import (
 	"fmt"
 	"io"
+	"net/http/httputil"
 	"os"
 	"runtime"
 	"strings"
@@ -205,19 +206,26 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 			DetachKeys:  cli.configFile.DetachKeys,
 		}
 
-		resp, err := cli.client.ContainerAttach(context.Background(), options)
-		if err != nil {
-			return err
+		resp, errAttach := cli.client.ContainerAttach(context.Background(), options)
+		if errAttach != nil && errAttach != httputil.ErrPersistEOF {
+			// ContainerAttach returns an ErrPersistEOF (connection closed)
+			// means server met an error and put it in Hijacked connection
+			// keep the error and read detailed error message from hijacked connection later
+			return errAttach
 		}
 		ctx, cancelFun = context.WithCancel(context.Background())
 		errCh = promise.Go(func() error {
-			return cli.holdHijackedConnection(ctx, config.Tty, in, out, stderr, resp)
+			errHijack := cli.holdHijackedConnection(ctx, config.Tty, in, out, stderr, resp)
+			if errHijack == nil {
+				return errAttach
+			}
+			return errHijack
 		})
 	}
 
 	if *flAutoRemove {
 		defer func() {
-			if err := cli.removeContainer(createResponse.ID, true, false, false); err != nil {
+			if err := cli.removeContainer(createResponse.ID, true, false, true); err != nil {
 				fmt.Fprintf(cli.err, "%v\n", err)
 			}
 		}()

api/client/start.go

@@ -3,6 +3,7 @@ package client
 import (
 	"fmt"
 	"io"
+	"net/http/httputil"
 	"os"
 	"strings"
 
@@ -94,14 +95,21 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 			in = cli.in
 		}
 
-		resp, err := cli.client.ContainerAttach(context.Background(), options)
-		if err != nil {
-			return err
+		resp, errAttach := cli.client.ContainerAttach(context.Background(), options)
+		if errAttach != nil && errAttach != httputil.ErrPersistEOF {
+			// ContainerAttach return an ErrPersistEOF (connection closed)
+			// means server met an error and put it in Hijacked connection
+			// keep the error and read detailed error message from hijacked connection
+			return errAttach
 		}
 		defer resp.Close()
 		ctx, cancelFun := context.WithCancel(context.Background())
 		cErr := promise.Go(func() error {
-			return cli.holdHijackedConnection(ctx, c.Config.Tty, in, cli.out, cli.err, resp)
+			errHijack := cli.holdHijackedConnection(ctx, c.Config.Tty, in, cli.out, cli.err, resp)
+			if errHijack == nil {
+				return errAttach
+			}
+			return errHijack
 		})
 
 		// 3. Start the container.

api/server/httputils/errors.go

@@ -24,11 +24,11 @@ type inputValidationError interface {
 	IsValidationError() bool
 }
 
-// WriteError decodes a specific docker error and sends it in the response.
-func WriteError(w http.ResponseWriter, err error) {
-	if err == nil || w == nil {
-		logrus.WithFields(logrus.Fields{"error": err, "writer": w}).Error("unexpected HTTP error handling")
-		return
+// GetHTTPErrorStatusCode retrieve status code from error message
+func GetHTTPErrorStatusCode(err error) int {
+	if err == nil {
+		logrus.WithFields(logrus.Fields{"error": err}).Error("unexpected HTTP error handling")
+		return http.StatusInternalServerError
 	}
 
 	var statusCode int
@@ -65,5 +65,16 @@ func WriteError(w http.ResponseWriter, err error) {
 		statusCode = http.StatusInternalServerError
 	}
 
-	http.Error(w, errMsg, statusCode)
+	return statusCode
+}
+
+// WriteError decodes a specific docker error and sends it in the response.
+func WriteError(w http.ResponseWriter, err error) {
+	if err == nil || w == nil {
+		logrus.WithFields(logrus.Fields{"error": err, "writer": w}).Error("unexpected HTTP error handling")
+		return
+	}
+
+	statusCode := GetHTTPErrorStatusCode(err)
+	http.Error(w, err.Error(), statusCode)
 }

api/server/router/container/container_routes.go

@@ -15,7 +15,6 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/pkg/ioutils"
 	"github.com/docker/docker/pkg/signal"
-	"github.com/docker/docker/pkg/term"
 	"github.com/docker/engine-api/types"
 	"github.com/docker/engine-api/types/container"
 	"github.com/docker/engine-api/types/filters"
@@ -408,15 +407,7 @@ func (s *containerRouter) postContainersAttach(ctx context.Context, w http.Respo
 	containerName := vars["name"]
 
 	_, upgrade := r.Header["Upgrade"]
-
-	keys := []byte{}
 	detachKeys := r.FormValue("detachKeys")
-	if detachKeys != "" {
-		keys, err = term.ToBytes(detachKeys)
-		if err != nil {
-			logrus.Warnf("Invalid escape keys provided (%s) using default : ctrl-p ctrl-q", detachKeys)
-		}
-	}
 
 	hijacker, ok := w.(http.Hijacker)
 	if !ok {
@@ -452,11 +443,24 @@ func (s *containerRouter) postContainersAttach(ctx context.Context, w http.Respo
 		UseStderr:  httputils.BoolValue(r, "stderr"),
 		Logs:       httputils.BoolValue(r, "logs"),
 		Stream:     httputils.BoolValue(r, "stream"),
-		DetachKeys: keys,
+		DetachKeys: detachKeys,
 		MuxStreams: true,
 	}
 
-	return s.backend.ContainerAttach(containerName, attachConfig)
+	if err = s.backend.ContainerAttach(containerName, attachConfig); err != nil {
+		logrus.Errorf("Handler for %s %s returned error: %v", r.Method, r.URL.Path, err)
+		// Remember to close stream if error happens
+		conn, _, errHijack := hijacker.Hijack()
+		if errHijack == nil {
+			statusCode := httputils.GetHTTPErrorStatusCode(err)
+			statusText := http.StatusText(statusCode)
+			fmt.Fprintf(conn, "HTTP/1.1 %d %s\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n%s\r\n", statusCode, statusText, err.Error())
+			httputils.CloseStreams(conn)
+		} else {
+			logrus.Errorf("Error Hijacking: %v", err)
+		}
+	}
+	return nil
 }
 
 func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
@@ -465,15 +469,8 @@ func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.Respons
 	}
 	containerName := vars["name"]
 
-	var keys []byte
 	var err error
 	detachKeys := r.FormValue("detachKeys")
-	if detachKeys != "" {
-		keys, err = term.ToBytes(detachKeys)
-		if err != nil {
-			logrus.Warnf("Invalid escape keys provided (%s) using default : ctrl-p ctrl-q", detachKeys)
-		}
-	}
 
 	done := make(chan struct{})
 	started := make(chan struct{})
@@ -499,7 +496,7 @@ func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.Respons
 		GetStreams: setupStreams,
 		Logs:       httputils.BoolValue(r, "logs"),
 		Stream:     httputils.BoolValue(r, "stream"),
-		DetachKeys: keys,
+		DetachKeys: detachKeys,
 		UseStdin:   true,
 		UseStdout:  true,
 		UseStderr:  true,

api/types/backend/backend.go

@@ -18,7 +18,7 @@ type ContainerAttachConfig struct {
 	UseStderr  bool
 	Logs       bool
 	Stream     bool
-	DetachKeys []byte
+	DetachKeys string
 
 	// Used to signify that streams are multiplexed and therefore need a StdWriter to encode stdout/sderr messages accordingly.
 	// TODO @cpuguy83: This shouldn't be needed. It was only added so that http and websocket endpoints can use the same function, and the websocket function was not using a stdwriter prior to this change...

daemon/attach.go

@@ -11,10 +11,20 @@ import (
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/errors"
 	"github.com/docker/docker/pkg/stdcopy"
+	"github.com/docker/docker/pkg/term"
 )
 
 // ContainerAttach attaches to logs according to the config passed in. See ContainerAttachConfig.
 func (daemon *Daemon) ContainerAttach(prefixOrName string, c *backend.ContainerAttachConfig) error {
+	keys := []byte{}
+	var err error
+	if c.DetachKeys != "" {
+		keys, err = term.ToBytes(c.DetachKeys)
+		if err != nil {
+			return fmt.Errorf("Invalid escape keys (%s) provided", c.DetachKeys)
+		}
+	}
+
 	container, err := daemon.GetContainer(prefixOrName)
 	if err != nil {
 		return err
@@ -48,7 +58,7 @@ func (daemon *Daemon) ContainerAttach(prefixOrName string, c *backend.ContainerA
 		stderr = errStream
 	}
 
-	if err := daemon.containerAttach(container, stdin, stdout, stderr, c.Logs, c.Stream, c.DetachKeys); err != nil {
+	if err := daemon.containerAttach(container, stdin, stdout, stderr, c.Logs, c.Stream, keys); err != nil {
 		fmt.Fprintf(outStream, "Error attaching: %s\n", err)
 	}
 	return nil

daemon/exec.go

@@ -101,7 +101,8 @@ func (d *Daemon) ContainerExecCreate(config *types.ExecConfig) (string, error) {
 	if config.DetachKeys != "" {
 		keys, err = term.ToBytes(config.DetachKeys)
 		if err != nil {
-			logrus.Warnf("Wrong escape keys provided (%s, error: %s) using default : ctrl-p ctrl-q", config.DetachKeys, err.Error())
+			err = fmt.Errorf("Invalid escape keys (%s) provided", config.DetachKeys)
+			return "", err
 		}
 	}
 

hack/vendor.sh

@@ -24,7 +24,7 @@ clone git golang.org/x/net 47990a1ba55743e6ef1affd3a14e5bac8553615d https://gith
 clone git golang.org/x/sys eb2c74142fd19a79b3f237334c7384d5167b1b46 https://github.com/golang/sys.git
 clone git github.com/docker/go-units 651fc226e7441360384da338d0fd37f2440ffbe3
 clone git github.com/docker/go-connections v0.2.0
-clone git github.com/docker/engine-api e37a82dfcea64559ca6a581776253c01d83357d9
+clone git github.com/docker/engine-api 8924d6900370b4c7e7984be5adc61f50a80d7537
 clone git github.com/RackSec/srslog 259aed10dfa74ea2961eddd1d9847619f6e98837
 clone git github.com/imdario/mergo 0.2.1
 

integration-cli/docker_api_attach_test.go

@@ -67,11 +67,18 @@ func (s *DockerSuite) TestGetContainersAttachWebsocket(c *check.C) {
 
 // regression gh14320
 func (s *DockerSuite) TestPostContainersAttachContainerNotFound(c *check.C) {
-	status, body, err := sockRequest("POST", "/containers/doesnotexist/attach", nil)
-	c.Assert(status, checker.Equals, http.StatusNotFound)
+	req, client, err := newRequestClient("POST", "/containers/doesnotexist/attach", nil, "")
 	c.Assert(err, checker.IsNil)
-	expected := "No such container: doesnotexist\n"
-	c.Assert(string(body), checker.Contains, expected)
+
+	resp, err := client.Do(req)
+	// connection will shutdown, err should be "persistent connection closed"
+	c.Assert(err, checker.NotNil) // Server shutdown connection
+
+	body, err := readBody(resp.Body)
+	c.Assert(err, checker.IsNil)
+	c.Assert(resp.StatusCode, checker.Equals, http.StatusNotFound)
+	expected := "No such container: doesnotexist\r\n"
+	c.Assert(string(body), checker.Equals, expected)
 }
 
 func (s *DockerSuite) TestGetContainersWsAttachContainerNotFound(c *check.C) {

integration-cli/docker_cli_run_unix_test.go

@@ -197,6 +197,38 @@ func (s *DockerSuite) TestRunAttachDetachFromFlag(c *check.C) {
 	c.Assert(running, checker.Equals, "true", check.Commentf("expected container to still be running"))
 }
 
+// TestRunDetach checks attaching and detaching with the escape sequence specified via flags.
+func (s *DockerSuite) TestRunAttachDetachFromInvalidFlag(c *check.C) {
+	name := "attach-detach"
+	dockerCmd(c, "run", "--name", name, "-itd", "busybox", "top")
+	c.Assert(waitRun(name), check.IsNil)
+
+	// specify an invalid detach key, container will ignore it and use default
+	cmd := exec.Command(dockerBinary, "attach", "--detach-keys='ctrl-A,a'", name)
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		c.Fatal(err)
+	}
+	cpty, tty, err := pty.Open()
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer cpty.Close()
+	cmd.Stdin = tty
+	if err := cmd.Start(); err != nil {
+		c.Fatal(err)
+	}
+
+	bufReader := bufio.NewReader(stdout)
+	out, err := bufReader.ReadString('\n')
+	if err != nil {
+		c.Fatal(err)
+	}
+	// it should print a warning to indicate the detach key flag is invalid
+	errStr := "Invalid escape keys (ctrl-A,a) provided"
+	c.Assert(strings.TrimSpace(out), checker.Equals, errStr)
+}
+
 // TestRunDetach checks attaching and detaching with the escape sequence specified via config file.
 func (s *DockerSuite) TestRunAttachDetachFromConfig(c *check.C) {
 	keyCtrlA := []byte{1}

vendor/src/github.com/docker/engine-api/client/container_wait.go

@@ -8,7 +8,7 @@ import (
 	"github.com/docker/engine-api/types"
 )
 
-// ContainerWait pauses execution util a container exits.
+// ContainerWait pauses execution until a container exits.
 // It returns the API status code as response of its readiness.
 func (cli *Client) ContainerWait(ctx context.Context, containerID string) (int, error) {
 	resp, err := cli.post(ctx, "/containers/"+containerID+"/wait", nil, nil, nil)

vendor/src/github.com/docker/engine-api/client/hijack.go

@@ -46,8 +46,7 @@ func (cli *Client) postHijacked(ctx context.Context, path string, query url.Valu
 	req.Header.Set("Connection", "Upgrade")
 	req.Header.Set("Upgrade", "tcp")
 
-	tlsConfig := cli.transport.TLSConfig()
-	conn, err := dial(cli.proto, cli.addr, tlsConfig)
+	conn, err := dial(cli.proto, cli.addr, cli.transport.TLSConfig())
 	if err != nil {
 		if strings.Contains(err.Error(), "connection refused") {
 			return types.HijackedResponse{}, fmt.Errorf("Cannot connect to the Docker daemon. Is 'docker daemon' running on this host?")
@@ -69,11 +68,11 @@ func (cli *Client) postHijacked(ctx context.Context, path string, query url.Valu
 	defer clientconn.Close()
 
 	// Server hijacks the connection, error 'connection closed' expected
-	clientconn.Do(req)
+	_, err = clientconn.Do(req)
 
 	rwc, br := clientconn.Hijack()
 
-	return types.HijackedResponse{Conn: rwc, Reader: br}, nil
+	return types.HijackedResponse{Conn: rwc, Reader: br}, err
 }
 
 func tlsDial(network, addr string, config *tls.Config) (net.Conn, error) {
@@ -126,6 +125,21 @@ func tlsDialWithDialer(dialer *net.Dialer, network, addr string, config *tls.Con
 		tcpConn.SetKeepAlivePeriod(30 * time.Second)
 	}
 
+	colonPos := strings.LastIndex(addr, ":")
+	if colonPos == -1 {
+		colonPos = len(addr)
+	}
+	hostname := addr[:colonPos]
+
+	// If no ServerName is set, infer the ServerName
+	// from the hostname we're connecting to.
+	if config.ServerName == "" {
+		// Make a copy to avoid polluting argument or default.
+		c := *config
+		c.ServerName = hostname
+		config = &c
+	}
+
 	conn := tls.Client(rawConn, config)
 
 	if timeout == 0 {

vendor/src/github.com/docker/engine-api/client/transport/transport.go

@@ -4,7 +4,6 @@ package transport
 import (
 	"fmt"
 	"net/http"
-	"strings"
 
 	"github.com/docker/go-connections/sockets"
 )
@@ -35,10 +34,6 @@ func NewTransportWithHTTP(proto, addr string, client *http.Client) (Client, erro
 		}
 	}
 
-	if transport.TLSClientConfig != nil && transport.TLSClientConfig.ServerName == "" {
-		transport.TLSClientConfig.ServerName = hostname(addr)
-	}
-
 	return &apiTransport{
 		Client:    client,
 		tlsInfo:   &tlsInfo{transport.TLSClientConfig},
@@ -59,12 +54,4 @@ func defaultTransport(proto, addr string) *http.Transport {
 	return tr
 }
 
-func hostname(addr string) string {
-	colonPos := strings.LastIndex(addr, ":")
-	if colonPos == -1 {
-		return addr
-	}
-	return addr[:colonPos]
-}
-
 var _ Client = &apiTransport{}

vendor/src/github.com/docker/engine-api/types/client.go

@@ -103,7 +103,7 @@ func (h *HijackedResponse) Close() {
 	h.Conn.Close()
 }
 
-// CloseWriter is an interface that implement structs
+// CloseWriter is an interface that implements structs
 // that close input streams to prevent from writing.
 type CloseWriter interface {
 	CloseWrite() error

vendor/src/github.com/docker/engine-api/types/container/host_config.go

@@ -236,10 +236,11 @@ type Resources struct {
 	Ulimits              []*units.Ulimit // List of ulimits to be set in the container
 
 	// Applicable to Windows
-	CPUCount     int64  `json:"CpuCount"`   // CPU count
-	CPUPercent   int64  `json:"CpuPercent"` // CPU percent
-	MaximumIOps  uint64 // Maximum IOps for the container system drive
-	MaximumIOBps uint64 // Maximum IO in bytes per second for the container system drive
+	CPUCount                int64  `json:"CpuCount"`   // CPU count
+	CPUPercent              int64  `json:"CpuPercent"` // CPU percent
+	IOMaximumIOps           uint64 // Maximum IOps for the container system drive
+	IOMaximumBandwidth      uint64 // Maximum IO in bytes per second for the container system drive
+	NetworkMaximumBandwidth uint64 // Maximum bandwidth of the network endpoint in bytes per second
 }
 
 // UpdateConfig holds the mutable attributes of a Container.

vendor/src/github.com/docker/engine-api/types/stats.go

@@ -8,7 +8,7 @@ import "time"
 type ThrottlingData struct {
 	// Number of periods with throttling active
 	Periods uint64 `json:"periods"`
-	// Number of periods when the container hit its throttling limit.
+	// Number of periods when the container hits its throttling limit.
 	ThrottledPeriods uint64 `json:"throttled_periods"`
 	// Aggregate time the container was throttled for in nanoseconds.
 	ThrottledTime uint64 `json:"throttled_time"`