Merge pull request #5229 from thaJeztah/exit_error

cmd/docker: split handling exit-code to a separate utility

cli-plugins/plugin/plugin.go

@@ -3,6 +3,7 @@ package plugin
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"sync"
@@ -34,7 +35,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
 
 	var persistentPreRunOnce sync.Once
 	PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
-		var err error
+		var retErr error
 		persistentPreRunOnce.Do(func() {
 			ctx, cancel := context.WithCancel(cmd.Context())
 			cmd.SetContext(ctx)
@@ -46,7 +47,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
 				opts = append(opts, withPluginClientConn(plugin.Name()))
 			}
 			opts = append(opts, command.WithEnableGlobalMeterProvider(), command.WithEnableGlobalTracerProvider())
-			err = tcmd.Initialize(opts...)
+			retErr = tcmd.Initialize(opts...)
 			ogRunE := cmd.RunE
 			if ogRunE == nil {
 				ogRun := cmd.Run
@@ -66,7 +67,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
 				return err
 			}
 		})
-		return err
+		return retErr
 	}
 
 	cmd, args, err := tcmd.HandleGlobalFlags()
@@ -92,18 +93,17 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) {
 	plugin := makeCmd(dockerCli)
 
 	if err := RunPlugin(dockerCli, plugin, meta); err != nil {
-		if sterr, ok := err.(cli.StatusError); ok {
-			if sterr.Status != "" {
-				fmt.Fprintln(dockerCli.Err(), sterr.Status)
-			}
+		var stErr cli.StatusError
+		if errors.As(err, &stErr) {
 			// StatusError should only be used for errors, and all errors should
 			// have a non-zero exit status, so never exit with 0
-			if sterr.StatusCode == 0 {
-				os.Exit(1)
+			if stErr.StatusCode == 0 { // FIXME(thaJeztah): this should never be used with a zero status-code. Check if we do this anywhere.
+				stErr.StatusCode = 1
 			}
-			os.Exit(sterr.StatusCode)
+			_, _ = fmt.Fprintln(dockerCli.Err(), stErr)
+			os.Exit(stErr.StatusCode)
 		}
-		fmt.Fprintln(dockerCli.Err(), err)
+		_, _ = fmt.Fprintln(dockerCli.Err(), err)
 		os.Exit(1)
 	}
 }

cmd/docker/docker.go

@@ -29,43 +29,41 @@ import (
 )
 
 func main() {
-	statusCode := dockerMain()
-	if statusCode != 0 {
-		os.Exit(statusCode)
+	err := dockerMain(context.Background())
+	if err != nil && !errdefs.IsCancelled(err) {
+		_, _ = fmt.Fprintln(os.Stderr, err)
+		os.Exit(getExitCode(err))
 	}
 }
 
-func dockerMain() int {
-	ctx, cancelNotify := signal.NotifyContext(context.Background(), platformsignals.TerminationSignals...)
+func dockerMain(ctx context.Context) error {
+	ctx, cancelNotify := signal.NotifyContext(ctx, platformsignals.TerminationSignals...)
 	defer cancelNotify()
 
 	dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))
 	if err != nil {
-		fmt.Fprintln(os.Stderr, err)
-		return 1
+		return err
 	}
 	logrus.SetOutput(dockerCli.Err())
 	otel.SetErrorHandler(debug.OTELErrorHandler)
 
-	if err := runDocker(ctx, dockerCli); err != nil {
-		if sterr, ok := err.(cli.StatusError); ok {
-			if sterr.Status != "" {
-				fmt.Fprintln(dockerCli.Err(), sterr.Status)
-			}
-			// StatusError should only be used for errors, and all errors should
-			// have a non-zero exit status, so never exit with 0
-			if sterr.StatusCode == 0 {
-				return 1
-			}
-			return sterr.StatusCode
-		}
-		if errdefs.IsCancelled(err) {
-			return 0
-		}
-		fmt.Fprintln(dockerCli.Err(), err)
-		return 1
+	return runDocker(ctx, dockerCli)
+}
+
+// getExitCode returns the exit-code to use for the given error.
+// If err is a [cli.StatusError] and has a StatusCode set, it uses the
+// status-code from it, otherwise it returns "1" for any error.
+func getExitCode(err error) int {
+	if err == nil {
+		return 0
 	}
-	return 0
+	var stErr cli.StatusError
+	if errors.As(err, &stErr) && stErr.StatusCode != 0 { // FIXME(thaJeztah): StatusCode should never be used with a zero status-code. Check if we do this anywhere.
+		return stErr.StatusCode
+	}
+
+	// No status-code provided; all errors should have a non-zero exit code.
+	return 1
 }
 
 func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand {

e2e/cli-plugins/plugins/presocket/main.go

@@ -39,18 +39,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			go func() {
 				<-cmd.Context().Done()
-				fmt.Fprintln(dockerCli.Out(), "context cancelled")
+				_, _ = fmt.Fprintln(dockerCli.Out(), "test-no-socket: exiting after context was done")
 				os.Exit(2)
 			}()
 			signalCh := make(chan os.Signal, 10)
 			signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
 			go func() {
 				for range signalCh {
-					fmt.Fprintln(dockerCli.Out(), "received SIGINT")
+					_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
 				}
 			}()
 			<-time.After(3 * time.Second)
-			fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
+			_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
 			return nil
 		},
 	})
@@ -64,18 +64,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			go func() {
 				<-cmd.Context().Done()
-				fmt.Fprintln(dockerCli.Out(), "context cancelled")
+				_, _ = fmt.Fprintln(dockerCli.Out(), "test-socket: exiting after context was done")
 				os.Exit(2)
 			}()
 			signalCh := make(chan os.Signal, 10)
 			signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
 			go func() {
 				for range signalCh {
-					fmt.Fprintln(dockerCli.Out(), "received SIGINT")
+					_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
 				}
 			}()
 			<-time.After(3 * time.Second)
-			fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
+			_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
 			return nil
 		},
 	})
