From 2ea9dac5e1d00b2820bd7156e3bea4b9fd98c1e6 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 24 Aug 2020 11:35:01 -0400 Subject: Send HTTP Hijack headers after successful attach Our previous flow was to perform a hijack before passing a connection into Libpod, and then Libpod would attach to the container's attach socket and begin forwarding traffic. A problem emerges: we write the attach header as soon as the attach complete. As soon as we write the header, the client assumes that all is ready, and sends a Start request. This Start may be processed *before* we successfully finish attaching, causing us to lose output. The solution is to handle hijacking inside Libpod. Unfortunately, this requires a downright extensive refactor of the Attach and HTTP Exec StartAndAttach code. I think the result is an improvement in some places (a lot more errors will be handled with a proper HTTP error code, before the hijack occurs) but other parts, like the relocation of printing container logs, are just *bad*. Still, we need this fixed now to get CI back into good shape... Fixes #7195 Signed-off-by: Matthew Heon --- libpod/container_api.go | 85 +++---------------------------------------------- 1 file changed, 5 insertions(+), 80 deletions(-) (limited to 'libpod/container_api.go') diff --git a/libpod/container_api.go b/libpod/container_api.go index c44e89042..0d7bbacd0 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -1,18 +1,14 @@ package libpod import ( - "bufio" "context" "io/ioutil" - "net" + "net/http" "os" - "strings" - "sync" "time" "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/libpod/events" - "github.com/containers/podman/v2/libpod/logs" "github.com/opentracing/opentracing-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -267,15 +263,10 @@ func (c *Container) Attach(streams *define.AttachStreams, keys string, resize <- // over the socket; if this is not set, but streamLogs is, only the logs will be // sent. // At least one of streamAttach and streamLogs must be set. -func (c *Container) HTTPAttach(httpCon net.Conn, httpBuf *bufio.ReadWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, streamAttach, streamLogs bool) (deferredErr error) { - isTerminal := false - if c.config.Spec.Process != nil { - isTerminal = c.config.Spec.Process.Terminal - } - // Ensure our contract of writing errors to and closing the HTTP conn is - // honored. +func (c *Container) HTTPAttach(r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, streamAttach, streamLogs bool, hijackDone chan<- bool) error { + // Ensure we don't leak a goroutine if we exit before hijack completes. defer func() { - hijackWriteErrorAndClose(deferredErr, c.ID(), isTerminal, httpCon, httpBuf) + close(hijackDone) }() if !c.batched { @@ -299,74 +290,8 @@ func (c *Container) HTTPAttach(httpCon net.Conn, httpBuf *bufio.ReadWriter, stre logrus.Infof("Performing HTTP Hijack attach to container %s", c.ID()) - logSize := 0 - if streamLogs { - // Get all logs for the container - logChan := make(chan *logs.LogLine) - logOpts := new(logs.LogOptions) - logOpts.Tail = -1 - logOpts.WaitGroup = new(sync.WaitGroup) - errChan := make(chan error) - go func() { - var err error - // In non-terminal mode we need to prepend with the - // stream header. - logrus.Debugf("Writing logs for container %s to HTTP attach", c.ID()) - for logLine := range logChan { - if !isTerminal { - device := logLine.Device - var header []byte - headerLen := uint32(len(logLine.Msg)) - logSize += len(logLine.Msg) - switch strings.ToLower(device) { - case "stdin": - header = makeHTTPAttachHeader(0, headerLen) - case "stdout": - header = makeHTTPAttachHeader(1, headerLen) - case "stderr": - header = makeHTTPAttachHeader(2, headerLen) - default: - logrus.Errorf("Unknown device for log line: %s", device) - header = makeHTTPAttachHeader(1, headerLen) - } - _, err = httpBuf.Write(header) - if err != nil { - break - } - } - _, err = httpBuf.Write([]byte(logLine.Msg)) - if err != nil { - break - } - _, err = httpBuf.Write([]byte("\n")) - if err != nil { - break - } - err = httpBuf.Flush() - if err != nil { - break - } - } - errChan <- err - }() - go func() { - logOpts.WaitGroup.Wait() - close(logChan) - }() - if err := c.ReadLog(context.Background(), logOpts, logChan); err != nil { - return err - } - logrus.Debugf("Done reading logs for container %s, %d bytes", c.ID(), logSize) - if err := <-errChan; err != nil { - return err - } - } - if !streamAttach { - return nil - } - c.newContainerEvent(events.Attach) - return c.ociRuntime.HTTPAttach(c, httpCon, httpBuf, streams, detachKeys, cancel) + return c.ociRuntime.HTTPAttach(c, r, w, streams, detachKeys, cancel, hijackDone, streamAttach, streamLogs) } // AttachResize resizes the container's terminal, which is displayed by Attach -- cgit v1.2.3-54-g00ecf