diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 0e449bb68a..978e1465d3 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -92,8 +92,10 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command { func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error { if err := validatePullOpt(options.pull); err != nil { - reportError(dockerCli.Err(), "create", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "create").Error(), + StatusCode: 125, + } } proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll())) newEnv := []string{} @@ -107,12 +109,16 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, copts.env = *opts.NewListOptsRef(&newEnv, nil) containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) if err != nil { - reportError(dockerCli.Err(), "create", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "create").Error(), + StatusCode: 125, + } } if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil { - reportError(dockerCli.Err(), "create", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "create").Error(), + StatusCode: 125, + } } id, err := createContainer(ctx, dockerCli, containerCfg, options) if err != nil { diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 43509a9b00..4485723296 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -189,8 +189,8 @@ func TestCreateContainerImagePullPolicyInvalid(t *testing.T) { statusErr := cli.StatusError{} assert.Check(t, errors.As(err, &statusErr)) - assert.Equal(t, statusErr.StatusCode, 125) - assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg)) + assert.Check(t, is.Equal(statusErr.StatusCode, 125)) + assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) }) } } @@ -285,7 +285,7 @@ func TestNewCreateCommandWithWarnings(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - cli := test.NewFakeCli(&fakeClient{ + fakeCLI := test.NewFakeCli(&fakeClient{ createContainerFunc: func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, @@ -295,15 +295,15 @@ func TestNewCreateCommandWithWarnings(t *testing.T) { return container.CreateResponse{}, nil }, }) - cmd := NewCreateCommand(cli) + cmd := NewCreateCommand(fakeCLI) cmd.SetOut(io.Discard) cmd.SetArgs(tc.args) err := cmd.Execute() assert.NilError(t, err) if tc.warning { - golden.Assert(t, cli.ErrBuffer().String(), tc.name+".golden") + golden.Assert(t, fakeCLI.ErrBuffer().String(), tc.name+".golden") } else { - assert.Equal(t, cli.ErrBuffer().String(), "") + assert.Equal(t, fakeCLI.ErrBuffer().String(), "") } }) } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index ef0986cba2..12b06f7b1e 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -83,8 +83,10 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command { func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error { if err := validatePullOpt(ropts.pull); err != nil { - reportError(dockerCli.Err(), "run", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 125, + } } proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll())) newEnv := []string{} @@ -99,12 +101,16 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) // just in case the parse does not exit if err != nil { - reportError(dockerCli.Err(), "run", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 125, + } } if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil { - reportError(dockerCli.Err(), "run", err.Error(), true) - return cli.StatusError{StatusCode: 125} + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 125, + } } return runContainer(ctx, dockerCli, ropts, copts, containerCfg) } @@ -139,8 +145,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions) if err != nil { - reportError(stderr, "run", err.Error(), true) - return runStartContainerErr(err) + return toStatusError(err) } if runOpts.sigProxy { sigc := notifyAllSignals() @@ -204,12 +209,11 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption <-errCh } - reportError(stderr, "run", err.Error(), false) if copts.autoRemove { // wait container to be removed <-statusChan } - return runStartContainerErr(err) + return toStatusError(err) } if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() { @@ -292,30 +296,40 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, containerID str return resp.Close, nil } -// reportError is a utility method that prints a user-friendly message -// containing the error that occurred during parsing and a suggestion to get help -func reportError(stderr io.Writer, name string, str string, withHelp bool) { - str = strings.TrimSuffix(str, ".") + "." - if withHelp { - str += "\nSee 'docker " + name + " --help'." - } - _, _ = fmt.Fprintln(stderr, "docker:", str) +// withHelp decorates the error with a suggestion to use "--help". +func withHelp(err error, commandName string) error { + return fmt.Errorf("docker: %w\n\nRun 'docker %s --help' for more information", err, commandName) } -// if container start fails with 'not found'/'no such' error, return 127 -// if container start fails with 'permission denied' error, return 126 -// return 125 for generic docker daemon failures -func runStartContainerErr(err error) error { - trimmedErr := strings.TrimPrefix(err.Error(), "Error response from daemon: ") - statusError := cli.StatusError{StatusCode: 125} - if strings.Contains(trimmedErr, "executable file not found") || - strings.Contains(trimmedErr, "no such file or directory") || - strings.Contains(trimmedErr, "system cannot find the file specified") { - statusError = cli.StatusError{StatusCode: 127} - } else if strings.Contains(trimmedErr, syscall.EACCES.Error()) || - strings.Contains(trimmedErr, syscall.EISDIR.Error()) { - statusError = cli.StatusError{StatusCode: 126} +// toStatusError attempts to detect specific error-conditions to assign +// an appropriate exit-code for situations where the problem originates +// from the container. It returns [cli.StatusError] with the original +// error message and the Status field set as follows: +// +// - 125: for generic failures sent back from the daemon +// - 126: if container start fails with 'permission denied' error +// - 127: if container start fails with 'not found'/'no such' error +func toStatusError(err error) error { + // TODO(thaJeztah): some of these errors originate from the container: should we actually suggest "--help" for those? + + errMsg := err.Error() + + if strings.Contains(errMsg, "executable file not found") || strings.Contains(errMsg, "no such file or directory") || strings.Contains(errMsg, "system cannot find the file specified") { + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 127, + } } - return statusError + if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) { + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 126, + } + } + + return cli.StatusError{ + Status: withHelp(err, "run").Error(), + StatusCode: 125, + } } diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 7c39b17ca6..661cf8bdea 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -145,8 +145,10 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) { cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() - assert.Assert(t, err != nil) - assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError)) + statusErr := cli.StatusError{} + assert.Check(t, errors.As(err, &statusErr)) + assert.Check(t, is.Equal(statusErr.StatusCode, 125)) + assert.Check(t, is.ErrorContains(err, tc.expectedError)) }) } } @@ -179,8 +181,8 @@ func TestRunContainerImagePullPolicyInvalid(t *testing.T) { statusErr := cli.StatusError{} assert.Check(t, errors.As(err, &statusErr)) - assert.Equal(t, statusErr.StatusCode, 125) - assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg)) + assert.Check(t, is.Equal(statusErr.StatusCode, 125)) + assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) }) } }