@@ -91,11 +91,11 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
 			signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
 			go func() {
 				for range signalCh {
-					fmt.Fprintln(dockerCli.Out(), "received SIGINT")
+					_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
 				}
 			}()
 			<-time.After(3 * time.Second)
-			fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
+			_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
 			return nil
 		},
 	})
@@ -113,7 +113,7 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
 			select {
 			case <-done:
 			case <-time.After(2 * time.Second):
-				fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
+				_, _ = fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
 			}
 			return nil
 		},

e2e/cli-plugins/socket_test.go

@@ -1,7 +1,7 @@
 package cliplugins
 
 import (
-	"bytes"
+	"errors"
 	"io"
 	"os/exec"
 	"strings"
@@ -11,6 +11,7 @@ import (
 
 	"github.com/creack/pty"
 	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 )
 
 // TestPluginSocketBackwardsCompatible executes a plugin binary
@@ -37,15 +38,15 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
 				err := syscall.Kill(-command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal process group")
 			}()
-			bytes, err := io.ReadAll(ptmx)
+			out, err := io.ReadAll(ptmx)
 			if err != nil && !strings.Contains(err.Error(), "input/output error") {
 				t.Fatal("failed to get command output")
 			}
 
 			// the plugin is attached to the TTY, so the parent process
 			// ignores the received signal, and the plugin receives a SIGINT
 			// as well
-			assert.Equal(t, string(bytes), "received SIGINT\r\nexit after 3 seconds\r\n")
+			assert.Equal(t, string(out), "received SIGINT\r\nexit after 3 seconds\r\n")
 		})
 
 		// ensure that we don't break plugins that attempt to read from the TTY
@@ -95,13 +96,13 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
 				err := syscall.Kill(command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal process group")
 			}()
-			bytes, err := command.CombinedOutput()
-			t.Log("command output: " + string(bytes))
+			out, err := command.CombinedOutput()
+			t.Log("command output: " + string(out))
 			assert.NilError(t, err, "failed to run command")
 
 			// the plugin process does not receive a SIGINT
 			// so it exits after 3 seconds and prints this message
-			assert.Equal(t, string(bytes), "exit after 3 seconds\n")
+			assert.Equal(t, string(out), "exit after 3 seconds\n")
 		})
 
 		t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
