From 51d99fc7cfeafe7dd95e6c1c633d81c1f05c42f3 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 28 Nov 2023 14:13:26 +0000 Subject: [PATCH] cli: Improve error handling for plugin commands (#24250) * Stop supporting vault plugin info and deregister without a type argument * Make a best-effort attempt to report whether a plugin was actually deregistered and give more descriptive errors * Fix error message for vault plugin reload --- changelog/24250.txt | 6 ++++ command/plugin_deregister.go | 48 +++++++++++++++++++++++++------ command/plugin_deregister_test.go | 31 +++++++++++++++++--- command/plugin_info.go | 20 ++++++------- command/plugin_info_test.go | 6 ++++ command/plugin_reload.go | 8 ++++-- command/plugin_reload_test.go | 6 ++-- 7 files changed, 95 insertions(+), 30 deletions(-) create mode 100644 changelog/24250.txt diff --git a/changelog/24250.txt b/changelog/24250.txt new file mode 100644 index 0000000000..e6aca7096a --- /dev/null +++ b/changelog/24250.txt @@ -0,0 +1,6 @@ +```release-note:change +cli: `vault plugin info` and `vault plugin deregister` now require 2 positional arguments instead of accepting either 1 or 2. +``` +```release-note:improvement +cli: Improved error messages for `vault plugin` sub-commands. +``` diff --git a/command/plugin_deregister.go b/command/plugin_deregister.go index 4b9de504db..5355a95157 100644 --- a/command/plugin_deregister.go +++ b/command/plugin_deregister.go @@ -4,7 +4,9 @@ package command import ( + "context" "fmt" + "net/http" "strings" semver "github.com/hashicorp/go-version" @@ -83,18 +85,16 @@ func (c *PluginDeregisterCommand) Run(args []string) int { var pluginNameRaw, pluginTypeRaw string args = f.Args() - switch len(args) { - case 0: - c.UI.Error("Not enough arguments (expected 1, or 2, got 0)") + positionalArgsCount := len(args) + switch positionalArgsCount { + case 0, 1: + c.UI.Error(fmt.Sprintf("Not enough arguments (expected 2, got %d)", positionalArgsCount)) return 1 - case 1: - pluginTypeRaw = "unknown" - pluginNameRaw = args[0] case 2: pluginTypeRaw = args[0] pluginNameRaw = args[1] default: - c.UI.Error(fmt.Sprintf("Too many arguments (expected 1, or 2, got %d)", len(args))) + c.UI.Error(fmt.Sprintf("Too many arguments (expected 2, got %d)", positionalArgsCount)) return 1 } @@ -118,7 +118,33 @@ func (c *PluginDeregisterCommand) Run(args []string) int { } } - if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{ + // The deregister endpoint returns 200 if the plugin doesn't exist, so first + // try fetching the plugin to help improve info printed to the user. + // 404 => Return early with a descriptive message. + // Other error => Continue attempting to deregister the plugin anyway. + // Plugin exists but is builtin => Error early. + // Otherwise => If deregister succeeds, we can report that the plugin really + // was deregistered (and not just already absent). + var pluginExists bool + if info, err := client.Sys().GetPluginWithContext(context.Background(), &api.GetPluginInput{ + Name: pluginName, + Type: pluginType, + Version: c.flagPluginVersion, + }); err != nil { + if respErr, ok := err.(*api.ResponseError); ok && respErr.StatusCode == http.StatusNotFound { + c.UI.Output(fmt.Sprintf("Plugin %q (type: %q, version %q) does not exist in the catalog", pluginName, pluginType, c.flagPluginVersion)) + return 0 + } + // Best-effort check, continue trying to deregister. + } else if info != nil { + if info.Builtin { + c.UI.Error(fmt.Sprintf("Plugin %q (type: %q) is a builtin plugin and cannot be deregistered", pluginName, pluginType)) + return 2 + } + pluginExists = true + } + + if err := client.Sys().DeregisterPluginWithContext(context.Background(), &api.DeregisterPluginInput{ Name: pluginName, Type: pluginType, Version: c.flagPluginVersion, @@ -127,6 +153,10 @@ func (c *PluginDeregisterCommand) Run(args []string) int { return 2 } - c.UI.Output(fmt.Sprintf("Success! Deregistered plugin (if it was registered): %s", pluginName)) + if pluginExists { + c.UI.Output(fmt.Sprintf("Success! Deregistered %s plugin: %s", pluginType, pluginName)) + } else { + c.UI.Output(fmt.Sprintf("Success! Deregistered %s plugin (if it was registered): %s", pluginType, pluginName)) + } return 0 } diff --git a/command/plugin_deregister_test.go b/command/plugin_deregister_test.go index f23c8b6c43..b5bf4f7be9 100644 --- a/command/plugin_deregister_test.go +++ b/command/plugin_deregister_test.go @@ -35,7 +35,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { }{ { "not_enough_args", - nil, + []string{"foo"}, "Not enough arguments", 1, }, @@ -109,7 +109,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { t.Errorf("expected %d to be %d", code, exp) } - expected := "Success! Deregistered plugin (if it was registered): " + expected := "Success! Deregistered auth plugin: " combined := ui.OutputWriter.String() + ui.ErrorWriter.String() if !strings.Contains(combined, expected) { t.Errorf("expected %q to contain %q", combined, expected) @@ -159,7 +159,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { t.Errorf("expected %d to be %d", code, exp) } - expected := "Success! Deregistered plugin (if it was registered): " + expected := "Success! Deregistered auth plugin: " combined := ui.OutputWriter.String() + ui.ErrorWriter.String() if !strings.Contains(combined, expected) { t.Errorf("expected %q to contain %q", combined, expected) @@ -206,7 +206,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { t.Errorf("expected %d to be %d", code, exp) } - expected := "Success! Deregistered plugin (if it was registered): " + expected := "does not exist in the catalog" combined := ui.OutputWriter.String() + ui.ErrorWriter.String() if !strings.Contains(combined, expected) { t.Errorf("expected %q to contain %q", combined, expected) @@ -230,6 +230,29 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { } }) + t.Run("deregister builtin", func(t *testing.T) { + t.Parallel() + + pluginDir, cleanup := corehelpers.MakeTestPluginDir(t) + defer cleanup(t) + + client, _, closer := testVaultServerPluginDir(t, pluginDir) + defer closer() + + ui, cmd := testPluginDeregisterCommand(t) + cmd.client = client + + expected := "is a builtin plugin" + if code := cmd.Run([]string{ + consts.PluginTypeCredential.String(), + "github", + }); code != 2 { + t.Errorf("expected %d to be %d", code, 2) + } else if !strings.Contains(ui.ErrorWriter.String(), expected) { + t.Errorf("expected %q to contain %q", ui.ErrorWriter.String(), expected) + } + }) + t.Run("communication_failure", func(t *testing.T) { t.Parallel() diff --git a/command/plugin_info.go b/command/plugin_info.go index b1cf7acb04..0211555027 100644 --- a/command/plugin_info.go +++ b/command/plugin_info.go @@ -77,23 +77,19 @@ func (c *PluginInfoCommand) Run(args []string) int { var pluginNameRaw, pluginTypeRaw string args = f.Args() + positionalArgsCount := len(args) switch { - case len(args) < 1: - c.UI.Error(fmt.Sprintf("Not enough arguments (expected 1 or 2, got %d)", len(args))) + case positionalArgsCount < 2: + c.UI.Error(fmt.Sprintf("Not enough arguments (expected 2, got %d)", positionalArgsCount)) return 1 - case len(args) > 2: - c.UI.Error(fmt.Sprintf("Too many arguments (expected 1 or 2, got %d)", len(args))) + case positionalArgsCount > 2: + c.UI.Error(fmt.Sprintf("Too many arguments (expected 2, got %d)", positionalArgsCount)) return 1 - - // These cases should come after invalid cases have been checked - case len(args) == 1: - pluginTypeRaw = "unknown" - pluginNameRaw = args[0] - case len(args) == 2: - pluginTypeRaw = args[0] - pluginNameRaw = args[1] } + pluginTypeRaw = args[0] + pluginNameRaw = args[1] + client, err := c.Client() if err != nil { c.UI.Error(err.Error()) diff --git a/command/plugin_info_test.go b/command/plugin_info_test.go index 3671243016..3ffaa69720 100644 --- a/command/plugin_info_test.go +++ b/command/plugin_info_test.go @@ -34,6 +34,12 @@ func TestPluginInfoCommand_Run(t *testing.T) { out string code int }{ + { + "not_enough_args", + []string{"foo"}, + "Not enough arguments", + 1, + }, { "too_many_args", []string{"foo", "bar", "fizz"}, diff --git a/command/plugin_reload.go b/command/plugin_reload.go index 4ec7acb99c..22cd91275d 100644 --- a/command/plugin_reload.go +++ b/command/plugin_reload.go @@ -90,12 +90,16 @@ func (c *PluginReloadCommand) Run(args []string) int { return 1 } + positionalArgs := len(f.Args()) switch { + case positionalArgs != 0: + c.UI.Error(fmt.Sprintf("Too many arguments (expected 0, got %d)", positionalArgs)) + return 1 case c.plugin == "" && len(c.mounts) == 0: - c.UI.Error(fmt.Sprintf("Not enough arguments (expected 1, got %d)", len(args))) + c.UI.Error("No plugins specified, must specify exactly one of -plugin or -mounts") return 1 case c.plugin != "" && len(c.mounts) > 0: - c.UI.Error(fmt.Sprintf("Too many arguments (expected 1, got %d)", len(args))) + c.UI.Error("Must specify exactly one of -plugin or -mounts") return 1 case c.scope != "" && c.scope != "global": c.UI.Error(fmt.Sprintf("Invalid reload scope: %s", c.scope)) diff --git a/command/plugin_reload_test.go b/command/plugin_reload_test.go index cf9f2d149c..7ae082ed84 100644 --- a/command/plugin_reload_test.go +++ b/command/plugin_reload_test.go @@ -46,13 +46,13 @@ func TestPluginReloadCommand_Run(t *testing.T) { { "not_enough_args", nil, - "Not enough arguments", + "No plugins specified, must specify exactly one of -plugin or -mounts", 1, }, { "too_many_args", []string{"-plugin", "foo", "-mounts", "bar"}, - "Too many arguments", + "Must specify exactly one of -plugin or -mounts", 1, }, } @@ -147,7 +147,7 @@ func TestPluginReloadStatusCommand_Run(t *testing.T) { client, closer := testVaultServer(t) defer closer() - ui, cmd := testPluginReloadCommand(t) + ui, cmd := testPluginReloadStatusCommand(t) cmd.client = client args := append([]string{}, tc.args...)