fix(ecs-agent): allocate a console for a pseudoTerminal container (#1054)

oci.WithTTY put Terminal in the OCI spec while the task was created with a cio
creator that asked for no terminal, so the shim made no console socket and runc
refused to start the container at all. The terminal flag now reaches the FIFO
set, and a terminal carries no separate stderr stream.

cmd/ecs-agent/runtime/containerd_run.go

@@ -98,22 +98,40 @@ func (h heldStdin) Read([]byte) (int, error) {
 	return 0, io.EOF
 }
 
-// containerIOCreator returns cio.NullIO for a non-interactive container (stdin
-// reads EOF immediately) or a creator holding stdin open until releaseStdin.
-// containerd copies stdin on a goroutine that exits only once the read returns,
-// so a reader that never returned would park it for the life of the agent.
-func (p *containerdPuller) containerIOCreator(containerID string, interactive bool) cio.Creator {
-	if !interactive {
+// ioStreamOpts describes the task IO for a container. A terminal container needs
+// a terminal FIFO set: oci.WithTTY alone puts Terminal in the OCI spec while the
+// shim creates no console socket, and runc refuses that pair outright. A
+// terminal carries no separate stderr, so that stream stays nil and the shim
+// merges it into the console.
+func ioStreamOpts(stdin io.Reader, terminal bool) []cio.Opt {
+	if terminal {
+		return []cio.Opt{cio.WithStreams(stdin, io.Discard, nil), cio.WithTerminal}
+	}
+	return []cio.Opt{cio.WithStreams(stdin, io.Discard, io.Discard)}
+}
+
+// containerIOCreator returns cio.NullIO for a container needing neither stdin
+// nor a terminal, else a creator carrying whichever it asked for. An interactive
+// container's stdin is held open until releaseStdin; containerd copies stdin on
+// a goroutine that exits only once the read returns, so a reader that never
+// returned would park it for the life of the agent. A terminal container that is
+// not interactive gets no stdin stream at all, which is what a nil reader means.
+func (p *containerdPuller) containerIOCreator(containerID string, interactive, terminal bool) cio.Creator {
+	if !interactive && !terminal {
 		return cio.NullIO
 	}
-	done := make(chan struct{})
-	p.mu.Lock()
-	if p.stdinDone == nil {
-		p.stdinDone = make(map[string]chan struct{})
+	var stdin io.Reader
+	if interactive {
+		done := make(chan struct{})
+		p.mu.Lock()
+		if p.stdinDone == nil {
+			p.stdinDone = make(map[string]chan struct{})
+		}
+		p.stdinDone[containerID] = done
+		p.mu.Unlock()
+		stdin = heldStdin{done: done}
 	}
-	p.stdinDone[containerID] = done
-	p.mu.Unlock()
-	return cio.NewCreator(cio.WithStreams(heldStdin{done: done}, io.Discard, io.Discard))
+	return cio.NewCreator(ioStreamOpts(stdin, terminal)...)
 }
 
 // releaseStdin closes a held stdin so containerd's copier goroutine exits. Safe
@@ -177,7 +195,8 @@ func (p *containerdPuller) Run(ctx context.Context, id string, spec RunSpec) (st
 	if spec.Privileged != nil && *spec.Privileged {
 		specOpts = append(specOpts, oci.WithPrivileged)
 	}
-	if spec.PseudoTerminal != nil && *spec.PseudoTerminal {
+	terminal := spec.PseudoTerminal != nil && *spec.PseudoTerminal
+	if terminal {
 		specOpts = append(specOpts, oci.WithTTY)
 	}
 	specOpts = append(specOpts, withSysctls(spec.SystemControls))
@@ -198,7 +217,7 @@ func (p *containerdPuller) Run(ctx context.Context, id string, spec RunSpec) (st
 	}
 
 	interactive := spec.Interactive != nil && *spec.Interactive
-	task, err := container.NewTask(ctx, p.containerIOCreator(id, interactive))
+	task, err := container.NewTask(ctx, p.containerIOCreator(id, interactive, terminal))
 	if err != nil {
 		p.releaseStdin(id)
 		_ = container.Delete(ctx, containerd.WithSnapshotCleanup)

cmd/ecs-agent/runtime/containerd_run_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/containerd/containerd/v2/pkg/cio"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
@@ -103,7 +104,7 @@ func TestWithSysctls(t *testing.T) {
 // for the life of the agent.
 func TestHeldStdin_ReadsEOFOnceReleased(t *testing.T) {
 	p := &containerdPuller{}
-	p.containerIOCreator("c1", true)
+	p.containerIOCreator("c1", true, false)
 
 	p.mu.Lock()
 	done := p.stdinDone["c1"]
@@ -139,7 +140,63 @@ func TestHeldStdin_ReadsEOFOnceReleased(t *testing.T) {
 // A non-interactive container uses cio.NullIO and registers nothing to release.
 func TestContainerIOCreator_NonInteractiveRegistersNothing(t *testing.T) {
 	p := &containerdPuller{}
-	p.containerIOCreator("c1", false)
+	p.containerIOCreator("c1", false, false)
+
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	if len(p.stdinDone) != 0 {
+		t.Fatalf("want no registered stdin channels, got %d", len(p.stdinDone))
+	}
+}
+
+// applyIOOpts resolves the cio options onto a Streams the way cio.NewCreator
+// does, which is the only way to observe what the creator was built with.
+func applyIOOpts(opts []cio.Opt) cio.Streams {
+	var s cio.Streams
+	for _, o := range opts {
+		o(&s)
+	}
+	return s
+}
+
+// A terminal container needs the terminal flag set on its FIFO set, else the
+// shim creates no console socket and runc refuses to start the container at all.
+// The terminal carries no separate stderr stream.
+func TestIOStreamOpts_TerminalRequestsAConsole(t *testing.T) {
+	s := applyIOOpts(ioStreamOpts(nil, true))
+	if !s.Terminal {
+		t.Error("terminal container: want Terminal set")
+	}
+	if s.Stderr != nil {
+		t.Errorf("terminal container: want no stderr stream, got %#v", s.Stderr)
+	}
+	if s.Stdout == nil {
+		t.Error("terminal container: want a stdout stream")
+	}
+}
+
+// A container with stdin but no terminal keeps both output streams and asks for
+// no console.
+func TestIOStreamOpts_NonTerminalKeepsStderr(t *testing.T) {
+	s := applyIOOpts(ioStreamOpts(heldStdin{}, false))
+	if s.Terminal {
+		t.Error("non-terminal container: want Terminal unset")
+	}
+	if s.Stderr == nil {
+		t.Error("non-terminal container: want a stderr stream")
+	}
+	if s.Stdin == nil {
+		t.Error("non-terminal interactive container: want a stdin stream")
+	}
+}
+
+// A terminal container that is not interactive still gets a creator rather than
+// cio.NullIO, and registers no stdin to release.
+func TestContainerIOCreator_TerminalWithoutInteractive(t *testing.T) {
+	p := &containerdPuller{}
+	if c := p.containerIOCreator("c1", false, true); c == nil {
+		t.Fatal("terminal container: want a creator")
+	}
 
 	p.mu.Lock()
 	defer p.mu.Unlock()
@@ -154,7 +211,7 @@ func TestReleaseStdin_UnknownAndRepeatedAreSafe(t *testing.T) {
 	p := &containerdPuller{}
 	p.releaseStdin("never-registered")
 
-	p.containerIOCreator("c1", true)
+	p.containerIOCreator("c1", true, false)
 	p.releaseStdin("c1")
 	p.releaseStdin("c1")
 }