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 --- pkg/api/handlers/compat/containers_attach.go | 82 ++++++---------------------- pkg/api/handlers/compat/exec.go | 24 +++++--- 2 files changed, 33 insertions(+), 73 deletions(-) (limited to 'pkg/api/handlers/compat') diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go index 2d63ac56d..e20d48d86 100644 --- a/pkg/api/handlers/compat/containers_attach.go +++ b/pkg/api/handlers/compat/containers_attach.go @@ -1,12 +1,7 @@ package compat import ( - "bufio" - "fmt" - "io" - "net" "net/http" - "strings" "github.com/containers/podman/v2/libpod" "github.com/containers/podman/v2/libpod/define" @@ -97,75 +92,30 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { return } - connection, buffer, err := AttachConnection(w, r) - if err != nil { - utils.InternalServerError(w, err) - return - } - logrus.Debugf("Hijack for attach of container %s successful", ctr.ID()) + idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) + hijackChan := make(chan bool, 1) // Perform HTTP attach. // HTTPAttach will handle everything about the connection from here on // (including closing it and writing errors to it). - if err := ctr.HTTPAttach(connection, buffer, streams, detachKeys, nil, query.Stream, query.Logs); err != nil { + if err := ctr.HTTPAttach(r, w, streams, detachKeys, nil, query.Stream, query.Logs, hijackChan); err != nil { + hijackComplete := <-hijackChan + // We can't really do anything about errors anymore. HTTPAttach // should be writing them to the connection. logrus.Errorf("Error attaching to container %s: %v", ctr.ID(), err) - } - logrus.Debugf("Attach for container %s completed successfully", ctr.ID()) -} - -type HijackedConnection struct { - net.Conn // Connection - idleTracker *idletracker.IdleTracker // Connection tracker -} - -func (c HijackedConnection) Close() error { - logrus.Debugf("Hijacked connection closed") - - c.idleTracker.TrackHijackedClosed() - return c.Conn.Close() -} - -func AttachConnection(w http.ResponseWriter, r *http.Request) (net.Conn, *bufio.ReadWriter, error) { - idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) - - // Hijack the connection - hijacker, ok := w.(http.Hijacker) - if !ok { - return nil, nil, errors.Errorf("unable to hijack connection") - } - - connection, buffer, err := hijacker.Hijack() - if err != nil { - return nil, nil, errors.Wrapf(err, "error hijacking connection") - } - trackedConnection := HijackedConnection{ - Conn: connection, - idleTracker: idleTracker, + if hijackComplete { + // We do need to tell the idle tracker that the + // connection has been closed, though. We can guarantee + // that is true after HTTPAttach exits. + idleTracker.TrackHijackedClosed() + } else { + // A hijack was not successfully completed. We need to + // report the error normally. + utils.InternalServerError(w, err) + } } - WriteAttachHeaders(r, trackedConnection) - - return trackedConnection, buffer, nil -} - -func WriteAttachHeaders(r *http.Request, connection io.Writer) { - // AttachHeader is the literal header sent for upgraded/hijacked connections for - // attach, sourced from Docker at: - // https://raw.githubusercontent.com/moby/moby/b95fad8e51bd064be4f4e58a996924f343846c85/api/server/router/container/container_routes.go - // Using literally to ensure compatibility with existing clients. - c := r.Header.Get("Connection") - proto := r.Header.Get("Upgrade") - if len(proto) == 0 || !strings.EqualFold(c, "Upgrade") { - // OK - can't upgrade if not requested or protocol is not specified - fmt.Fprintf(connection, - "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n") - } else { - // Upraded - fmt.Fprintf(connection, - "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: %s\r\n\r\n", - proto) - } + logrus.Debugf("Attach for container %s completed successfully", ctr.ID()) } diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index 7a62a2b58..1db950f85 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/pkg/api/handlers/compat/exec.go @@ -10,6 +10,7 @@ import ( "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/pkg/api/handlers" "github.com/containers/podman/v2/pkg/api/handlers/utils" + "github.com/containers/podman/v2/pkg/api/server/idletracker" "github.com/containers/podman/v2/pkg/specgen/generate" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -173,15 +174,24 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { return } - connection, buffer, err := AttachConnection(w, r) - if err != nil { - utils.InternalServerError(w, err) - return - } - logrus.Debugf("Hijack for attach of container %s exec session %s successful", sessionCtr.ID(), sessionID) + idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) + hijackChan := make(chan bool, 1) + + if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan); err != nil { + hijackComplete := <-hijackChan - if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, connection, buffer, nil, nil, nil); err != nil { logrus.Errorf("Error attaching to container %s exec session %s: %v", sessionCtr.ID(), sessionID, err) + + if hijackComplete { + // We do need to tell the idle tracker that the + // connection has been closed, though. We can guarantee + // that is true after HTTPAttach exits. + idleTracker.TrackHijackedClosed() + } else { + // A hijack was not successfully completed. We need to + // report the error normally. + utils.InternalServerError(w, err) + } } logrus.Debugf("Attach for container %s exec session %s completed successfully", sessionCtr.ID(), sessionID) -- cgit v1.2.3-54-g00ecf