diff --git a/cli-plugins/manager/hooks.go b/cli-plugins/manager/hooks.go index 9d9c4629d106..6a3212315f9a 100644 --- a/cli-plugins/manager/hooks.go +++ b/cli-plugins/manager/hooks.go @@ -42,11 +42,11 @@ func RunCLICommandHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, // RunPluginHooks is the entrypoint for the hooks execution flow // after a plugin command was just executed by the CLI. -func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string) { +func RunPluginHooks(ctx context.Context, dockerCLI config.Provider, rootCmd, subCommand *cobra.Command, args []string, cmdErrorMessage string) { commandName := strings.Join(args, " ") flags := getNaiveFlags(args) - runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, "") + runHooks(ctx, dockerCLI.ConfigFile(), rootCmd, subCommand, commandName, flags, cmdErrorMessage) } func runHooks(ctx context.Context, cfg *configfile.ConfigFile, rootCmd, subCommand *cobra.Command, invokedCommand string, flags map[string]string, cmdErrorMessage string) { @@ -70,7 +70,7 @@ func invokeAndCollectHooks(ctx context.Context, cfg *configfile.ConfigFile, root pluginDirs := getPluginDirs(cfg) nextSteps := make([]string, 0, len(pluginsCfg)) for pluginName, pluginCfg := range pluginsCfg { - match, ok := pluginMatch(pluginCfg, subCmdStr) + match, ok := pluginMatch(pluginCfg, subCmdStr, cmdErrorMessage) if !ok { continue } @@ -138,13 +138,34 @@ func appendNextSteps(nextSteps []string, processed []string) ([]string, bool) { // command being executed (such as 'image ls' – the root 'docker' is omitted) // and, if the configuration includes a hook for the invoked command, returns // the configured hook string. -func pluginMatch(pluginCfg map[string]string, subCmd string) (string, bool) { - configuredPluginHooks, ok := pluginCfg["hooks"] - if !ok || configuredPluginHooks == "" { +// +// Plugins can declare two types of hooks in their configuration: +// - "hooks": fires on every command invocation (success or failure) +// - "error-hooks": fires only when a command fails (cmdErrorMessage is non-empty) +func pluginMatch(pluginCfg map[string]string, subCmd string, cmdErrorMessage string) (string, bool) { + // Check "hooks" first — these always fire regardless of command outcome. + if match, ok := matchHookConfig(pluginCfg["hooks"], subCmd); ok { + return match, true + } + + // Check "error-hooks" — these only fire when there was an error. + if cmdErrorMessage != "" { + if match, ok := matchHookConfig(pluginCfg["error-hooks"], subCmd); ok { + return match, true + } + } + + return "", false +} + +// matchHookConfig checks if a comma-separated hook configuration string +// contains a prefix match for the given subcommand. +func matchHookConfig(configuredHooks string, subCmd string) (string, bool) { + if configuredHooks == "" { return "", false } - for hookCmd := range strings.SplitSeq(configuredPluginHooks, ",") { + for hookCmd := range strings.SplitSeq(configuredHooks, ",") { if hookMatch(hookCmd, subCmd) { return hookCmd, true } diff --git a/cli-plugins/manager/hooks_test.go b/cli-plugins/manager/hooks_test.go index 3060a2e9f112..e39a1383118f 100644 --- a/cli-plugins/manager/hooks_test.go +++ b/cli-plugins/manager/hooks_test.go @@ -1,12 +1,23 @@ package manager import ( + "context" "testing" + "github.com/docker/cli/cli/config/configfile" + "github.com/spf13/cobra" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) +type fakeConfigProvider struct { + cfg *configfile.ConfigFile +} + +func (f *fakeConfigProvider) ConfigFile() *configfile.ConfigFile { + return f.cfg +} + func TestGetNaiveFlags(t *testing.T) { testCases := []struct { args []string @@ -40,12 +51,15 @@ func TestGetNaiveFlags(t *testing.T) { func TestPluginMatch(t *testing.T) { testCases := []struct { - commandString string - pluginConfig map[string]string - expectedMatch string - expectedOk bool + doc string + commandString string + pluginConfig map[string]string + cmdErrorMessage string + expectedMatch string + expectedOk bool }{ { + doc: "hooks prefix match", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "image", @@ -54,6 +68,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks no match", commandString: "context ls", pluginConfig: map[string]string{ "hooks": "build", @@ -62,6 +77,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks exact match", commandString: "context ls", pluginConfig: map[string]string{ "hooks": "context ls", @@ -70,6 +86,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks first match wins", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "image ls,image", @@ -78,6 +95,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: true, }, { + doc: "hooks empty string", commandString: "image ls", pluginConfig: map[string]string{ "hooks": "", @@ -86,6 +104,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks partial token no match", commandString: "image inspect", pluginConfig: map[string]string{ "hooks": "image i", @@ -94,6 +113,7 @@ func TestPluginMatch(t *testing.T) { expectedOk: false, }, { + doc: "hooks prefix token match", commandString: "image inspect", pluginConfig: map[string]string{ "hooks": "image", @@ -101,12 +121,140 @@ func TestPluginMatch(t *testing.T) { expectedMatch: "image", expectedOk: true, }, + { + doc: "error-hooks match on error", + commandString: "build", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "error-hooks no match on success", + commandString: "build", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "error-hooks prefix match on error", + commandString: "compose up", + pluginConfig: map[string]string{ + "error-hooks": "compose", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "compose", + expectedOk: true, + }, + { + doc: "error-hooks no match for wrong command", + commandString: "pull", + pluginConfig: map[string]string{ + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "hooks takes precedence over error-hooks", + commandString: "build", + pluginConfig: map[string]string{ + "hooks": "build", + "error-hooks": "build", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "hooks fires on success even with error-hooks configured", + commandString: "build", + pluginConfig: map[string]string{ + "hooks": "build", + "error-hooks": "build", + }, + cmdErrorMessage: "", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "error-hooks with multiple commands", + commandString: "compose up", + pluginConfig: map[string]string{ + "error-hooks": "build,compose up,pull", + }, + cmdErrorMessage: "exit status 1", + expectedMatch: "compose up", + expectedOk: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.doc, func(t *testing.T) { + match, ok := pluginMatch(tc.pluginConfig, tc.commandString, tc.cmdErrorMessage) + assert.Equal(t, ok, tc.expectedOk) + assert.Equal(t, match, tc.expectedMatch) + }) + } +} + +func TestMatchHookConfig(t *testing.T) { + testCases := []struct { + doc string + configuredHooks string + subCmd string + expectedMatch string + expectedOk bool + }{ + { + doc: "empty config", + configuredHooks: "", + subCmd: "build", + expectedMatch: "", + expectedOk: false, + }, + { + doc: "exact match", + configuredHooks: "build", + subCmd: "build", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "prefix match", + configuredHooks: "image", + subCmd: "image ls", + expectedMatch: "image", + expectedOk: true, + }, + { + doc: "comma-separated match", + configuredHooks: "pull,build,push", + subCmd: "build", + expectedMatch: "build", + expectedOk: true, + }, + { + doc: "no match", + configuredHooks: "pull,push", + subCmd: "build", + expectedMatch: "", + expectedOk: false, + }, } for _, tc := range testCases { - match, ok := pluginMatch(tc.pluginConfig, tc.commandString) - assert.Equal(t, ok, tc.expectedOk) - assert.Equal(t, match, tc.expectedMatch) + t.Run(tc.doc, func(t *testing.T) { + match, ok := matchHookConfig(tc.configuredHooks, tc.subCmd) + assert.Equal(t, ok, tc.expectedOk) + assert.Equal(t, match, tc.expectedMatch) + }) } } @@ -141,3 +289,87 @@ func TestAppendNextSteps(t *testing.T) { }) } } + +func TestRunPluginHooksPassesErrorMessage(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "test-plugin": {"hooks": "build"}, + } + provider := &fakeConfigProvider{cfg: cfg} + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // Should not panic with empty error message (success case) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "") + + // Should not panic with non-empty error message (failure case) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1") +} + +func TestRunPluginHooksErrorHooks(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "test-plugin": {"error-hooks": "build"}, + } + provider := &fakeConfigProvider{cfg: cfg} + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // Should not panic — error-hooks with error message + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "exit status 1") + + // Should not panic — error-hooks with no error (should be skipped) + RunPluginHooks(context.Background(), provider, root, sub, []string{"build"}, "") +} + +func TestInvokeAndCollectHooksErrorHooksSkippedOnSuccess(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "nonexistent": {"error-hooks": "build"}, + } + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + // On success, error-hooks should not match, so the plugin + // binary is never looked up and no results are returned. + result := invokeAndCollectHooks( + context.Background(), cfg, root, sub, + "build", map[string]string{}, "", + ) + assert.Check(t, is.Len(result, 0)) +} + +func TestInvokeAndCollectHooksNoPlugins(t *testing.T) { + cfg := configfile.New("") + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + result := invokeAndCollectHooks( + context.Background(), cfg, root, sub, + "build", map[string]string{}, "some error", + ) + assert.Check(t, is.Len(result, 0)) +} + +func TestInvokeAndCollectHooksCancelledContext(t *testing.T) { + cfg := configfile.New("") + cfg.Plugins = map[string]map[string]string{ + "test-plugin": {"hooks": "build"}, + } + root := &cobra.Command{Use: "docker"} + sub := &cobra.Command{Use: "build"} + root.AddCommand(sub) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + result := invokeAndCollectHooks( + ctx, cfg, root, sub, + "build", map[string]string{}, "exit status 1", + ) + assert.Check(t, is.Nil(result)) +} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index f09b6d74a745..4936f2b28e48 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -119,6 +119,20 @@ func getExitCode(err error) int { return 1 } +// cmdErrorMessage extracts an error message suitable for passing to plugin +// hooks. If the error's message is empty (e.g. a StatusError with only an +// exit code), it falls back to a generic message so that hooks can still +// detect that the command failed. +func cmdErrorMessage(err error) string { + if err == nil { + return "" + } + if msg := err.Error(); msg != "" { + return msg + } + return fmt.Sprintf("exited with code %d", getExitCode(err)) +} + func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { var ( opts *cliflags.ClientOptions @@ -480,10 +494,11 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { subCommand = ccmd if err != nil || pluginmanager.IsPluginCommand(ccmd) { err := tryPluginRun(ctx, dockerCli, cmd, args[0], envs) + if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() && !errdefs.IsNotFound(err) { + errMessage := cmdErrorMessage(err) + pluginmanager.RunPluginHooks(ctx, dockerCli, cmd, ccmd, args, errMessage) + } if err == nil { - if ccmd != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { - pluginmanager.RunPluginHooks(ctx, dockerCli, cmd, ccmd, args) - } return nil } if !errdefs.IsNotFound(err) { @@ -507,11 +522,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { // If the command is being executed in an interactive terminal // and hook are enabled, run the plugin hooks. if subCommand != nil && dockerCli.Out().IsTerminal() && dockerCli.HooksEnabled() { - var errMessage string - if err != nil { - errMessage = err.Error() - } - pluginmanager.RunCLICommandHooks(ctx, dockerCli, cmd, subCommand, errMessage) + pluginmanager.RunCLICommandHooks(ctx, dockerCli, cmd, subCommand, cmdErrorMessage(err)) } return err diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index d3cb536be1c5..171047e4be45 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "errors" + "fmt" "io" "os" "syscall" "testing" "time" + dockercli "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/commands" "github.com/docker/cli/cli/debug" @@ -126,6 +128,41 @@ func TestUserTerminatedError(t *testing.T) { assert.Equal(t, getExitCode(context.Cause(notifyCtx)), 143) } +func TestGetExitCode(t *testing.T) { + t.Run("nil error returns 0", func(t *testing.T) { + assert.Equal(t, getExitCode(nil), 0) + }) + + t.Run("generic error returns 1", func(t *testing.T) { + assert.Equal(t, getExitCode(errors.New("some failure")), 1) + }) + + t.Run("StatusError with code", func(t *testing.T) { + err := dockercli.StatusError{StatusCode: 42} + assert.Equal(t, getExitCode(err), 42) + }) + + t.Run("StatusError with zero code falls back to 1", func(t *testing.T) { + err := dockercli.StatusError{StatusCode: 0, Status: "something went wrong"} + assert.Equal(t, getExitCode(err), 1) + }) + + t.Run("wrapped StatusError", func(t *testing.T) { + err := fmt.Errorf("wrapper: %w", dockercli.StatusError{StatusCode: 99}) + assert.Equal(t, getExitCode(err), 99) + }) + + t.Run("SIGINT returns 130", func(t *testing.T) { + err := errCtxSignalTerminated{signal: syscall.SIGINT} + assert.Equal(t, getExitCode(err), 130) + }) + + t.Run("SIGTERM returns 143", func(t *testing.T) { + err := errCtxSignalTerminated{signal: syscall.SIGTERM} + assert.Equal(t, getExitCode(err), 143) + }) +} + func TestVisitAll(t *testing.T) { root := &cobra.Command{Use: "root"} sub1 := &cobra.Command{Use: "sub1"}