Merge pull request #1014 from AkihiroSuda/run-i-t-take2

Fix `flag -t needs -i to be specified together` restriction

Author: AkihiroSuda

README.md

@@ -356,7 +356,7 @@ Usage: `nerdctl run [OPTIONS] IMAGE [COMMAND] [ARG...]`
 Basic flags:
 - :whale: :blue_square: `-i, --interactive`: Keep STDIN open even if not attached"
 - :whale: :blue_square: `-t, --tty`: Allocate a pseudo-TTY
-  - :warning: WIP: currently `-t` requires `-i`, and conflicts with `-d`
+  - :warning: WIP: currently `-t` conflicts with `-d`
 - :whale: :blue_square: `-d, --detach`: Run container in background and print container ID
 - :whale: `--restart=(no|always)`: Restart policy to apply when a container exits
   - Default: "no"
@@ -611,7 +611,7 @@ Usage: `nerdctl exec [OPTIONS] CONTAINER COMMAND [ARG...]`
 Flags:
 - :whale: `-i, --interactive`: Keep STDIN open even if not attached
 - :whale: `-t, --tty`: Allocate a pseudo-TTY
-  - :warning: WIP: currently `-t` requires `-i`, and conflicts with `-d`
+  - :warning: WIP: currently `-t` conflicts with `-d`
 - :whale: `-d, --detach`: Detached mode: run command in the background
 - :whale: `-w, --workdir`: Working directory inside the container
 - :whale: `-e, --env`: Set environment variables

cmd/nerdctl/exec.go

@@ -121,9 +121,6 @@ func execActionWithContainer(ctx context.Context, cmd *cobra.Command, args []str
 		if flagD {
 			return errors.New("currently flag -t and -d cannot be specified together (FIXME)")
 		}
-		if !flagI {
-			return errors.New("currently flag -t needs -i to be specified together (FIXME)")
-		}
 	}
 
 	pspec, err := generateExecProcessSpec(ctx, cmd, args, container, client)

cmd/nerdctl/exec_linux_test.go

@@ -48,3 +48,24 @@ func TestExecWithUser(t *testing.T) {
 		base.Cmd(cmd...).AssertOutContains(expected)
 	}
 }
+
+func TestExecTTY(t *testing.T) {
+	t.Parallel()
+	base := testutil.NewBase(t)
+	if testutil.GetTarget() == testutil.Nerdctl {
+		testutil.RequireDaemonVersion(base, ">= 1.6.0-0")
+	}
+
+	testContainer := testutil.Identifier(t)
+	defer base.Cmd("rm", "-f", testContainer).Run()
+	base.Cmd("run", "-d", "--name", testContainer, testutil.CommonImage, "sleep", "infinity").AssertOK()
+
+	const sttyPartialOutput = "speed 38400 baud"
+	// unbuffer(1) emulates tty, which is required by `nerdctl run -t`.
+	// unbuffer(1) can be installed with `apt-get install expect`.
+	unbuffer := []string{"unbuffer"}
+	base.CmdWithHelper(unbuffer, "exec", "-it", testContainer, "stty").AssertOutContains(sttyPartialOutput)
+	base.CmdWithHelper(unbuffer, "exec", "-t", testContainer, "stty").AssertOutContains(sttyPartialOutput)
+	base.Cmd("exec", "-i", testContainer, "stty").AssertFail()
+	base.Cmd("exec", testContainer, "stty").AssertFail()
+}

cmd/nerdctl/exec_test.go

@@ -17,6 +17,7 @@
 package main
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/containerd/nerdctl/pkg/testutil"
@@ -44,3 +45,21 @@ func TestExecWithDoubleDash(t *testing.T) {
 
 	base.Cmd("exec", testContainer, "--", "echo", "success").AssertOutExactly("success\n")
 }
+
+func TestExecStdin(t *testing.T) {
+	t.Parallel()
+	base := testutil.NewBase(t)
+	if testutil.GetTarget() == testutil.Nerdctl {
+		testutil.RequireDaemonVersion(base, ">= 1.6.0-0")
+	}
+
+	testContainer := testutil.Identifier(t)
+	defer base.Cmd("rm", "-f", testContainer).Run()
+	base.Cmd("run", "-d", "--name", testContainer, testutil.CommonImage, "sleep", "1h").AssertOK()
+
+	const testStr = "test-exec-stdin"
+	opts := []func(*testutil.Cmd){
+		testutil.WithStdin(strings.NewReader(testStr)),
+	}
+	base.Cmd("exec", "-i", testContainer, "cat").CmdOption(opts...).AssertOutExactly(testStr)
+}