@@ -130,13 +131,18 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
 				err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal process group")
 			}()
-			bytes, err := command.CombinedOutput()
-			assert.ErrorContains(t, err, "exit status 1")
+			out, err := command.CombinedOutput()
+
+			var exitError *exec.ExitError
+			assert.Assert(t, errors.As(err, &exitError))
+			assert.Check(t, exitError.Exited())
+			assert.Check(t, is.Equal(exitError.ExitCode(), 1))
+			assert.Check(t, is.ErrorContains(err, "exit status 1"))
 
 			// the plugin process does not receive a SIGINT and does
 			// the CLI cannot cancel it over the socket, so it kills
 			// the plugin process and forcefully exits
-			assert.Equal(t, string(bytes), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
+			assert.Equal(t, string(out), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
 		})
 	})
 }
@@ -161,25 +167,22 @@ func TestPluginSocketCommunication(t *testing.T) {
 				err := syscall.Kill(-command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal process group")
 			}()
-			bytes, err := io.ReadAll(ptmx)
+			out, err := io.ReadAll(ptmx)
 			if err != nil && !strings.Contains(err.Error(), "input/output error") {
 				t.Fatal("failed to get command output")
 			}
 
 			// the plugin is attached to the TTY, so the parent process
 			// ignores the received signal, and the plugin receives a SIGINT
 			// as well
-			assert.Equal(t, string(bytes), "received SIGINT\r\nexit after 3 seconds\r\n")
+			assert.Equal(t, string(out), "received SIGINT\r\nexit after 3 seconds\r\n")
 		})
 	})
 
 	t.Run("detached", func(t *testing.T) {
 		t.Run("the plugin does not get signalled", func(t *testing.T) {
 			cmd := run("presocket", "test-socket")
 			command := exec.Command(cmd.Command[0], cmd.Command[1:]...)
-			outB := bytes.Buffer{}
-			command.Stdout = &outB
-			command.Stderr = &outB
 			command.SysProcAttr = &syscall.SysProcAttr{
 				Setpgid: true,
 			}
@@ -190,13 +193,19 @@ func TestPluginSocketCommunication(t *testing.T) {
 				err := syscall.Kill(command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal CLI process")
 			}()
-			err := command.Run()
-			t.Log(outB.String())
-			assert.ErrorContains(t, err, "exit status 2")
-
-			// the plugin does not get signalled, but it does get it's
-			// context cancelled by the CLI through the socket
-			assert.Equal(t, outB.String(), "context cancelled\n")
+			out, err := command.CombinedOutput()
+
+			var exitError *exec.ExitError
+			assert.Assert(t, errors.As(err, &exitError))
+			assert.Check(t, exitError.Exited())
+			assert.Check(t, is.Equal(exitError.ExitCode(), 2))
+			assert.Check(t, is.ErrorContains(err, "exit status 2"))
+
+			// the plugin does not get signalled, but it does get its
+			// context canceled by the CLI through the socket
+			const expected = "test-socket: exiting after context was done\nexit status 2"
+			actual := strings.TrimSpace(string(out))
+			assert.Check(t, is.Equal(actual, expected))
 		})
 
 		t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
@@ -223,13 +232,18 @@ func TestPluginSocketCommunication(t *testing.T) {
 				err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
 				assert.NilError(t, err, "failed to signal CLI process§")
 			}()
-			bytes, err := command.CombinedOutput()
-			assert.ErrorContains(t, err, "exit status 1")
+			out, err := command.CombinedOutput()
+
+			var exitError *exec.ExitError
+			assert.Assert(t, errors.As(err, &exitError))
+			assert.Check(t, exitError.Exited())
+			assert.Check(t, is.Equal(exitError.ExitCode(), 1))
+			assert.Check(t, is.ErrorContains(err, "exit status 1"))
 
 			// the plugin process does not receive a SIGINT and does
-			// not exit after having it's context cancelled, so the CLI
+			// not exit after having it's context canceled, so the CLI
 			// kills the plugin process and forcefully exits
-			assert.Equal(t, string(bytes), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
+			assert.Equal(t, string(out), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
 		})
 	})
 }