From a77b7dd2278106b9081d0ef2260fbeea790a91ef Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 5 Jan 2016 16:23:24 -0500 Subject: [PATCH] cleanup attach api calls Signed-off-by: Brian Goff --- api/server/router/container/backend.go | 3 +- .../router/container/container_routes.go | 89 ++++++++++++++----- api/types/backend/backend.go | 23 ++--- builder/builder.go | 2 +- builder/dockerfile/internals.go | 2 +- daemon/attach.go | 49 +++------- 6 files changed, 91 insertions(+), 77 deletions(-) diff --git a/api/server/router/container/backend.go b/api/server/router/container/backend.go index 4517b1a6aa..cc25f10919 100644 --- a/api/server/router/container/backend.go +++ b/api/server/router/container/backend.go @@ -59,8 +59,7 @@ type monitorBackend interface { // attachBackend includes function to implement to provide container attaching functionality. type attachBackend interface { - ContainerAttachWithLogs(name string, c *backend.ContainerAttachWithLogsConfig) error - ContainerWsAttachWithLogs(name string, c *backend.ContainerWsAttachWithLogsConfig) error + ContainerAttach(name string, c *backend.ContainerAttachConfig) error } // Backend is all the methods that need to be implemented to provide container specific functionality. diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 9bd5092cd8..07d40168be 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -3,6 +3,7 @@ package container import ( "encoding/json" "fmt" + "io" "net/http" "strconv" "strings" @@ -14,6 +15,7 @@ import ( "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types/backend" derr "github.com/docker/docker/errors" + "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/term" "github.com/docker/docker/runconfig" @@ -425,18 +427,45 @@ func (s *containerRouter) postContainersAttach(ctx context.Context, w http.Respo } } - attachWithLogsConfig := &backend.ContainerAttachWithLogsConfig{ - Hijacker: w.(http.Hijacker), - Upgrade: upgrade, + hijacker, ok := w.(http.Hijacker) + if !ok { + return derr.ErrorCodeNoHijackConnection.WithArgs(containerName) + } + + setupStreams := func() (io.ReadCloser, io.Writer, io.Writer, error) { + conn, _, err := hijacker.Hijack() + if err != nil { + return nil, nil, nil, err + } + + // set raw mode + conn.Write([]byte{}) + + if upgrade { + fmt.Fprintf(conn, "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n") + } else { + fmt.Fprintf(conn, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n") + } + + closer := func() error { + httputils.CloseStreams(conn) + return nil + } + return ioutils.NewReadCloserWrapper(conn, closer), conn, conn, nil + } + + attachConfig := &backend.ContainerAttachConfig{ + GetStreams: setupStreams, UseStdin: httputils.BoolValue(r, "stdin"), UseStdout: httputils.BoolValue(r, "stdout"), UseStderr: httputils.BoolValue(r, "stderr"), Logs: httputils.BoolValue(r, "logs"), Stream: httputils.BoolValue(r, "stream"), DetachKeys: keys, + MuxStreams: true, } - return s.backend.ContainerAttachWithLogs(containerName, attachWithLogsConfig) + return s.backend.ContainerAttach(containerName, attachConfig) } func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { @@ -455,24 +484,44 @@ func (s *containerRouter) wsContainersAttach(ctx context.Context, w http.Respons } } - h := websocket.Handler(func(ws *websocket.Conn) { - defer ws.Close() + done := make(chan struct{}) + started := make(chan struct{}) - wsAttachWithLogsConfig := &backend.ContainerWsAttachWithLogsConfig{ - InStream: ws, - OutStream: ws, - ErrStream: ws, - Logs: httputils.BoolValue(r, "logs"), - Stream: httputils.BoolValue(r, "stream"), - DetachKeys: keys, + setupStreams := func() (io.ReadCloser, io.Writer, io.Writer, error) { + wsChan := make(chan *websocket.Conn) + h := func(conn *websocket.Conn) { + wsChan <- conn + <-done } - if err := s.backend.ContainerWsAttachWithLogs(containerName, wsAttachWithLogsConfig); err != nil { - logrus.Errorf("Error attaching websocket: %s", utils.GetErrorMessage(err)) - } - }) - ws := websocket.Server{Handler: h, Handshake: nil} - ws.ServeHTTP(w, r) + srv := websocket.Server{Handler: h, Handshake: nil} + go func() { + close(started) + srv.ServeHTTP(w, r) + }() - return nil + conn := <-wsChan + return conn, conn, conn, nil + } + + attachConfig := &backend.ContainerAttachConfig{ + GetStreams: setupStreams, + Logs: httputils.BoolValue(r, "logs"), + Stream: httputils.BoolValue(r, "stream"), + DetachKeys: keys, + UseStdin: true, + UseStdout: true, + UseStderr: true, + MuxStreams: false, // TODO: this should be true since it's a single stream for both stdout and stderr + } + + err = s.backend.ContainerAttach(containerName, attachConfig) + close(done) + select { + case <-started: + logrus.Errorf("Error attaching websocket: %s", err) + return nil + default: + } + return err } diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go index 517887014c..c871a148c4 100644 --- a/api/types/backend/backend.go +++ b/api/types/backend/backend.go @@ -5,32 +5,25 @@ package backend import ( "io" - "net/http" "github.com/docker/engine-api/types" ) -// ContainerAttachWithLogsConfig holds the streams to use when connecting to a container to view logs. -type ContainerAttachWithLogsConfig struct { - Hijacker http.Hijacker - Upgrade bool +// ContainerAttachConfig holds the streams to use when connecting to a container to view logs. +type ContainerAttachConfig struct { + GetStreams func() (io.ReadCloser, io.Writer, io.Writer, error) UseStdin bool UseStdout bool UseStderr bool Logs bool Stream bool DetachKeys []byte -} -// ContainerWsAttachWithLogsConfig attach with websockets, since all -// stream data is delegated to the websocket to handle there. -type ContainerWsAttachWithLogsConfig struct { - InStream io.ReadCloser // Reader to attach to stdin of container - OutStream io.Writer // Writer to attach to stdout of container - ErrStream io.Writer // Writer to attach to stderr of container - Logs bool // If true return log output - Stream bool // If true return stream output - DetachKeys []byte + // 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... + // HOWEVER, the websocket endpoint is using a single stream and SHOULD be encoded with stdout/stderr as is done for HTTP since it is still just a single stream. + // Since such a change is an API change unrelated to the current changeset we'll keep it as is here and change separately. + MuxStreams bool } // ContainerLogsConfig holds configs for logging operations. Exists diff --git a/builder/builder.go b/builder/builder.go index da7aaf251e..e20893f18e 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -106,7 +106,7 @@ type Backend interface { // Pull tells Docker to pull image referenced by `name`. PullOnBuild(name string, authConfigs map[string]types.AuthConfig, output io.Writer) (Image, error) // ContainerAttach attaches to container. - ContainerAttachOnBuild(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool) error + ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool) error // ContainerCreate creates a new Docker container and returns potential warnings ContainerCreate(types.ContainerCreateConfig) (types.ContainerCreateResponse, error) // ContainerRm removes a container specified by `id`. diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index c5549820e7..bd352ae036 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -541,7 +541,7 @@ func (b *Builder) create() (string, error) { func (b *Builder) run(cID string) (err error) { errCh := make(chan error) go func() { - errCh <- b.docker.ContainerAttachOnBuild(cID, nil, b.Stdout, b.Stderr, true) + errCh <- b.docker.ContainerAttachRaw(cID, nil, b.Stdout, b.Stderr, true) }() finished := make(chan struct{}) diff --git a/daemon/attach.go b/daemon/attach.go index 4faa460e37..1beedbf138 100644 --- a/daemon/attach.go +++ b/daemon/attach.go @@ -13,11 +13,8 @@ import ( "github.com/docker/docker/pkg/stdcopy" ) -// ContainerAttachWithLogs attaches to logs according to the config passed in. See ContainerAttachWithLogsConfig. -func (daemon *Daemon) ContainerAttachWithLogs(prefixOrName string, c *backend.ContainerAttachWithLogsConfig) error { - if c.Hijacker == nil { - return derr.ErrorCodeNoHijackConnection.WithArgs(prefixOrName) - } +// ContainerAttach attaches to logs according to the config passed in. See ContainerAttachConfig. +func (daemon *Daemon) ContainerAttach(prefixOrName string, c *backend.ContainerAttachConfig) error { container, err := daemon.GetContainer(prefixOrName) if err != nil { return derr.ErrorCodeNoSuchContainer.WithArgs(prefixOrName) @@ -26,29 +23,15 @@ func (daemon *Daemon) ContainerAttachWithLogs(prefixOrName string, c *backend.Co return derr.ErrorCodePausedContainer.WithArgs(prefixOrName) } - conn, _, err := c.Hijacker.Hijack() + inStream, outStream, errStream, err := c.GetStreams() if err != nil { return err } - defer conn.Close() - // Flush the options to make sure the client sets the raw mode - conn.Write([]byte{}) - inStream := conn.(io.ReadCloser) - outStream := conn.(io.Writer) + defer inStream.Close() - if c.Upgrade { - fmt.Fprintf(outStream, "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n") - } else { - fmt.Fprintf(outStream, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n") - } - - var errStream io.Writer - - if !container.Config.Tty { - errStream = stdcopy.NewStdWriter(outStream, stdcopy.Stderr) + if !container.Config.Tty && c.MuxStreams { + errStream = stdcopy.NewStdWriter(errStream, stdcopy.Stderr) outStream = stdcopy.NewStdWriter(outStream, stdcopy.Stdout) - } else { - errStream = outStream } var stdin io.ReadCloser @@ -64,32 +47,22 @@ func (daemon *Daemon) ContainerAttachWithLogs(prefixOrName string, c *backend.Co stderr = errStream } - if err := daemon.attachWithLogs(container, stdin, stdout, stderr, c.Logs, c.Stream, c.DetachKeys); err != nil { + if err := daemon.containerAttach(container, stdin, stdout, stderr, c.Logs, c.Stream, c.DetachKeys); err != nil { fmt.Fprintf(outStream, "Error attaching: %s\n", err) } return nil } -// ContainerWsAttachWithLogs websocket connection -func (daemon *Daemon) ContainerWsAttachWithLogs(prefixOrName string, c *backend.ContainerWsAttachWithLogsConfig) error { +// ContainerAttachRaw attaches the provided streams to the container's stdio +func (daemon *Daemon) ContainerAttachRaw(prefixOrName string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool) error { container, err := daemon.GetContainer(prefixOrName) if err != nil { return err } - return daemon.attachWithLogs(container, c.InStream, c.OutStream, c.ErrStream, c.Logs, c.Stream, c.DetachKeys) + return daemon.containerAttach(container, stdin, stdout, stderr, false, stream, nil) } -// ContainerAttachOnBuild attaches streams to the container cID. If stream is true, it streams the output. -func (daemon *Daemon) ContainerAttachOnBuild(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool) error { - return daemon.ContainerWsAttachWithLogs(cID, &backend.ContainerWsAttachWithLogsConfig{ - InStream: stdin, - OutStream: stdout, - ErrStream: stderr, - Stream: stream, - }) -} - -func (daemon *Daemon) attachWithLogs(container *container.Container, stdin io.ReadCloser, stdout, stderr io.Writer, logs, stream bool, keys []byte) error { +func (daemon *Daemon) containerAttach(container *container.Container, stdin io.ReadCloser, stdout, stderr io.Writer, logs, stream bool, keys []byte) error { if logs { logDriver, err := daemon.getLogger(container) if err != nil {