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 <[email protected]>
docker · Dec 3, 2024 · b6c8353 · b6c8353
1 parent 5afa739
commit b6c8353
Show file tree

Hide file tree

Showing 15 changed files with 32 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -6,16 +6,21 @@ import (
 
 // StatusError reports an unsuccessful exit by a command.
 type StatusError struct {
-	Status     string
+	Status     error
 	StatusCode int
 }
 
 // Error formats the error for printing. If a custom Status is provided,
 // 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
@@ -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))
 	}