cmd/nerdctl/run.go

@@ -452,9 +452,6 @@ func createContainer(cmd *cobra.Command, ctx context.Context, client *containerd
 		if flagD {
 			return nil, "", nil, errors.New("currently flag -t and -d cannot be specified together (FIXME)")
 		}
-		if !flagI {
-			return nil, "", nil, errors.New("currently flag -t needs -i to be specified together (FIXME)")
-		}
 		opts = append(opts, oci.WithTTY)
 	}
 

cmd/nerdctl/run_linux_test.go

@@ -219,3 +219,20 @@ ENTRYPOINT ["./main"]
 	assert.Assert(logsCMD.Base.T, result.ExitCode == 0, stdoutContent)
 	assert.Equal(logsCMD.Base.T, strings.Contains(stdoutContent, "signal: 15"), true)
 }
+
+func TestRunTTY(t *testing.T) {
+	t.Parallel()
+	base := testutil.NewBase(t)
+	if testutil.GetTarget() == testutil.Nerdctl {
+		testutil.RequireDaemonVersion(base, ">= 1.6.0-0")
+	}
+
+	const sttyPartialOutput = "speed 38400 baud"
+	// unbuffer(1) emulates tty, which is required by `nerdctl run -t`.
+	// unbuffer(1) can be installed with `apt-get install expect`.
+	unbuffer := []string{"unbuffer"}
+	base.CmdWithHelper(unbuffer, "run", "--rm", "-it", testutil.CommonImage, "stty").AssertOutContains(sttyPartialOutput)
+	base.CmdWithHelper(unbuffer, "run", "--rm", "-t", testutil.CommonImage, "stty").AssertOutContains(sttyPartialOutput)
+	base.Cmd("run", "--rm", "-i", testutil.CommonImage, "stty").AssertFail()
+	base.Cmd("run", "--rm", testutil.CommonImage, "stty").AssertFail()
+}

cmd/nerdctl/run_test.go

@@ -198,3 +198,17 @@ func TestRunEnv(t *testing.T) {
 		return nil
 	})
 }
+
+func TestRunStdin(t *testing.T) {
+	t.Parallel()
+	base := testutil.NewBase(t)
+	if testutil.GetTarget() == testutil.Nerdctl {
+		testutil.RequireDaemonVersion(base, ">= 1.6.0-0")
+	}
+
+	const testStr = "test-run-stdin"
+	opts := []func(*testutil.Cmd){
+		testutil.WithStdin(strings.NewReader(testStr)),
+	}
+	base.Cmd("run", "--rm", "-i", testutil.CommonImage, "cat").CmdOption(opts...).AssertOutExactly(testStr)
+}

pkg/infoutil/infoutil.go

@@ -18,11 +18,13 @@ package infoutil
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"runtime"
 	"strings"
 	"time"
 
+	"github.com/Masterminds/semver/v3"
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/services/introspection"
 	"github.com/containerd/nerdctl/pkg/inspecttypes/dockercompat"
@@ -135,3 +137,15 @@ func ServerVersion(ctx context.Context, client *containerd.Client) (*dockercompa
 	}
 	return v, nil
 }
+
+func ServerSemVer(ctx context.Context, client *containerd.Client) (*semver.Version, error) {
+	v, err := client.Version(ctx)
+	if err != nil {
+		return nil, err
+	}
+	sv, err := semver.NewVersion(v.Version)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse the containerd version %q: %w", v.Version, err)
+	}
+	return sv, nil
+}

pkg/taskutil/taskutil.go

