Enable detached exec for remote

The biggest obstacle here was cleanup - we needed a way to remove
detached exec sessions after they exited, but there's no way to
tell if an exec session will be attached or detached when it's
created, and that's when we must add the exit command that would
do the removal. The solution was adding a delay to the exit
command (5 minutes), which gives sufficient time for attached
exec sessions to retrieve the exit code of the session after it
exits, but still guarantees that they will be removed, even for
detached sessions. This requires Conmon 2.0.17, which has the new
`--exit-delay` flag.

As part of the exit command rework, we can drop the hack we were
using to clean up exec sessions (remove them as part of inspect).
This is a lot cleaner, and I'm a lot happier about it.

Otherwise, this is just plumbing - we need a bindings call for
detached exec, and that needed to be added to the tunnel mode
backend for entities.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

6 files changed, 100 insertions, 30 deletions

diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
index 6865a3319..8f7016903 100644
--- a/pkg/api/handlers/compat/exec.go
+++ b/pkg/api/handlers/compat/exec.go
@@ -10,6 +10,7 @@ import (
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/api/handlers"
 	"github.com/containers/libpod/pkg/api/handlers/utils"
+	"github.com/containers/libpod/pkg/specgen/generate"
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -54,6 +55,24 @@ func ExecCreateHandler(w http.ResponseWriter, r *http.Request) {
 	libpodConfig.Privileged = input.Privileged
 	libpodConfig.User = input.User
 
+	// Make our exit command
+	storageConfig := runtime.StorageConfig()
+	runtimeConfig, err := runtime.GetConfig()
+	if err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+	exitCommandArgs, err := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, false, true, true)
+	if err != nil {
+		utils.InternalServerError(w, err)
+		return
+	}
+	libpodConfig.ExitCommand = exitCommandArgs
+
+	// Run the exit command after 5 minutes, to mimic Docker's exec cleanup
+	// behavior.
+	libpodConfig.ExitCommandDelay = 5 * 60
+
 	sessID, err := ctr.ExecCreate(libpodConfig)
 	if err != nil {
 		if errors.Cause(err) == define.ErrCtrStateInvalid {
@@ -104,15 +123,6 @@ func ExecInspectHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	utils.WriteResponse(w, http.StatusOK, inspectOut)
-
-	// Only for the Compat API: we want to remove sessions that were
-	// stopped. This is very hacky, but should suffice for now.
-	if !utils.IsLibpodRequest(r) && inspectOut.CanRemove {
-		logrus.Infof("Pruning stale exec session %s from container %s", sessionID, sessionCtr.ID())
-		if err := sessionCtr.ExecRemove(sessionID, false); err != nil && errors.Cause(err) != define.ErrNoSuchExecSession {
-			logrus.Errorf("Error removing stale exec session %s from container %s: %v", sessionID, sessionCtr.ID(), err)
-		}
-	}
 }
 
 // ExecStartHandler runs a given exec session.
@@ -121,7 +131,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 
 	sessionID := mux.Vars(r)["id"]
 
-	// TODO: We should read/support Tty and Detach from here.
+	// TODO: We should read/support Tty from here.
 	bodyParams := new(handlers.ExecStartConfig)
 
 	if err := json.NewDecoder(r.Body).Decode(&bodyParams); err != nil {
@@ -129,11 +139,6 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 			errors.Wrapf(err, "failed to decode parameters for %s", r.URL.String()))
 		return
 	}
-	if bodyParams.Detach {
-		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
-			errors.Errorf("Detached exec is not yet supported"))
-		return
-	}
 	// TODO: Verify TTY setting against what inspect session was made with
 
 	sessionCtr, err := runtime.GetExecSessionContainer(sessionID)
