From 90058df30519cd3c479ae77820dfbac3b77d0dc9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 4 Jul 2024 13:18:03 +0200 Subject: [PATCH] cli/command/container: remove reportError, and put StatusError to use The `reportError` utility was present because cli.StatusError would print the error decorated with `Status: , Code: `. That was not desirable in many cases as it would mess-up the output. To prevent this, the CLI had code to check for an empty `Status` (error message) in which case the error would be "ignored" (and only used for the exit-status), and the `reportError` utility would be used to manually print a custom error message before returning the error. Now that bca209006153d3e025cb3d31c3cd55eb2aec0c4f fixed the output format of `cli.StatusError`, and 3dd6fc365d853e21f0e11f9e6ab62c4f8ae438e7 and 350a0b68a9584ec9ae712b6eca906c1018ba6dac no longer discard these error, we can get rid of this utility, and just set the error-message for the status-error. This patch: - Introduces a `withHelp` which takes care of decorating errors with a "Run --help" hint for the user. - Introduces a `toStatusError` utility that detects certain errors in the container to assign a corresponding exit-code (these error-codes can be used to distinguish "client" errors from "container" errors). - Removes the `reportError` utility, and removes code that manually printed errors before returning. Behavior is mostly unmodified, with the exception of some slight reformatting of the errors: - `withHelp` adds a `docker:` prefix to the error, to indicate the error is produced by the `docker` command. This prefix was already present in most cases. - The "--help" hint is slightly updated ("Run 'docker run --help' for more information" instead of "See 'docker run --help'"), to make it more clear that it's a "call to action". - An empty is added before the "--help" hint to separate it better from the error-message. Before this patch: $ docker run --pull=invalid-option alpine docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never". See 'docker run --help'. $ echo $? 125 $ docker run --rm alpine nosuchcommand docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown. $ echo $? 127 With this patch: $ docker run --pull=invalid-option alpine docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never" Run 'docker run --help' for more information $ echo $? 125 $ docker run --rm alpine nosuchcommand docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown. Run 'docker run --help' for more information $ echo $? 127 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 18 ++++--- cli/command/container/create_test.go | 12 ++--- cli/command/container/run.go | 78 ++++++++++++++++------------ cli/command/container/run_test.go | 10 ++-- 4 files changed, 70 insertions(+), 48 deletions(-) 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)) }) } }