From 06b8da120e0d687ae34d49829bc19fecc3993f2e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 6 Nov 2023 15:49:53 +0100 Subject: [PATCH 1/2] WIP: attach: pass container as argument Trying to make "runAttach" not depend on attachOptions, and to see if we can make it accept container.AttachOptions instead relates to https://github.com/docker/cli/pull/4637 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/attach.go | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index bf8341af5077..9e38d91082d3 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -73,26 +73,35 @@ func NewAttachCommand(dockerCLI command.Cli) *cobra.Command { func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, opts *AttachOptions) error { apiClient := dockerCLI.Client() - // request channel to wait for client - resultC, errC := apiClient.ContainerWait(ctx, containerID, "") + attachStdIn := true + if opts.NoStdin { + // TODO(thaJeztah): this is the tricky one: can we use container.AttachOptions for this one without it being ambiguous? + attachStdIn = false + } c, err := inspectContainerAndCheckState(ctx, apiClient, containerID) if err != nil { return err } - if err := dockerCLI.In().CheckTty(!opts.NoStdin, c.Config.Tty); err != nil { - return err + if attachStdIn { + if err := dockerCLI.In().CheckTty(attachStdIn, c.Config.Tty); err != nil { + return err + } + if !c.Config.OpenStdin { + // TODO(thaJeztah): should this produce an error? + attachStdIn = false + } } - detachKeys := dockerCLI.ConfigFile().DetachKeys - if opts.DetachKeys != "" { - detachKeys = opts.DetachKeys + detachKeys := opts.DetachKeys + if opts.DetachKeys == "" { + detachKeys = dockerCLI.ConfigFile().DetachKeys } options := container.AttachOptions{ Stream: true, - Stdin: !opts.NoStdin && c.Config.OpenStdin, + Stdin: attachStdIn, Stdout: true, Stderr: true, DetachKeys: detachKeys, @@ -146,6 +155,8 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o return err } + // request channel to wait for client + resultC, errC := apiClient.ContainerWait(ctx, containerID, "") return getExitStatus(errC, resultC) } From 3498ec655044d0f8972478f2b4e190c72ce75f0c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 7 Nov 2023 12:47:12 +0100 Subject: [PATCH 2/2] WIP: refactor RunAttach to receive container.AttachOptions Signed-off-by: Sebastiaan van Stijn --- cli/command/container/attach.go | 58 ++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index 9e38d91082d3..8387146599e9 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -51,8 +51,21 @@ func NewAttachCommand(dockerCLI command.Cli) *cobra.Command { Short: "Attach local standard input, output, and error streams to a running container", Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + attachStdIn := true + if opts.NoStdin { + // TODO(thaJeztah): this is the tricky one: can we use container.AttachOptions for this one without it being ambiguous? + attachStdIn = false + } + containerID := args[0] - return RunAttach(cmd.Context(), dockerCLI, containerID, &opts) + disableSignalProxy := !opts.Proxy + return RunAttach(cmd.Context(), dockerCLI, containerID, disableSignalProxy, container.AttachOptions{ + Stream: true, + Stdin: attachStdIn, + Stdout: true, + Stderr: true, + DetachKeys: opts.DetachKeys, + }) }, Annotations: map[string]string{ "aliases": "docker container attach, docker attach", @@ -64,61 +77,54 @@ func NewAttachCommand(dockerCLI command.Cli) *cobra.Command { flags := cmd.Flags() flags.BoolVar(&opts.NoStdin, "no-stdin", false, "Do not attach STDIN") + // Is this feature still used? + // It was added in https://github.com/moby/moby/commit/4918769b1ac2d38f23087b766140e6a7f8979310 to allow forwarding signals to containers + // Changed in https://github.com/moby/moby/commit/333bc23f21e8423d3085632db99a6d1df227c5f1 + // And changed to be enabled by default in https://github.com/moby/moby/commit/e0b59ab52b87b8fc15dd5534c3231fdd74843f9f (unless a TTY is attached) + // related: https://github.com/moby/moby/commit/e0b59ab52b87b8fc15dd5534c3231fdd74843f9f#commitcomment-25897874 + // related: https://github.com/moby/moby/issues/9098 + // related: https://github.com/docker/cli/pull/1841 flags.BoolVar(&opts.Proxy, "sig-proxy", true, "Proxy all received signals to the process") flags.StringVar(&opts.DetachKeys, "detach-keys", "", "Override the key sequence for detaching a container") return cmd } -// RunAttach executes an `attach` command -func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, opts *AttachOptions) error { +// RunAttach attaches to the given container. +func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, disableSignalProxy bool, opts container.AttachOptions) error { apiClient := dockerCLI.Client() - attachStdIn := true - if opts.NoStdin { - // TODO(thaJeztah): this is the tricky one: can we use container.AttachOptions for this one without it being ambiguous? - attachStdIn = false - } - c, err := inspectContainerAndCheckState(ctx, apiClient, containerID) if err != nil { return err } - if attachStdIn { - if err := dockerCLI.In().CheckTty(attachStdIn, c.Config.Tty); err != nil { + if opts.Stdin { + if err := dockerCLI.In().CheckTty(opts.Stdin, c.Config.Tty); err != nil { return err } if !c.Config.OpenStdin { // TODO(thaJeztah): should this produce an error? - attachStdIn = false + opts.Stdin = false } } - detachKeys := opts.DetachKeys if opts.DetachKeys == "" { - detachKeys = dockerCLI.ConfigFile().DetachKeys - } - - options := container.AttachOptions{ - Stream: true, - Stdin: attachStdIn, - Stdout: true, - Stderr: true, - DetachKeys: detachKeys, + opts.DetachKeys = dockerCLI.ConfigFile().DetachKeys } var in io.ReadCloser - if options.Stdin { + if opts.Stdin { in = dockerCLI.In() } - if opts.Proxy && !c.Config.Tty { + // TODO(thaJeztah): should this still depend on Config.Tty? It's unconditionally enabled on `docker exec` since https://github.com/docker/cli/pull/1841/files + if !disableSignalProxy && !c.Config.Tty { sigc := notifyAllSignals() go ForwardAllSignals(ctx, apiClient, containerID, sigc) defer signal.StopCatch(sigc) } - resp, errAttach := apiClient.ContainerAttach(ctx, containerID, options) + resp, errAttach := apiClient.ContainerAttach(ctx, containerID, opts) if errAttach != nil { return errAttach } @@ -148,7 +154,7 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o errorStream: dockerCLI.Err(), resp: resp, tty: c.Config.Tty, - detachKeys: options.DetachKeys, + detachKeys: opts.DetachKeys, } if err := streamer.stream(ctx); err != nil {