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>

1 files changed, 11 insertions, 13 deletions

diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index bfeae0a11..2a852ab81 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -1,9 +1,8 @@
 package libpod
 
 import (
-	"bufio"
 	"io/ioutil"
-	"net"
+	"net/http"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -373,17 +372,12 @@ func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachS
 }
 
 // ExecHTTPStartAndAttach starts and performs an HTTP attach to an exec session.
-func (c *Container) ExecHTTPStartAndAttach(sessionID string, httpCon net.Conn, httpBuf *bufio.ReadWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool) (deferredErr error) {
+func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, hijackDone chan<- bool) error {
 	// TODO: How do we combine streams with the default streams set in the exec session?
 
-	// The flow here is somewhat strange, because we need to determine if
-	// there's a terminal ASAP (for error handling).
-	// Until we know, assume it's true (don't add standard stream headers).
-	// Add a defer to ensure our invariant (HTTP session is closed) is
-	// maintained.
-	isTerminal := true
+	// Ensure that we don't leak a goroutine here
 	defer func() {
-		hijackWriteErrorAndClose(deferredErr, c.ID(), isTerminal, httpCon, httpBuf)
+		close(hijackDone)
 	}()
 
 	if !c.batched {
@@ -399,8 +393,6 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, httpCon net.Conn, h
 	if !ok {
 		return errors.Wrapf(define.ErrNoSuchExecSession, "container %s has no exec session with ID %s", c.ID(), sessionID)
 	}
-	// We can now finally get the real value of isTerminal.
-	isTerminal = session.Config.Terminal
 
 	// Verify that we are in a good state to continue
 	if !c.ensureState(define.ContainerStateRunning) {
@@ -432,7 +424,13 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, httpCon net.Conn, h
 		streams.Stderr = session.Config.AttachStderr
 	}
 
-	pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, httpCon, httpBuf, streams, cancel)
+	holdConnOpen := make(chan bool)
+
+	defer func() {
+		close(holdConnOpen)
+	}()
+
+	pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, r, w, streams, cancel, hijackDone, holdConnOpen)
 	if err != nil {
 		session.State = define.ExecStateStopped
 		session.ExitCode = define.TranslateExecErrorToExitCode(define.ExecErrorCodeGeneric, err)