feat: add detachKeys option to container start (runfinch#159)

feat: add detachKeys option to container start

Signed-off-by: Arjun Raja Yogidas <arjunry@amazon.com>

api/handlers/container/container.go

@@ -22,9 +22,9 @@ type Service interface {
 	GetPathToFilesInContainer(ctx context.Context, cid string, path string) (string, func(), error)
 	Remove(ctx context.Context, cid string, force, removeVolumes bool) error
 	Wait(ctx context.Context, cid string, condition string) (code int64, err error)
-	Start(ctx context.Context, cid string) error
+	Start(ctx context.Context, cid string, options ncTypes.ContainerStartOptions) error
 	Stop(ctx context.Context, cid string, timeout *time.Duration) error
-	Restart(ctx context.Context, cid string, timeout time.Duration) error
+	Restart(ctx context.Context, cid string, options ncTypes.ContainerRestartOptions) error
 	Create(ctx context.Context, image string, cmd []string, createOpt ncTypes.ContainerCreateOptions, netOpt ncTypes.NetworkOptions) (string, error)
 	Inspect(ctx context.Context, cid string, size bool) (*types.Container, error)
 	WriteFilesAsTarArchive(filePath string, writer io.Writer, slashDot bool) error

api/handlers/container/container_test.go

@@ -60,7 +60,7 @@ var _ = Describe("Container API", func() {
 		})
 		It("should call container start method", func() {
 			// setup mocks
-			service.EXPECT().Start(gomock.Any(), gomock.Any()).Return(fmt.Errorf("error from start api"))
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("error from start api"))
 			req, _ = http.NewRequest(http.MethodPost, "/containers/123/start", nil)
 			// call the API to check if it returns the error generated from start method
 			router.ServeHTTP(rr, req)

api/handlers/container/restart.go

@@ -5,10 +5,12 @@ package container
 
 import (
 	"net/http"
+	"os"
 	"strconv"
 	"time"
 
 	"github.com/containerd/containerd/v2/pkg/namespaces"
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/gorilla/mux"
 	"github.com/runfinch/finch-daemon/api/response"
 	"github.com/runfinch/finch-daemon/pkg/errdefs"
@@ -22,8 +24,21 @@ func (h *handler) restart(w http.ResponseWriter, r *http.Request) {
 	}
 	timeout := time.Second * time.Duration(t)
 
+	devNull, err := os.OpenFile("/dev/null", os.O_WRONLY, 0600)
+	if err != nil {
+		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg("failed to open /dev/null"))
+		return
+	}
+	defer devNull.Close()
+
 	ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace)
-	err = h.service.Restart(ctx, cid, timeout)
+	globalOpt := ncTypes.GlobalCommandOptions(*h.Config)
+	options := ncTypes.ContainerRestartOptions{
+		GOption: globalOpt,
+		Stdout:  devNull,
+		Timeout: &timeout,
+	}
+	err = h.service.Restart(ctx, cid, options)
 	// map the error into http status code and send response.
 	if err != nil {
 		var code int

api/handlers/container/restart_test.go

@@ -42,7 +42,7 @@ var _ = Describe("Container Restart API ", func() {
 	Context("handler", func() {
 		It("should return 204 as success response", func() {
 			// service mock returns nil to mimic handler started the container successfully.
-			service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return(nil)
+			service.EXPECT().Restart(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 
 			//handler should return success message with 204 status code.
 			h.restart(rr, req)
@@ -51,7 +51,7 @@ var _ = Describe("Container Restart API ", func() {
 
 		It("should return 404 not found response", func() {
 			// service mock returns not found error to mimic user trying to start container that does not exist
-			service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return(
+			service.EXPECT().Restart(gomock.Any(), gomock.Any(), gomock.Any()).Return(
 				errdefs.NewNotFound(fmt.Errorf("container not found")))
 			logger.EXPECT().Debugf("Restart container API responding with error code. Status code %d, Message: %s", 404, "container not found")
 
@@ -63,7 +63,7 @@ var _ = Describe("Container Restart API ", func() {
 		It("should return 500 internal error response", func() {
 			// service mock return error to mimic a user trying to start a container with an id that has
 			// multiple containers with same prefix.
-			service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return(
+			service.EXPECT().Restart(gomock.Any(), gomock.Any(), gomock.Any()).Return(
 				fmt.Errorf("multiple IDs found with provided prefix"))
 			logger.EXPECT().Debugf("Restart container API responding with error code. Status code %d, Message: %s", 500, "multiple IDs found with provided prefix")
 

api/handlers/container/start.go

@@ -4,9 +4,14 @@
 package container
 
 import (
+	"fmt"
 	"net/http"
+	"os"
+	"strings"
+	"unicode"
 
 	"github.com/containerd/containerd/v2/pkg/namespaces"
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
 
@@ -17,7 +22,30 @@ import (
 func (h *handler) start(w http.ResponseWriter, r *http.Request) {
 	cid := mux.Vars(r)["id"]
 	ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace)
-	err := h.service.Start(ctx, cid)
+
+	detachKeys := r.URL.Query().Get("detachKeys")
+
+	if err := validateDetachKeys(detachKeys); err != nil {
+		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg(fmt.Sprintf("Invalid detach keys: %v", err)))
+		return
+	}
+
+	devNull, err := os.OpenFile("/dev/null", os.O_WRONLY, 0600)
+	if err != nil {
+		response.JSON(w, http.StatusBadRequest, response.NewErrorFromMsg("failed to open /dev/null"))
+		return
+	}
+	defer devNull.Close()
+
+	globalOpt := ncTypes.GlobalCommandOptions(*h.Config)
+	options := ncTypes.ContainerStartOptions{
+		GOptions:   globalOpt,
+		Stdout:     devNull,
+		Attach:     false,
+		DetachKeys: detachKeys,
+	}
+
+	err = h.service.Start(ctx, cid, options)
 	// map the error into http status code and send response.
 	if err != nil {
 		var code int
@@ -36,3 +64,40 @@ func (h *handler) start(w http.ResponseWriter, r *http.Request) {
 	// successfully started the container. Send no content status.
 	response.Status(w, http.StatusNoContent)
 }
+
+func validateDetachKeys(detachKeys string) error {
+	if detachKeys == "" {
+		return nil // Empty string is valid (use default)
+	}
+
+	parts := strings.Split(detachKeys, ",")
+	for _, part := range parts {
+		if err := validateSingleKey(part); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func validateSingleKey(key string) error {
+	key = strings.TrimSpace(key)
+	if len(key) == 1 {
+		// Single character key
+		if !unicode.IsLetter(rune(key[0])) {
+			return fmt.Errorf("invalid key: %s - single character must be a letter", key)
+		}
+	} else if strings.HasPrefix(key, "ctrl-") {
+		ctrlKey := strings.TrimPrefix(key, "ctrl-")
+		if len(ctrlKey) != 1 {
+			return fmt.Errorf("invalid ctrl key: %s - must be a single character", ctrlKey)
+		}
+		validCtrlKeys := "abcdefghijklmnopqrstuvwxyz@[\\]^_"
+		if !strings.Contains(validCtrlKeys, strings.ToLower(ctrlKey)) {
+			return fmt.Errorf("invalid ctrl key: %s - must be one of %s", ctrlKey, validCtrlKeys)
+		}
+	} else {
+		return fmt.Errorf("invalid key: %s - must be a single character or ctrl- combination", key)
+	}
+	return nil
+}

api/handlers/container/start_test.go

@@ -4,10 +4,12 @@
 package container
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/config"
 	"github.com/golang/mock/gomock"
 	"github.com/gorilla/mux"
@@ -42,15 +44,15 @@ var _ = Describe("Container Start API ", func() {
 	Context("handler", func() {
 		It("should return 204 as success response", func() {
 			// service mock returns nil to mimic handler started the container successfully.
-			service.EXPECT().Start(gomock.Any(), "123").Return(nil)
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 
 			h.start(rr, req)
 			Expect(rr).Should(HaveHTTPStatus(http.StatusNoContent))
 		})
 
 		It("should return 404 not found response", func() {
 			// service mock returns not found error to mimic user trying to start container that does not exist
-			service.EXPECT().Start(gomock.Any(), "123").Return(
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Return(
 				errdefs.NewNotFound(fmt.Errorf("container not found")))
 			h.start(rr, req)
 			Expect(rr).Should(HaveHTTPStatus(http.StatusNotFound))
@@ -59,7 +61,7 @@ var _ = Describe("Container Start API ", func() {
 		It("should return 500 internal error response", func() {
 			// service mock return error to mimic a user trying to start a container with an id that has
 			// multiple containers with same prefix.
-			service.EXPECT().Start(gomock.Any(), "123").Return(
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Return(
 				fmt.Errorf("multiple IDs found with provided prefix"))
 
 			h.start(rr, req)
@@ -68,11 +70,38 @@ var _ = Describe("Container Start API ", func() {
 		})
 		It("should return 304 not-modified error when container is already running", func() {
 			// service mock returns not found error to mimic user trying to start container that is running
-			service.EXPECT().Start(gomock.Any(), "123").Return(
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Return(
 				errdefs.NewNotModified(fmt.Errorf("container already running")))
 
 			h.start(rr, req)
 			Expect(rr).Should(HaveHTTPStatus(http.StatusNotModified))
 		})
+		It("should pass detachKeys to the service", func() {
+			// Set up the request with detachKeys query parameter
+			req, _ = http.NewRequest(http.MethodPost, "/containers/123/start?detachKeys=ctrl-p,ctrl-q", nil)
+			req = mux.SetURLVars(req, map[string]string{"id": "123"})
+
+			// Expect the service to be called with the correct options
+			service.EXPECT().Start(
+				gomock.Any(),
+				gomock.Any(),
+				gomock.Any(),
+			).DoAndReturn(func(_ context.Context, cid string, options ncTypes.ContainerStartOptions) error {
+				Expect(cid).To(Equal("123"))
+				Expect(options.DetachKeys).To(Equal("ctrl-p,ctrl-q"))
+				return nil
+			})
+
+			h.start(rr, req)
+			Expect(rr).Should(HaveHTTPStatus(http.StatusNoContent))
+		})
+		It("should return 400 Bad Request for invalid ctrl- combination", func() {
+			req, _ = http.NewRequest(http.MethodPost, "/containers/123/start?detachKeys=ctrl-1", nil)
+			req = mux.SetURLVars(req, map[string]string{"id": "123"})
+			service.EXPECT().Start(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+			h.start(rr, req)
+			Expect(rr).Should(HaveHTTPStatus(http.StatusBadRequest))
+			Expect(rr.Body).Should(MatchJSON(`{"message": "Invalid detach keys: invalid ctrl key: 1 - must be one of abcdefghijklmnopqrstuvwxyz@[\\]^_"}`))
+		})
 	})
 })

internal/backend/container.go

@@ -27,7 +27,7 @@ import (
 //go:generate mockgen --destination=../../mocks/mocks_backend/nerdctlcontainersvc.go -package=mocks_backend github.com/runfinch/finch-daemon/internal/backend NerdctlContainerSvc
 type NerdctlContainerSvc interface {
 	RemoveContainer(ctx context.Context, c containerd.Container, force bool, removeAnonVolumes bool) error
-	StartContainer(ctx context.Context, container containerd.Container) error
+	StartContainer(ctx context.Context, cid string, options types.ContainerStartOptions) error
 	StopContainer(ctx context.Context, container containerd.Container, timeout *time.Duration) error
 	CreateContainer(ctx context.Context, args []string, netManager containerutil.NetworkOptionsManager, options types.ContainerCreateOptions) (containerd.Container, func(), error)
 	InspectContainer(ctx context.Context, c containerd.Container, size bool) (*dockercompat.Container, error)
@@ -51,8 +51,8 @@ func (w *NerdctlWrapper) RemoveContainer(ctx context.Context, c containerd.Conta
 }
 
 // StartContainer wrapper function to call nerdctl function to start a container.
-func (w *NerdctlWrapper) StartContainer(ctx context.Context, container containerd.Container) error {
-	return containerutil.Start(ctx, container, false, w.clientWrapper.client, "")
+func (w *NerdctlWrapper) StartContainer(ctx context.Context, cid string, options types.ContainerStartOptions) error {
+	return container.Start(ctx, w.clientWrapper.client, []string{cid}, options)
 }
 
 // StopContainer wrapper function to call nerdctl function to stop a container.

internal/service/container/restart.go

@@ -5,12 +5,12 @@ package container
 
 import (
 	"context"
-	"time"
 
+	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/runfinch/finch-daemon/pkg/errdefs"
 )
 
-func (s *service) Restart(ctx context.Context, cid string, timeout time.Duration) error {
+func (s *service) Restart(ctx context.Context, cid string, options types.ContainerRestartOptions) error {
 	con, err := s.getContainer(ctx, cid)
 	if err != nil {
 		return err
@@ -19,11 +19,19 @@ func (s *service) Restart(ctx context.Context, cid string, timeout time.Duration
 	// restart the container and if error occurs then return error otherwise return nil
 	// swallow IsNotModified error on StopContainer for already stopped container, simply call StartContainer
 	s.logger.Debugf("restarting container: %s", cid)
-	if err := s.nctlContainerSvc.StopContainer(ctx, con, &timeout); err != nil && !errdefs.IsNotModified(err) {
+	if err := s.nctlContainerSvc.StopContainer(ctx, con, options.Timeout); err != nil && !errdefs.IsNotModified(err) {
 		s.logger.Errorf("Failed to stop container: %s. Error: %v", cid, err)
 		return err
 	}
-	if err = s.nctlContainerSvc.StartContainer(ctx, con); err != nil {
+
+	startContainerOptions := types.ContainerStartOptions{
+		Stdout:     options.Stdout,
+		GOptions:   options.GOption,
+		DetachKeys: "",
+		Attach:     false,
+	}
+
+	if err = s.nctlContainerSvc.StartContainer(ctx, cid, startContainerOptions); err != nil {
 		s.logger.Errorf("Failed to start container: %s. Error: %v", cid, err)
 		return err
 	}