Merge pull request #5662 from laurazard/consistent-attach-check

run: correctly handle only STDIN attached
This commit is contained in:
Sebastiaan van Stijn 2024-12-09 18:01:45 +01:00 committed by GitHub
commit e554bfef6c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 60 additions and 17 deletions

View File

@ -161,7 +161,8 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
waitDisplayID chan struct{} waitDisplayID chan struct{}
errCh chan error errCh chan error
) )
if !config.AttachStdout && !config.AttachStderr { attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
if !attach {
// Make this asynchronous to allow the client to write to stdin before having to read the ID // Make this asynchronous to allow the client to write to stdin before having to read the ID
waitDisplayID = make(chan struct{}) waitDisplayID = make(chan struct{})
go func() { go func() {
@ -169,7 +170,6 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
_, _ = fmt.Fprintln(stdout, containerID) _, _ = fmt.Fprintln(stdout, containerID)
}() }()
} }
attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
if attach { if attach {
detachKeys := dockerCli.ConfigFile().DetachKeys detachKeys := dockerCli.ConfigFile().DetachKeys
if runOpts.detachKeys != "" { if runOpts.detachKeys != "" {
@ -215,14 +215,22 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
return toStatusError(err) return toStatusError(err)
} }
if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() { // Detached mode: wait for the id to be displayed and return.
if !attach {
// Detached mode
<-waitDisplayID
return nil
}
if config.Tty && dockerCli.Out().IsTerminal() {
if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil { if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil {
_, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err) _, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
} }
} }
if errCh != nil { select {
if err := <-errCh; err != nil { case err := <-errCh:
if err != nil {
if _, ok := err.(term.EscapeError); ok { if _, ok := err.(term.EscapeError); ok {
// The user entered the detach escape sequence. // The user entered the detach escape sequence.
return nil return nil
@ -231,19 +239,20 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
logrus.Debugf("Error hijack: %s", err) logrus.Debugf("Error hijack: %s", err)
return err return err
} }
status := <-statusChan
if status != 0 {
return cli.StatusError{StatusCode: status}
}
case status := <-statusChan:
// notify hijackedIOStreamer that we're exiting and wait
// so that the terminal can be restored.
cancelFun()
<-errCh
if status != 0 {
return cli.StatusError{StatusCode: status}
}
} }
// Detached mode: wait for the id to be displayed and return.
if !config.AttachStdout && !config.AttachStderr {
// Detached mode
<-waitDisplayID
return nil
}
status := <-statusChan
if status != 0 {
return cli.StatusError{StatusCode: status}
}
return nil return nil
} }

View File

@ -37,7 +37,6 @@ func TestAttachInterrupt(t *testing.T) {
// todo(laurazard): make this test work w/ dind over ssh // todo(laurazard): make this test work w/ dind over ssh
skip.If(t, strings.Contains(os.Getenv("DOCKER_HOST"), "ssh://")) skip.If(t, strings.Contains(os.Getenv("DOCKER_HOST"), "ssh://"))
// if
result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage,
"sh", "-c", "trap \"exit 33\" SIGINT; for i in $(seq 100); do sleep 0.1; done; exit 34") "sh", "-c", "trap \"exit 33\" SIGINT; for i in $(seq 100); do sleep 0.1; done; exit 34")
result.Assert(t, icmd.Success) result.Assert(t, icmd.Success)

View File

@ -3,11 +3,13 @@ package container
import ( import (
"bytes" "bytes"
"fmt" "fmt"
"os/exec"
"strings" "strings"
"syscall" "syscall"
"testing" "testing"
"time" "time"
"github.com/creack/pty"
"github.com/docker/cli/e2e/internal/fixtures" "github.com/docker/cli/e2e/internal/fixtures"
"github.com/docker/cli/internal/test/environment" "github.com/docker/cli/internal/test/environment"
"github.com/docker/docker/api/types/versions" "github.com/docker/docker/api/types/versions"
@ -38,6 +40,39 @@ func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) {
golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden") golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden")
} }
func TestRunAttach(t *testing.T) {
skip.If(t, environment.RemoteDaemon())
t.Parallel()
streams := []string{"stdin", "stdout", "stderr"}
for _, stream := range streams {
t.Run(stream, func(t *testing.T) {
t.Parallel()
c := exec.Command("docker", "run", "-a", stream, "--rm", "alpine",
"sh", "-c", "sleep 1 && exit 7")
d := bytes.Buffer{}
c.Stdout = &d
c.Stderr = &d
_, err := pty.Start(c)
assert.NilError(t, err)
done := make(chan error)
go func() {
done <- c.Wait()
}()
select {
case <-time.After(20 * time.Second):
t.Fatal("docker run took too long, likely hang", d.String())
case <-done:
}
assert.Equal(t, c.ProcessState.ExitCode(), 7)
assert.Check(t, is.Contains(d.String(), "exit status 7"))
})
}
}
// Regression test for https://github.com/docker/cli/issues/5053 // Regression test for https://github.com/docker/cli/issues/5053
func TestRunInvalidEntrypointWithAutoremove(t *testing.T) { func TestRunInvalidEntrypointWithAutoremove(t *testing.T) {
environment.SkipIfDaemonNotLinux(t) environment.SkipIfDaemonNotLinux(t)