Vault 36295 Improve plugin mgmt ux in api and cli

api/sys_plugins.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/mitchellh/mapstructure"
@@ -217,28 +218,54 @@ type RegisterPluginInput struct {
 	Env []string `json:"env,omitempty"`
 }
 
+type RegisterPluginResponse struct {
+	Warnings []string `json:"warnings"`
+}
+
 // RegisterPlugin wraps RegisterPluginWithContext using context.Background.
-func (c *Sys) RegisterPlugin(i *RegisterPluginInput) error {
+func (c *Sys) RegisterPlugin(i *RegisterPluginInput) (*RegisterPluginResponse, error) {
 	return c.RegisterPluginWithContext(context.Background(), i)
 }
 
 // RegisterPluginWithContext registers the plugin with the given information.
-func (c *Sys) RegisterPluginWithContext(ctx context.Context, i *RegisterPluginInput) error {
+func (c *Sys) RegisterPluginWithContext(ctx context.Context, i *RegisterPluginInput) (*RegisterPluginResponse, error) {
 	ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
 	defer cancelFunc()
 
 	path := catalogPathByType(i.Type, i.Name)
 	req := c.c.NewRequest(http.MethodPut, path)
 
 	if err := req.SetJSONBody(i); err != nil {
-		return err
+		return nil, err
 	}
 
 	resp, err := c.c.rawRequestWithContext(ctx, req)
 	if err == nil {
 		defer resp.Body.Close()
+	} else {
+		return nil, err
 	}
-	return err
+
+	var registerResp RegisterPluginResponse
+	err = resp.DecodeJSON(&registerResp)
+	if err != nil {
+		return nil, err
+	}
+
+	// Filter out the `Endpoint replaced the value of these parameters with the values captured from the endpoint's path: [type]`
+	// warning because it is expected behavior from this function, as we set the type parameter in both the path and request body,
+	// and the warning informs us the path parameter takes precedence. However, this warning is not relevant for an end user so we
+	// omit it before returning to any client.
+	// TODO: This can likely be removed once https://hashicorp.atlassian.net/browse/VAULT-36722 is addressed.
+	filteredWarnings := make([]string, 0, len(registerResp.Warnings))
+	for _, warning := range registerResp.Warnings {
+		if !strings.Contains(warning, "Endpoint replaced the value of these parameters with the values captured from the endpoint's path") {
+			filteredWarnings = append(filteredWarnings, warning)
+		}
+	}
+	registerResp.Warnings = filteredWarnings
+
+	return &registerResp, err
 }
 
 // DeregisterPluginInput is used as input to the DeregisterPlugin function.

api/sys_plugins_test.go

@@ -24,12 +24,15 @@ func TestRegisterPlugin(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = client.Sys().RegisterPluginWithContext(context.Background(), &RegisterPluginInput{
+	resp, err := client.Sys().RegisterPluginWithContext(context.Background(), &RegisterPluginInput{
 		Version: "v1.0.0",
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
+	if len(resp.Warnings) > 0 {
+		t.Errorf("expected no warnings, got: %v", resp.Warnings)
+	}
 }
 
 func TestListPlugins(t *testing.T) {

command/plugin_deregister_test.go

@@ -91,14 +91,18 @@ func TestPluginDeregisterCommand_Run(t *testing.T) {
 		ui, cmd := testPluginDeregisterCommand(t)
 		cmd.client = client
 
-		if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+		registerResp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 			Name:    pluginName,
 			Type:    api.PluginTypeCredential,
 			Command: pluginName,
 			SHA256:  sha256Sum,
-		}); err != nil {
+		})
+		if err != nil {
 			t.Fatal(err)
 		}
+		if len(registerResp.Warnings) > 0 {
+			t.Errorf("expected no warnings, got %q", registerResp.Warnings)
+		}
 
 		code := cmd.Run([]string{
 			consts.PluginTypeCredential.String(),
@@ -114,23 +118,23 @@ func TestPluginDeregisterCommand_Run(t *testing.T) {
 			t.Errorf("expected %q to contain %q", combined, expected)
 		}
 
-		resp, err := client.Sys().ListPlugins(&api.ListPluginsInput{
+		listResp, err := client.Sys().ListPlugins(&api.ListPluginsInput{
 			Type: api.PluginTypeCredential,
 		})
 		if err != nil {
 			t.Fatal(err)
 		}
 
 		found := false
-		for _, plugins := range resp.PluginsByType {
+		for _, plugins := range listResp.PluginsByType {
 			for _, p := range plugins {
 				if p == pluginName {
 					found = true
 				}
 			}
 		}
 		if found {
-			t.Errorf("expected %q to not be in %q", pluginName, resp.PluginsByType)
+			t.Errorf("expected %q to not be in %q", pluginName, listResp.PluginsByType)
 		}
 	})
 

command/plugin_register.go

@@ -81,22 +81,27 @@ func (c *PluginRegisterCommand) Flags() *FlagSets {
 		Name:       "command",
 		Target:     &c.flagCommand,
 		Completion: complete.PredictAnything,
-		Usage: "Command to spawn the plugin. This defaults to the name of the " +
-			"plugin if both oci_image and command are unspecified.",
+		Usage: "Command to spawn the plugin. If sha256 is provided to register with a plugin binary, " +
+			"this defaults to the name of the plugin if both oci_image and command are unspecified. " +
+			"Otherwise, if sha256 is not provided, a plugin artifact is expected for registration, and " +
+			"this will be ignored because the run command is known.",
 	})
 
 	f.StringVar(&StringVar{
 		Name:       "sha256",
 		Target:     &c.flagSHA256,
 		Completion: complete.PredictAnything,
-		Usage:      "SHA256 of the plugin binary or the oci_image provided. This is required for all plugins.",
+		Usage: "SHA256 of the plugin binary or the OCI image provided. " +
+			"This is required to register with a plugin binary but should not be " +
+			"specified when registering with a plugin artifact.",
 	})
 
 	f.StringVar(&StringVar{
 		Name:       "version",
 		Target:     &c.flagVersion,
 		Completion: complete.PredictAnything,
-		Usage:      "Semantic version of the plugin. Used as the tag when specifying oci_image, but with any leading 'v' trimmed. Optional.",
+		Usage: "Semantic version of the plugin. Used as the tag when specifying oci_image, but with any leading 'v' trimmed. " +
+			"This is required to register with a plugin artifact but optional when registering with a plugin binary.",
 	})
 
 	f.StringVar(&StringVar{
@@ -151,7 +156,7 @@ func (c *PluginRegisterCommand) Run(args []string) int {
 		c.UI.Error(fmt.Sprintf("Too many arguments (expected 1 or 2, got %d)", len(args)))
 		return 1
 	case c.flagSHA256 == "" && c.flagVersion == "":
-		c.UI.Error("One of -sha256 or -version is required. If registering with binary, please provide at least -sha256 (-version optional). If registering with extracted artifact directory, please provide -version only.")
+		c.UI.Error("One of -sha256 or -version is required. If registering with a binary, please provide at least -sha256 (-version optional). If registering with an artifact, please provide -version only.")
 		return 1
 
 	// These cases should come after invalid cases have been checked
@@ -177,11 +182,13 @@ func (c *PluginRegisterCommand) Run(args []string) int {
 	pluginName := strings.TrimSpace(pluginNameRaw)
 
 	command := c.flagCommand
-	if command == "" && c.flagOCIImage == "" {
-		command = pluginName
+	if c.flagSHA256 != "" {
+		if command == "" && c.flagOCIImage == "" {
+			command = pluginName
+		}
 	}
 
-	if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+	resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 		Name:     pluginName,
 		Type:     pluginType,
 		Args:     c.flagArgs,
@@ -191,11 +198,20 @@ func (c *PluginRegisterCommand) Run(args []string) int {
 		OCIImage: c.flagOCIImage,
 		Runtime:  c.flagRuntime,
 		Env:      c.flagEnv,
-	}); err != nil {
+	})
+	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error registering plugin %s: %s", pluginName, err))
 		return 2
 	}
 
+	if resp != nil && len(resp.Warnings) > 0 {
+		c.UI.Warn(wrapAtLength(fmt.Sprintf(
+			"Warnings while registering plugin %s: %s",
+			pluginName,
+			strings.Join(resp.Warnings, "\n\n"),
+		)) + "\n")
+	}
+
 	c.UI.Output(fmt.Sprintf("Success! Registered plugin: %s", pluginName))
 	return 0
 }

command/plugin_reload_test.go

@@ -108,14 +108,18 @@ func TestPluginReloadCommand_Run(t *testing.T) {
 		ui, cmd := testPluginReloadCommand(t)
 		cmd.client = client
 
-		if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+		resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 			Name:    pluginName,
 			Type:    api.PluginTypeCredential,
 			Command: pluginName,
 			SHA256:  sha256Sum,
-		}); err != nil {
+		})
+		if err != nil {
 			t.Fatal(err)
 		}
+		if len(resp.Warnings) > 0 {
+			t.Errorf("expected no warnings, got: %v", resp.Warnings)
+		}
 
 		code := cmd.Run([]string{
 			"-plugin", pluginName,

command/plugin_test.go

@@ -45,15 +45,19 @@ func testPluginCreateAndRegister(tb testing.TB, client *api.Client, dir, name st
 
 	pth, sha256Sum := testPluginCreate(tb, dir, name)
 
-	if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+	resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 		Name:    name,
 		Type:    pluginType,
 		Command: name,
 		SHA256:  sha256Sum,
 		Version: version,
-	}); err != nil {
+	})
+	if err != nil {
 		tb.Fatal(err)
 	}
+	if len(resp.Warnings) > 0 {
+		tb.Errorf("expected no warnings, got: %v", resp.Warnings)
+	}
 
 	return pth, sha256Sum
 }
@@ -64,15 +68,19 @@ func testPluginCreateAndRegisterVersioned(tb testing.TB, client *api.Client, dir
 
 	pth, sha256Sum := testPluginCreate(tb, dir, name)
 
-	if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+	resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 		Name:    name,
 		Type:    pluginType,
 		Command: name,
 		SHA256:  sha256Sum,
 		Version: "v1.0.0",
-	}); err != nil {
+	})
+	if err != nil {
 		tb.Fatal(err)
 	}
+	if len(resp.Warnings) > 0 {
+		tb.Errorf("expected no warnings, got: %v", resp.Warnings)
+	}
 
 	return pth, sha256Sum, "v1.0.0"
 }

command/util.go

@@ -4,6 +4,7 @@
 package command
 
 import (
+	"bytes"
 	"fmt"
 	"io"
 	"net/http"
@@ -193,6 +194,7 @@ func (r *recordingRoundTripper) RoundTrip(req *http.Request) (*http.Response, er
 	r.body = body
 	return &http.Response{
 		StatusCode: 200,
+		Body:       io.NopCloser(bytes.NewReader([]byte(`{"warnings": []}`))),
 	}, nil
 }
 

vault/external_tests/plugin/external_plugin_test.go

@@ -106,15 +106,19 @@ func TestExternalPlugin_RollbackAndReload(t *testing.T) {
 
 func testRegisterVersion(t *testing.T, client *api.Client, plugin pluginhelpers.TestPlugin, version string) {
 	t.Helper()
-	if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+	resp, err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
 		Name:    plugin.Name,
 		Type:    api.PluginType(plugin.Typ),
 		Command: plugin.Name,
 		SHA256:  plugin.Sha256,
 		Version: version,
-	}); err != nil {
+	})
+	if err != nil {
 		t.Fatal(err)
 	}
+	if len(resp.Warnings) > 0 {
+		t.Errorf("expected no warnings, got: %v", resp.Warnings)
+	}
 }
 
 func testEnableVersion(t *testing.T, client *api.Client, plugin pluginhelpers.TestPlugin, version string) {

vault/logical_system.go

@@ -553,8 +553,16 @@ func (b *SystemBackend) handlePluginCatalogUpdate(ctx context.Context, _ *logica
 
 	command := d.Get("command").(string)
 	ociImage := d.Get("oci_image").(string)
-	if command == "" && ociImage == "" {
-		return logical.ErrorResponse("must provide at least one of command or oci_image"), nil
+	var resp logical.Response
+
+	if sha256 == "" {
+		if command != "" {
+			resp.AddWarning(fmt.Sprintf("When sha256 is unspecified, a plugin artifact is expected for registration and the command parameter %q will be ignored.", command))
+		}
+	} else {
+		if command == "" && ociImage == "" {
+			return logical.ErrorResponse("must provide at least one of command or oci_image"), nil
+		}
 	}
 
 	if ociImage == "" {
@@ -619,7 +627,7 @@ func (b *SystemBackend) handlePluginCatalogUpdate(ctx context.Context, _ *logica
 		return nil, err
 	}
 
-	return nil, nil
+	return &resp, nil
 }
 
 func (b *SystemBackend) handlePluginCatalogRead(ctx context.Context, _ *logical.Request, d *framework.FieldData) (*logical.Response, error) {

website/content/api-docs/system/plugins-catalog.mdx

@@ -156,17 +156,21 @@ supplied name.
   See [/sys/plugins/runtimes/catalog](/vault/api-docs/system/plugins-runtimes-catalog) for additional information.
 
 - `version` `(string: "")` - Specifies the semantic version of the plugin. Used as the tag
-  when specifying `oci_image`, but with any leading 'v' trimmed.
+  when specifying `oci_image`, but with any leading 'v' trimmed. This is required to register
+  with a plugin artifact but optional when registering with a plugin binary.
 
-- `sha256` `(string: <required>)` – The SHA256 sum of a Community plugin
-  binary or the OCI image. Before a plugin is run, its SHA will be checked against this value.
-  If the actual SHA of the plugin binary and the SHA provided in `sha256` do not match, Vault will not run the plugin. The `sha256` parameter is only required for Community plugins. Enterprise plugins do not require SHA confirmation.
+- `sha256` `(string: "")` – The SHA256 sum of a plugin binary or the OCI image.
+  Before a plugin is run, its SHA will be checked against this value. If the actual SHA of the 
+  plugin binary and the SHA provided in `sha256` do not match, Vault will not run the plugin.
+  This is required to register with a plugin binary but should not be specified when registering
+  with a plugin artifact.
 
-- `command` `(string: <required>)` - Specifies the command used to execute the
+- `command` `(string: "")` - Specifies the command used to execute the
   plugin. This is relative to the plugin directory. e.g. `"myplugin"`, or if `oci_image`
   is also specified, it is relative to the image's working directory.
-  The `command` parameter is only required for Community plugins as
-  the run command is known for Enterprise plugins.
+  This is required to register with a plugin binary but should not be specified when
+  registering with a plugin artifact; if it is specified for an artifact, it will be
+  ignored as the run command is known.
 
 - `args` `(array: [])` – Specifies the arguments used to execute the plugin. If
   the arguments are provided here, the `command` parameter should only contain