Vault 36295 Improve plugin mgmt ux in api and cli (#30811)

* cli: only set default command parameter to plugin name if sha256 is provided

* api: write warnings to RegisterPluginResponse, propagate up to cli

* api: filter out 'Endpoint replaced the value of these parameters' warning before returning in RegisterPluginWithContext

* docs

* add TODO on filtering that links to api type parameter deprecation ticket

* fix tests

* allocate filteredWarning slice only if there are warnings

* improve deferred resp close and early error return conditionals in RegisterPluginWithContext

* refer to sha256 as cli option -sha256 in command cli usage

* break up ui error lines for sha256 and version flag check

* consolidate if statements for sha256 and command, oci_image check in cli

* consolidate if statements for sha256 and command, oci_image check in api

* new RegisterPluginV2 and RegisterPluginWithContextV2 api client functions for backward compatibility

* add changelog

* more descriptive changelog

* rename RegisterPluginV2 to RegisterPluginDetailed and RegisterPluginWithContextV2 to RegisterPluginWithContextDetailed

* return nil, nil if no warnings to preserve status code

* fix eof from decoding (check if no content before decoding)

* doc for RegisterPluginResponse

* only validate plugin.Command in plugin catalog set for downloaded and binary plugins, which rely on plugin.Command input; extracted artifact plugins don't rely on plugin.Command input

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

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

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

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

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

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/docs/commands/plugin/register.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/docs/commands/plugin/register.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/docs/commands/plugin/register.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/docs/commands/plugin/register.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* move up enterprise note on plugin register command doc

* [DOCS] Editorial suggestions for PR #30811 (#31111)

* suggestions

* move common reqs to a partial

* fix typo

* tweak reqs

* Update website/content/partials/plugins/prepare-plugin.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* Update website/content/partials/plugins/prepare-plugin.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* Update website/content/partials/plugins/prepare-plugin.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* tweak feedback

* remove deprecation

* Update website/content/partials/plugins/common-requirements.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* save

* Update website/content/docs/plugins/rollback.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* Update website/content/docs/plugins/upgrade.mdx

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

* fix formatting

---------

Co-authored-by: helenfufu <25168806+helenfufu@users.noreply.github.com>

---------

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

Author: helenfufu

api/sys_plugins.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/mitchellh/mapstructure"
@@ -220,12 +221,19 @@ type RegisterPluginInput struct {
 	Download bool `json:"download,omitempty"`
 }
 
+// RegisterPluginResponse is the response from the RegisterPluginDetailed call.
+type RegisterPluginResponse struct {
+	Warnings []string `json:"warnings"`
+}
+
 // RegisterPlugin wraps RegisterPluginWithContext using context.Background.
+// Deprecated: Use RegisterPluginDetailed instead.
 func (c *Sys) RegisterPlugin(i *RegisterPluginInput) error {
 	return c.RegisterPluginWithContext(context.Background(), i)
 }
 
 // RegisterPluginWithContext registers the plugin with the given information.
+// Deprecated: Use RegisterPluginWithContextDetailed instead.
 func (c *Sys) RegisterPluginWithContext(ctx context.Context, i *RegisterPluginInput) error {
 	ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
 	defer cancelFunc()
@@ -244,6 +252,58 @@ func (c *Sys) RegisterPluginWithContext(ctx context.Context, i *RegisterPluginIn
 	return err
 }
 
+// RegisterPluginDetailed wraps RegisterPluginWtihContextDetailed using context.Background.
+func (c *Sys) RegisterPluginDetailed(i *RegisterPluginInput) (*RegisterPluginResponse, error) {
+	return c.RegisterPluginWithContextDetailed(context.Background(), i)
+}
+
+// RegisterPluginWithContextDetailed registers the plugin with the given information.
+func (c *Sys) RegisterPluginWithContextDetailed(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 nil, err
+	}
+
+	resp, err := c.c.rawRequestWithContext(ctx, req)
+	if resp != nil {
+		defer resp.Body.Close()
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	var registerResp RegisterPluginResponse
+	if resp != nil && resp.StatusCode != http.StatusNoContent {
+		if err := resp.DecodeJSON(&registerResp); 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.
+	var filteredWarnings []string
+	if len(registerResp.Warnings) > 0 {
+		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.
 type DeregisterPluginInput struct {
 	// Name is the name of the plugin. Required.

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().RegisterPluginWithContextDetailed(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) {

changelog/30811.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+plugins: Clarify usage of sha256, command, and version for plugin registration of binary or artifact with API and CLI. Introduce new RegisterPluginDetailed and RegisterPluginWtihContextDetailed functions to API client to propagate response along with error, and mark RegisterPlugin and RegisterPluginWithContext as deprecated.
+```

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().RegisterPluginDetailed(&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,9 @@ 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 +184,11 @@ func (c *PluginRegisterCommand) Run(args []string) int {
 	pluginName := strings.TrimSpace(pluginNameRaw)
 
 	command := c.flagCommand
-	if command == "" && c.flagOCIImage == "" {
+	if c.flagSHA256 != "" && (command == "" && c.flagOCIImage == "") {
 		command = pluginName
 	}
 
-	if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
+	resp, err := client.Sys().RegisterPluginDetailed(&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().RegisterPluginDetailed(&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().RegisterPluginDetailed(&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().RegisterPluginDetailed(&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().RegisterPluginDetailed(&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,7 +553,11 @@ func (b *SystemBackend) handlePluginCatalogUpdate(ctx context.Context, _ *logica
 
 	command := d.Get("command").(string)
 	ociImage := d.Get("oci_image").(string)
-	if command == "" && ociImage == "" {
+	var resp logical.Response
+
+	if sha256 == "" && 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 sha256 != "" && (command == "" && ociImage == "") {
 		return logical.ErrorResponse("must provide at least one of command or oci_image"), nil
 	}
 
@@ -619,7 +623,11 @@ func (b *SystemBackend) handlePluginCatalogUpdate(ctx context.Context, _ *logica
 		return nil, err
 	}
 
-	return nil, nil
+	if len(resp.Warnings) == 0 {
+		return nil, nil
+	}
+
+	return &resp, nil
 }
 
 func (b *SystemBackend) handlePluginCatalogRead(ctx context.Context, _ *logical.Request, d *framework.FieldData) (*logical.Response, error) {