diff --git a/cmd/ecs-agent/runtime/containerd_run.go b/cmd/ecs-agent/runtime/containerd_run.go index f400219f8..756c78ebe 100644 --- a/cmd/ecs-agent/runtime/containerd_run.go +++ b/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) diff --git a/cmd/ecs-agent/runtime/containerd_run_test.go b/cmd/ecs-agent/runtime/containerd_run_test.go index 323b681f6..f7c36badb 100644 --- a/cmd/ecs-agent/runtime/containerd_run_test.go +++ b/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") }