From 0c5adb2e984014317a6d1df405af9c33a56df456 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 8 Jun 2023 15:23:26 +0200 Subject: [PATCH 1/4] cli/command/container: attach: rename var that collided with import Signed-off-by: Sebastiaan van Stijn --- cli/command/container/attach.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index bd17696b80..ad8c04d04c 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -72,12 +72,12 @@ func NewAttachCommand(dockerCli command.Cli) *cobra.Command { func runAttach(dockerCli command.Cli, opts *attachOptions) error { ctx := context.Background() - client := dockerCli.Client() + apiClient := dockerCli.Client() // request channel to wait for client - resultC, errC := client.ContainerWait(ctx, opts.container, "") + resultC, errC := apiClient.ContainerWait(ctx, opts.container, "") - c, err := inspectContainerAndCheckState(ctx, client, opts.container) + c, err := inspectContainerAndCheckState(ctx, apiClient, opts.container) if err != nil { return err } @@ -109,7 +109,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { defer signal.StopCatch(sigc) } - resp, errAttach := client.ContainerAttach(ctx, opts.container, options) + resp, errAttach := apiClient.ContainerAttach(ctx, opts.container, options) if errAttach != nil { return errAttach } @@ -123,7 +123,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { // the container and not exit. // // Recheck the container's state to avoid attach block. - _, err = inspectContainerAndCheckState(ctx, client, opts.container) + _, err = inspectContainerAndCheckState(ctx, apiClient, opts.container) if err != nil { return err } From c2c6fbe23cb899de0057537c266da004c17d0f2d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 8 Jun 2023 16:34:08 +0200 Subject: [PATCH 2/4] cli/command/container: TestCreateContainerImagePullPolicy: use sub-tests Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create_test.go | 92 +++++++++++++++------------- 1 file changed, 48 insertions(+), 44 deletions(-) diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 725b6c9326..be7c039eb1 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -79,8 +79,10 @@ func TestCIDFileCloseWithWrite(t *testing.T) { } func TestCreateContainerImagePullPolicy(t *testing.T) { - imageName := "does-not-exist-locally" - containerID := "abcdef" + const ( + imageName = "does-not-exist-locally" + containerID = "abcdef" + ) config := &containerConfig{ Config: &container.Config{ Image: imageName, @@ -110,50 +112,52 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { ExpectedErrMsg: "error fake not found", }, } - for _, c := range cases { - c := c - pullCounter := 0 + for _, tc := range cases { + tc := tc + t.Run(tc.PullPolicy, func(t *testing.T) { + pullCounter := 0 - client := &fakeClient{ - createContainerFunc: func( - config *container.Config, - hostConfig *container.HostConfig, - networkingConfig *network.NetworkingConfig, - platform *specs.Platform, - containerName string, - ) (container.CreateResponse, error) { - defer func() { c.ResponseCounter++ }() - switch c.ResponseCounter { - case 0: - return container.CreateResponse{}, fakeNotFound{} - default: - return container.CreateResponse{ID: containerID}, nil - } - }, - imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) { - defer func() { pullCounter++ }() - return io.NopCloser(strings.NewReader("")), nil - }, - infoFunc: func() (types.Info, error) { - return types.Info{IndexServerAddress: "https://indexserver.example.com"}, nil - }, - } - cli := test.NewFakeCli(client) - body, err := createContainer(context.Background(), cli, config, &createOptions{ - name: "name", - platform: runtime.GOOS, - untrusted: true, - pull: c.PullPolicy, + client := &fakeClient{ + createContainerFunc: func( + config *container.Config, + hostConfig *container.HostConfig, + networkingConfig *network.NetworkingConfig, + platform *specs.Platform, + containerName string, + ) (container.CreateResponse, error) { + defer func() { tc.ResponseCounter++ }() + switch tc.ResponseCounter { + case 0: + return container.CreateResponse{}, fakeNotFound{} + default: + return container.CreateResponse{ID: containerID}, nil + } + }, + imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) { + defer func() { pullCounter++ }() + return io.NopCloser(strings.NewReader("")), nil + }, + infoFunc: func() (types.Info, error) { + return types.Info{IndexServerAddress: "https://indexserver.example.com"}, nil + }, + } + fakeCLI := test.NewFakeCli(client) + body, err := createContainer(context.Background(), fakeCLI, config, &createOptions{ + name: "name", + platform: runtime.GOOS, + untrusted: true, + pull: tc.PullPolicy, + }) + + if tc.ExpectedErrMsg != "" { + assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) + } else { + assert.Check(t, err) + assert.Check(t, is.DeepEqual(tc.ExpectedBody, *body)) + } + + assert.Check(t, is.Equal(tc.ExpectedPulls, pullCounter)) }) - - if c.ExpectedErrMsg != "" { - assert.ErrorContains(t, err, c.ExpectedErrMsg) - } else { - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(c.ExpectedBody, *body)) - } - - assert.Check(t, is.Equal(c.ExpectedPulls, pullCounter)) } } From 23e26f40feb71d6c45c495e6fd9f0010c601cac9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 8 Jun 2023 16:46:15 +0200 Subject: [PATCH 3/4] cli/command/container: createContainer(): return container-ID This function returned the whole response, but we already handled the warnings included in the response as part of the function. All consumers of this function only used the container-ID, so let's simplify and return just that (it's a non-exported func, so we can change the signature again if we really need it). Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 26 +++++++++++++------------- cli/command/container/create_test.go | 10 +++++----- cli/command/container/run.go | 16 ++++++++-------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 4758714d1a..1ad4ba6c52 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -104,11 +104,11 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, options *createOptio reportError(dockerCli.Err(), "create", err.Error(), true) return cli.StatusError{StatusCode: 125} } - response, err := createContainer(context.Background(), dockerCli, containerCfg, options) + id, err := createContainer(context.Background(), dockerCli, containerCfg, options) if err != nil { return err } - fmt.Fprintln(dockerCli.Out(), response.ID) + _, _ = fmt.Fprintln(dockerCli.Out(), id) return nil } @@ -185,7 +185,7 @@ func newCIDFile(path string) (*cidFile, error) { } //nolint:gocyclo -func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *containerConfig, opts *createOptions) (*container.CreateResponse, error) { +func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *containerConfig, opts *createOptions) (containerID string, err error) { config := containerCfg.Config hostConfig := containerCfg.HostConfig networkingConfig := containerCfg.NetworkingConfig @@ -200,13 +200,13 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c containerIDFile, err := newCIDFile(hostConfig.ContainerIDFile) if err != nil { - return nil, err + return "", err } defer containerIDFile.Close() ref, err := reference.ParseAnyReference(config.Image) if err != nil { - return nil, err + return "", err } if named, ok := ref.(reference.Named); ok { namedRef = reference.TagNameOnly(named) @@ -215,7 +215,7 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c var err error trustedRef, err = image.TrustedReference(ctx, dockerCli, taggedRef) if err != nil { - return nil, err + return "", err } config.Image = reference.FamiliarString(trustedRef) } @@ -239,14 +239,14 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c if opts.platform != "" && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.41") { p, err := platforms.Parse(opts.platform) if err != nil { - return nil, errors.Wrap(err, "error parsing specified platform") + return "", errors.Wrap(err, "error parsing specified platform") } platform = &p } if opts.pull == PullImageAlways { if err := pullAndTagImage(); err != nil { - return nil, err + return "", err } } @@ -262,24 +262,24 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c } if err := pullAndTagImage(); err != nil { - return nil, err + return "", err } var retryErr error response, retryErr = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, platform, opts.name) if retryErr != nil { - return nil, retryErr + return "", retryErr } } else { - return nil, err + return "", err } } for _, w := range response.Warnings { - fmt.Fprintf(dockerCli.Err(), "WARNING: %s\n", w) + _, _ = fmt.Fprintf(dockerCli.Err(), "WARNING: %s\n", w) } err = containerIDFile.Write(response.ID) - return &response, err + return response.ID, err } func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) { diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index be7c039eb1..7de0dc6ddb 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -93,18 +93,18 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { cases := []struct { PullPolicy string ExpectedPulls int - ExpectedBody container.CreateResponse + ExpectedID string ExpectedErrMsg string ResponseCounter int }{ { PullPolicy: PullImageMissing, ExpectedPulls: 1, - ExpectedBody: container.CreateResponse{ID: containerID}, + ExpectedID: containerID, }, { PullPolicy: PullImageAlways, ExpectedPulls: 1, - ExpectedBody: container.CreateResponse{ID: containerID}, + ExpectedID: containerID, ResponseCounter: 1, // This lets us return a container on the first pull }, { PullPolicy: PullImageNever, @@ -142,7 +142,7 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { }, } fakeCLI := test.NewFakeCli(client) - body, err := createContainer(context.Background(), fakeCLI, config, &createOptions{ + id, err := createContainer(context.Background(), fakeCLI, config, &createOptions{ name: "name", platform: runtime.GOOS, untrusted: true, @@ -153,7 +153,7 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) } else { assert.Check(t, err) - assert.Check(t, is.DeepEqual(tc.ExpectedBody, *body)) + assert.Check(t, is.Equal(tc.ExpectedID, id)) } assert.Check(t, is.Equal(tc.ExpectedPulls, pullCounter)) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index c7e680e763..cfb7a7fb75 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -144,14 +144,14 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio ctx, cancelFun := context.WithCancel(context.Background()) defer cancelFun() - createResponse, err := createContainer(ctx, dockerCli, containerCfg, &opts.createOptions) + containerID, err := createContainer(ctx, dockerCli, containerCfg, &opts.createOptions) if err != nil { reportError(stderr, "run", err.Error(), true) return runStartContainerErr(err) } if opts.sigProxy { sigc := notifyAllSignals() - go ForwardAllSignals(ctx, dockerCli, createResponse.ID, sigc) + go ForwardAllSignals(ctx, dockerCli, containerID, sigc) defer signal.StopCatch(sigc) } @@ -164,7 +164,7 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio waitDisplayID = make(chan struct{}) go func() { defer close(waitDisplayID) - fmt.Fprintln(stdout, createResponse.ID) + _, _ = fmt.Fprintln(stdout, containerID) }() } attach := config.AttachStdin || config.AttachStdout || config.AttachStderr @@ -173,17 +173,17 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio dockerCli.ConfigFile().DetachKeys = opts.detachKeys } - closeFn, err := attachContainer(ctx, dockerCli, &errCh, config, createResponse.ID) + closeFn, err := attachContainer(ctx, dockerCli, &errCh, config, containerID) if err != nil { return err } defer closeFn() } - statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, copts.autoRemove) + statusChan := waitExitOrRemoved(ctx, dockerCli, containerID, copts.autoRemove) // start the container - if err := client.ContainerStart(ctx, createResponse.ID, types.ContainerStartOptions{}); err != nil { + if err := client.ContainerStart(ctx, containerID, types.ContainerStartOptions{}); err != nil { // If we have hijackedIOStreamer, we should notify // hijackedIOStreamer we are going to exit and wait // to avoid the terminal are not restored. @@ -201,8 +201,8 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio } if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() { - if err := MonitorTtySize(ctx, dockerCli, createResponse.ID, false); err != nil { - fmt.Fprintln(stderr, "Error monitoring TTY size:", err) + if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil { + _, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err) } } From 2331e4d521af05992cd60953ae304560640dec40 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 8 Jun 2023 16:53:30 +0200 Subject: [PATCH 4/4] cli/command/container: don't mutate ConfigFile.DetachKeys This code was introduced in https://github.com/moby/moby/commit/15aa2a663b47b6126a66efefcadb64edfbffb9f5, but from those changes, it appears that overwriting the config value was merely out of convenience, and that struct being used as an intermediate. While changing the config here should be mostly ephemeral, and not written back to the config-file, let's be clear on intent, and not mutatte the config as part of this code. Signed-off-by: Sebastiaan van Stijn --- cli/command/container/attach.go | 5 +++-- cli/command/container/run.go | 27 +++++++++++++-------------- cli/command/container/start.go | 5 +++-- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index ad8c04d04c..4785434c91 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -86,8 +86,9 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { return err } + detachKeys := dockerCli.ConfigFile().DetachKeys if opts.detachKeys != "" { - dockerCli.ConfigFile().DetachKeys = opts.detachKeys + detachKeys = opts.detachKeys } options := types.ContainerAttachOptions{ @@ -95,7 +96,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { Stdin: !opts.noStdin && c.Config.OpenStdin, Stdout: true, Stderr: true, - DetachKeys: dockerCli.ConfigFile().DetachKeys, + DetachKeys: detachKeys, } var in io.ReadCloser diff --git a/cli/command/container/run.go b/cli/command/container/run.go index cfb7a7fb75..fe4969cd0b 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -169,11 +169,18 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio } attach := config.AttachStdin || config.AttachStdout || config.AttachStderr if attach { + detachKeys := dockerCli.ConfigFile().DetachKeys if opts.detachKeys != "" { - dockerCli.ConfigFile().DetachKeys = opts.detachKeys + detachKeys = opts.detachKeys } - closeFn, err := attachContainer(ctx, dockerCli, &errCh, config, containerID) + closeFn, err := attachContainer(ctx, dockerCli, containerID, &errCh, config, types.ContainerAttachOptions{ + Stream: true, + Stdin: config.AttachStdin, + Stdout: config.AttachStdout, + Stderr: config.AttachStderr, + DetachKeys: detachKeys, + }) if err != nil { return err } @@ -232,15 +239,7 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio return nil } -func attachContainer(ctx context.Context, dockerCli command.Cli, errCh *chan error, config *container.Config, containerID string) (func(), error) { - options := types.ContainerAttachOptions{ - Stream: true, - Stdin: config.AttachStdin, - Stdout: config.AttachStdout, - Stderr: config.AttachStderr, - DetachKeys: dockerCli.ConfigFile().DetachKeys, - } - +func attachContainer(ctx context.Context, dockerCli command.Cli, containerID string, errCh *chan error, config *container.Config, options types.ContainerAttachOptions) (func(), error) { resp, errAttach := dockerCli.Client().ContainerAttach(ctx, containerID, options) if errAttach != nil { return nil, errAttach @@ -250,13 +249,13 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, errCh *chan err out, cerr io.Writer in io.ReadCloser ) - if config.AttachStdin { + if options.Stdin { in = dockerCli.In() } - if config.AttachStdout { + if options.Stdout { out = dockerCli.Out() } - if config.AttachStderr { + if options.Stderr { if config.Tty { cerr = dockerCli.Out() } else { diff --git a/cli/command/container/start.go b/cli/command/container/start.go index 9a8a083e6b..b50f6ba19a 100644 --- a/cli/command/container/start.go +++ b/cli/command/container/start.go @@ -94,8 +94,9 @@ func RunStart(dockerCli command.Cli, opts *StartOptions) error { defer signal.StopCatch(sigc) } + detachKeys := dockerCli.ConfigFile().DetachKeys if opts.DetachKeys != "" { - dockerCli.ConfigFile().DetachKeys = opts.DetachKeys + detachKeys = opts.DetachKeys } options := types.ContainerAttachOptions{ @@ -103,7 +104,7 @@ func RunStart(dockerCli command.Cli, opts *StartOptions) error { Stdin: opts.OpenStdin && c.Config.OpenStdin, Stdout: true, Stderr: true, - DetachKeys: dockerCli.ConfigFile().DetachKeys, + DetachKeys: detachKeys, } var in io.ReadCloser