@@ -22,23 +22,35 @@ import (
 	"io"
 	"net/url"
 	"os"
+	"runtime"
+	"sync"
+	"syscall"
 
+	"github.com/Masterminds/semver/v3"
 	"github.com/containerd/console"
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/cio"
+	"github.com/containerd/nerdctl/pkg/infoutil"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/term"
 )
 
 // NewTask is from https://github.com/containerd/containerd/blob/v1.4.3/cmd/ctr/commands/tasks/tasks_unix.go#L70-L108
 func NewTask(ctx context.Context, client *containerd.Client, container containerd.Container, flagI, flagT, flagD bool, con console.Console, logURI string) (containerd.Task, error) {
-	stdinC := &StdinCloser{
-		Stdin: os.Stdin,
-	}
 	var ioCreator cio.Creator
 	if flagT {
 		if con == nil {
 			return nil, errors.New("got nil con with flagT=true")
 		}
-		ioCreator = cio.NewCreator(cio.WithStreams(con, con, nil), cio.WithTerminal)
+		var in io.Reader
+		if flagI {
+			// FIXME: check IsTerminal on Windows too
+			if runtime.GOOS != "windows" && !term.IsTerminal(0) {
+				return nil, errors.New("the input device is not a TTY")
+			}
+			in = con
+		}
+		ioCreator = cio.NewCreator(cio.WithStreams(in, con, nil), cio.WithTerminal)
 	} else if flagD && logURI != "" {
 		// TODO: support logURI for `nerdctl run -it`
 		u, err := url.Parse(logURI)
@@ -49,6 +61,21 @@ func NewTask(ctx context.Context, client *containerd.Client, container container
 	} else {
 		var in io.Reader
 		if flagI {
+			if sv, err := infoutil.ServerSemVer(ctx, client); err != nil {
+				logrus.Warn(err)
+			} else if sv.LessThan(semver.MustParse("1.6.0-0")) {
+				logrus.Warnf("`nerdctl (run|exec) -i` without `-t` expects containerd 1.6 or later, got containerd %v", sv)
+			}
+			stdinC := &StdinCloser{
+				Stdin: os.Stdin,
+				Closer: func() {
+					if t, err := container.Task(ctx, nil); err != nil {
+						logrus.WithError(err).Debugf("failed to get task for StdinCloser")
+					} else {
+						t.CloseIO(ctx, containerd.WithStdinCloser)
+					}
+				},
+			}
 			in = stdinC
 		}
 		ioCreator = cio.NewCreator(cio.WithStreams(in, os.Stdout, os.Stderr))
@@ -57,23 +84,28 @@ func NewTask(ctx context.Context, client *containerd.Client, container container
 	if err != nil {
 		return nil, err
 	}
-	stdinC.Closer = func() {
-		t.CloseIO(ctx, containerd.WithStdinCloser)
-	}
 	return t, nil
 }
 
 // StdinCloser is from https://github.com/containerd/containerd/blob/v1.4.3/cmd/ctr/commands/tasks/exec.go#L181-L194
 type StdinCloser struct {
+	mu     sync.Mutex
 	Stdin  *os.File
 	Closer func()
+	closed bool
 }
 
 func (s *StdinCloser) Read(p []byte) (int, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if s.closed {
+		return 0, syscall.EBADF
+	}
 	n, err := s.Stdin.Read(p)
-	if err == io.EOF {
+	if err != nil {
 		if s.Closer != nil {
 			s.Closer()
+			s.closed = true
 		}
 	}
 	return n, err

pkg/testutil/testutil.go

@@ -27,12 +27,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/Masterminds/semver/v3"
 	"github.com/containerd/nerdctl/pkg/buildkitutil"
 	"github.com/containerd/nerdctl/pkg/inspecttypes/dockercompat"
 	"github.com/containerd/nerdctl/pkg/inspecttypes/native"
 	"github.com/containerd/nerdctl/pkg/platformutil"
 	"github.com/opencontainers/go-digest"
-
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/icmd"
 )
@@ -406,6 +406,22 @@ func RequireExecPlatform(t testing.TB, ss ...string) {
 	}
 }
 
+func RequireDaemonVersion(b *Base, constraint string) {
+	b.T.Helper()
+	c, err := semver.NewConstraint(constraint)
+	if err != nil {
+		b.T.Fatal(err)
+	}
+	info := b.Info()
+	sv, err := semver.NewVersion(info.ServerVersion)
+	if err != nil {
+		b.T.Skip(err)
+	}
+	if !c.Check(sv) {
+		b.T.Skipf("version %v does not satisfy constraints %v", sv, c)
+	}
+}
+
 const Namespace = "nerdctl-test"
 
 func NewBase(t *testing.T) *Base {