From c63599d361396c8bd02cc4bfccff0e7d97653146 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 26 Jan 2021 15:38:05 +0100 Subject: [PATCH] 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 --- libpod/oci_conmon_exec_linux.go | 48 ++++++++++++++++++++++++++------- libpod/util.go | 17 +++++++----- test/system/075-exec.bats | 2 -- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go index dc5dd03df40a..faf86ea5b53c 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 bf9bf2542789..391208fb9020 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) diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats index c028e16c9802..badf44c49014 100644 --- a/test/system/075-exec.bats +++ b/test/system/075-exec.bats @@ -6,8 +6,6 @@ load helpers @test "podman exec - basic test" { - skip_if_remote "FIXME: pending #7241" - rand_filename=$(random_string 20) rand_content=$(random_string 50)