remote exec: write conmon error on hijacked connection

Make sure to write error from conmon on the hijacked http connection.
This fixes issues where errors were not reported on the client side,
for instance, when specified command was not found on the container.

To future generations: I am sorry.  The code is complex, and there are
many interdependencies among the concurrent goroutines.  I added more
complexity on top but I don't have a good idea of how to reduce
complexity in the available time.

Fixes: #8281
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

2 files changed, 49 insertions, 16 deletions

diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index dc5dd03df..faf86ea5b 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -126,20 +126,25 @@ func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, o
 	}()
 
 	attachChan := make(chan error)
+	conmonPipeDataChan := make(chan conmonPipeData)
 	go func() {
 		// attachToExec is responsible for closing pipes
-		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen)
+		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog)
 		close(attachChan)
 	}()
 
-	// Wait for conmon to succeed, when return.
-	if err := execCmd.Wait(); err != nil {
-		return -1, nil, errors.Wrapf(err, "cannot run conmon")
-	}
+	// NOTE: the channel is needed to communicate conmon's data.  In case
+	// of an error, the error will be written on the hijacked http
+	// connection such that remote clients will receive the error.
+	pipeData := <-conmonPipeDataChan
 
-	pid, err := readConmonPipeData(pipes.syncPipe, ociLog)
+	return pipeData.pid, attachChan, pipeData.err
+}
 
-	return pid, attachChan, err
+// conmonPipeData contains the data when reading from conmon's pipe.
+type conmonPipeData struct {
+	pid int
+	err error
 }
 
 // ExecContainerDetached executes a command in a running container, but does
@@ -488,9 +493,16 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 }
 
 // Attach to a container over HTTP
-func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (deferredErr error) {
+func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string) (deferredErr error) {
+	// NOTE: As you may notice, the attach code is quite complex.
+	// Many things happen concurrently and yet are interdependent.
+	// If you ever change this function, make sure to write to the
+	// conmonPipeDataChan in case of an error.
+
 	if pipes == nil || pipes.startPipe == nil || pipes.attachPipe == nil {
-		return errors.Wrapf(define.ErrInvalidArg, "must provide a start and attach pipe to finish an exec attach")
+		err := errors.Wrapf(define.ErrInvalidArg, "must provide a start and attach pipe to finish an exec attach")
+		conmonPipeDataChan <- conmonPipeData{-1, err}
+		return err
 	}
 
 	defer func() {
@@ -509,17 +521,20 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 	// set up the socket path, such that it is the correct length and location for exec
 	sockPath, err := c.execAttachSocketPath(sessionID)
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return err
 	}
 
 	// 2: read from attachFd that the parent process has set up the console socket
 	if _, err := readConmonPipeData(pipes.attachPipe, ""); err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return err
 	}
 
 	// 2: then attach
 	conn, err := openUnixSocket(sockPath)
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "failed to connect to container's attach socket: %v", sockPath)
 	}
 	defer func() {
@@ -540,11 +555,13 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 	// Perform hijack
 	hijacker, ok := w.(http.Hijacker)
 	if !ok {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Errorf("unable to hijack connection")
 	}
 
 	httpCon, httpBuf, err := hijacker.Hijack()
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "error hijacking connection")
 	}
 
@@ -555,10 +572,23 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 
 	// Force a flush after the header is written.
 	if err := httpBuf.Flush(); err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "error flushing HTTP hijack header")
 	}
 
 	go func() {
+		// Wait for conmon to succeed, when return.
+		if err := execCmd.Wait(); err != nil {
+			conmonPipeDataChan <- conmonPipeData{-1, err}
+		} else {
+			pid, err := readConmonPipeData(pipes.syncPipe, ociLog)
+			if err != nil {
+				hijackWriteError(err, c.ID(), isTerminal, httpBuf)
+				conmonPipeDataChan <- conmonPipeData{pid, err}
+			} else {
+				conmonPipeDataChan <- conmonPipeData{pid, err}
+			}
+		}
 		// We need to hold the connection open until the complete exec
 		// function has finished. This channel will be closed in a defer
 		// in that function, so we can wait for it here.
diff --git a/libpod/util.go b/libpod/util.go
index bf9bf2542..391208fb9 100644
--- a/libpod/util.go
+++ b/libpod/util.go
@@ -235,20 +235,16 @@ func checkDependencyContainer(depCtr, ctr *Container) error {
 	return nil
 }
 
-// hijackWriteErrorAndClose writes an error to a hijacked HTTP session and
-// closes it. Intended to HTTPAttach function.
-// If error is nil, it will not be written; we'll only close the connection.
-func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon io.Closer, httpBuf *bufio.ReadWriter) {
+// hijackWriteError writes an error to a hijacked HTTP session.
+func hijackWriteError(toWrite error, cid string, terminal bool, httpBuf *bufio.ReadWriter) {
 	if toWrite != nil {
-		errString := []byte(fmt.Sprintf("%v\n", toWrite))
+		errString := []byte(fmt.Sprintf("Error: %v\n", toWrite))
 		if !terminal {
 			// We need a header.
 			header := makeHTTPAttachHeader(2, uint32(len(errString)))
 			if _, err := httpBuf.Write(header); err != nil {
 				logrus.Errorf("Error writing header for container %s attach connection error: %v", cid, err)
 			}
-			// TODO: May want to return immediately here to avoid
-			// writing garbage to the socket?
 		}
 		if _, err := httpBuf.Write(errString); err != nil {
 			logrus.Errorf("Error writing error to container %s HTTP attach connection: %v", cid, err)
@@ -257,6 +253,13 @@ func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon
 			logrus.Errorf("Error flushing HTTP buffer for container %s HTTP attach connection: %v", cid, err)
 		}
 	}
+}
+
+// hijackWriteErrorAndClose writes an error to a hijacked HTTP session and
+// closes it. Intended to HTTPAttach function.
+// If error is nil, it will not be written; we'll only close the connection.
+func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon io.Closer, httpBuf *bufio.ReadWriter) {
+	hijackWriteError(toWrite, cid, terminal, httpBuf)
 
 	if err := httpCon.Close(); err != nil {
 		logrus.Errorf("Error closing container %s HTTP attach connection: %v", cid, err)