@@ -154,6 +159,19 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if bodyParams.Detach {
+		// If we are detaching, we do NOT want to hijack.
+		// Instead, we perform a detached start, and return 200 if
+		// successful.
+		if err := sessionCtr.ExecStart(sessionID); err != nil {
+			utils.InternalServerError(w, err)
+			return
+		}
+		// This is a 200 despite having no content
+		utils.WriteResponse(w, http.StatusOK, "")
+		return
+	}
+
 	// Hijack the connection
 	hijacker, ok := w.(http.Hijacker)
 	if !ok {
diff --git a/pkg/api/server/register_exec.go b/pkg/api/server/register_exec.go
index 17181d286..af9a83496 100644
--- a/pkg/api/server/register_exec.go
+++ b/pkg/api/server/register_exec.go
@@ -13,7 +13,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec (compat)
 	// summary: Create an exec instance
-	// description: Run a command inside a running container.
+	// description: Create an exec session to run a command inside a running container. Exec sessions will be automatically removed 5 minutes after they exit.
 	// parameters:
 	//  - in: path
 	//    name: name
@@ -153,7 +153,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec (compat)
 	// summary: Inspect an exec instance
-	// description: Return low-level information about an exec instance. Stale (stopped) exec sessions will be auto-removed after inspect runs.
+	// description: Return low-level information about an exec instance.
 	// parameters:
 	//  - in: path
 	//    name: id
@@ -182,7 +182,7 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
 	// tags:
 	//   - exec
 	// summary: Create an exec instance
-	// description: Run a command inside a running container.
+	// description: Create an exec session to run a command inside a running container. Exec sessions will be automatically removed 5 minutes after they exit.
 	// parameters:
 	//  - in: path
 	//    name: name
diff --git a/pkg/bindings/containers/exec.go b/pkg/bindings/containers/exec.go
index 2aeeae1f8..73cfb5079 100644
--- a/pkg/bindings/containers/exec.go
+++ b/pkg/bindings/containers/exec.go
@@ -1,6 +1,7 @@
 package containers
 
 import (
+	"bytes"
 	"context"
 	"net/http"
 	"strings"
@@ -69,3 +70,31 @@ func ExecInspect(ctx context.Context, sessionID string) (*define.InspectExecSess
 
 	return respStruct, nil
 }
+
+// ExecStart starts (but does not attach to) a given exec session.
+func ExecStart(ctx context.Context, sessionID string) error {
+	conn, err := bindings.GetClient(ctx)
+	if err != nil {
+		return err
+	}
+
+	logrus.Debugf("Starting exec session ID %q", sessionID)
+
+	// We force Detach to true
+	body := struct {
+		Detach bool `json:"Detach"`
+	}{
+		Detach: true,
+	}
+	bodyJSON, err := json.Marshal(body)
+	if err != nil {
+		return err
+	}
+
+	resp, err := conn.DoRequest(bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/start", nil, nil, sessionID)
+	if err != nil {
+		return err
+	}
+
+	return resp.Process(nil)
+}
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
index e982c7c11..706fcec47 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -613,12 +613,11 @@ func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrId s
 	if err != nil {
 		return "", errors.Wrapf(err, "error retrieving Libpod configuration to build exec exit command")
 	}
-	podmanPath, err := os.Executable()
+	// TODO: Add some ability to toggle syslog
+	exitCommandArgs, err := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, false, true, true)
 	if err != nil {
-		return "", errors.Wrapf(err, "error retrieving executable to build exec exit command")
+		return "", errors.Wrapf(err, "error constructing exit command for exec session")
 	}
-	// TODO: Add some ability to toggle syslog
-	exitCommandArgs := generate.CreateExitCommandArgs(storageConfig, runtimeConfig, podmanPath, false, true, true)
 	execConfig.ExitCommand = exitCommandArgs
 
 	// Create and start the exec session
diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index e1c859e7c..af9d94293 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -376,7 +376,7 @@ func (ic *ContainerEngine) ContainerAttach(ctx context.Context, nameOrId string,
 	return containers.Attach(ic.ClientCxt, nameOrId, &options.DetachKeys, nil, bindings.PTrue, options.Stdin, options.Stdout, options.Stderr, nil)
 }
 
-func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, options entities.ExecOptions, streams define.AttachStreams) (int, error) {
+func makeExecConfig(options entities.ExecOptions) *handlers.ExecCreateConfig {
 	env := []string{}
 	for k, v := range options.Envs {
 		env = append(env, fmt.Sprintf("%s=%s", k, v))
@@ -395,6 +395,12 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, o
 	createConfig.WorkingDir = options.WorkDir
 	createConfig.Cmd = options.Cmd
 
+	return createConfig
+}
+
+func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, options entities.ExecOptions, streams define.AttachStreams) (int, error) {
+	createConfig := makeExecConfig(options)
+
 	sessionID, err := containers.ExecCreate(ic.ClientCxt, nameOrId, createConfig)
 	if err != nil {
 		return 125, err
@@ -412,8 +418,19 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrId string, o
 	return inspectOut.ExitCode, nil
 }
 
-func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (string, error) {
-	return "", errors.New("not implemented")
+func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrId string, options entities.ExecOptions) (string, error) {
+	createConfig := makeExecConfig(options)
+
+	sessionID, err := containers.ExecCreate(ic.ClientCxt, nameOrId, createConfig)
+	if err != nil {
+		return "", err
+	}
+
+	if err := containers.ExecStart(ic.ClientCxt, sessionID); err != nil {
+		return "", err
+	}
+
+	return sessionID, nil
 }
 
 func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, input, output, errput *os.File) error { //nolint
diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
index ffd7fd4dd..7ddfed339 100644
--- a/pkg/specgen/generate/container_create.go
+++ b/pkg/specgen/generate/container_create.go
@@ -107,12 +107,12 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 	}
 	options = append(options, opts...)
 
-	podmanPath, err := os.Executable()
+	// TODO: Enable syslog support - we'll need to put this in SpecGen.
+	exitCommandArgs, err := CreateExitCommandArgs(rt.StorageConfig(), rtc, false, s.Remove, false)
 	if err != nil {
 		return nil, err
 	}
-	// TODO: Enable syslog support - we'll need to put this in SpecGen.
-	options = append(options, libpod.WithExitCommand(CreateExitCommandArgs(rt.StorageConfig(), rtc, podmanPath, false, s.Remove, false)))
+	options = append(options, libpod.WithExitCommand(exitCommandArgs))
 
 	runtimeSpec, err := SpecGenToOCI(ctx, s, rt, rtc, newImage, finalMounts)
 	if err != nil {
@@ -229,13 +229,18 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen.
 	return options, nil
 }
 
-func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Config, podmanPath string, syslog, rm bool, exec bool) []string {
+func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Config, syslog, rm, exec bool) ([]string, error) {
 	// We need a cleanup process for containers in the current model.
 	// But we can't assume that the caller is Podman - it could be another
 	// user of the API.
 	// As such, provide a way to specify a path to Podman, so we can
 	// still invoke a cleanup process.
 
+	podmanPath, err := os.Executable()
+	if err != nil {
+		return nil, err
+	}
+
 	command := []string{podmanPath,
 		"--root", storageConfig.GraphRoot,
 		"--runroot", storageConfig.RunRoot,
@@ -265,9 +270,11 @@ func CreateExitCommandArgs(storageConfig storage.StoreOptions, config *config.Co
 		command = append(command, "--rm")
 	}
 
+	// This has to be absolutely last, to ensure that the exec session ID
+	// will be added after it by Libpod.
 	if exec {
 		command = append(command, "--exec")
 	}
 
-	return command
+	return command, nil
 }