From b6c8353c11c8d39ee8f5db2e1cd9e922132c0807 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 3 Dec 2024 14:00:29 +0100 Subject: [PATCH] cli/command: ctx cancel should not print or produce a non zero exit code The user might kill the CLI through a SIGINT/SIGTERM which cancels the main context we pass around. Currently the context cancel is error is printed alongside any other wrapped error with a generic exit code (125). This patch improves on this behavior and prevents any error from being printed when they match `context.Cancelled`. The `cli.StatusError` error would wrap errors but not provide a way to unwrap them. This would lead to situations where `errors.Is` would not match the underlying error. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/cobra.go | 2 +- cli/command/config/inspect.go | 2 +- cli/command/container/create.go | 6 +++--- cli/command/container/run.go | 12 ++++++------ cli/command/container/run_test.go | 8 ++++---- cli/command/image/build.go | 2 +- cli/command/inspect/inspector.go | 4 ++-- cli/command/node/inspect.go | 2 +- cli/command/secret/inspect.go | 2 +- cli/command/service/inspect.go | 2 +- cli/command/system/events.go | 2 +- cli/command/system/info.go | 2 +- cli/command/system/version.go | 2 +- cli/error.go | 9 +++++++-- cmd/docker/docker.go | 2 +- 15 files changed, 32 insertions(+), 27 deletions(-) diff --git a/cli/cobra.go b/cli/cobra.go index feab2b4a91c8..456cf5edcbe3 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -93,7 +93,7 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error { } return StatusError{ - Status: fmt.Sprintf("%s\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), + Status: fmt.Errorf("%w\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), StatusCode: 125, } } diff --git a/cli/command/config/inspect.go b/cli/command/config/inspect.go index 7ae4a4c40435..7a1fbc619630 100644 --- a/cli/command/config/inspect.go +++ b/cli/command/config/inspect.go @@ -67,7 +67,7 @@ func RunConfigInspect(ctx context.Context, dockerCli command.Cli, opts InspectOp } if err := InspectFormatWrite(configCtx, opts.Names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Status: err} } return nil } diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 9f73b7f0afe3..cf82fc82529c 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -93,7 +93,7 @@ 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 { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Status: withHelp(err, "create"), StatusCode: 125, } } @@ -110,13 +110,13 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) if err != nil { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Status: withHelp(err, "create"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil { return cli.StatusError{ - Status: withHelp(err, "create").Error(), + Status: withHelp(err, "create"), StatusCode: 125, } } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index bb0340ed1b49..5331d03937fb 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -85,7 +85,7 @@ 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 { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Status: withHelp(err, "run"), StatusCode: 125, } } @@ -103,13 +103,13 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro // just in case the parse does not exit if err != nil { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Status: withHelp(err, "run"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Status: withHelp(err, "run"), StatusCode: 125, } } @@ -315,20 +315,20 @@ func toStatusError(err error) 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(), + Status: withHelp(err, "run"), StatusCode: 127, } } if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) { return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Status: withHelp(err, "run"), StatusCode: 126, } } return cli.StatusError{ - Status: withHelp(err, "run").Error(), + Status: withHelp(err, "run"), StatusCode: 125, } } diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 81b176d904c9..c48ba9415649 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -294,10 +294,10 @@ func TestRunPullTermination(t *testing.T) { select { case cmdErr := <-cmdErrC: - assert.Equal(t, cmdErr, cli.StatusError{ - StatusCode: 125, - Status: "docker: context canceled\n\nRun 'docker run --help' for more information", - }) + assert.ErrorIs(t, cmdErr, context.Canceled) + v, ok := cmdErr.(cli.StatusError) + assert.Check(t, ok) + assert.Check(t, is.Equal(v.StatusCode, 125)) case <-time.After(10 * time.Second): t.Fatal("cmd did not return before the timeout") } diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 4a258646dcce..c429aec499c3 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -371,7 +371,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) if options.quiet { fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff) } - return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code} + return cli.StatusError{Status: jerr, StatusCode: jerr.Code} } return err } diff --git a/cli/command/inspect/inspector.go b/cli/command/inspect/inspector.go index 46ba8750d6a4..ff78eed9a72f 100644 --- a/cli/command/inspect/inspector.go +++ b/cli/command/inspect/inspector.go @@ -67,7 +67,7 @@ type GetRefFunc func(ref string) (any, []byte, error) func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFunc) error { inspector, err := NewTemplateInspectorFromString(out, tmplStr) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return cli.StatusError{StatusCode: 64, Status: err} } var inspectErrs []string @@ -90,7 +90,7 @@ func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFu if len(inspectErrs) != 0 { return cli.StatusError{ StatusCode: 1, - Status: strings.Join(inspectErrs, "\n"), + Status: errors.New(strings.Join(inspectErrs, "\n")), } } return nil diff --git a/cli/command/node/inspect.go b/cli/command/node/inspect.go index 270a14bd2bdd..5ecd7455bccc 100644 --- a/cli/command/node/inspect.go +++ b/cli/command/node/inspect.go @@ -69,7 +69,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(nodeCtx, opts.nodeIds, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Status: err} } return nil } diff --git a/cli/command/secret/inspect.go b/cli/command/secret/inspect.go index 5559d43d383e..26bc0db1dedb 100644 --- a/cli/command/secret/inspect.go +++ b/cli/command/secret/inspect.go @@ -65,7 +65,7 @@ func runSecretInspect(ctx context.Context, dockerCli command.Cli, opts inspectOp } if err := InspectFormatWrite(secretCtx, opts.names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Status: err} } return nil } diff --git a/cli/command/service/inspect.go b/cli/command/service/inspect.go index 28b957f77310..4ea74a14bb10 100644 --- a/cli/command/service/inspect.go +++ b/cli/command/service/inspect.go @@ -94,7 +94,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(serviceCtx, opts.refs, getRef, getNetwork); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return cli.StatusError{StatusCode: 1, Status: err} } return nil } diff --git a/cli/command/system/events.go b/cli/command/system/events.go index 9b5f965e5b3c..d3fe17f2a7a3 100644 --- a/cli/command/system/events.go +++ b/cli/command/system/events.go @@ -60,7 +60,7 @@ func runEvents(ctx context.Context, dockerCli command.Cli, options *eventsOption if err != nil { return cli.StatusError{ StatusCode: 64, - Status: "Error parsing format: " + err.Error(), + Status: fmt.Errorf("Error parsing format: %w", err), } } ctx, cancel := context.WithCancel(ctx) diff --git a/cli/command/system/info.go b/cli/command/system/info.go index 108a65a5f1f4..b34332aa977d 100644 --- a/cli/command/system/info.go +++ b/cli/command/system/info.go @@ -459,7 +459,7 @@ func formatInfo(output io.Writer, info dockerInfo, format string) error { if err != nil { return cli.StatusError{ StatusCode: 64, - Status: "template parsing error: " + err.Error(), + Status: fmt.Errorf("template parsing error: %w", err), } } err = tmpl.Execute(output, info) diff --git a/cli/command/system/version.go b/cli/command/system/version.go index f25446b06dd8..525ee6bf4c6c 100644 --- a/cli/command/system/version.go +++ b/cli/command/system/version.go @@ -148,7 +148,7 @@ func runVersion(ctx context.Context, dockerCli command.Cli, opts *versionOptions var err error tmpl, err := newVersionTemplate(opts.format) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return cli.StatusError{StatusCode: 64, Status: err} } // TODO print error if kubernetes is used? diff --git a/cli/error.go b/cli/error.go index 8c4a5f952bce..3cd1bac0d622 100644 --- a/cli/error.go +++ b/cli/error.go @@ -6,7 +6,7 @@ import ( // StatusError reports an unsuccessful exit by a command. type StatusError struct { - Status string + Status error StatusCode int } @@ -14,8 +14,13 @@ type StatusError struct { // it is returned as-is, otherwise it generates a generic error-message // based on the StatusCode. func (e StatusError) Error() string { - if e.Status == "" { + if e.Status == nil { return "exit status " + strconv.Itoa(e.StatusCode) } + return e.Status.Error() +} + +// Allow wrapped errors stored in StatusError to be checked using `errors.Is`. +func (e StatusError) Unwrap() error { return e.Status } diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 29a3ed595755..ab2dd08f7f15 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -30,7 +30,7 @@ import ( func main() { err := dockerMain(context.Background()) - if err != nil && !errdefs.IsCancelled(err) { + if err != nil && !errdefs.IsCancelled(err) && !errdefs.IsContext(err) { _, _ = fmt.Fprintln(os.Stderr, err) os.Exit(getExitCode(err)) }