diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index 8cce73d9ac..4ae85dbce4 100644 --- a/cli-plugins/plugin/plugin.go +++ b/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) } } diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 7825c7d354..a8596388fa 100644 --- a/cmd/docker/docker.go +++ b/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 { diff --git a/e2e/cli-plugins/plugins/presocket/main.go b/e2e/cli-plugins/plugins/presocket/main.go index 6cdf87a424..8c8ad6df66 100644 --- a/e2e/cli-plugins/plugins/presocket/main.go +++ b/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 }, diff --git a/e2e/cli-plugins/socket_test.go b/e2e/cli-plugins/socket_test.go index 5e0b1cbb7c..c641f0b7da 100644 --- a/e2e/cli-plugins/socket_test.go +++ b/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,7 +38,7 @@ 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") } @@ -45,7 +46,7 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) { // 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,7 +167,7 @@ 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") } @@ -169,7 +175,7 @@ func TestPluginSocketCommunication(t *testing.T) { // 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") }) }) @@ -177,9 +183,6 @@ func TestPluginSocketCommunication(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") + out, err := command.CombinedOutput() - // 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") + 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") }) }) }