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 <matthew.heon@pm.me>

4 files changed, 37 insertions, 73 deletions

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)
diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go
index c035b6391..3bd85fbae 100644
--- a/pkg/bindings/containers/attach.go
+++ b/pkg/bindings/containers/attach.go
@@ -46,6 +46,8 @@ func Attach(ctx context.Context, nameOrID string, detachKeys *string, logs, stre
 		stderr = (io.Writer)(nil)
 	}
 
+	logrus.Infof("Going to attach to container %q", nameOrID)
+
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
 		return err
diff --git a/pkg/bindings/containers/containers.go b/pkg/bindings/containers/containers.go
index c1eb23233..981912665 100644
--- a/pkg/bindings/containers/containers.go
+++ b/pkg/bindings/containers/containers.go
@@ -13,6 +13,7 @@ import (
 	"github.com/containers/podman/v2/pkg/bindings"
 	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 var (
@@ -180,6 +181,7 @@ func Restart(ctx context.Context, nameOrID string, timeout *int) error {
 // or a partial/full ID. The optional parameter for detach keys are to override the default
 // detach key sequence.
 func Start(ctx context.Context, nameOrID string, detachKeys *string) error {
+	logrus.Infof("Going to start container %q", nameOrID)
 	conn, err := bindings.GetClient(ctx)
 	if err != nil {
 		return err