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

@@ -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.
+```

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) {
+	positionalArgsCount := len(args)
-	case 0:
+	switch positionalArgsCount {
-		c.UI.Error("Not enough arguments (expected 1, or 2, got 0)")
+	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
 }

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()
 

command/plugin_info.go

@@ -77,22 +77,18 @@ func (c *PluginInfoCommand) Run(args []string) int {
 
 	var pluginNameRaw, pluginTypeRaw string
 	args = f.Args()
+	positionalArgsCount := len(args)
 	switch {
-	case len(args) < 1:
+	case positionalArgsCount < 2:
-		c.UI.Error(fmt.Sprintf("Not enough arguments (expected 1 or 2, got %d)", len(args)))
+		c.UI.Error(fmt.Sprintf("Not enough arguments (expected 2, got %d)", positionalArgsCount))
 		return 1
-	case len(args) > 2:
+	case positionalArgsCount > 2:
-		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
+	}
 
-	// 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]
-	}
 
 	client, err := c.Client()
 	if err != nil {

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"},

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))

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...)