fix plugin register cli error on unfound oci images (#24990)

---------

Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>

changelog/24990.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+cli: fixes plugin register CLI failure to error when plugin image doesn't exist
+```

vault/logical_system.go

@@ -596,9 +596,12 @@ func (b *SystemBackend) handlePluginCatalogUpdate(ctx context.Context, _ *logica
 		Sha256:   sha256Bytes,
 	})
 	if err != nil {
-		if errors.Is(err, plugincatalog.ErrPluginNotFound) || strings.HasPrefix(err.Error(), "plugin version mismatch") {
+		if errors.Is(err, plugincatalog.ErrPluginNotFound) ||
+			errors.Is(err, plugincatalog.ErrPluginVersionMismatch) ||
+			errors.Is(err, plugincatalog.ErrPluginUnableToRun) {
 			return logical.ErrorResponse(err.Error()), nil
 		}
+
 		return nil, err
 	}
 

vault/logical_system_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/helper/random"
 	"github.com/hashicorp/vault/helper/testhelpers/corehelpers"
+	"github.com/hashicorp/vault/helper/testhelpers/pluginhelpers"
 	"github.com/hashicorp/vault/helper/versions"
 	"github.com/hashicorp/vault/internalshared/configutil"
 	"github.com/hashicorp/vault/sdk/framework"
@@ -3879,6 +3880,10 @@ func TestSystemBackend_PluginCatalogPins_CRUD(t *testing.T) {
 // TestSystemBackend_PluginCatalog_ContainerCRUD tests that plugins registered
 // with oci_image set get recorded properly in the catalog.
 func TestSystemBackend_PluginCatalog_ContainerCRUD(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("Containerized plugins only supported on Linux")
+	}
+
 	sym, err := filepath.EvalSymlinks(os.TempDir())
 	if err != nil {
 		t.Fatalf("error: %v", err)
@@ -3888,18 +3893,25 @@ func TestSystemBackend_PluginCatalog_ContainerCRUD(t *testing.T) {
 	})
 	b := c.systemBackend
 
+	latestPlugin := pluginhelpers.CompilePlugin(t, consts.PluginTypeDatabase, "", c.pluginDirectory)
+	latestPlugin.Image, latestPlugin.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, latestPlugin, c.pluginDirectory)
+
+	const pluginVersion = "v1.0.0"
+	pluginV100 := pluginhelpers.CompilePlugin(t, consts.PluginTypeDatabase, pluginVersion, c.pluginDirectory)
+	pluginV100.Image, pluginV100.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, pluginV100, c.pluginDirectory)
+
 	for name, tc := range map[string]struct {
 		in, expected map[string]any
 	}{
 		"minimal": {
 			in: map[string]any{
-				"oci_image": "foo-image",
-				"sha256":    hex.EncodeToString([]byte{'1'}),
+				"oci_image": latestPlugin.Image,
+				"sha_256":   latestPlugin.ImageSha256,
 			},
 			expected: map[string]interface{}{
 				"name":      "test-plugin",
-				"oci_image": "foo-image",
-				"sha256":    "31",
+				"oci_image": latestPlugin.Image,
+				"sha256":    latestPlugin.ImageSha256,
 				"command":   "",
 				"args":      []string{},
 				"builtin":   false,
@@ -3908,21 +3920,21 @@ func TestSystemBackend_PluginCatalog_ContainerCRUD(t *testing.T) {
 		},
 		"fully specified": {
 			in: map[string]any{
-				"oci_image": "foo-image",
-				"sha256":    hex.EncodeToString([]byte{'1'}),
-				"command":   "foo-command",
+				"oci_image": pluginV100.Image,
+				"sha256":    pluginV100.ImageSha256,
+				"command":   "plugin",
 				"args":      []string{"--a=1"},
-				"version":   "v1.0.0",
+				"version":   pluginVersion,
 				"env":       []string{"X=2"},
 			},
 			expected: map[string]interface{}{
 				"name":      "test-plugin",
-				"oci_image": "foo-image",
-				"sha256":    "31",
-				"command":   "foo-command",
+				"oci_image": pluginV100.Image,
+				"sha256":    pluginV100.ImageSha256,
+				"command":   "plugin",
 				"args":      []string{"--a=1"},
 				"builtin":   false,
-				"version":   "v1.0.0",
+				"version":   pluginVersion,
 			},
 		},
 	} {
@@ -3935,15 +3947,6 @@ func TestSystemBackend_PluginCatalog_ContainerCRUD(t *testing.T) {
 
 			resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 
-			// We should get a nice error back from the API if we're not on linux, but
-			// that's all we can test on non-linux.
-			if runtime.GOOS != "linux" {
-				if err != nil || resp.Error() == nil {
-					t.Fatalf("err: %v %v", err, resp.Error())
-				}
-				return
-			}
-
 			if err != nil || resp.Error() != nil {
 				t.Fatalf("err: %v %v", err, resp.Error())
 			}
@@ -6687,7 +6690,7 @@ func TestGetSealBackendStatus(t *testing.T) {
 
 func TestSystemBackend_pluginRuntime_CannotDeleteRuntimeWithReferencingPlugins(t *testing.T) {
 	if runtime.GOOS != "linux" {
-		t.Skip("Currently plugincontainer only supports linux")
+		t.Skip("Containerized plugins only supported on Linux")
 	}
 	sym, err := filepath.EvalSymlinks(os.TempDir())
 	if err != nil {
@@ -6698,24 +6701,24 @@ func TestSystemBackend_pluginRuntime_CannotDeleteRuntimeWithReferencingPlugins(t
 	})
 	b := c.systemBackend
 
+	const runtime = "custom-runtime"
+	const ociRuntime = "runc"
 	conf := pluginruntimeutil.PluginRuntimeConfig{
-		Name:         "foo",
-		Type:         consts.PluginRuntimeTypeContainer,
-		OCIRuntime:   "some-oci-runtime",
-		CgroupParent: "/cpulimit/",
-		CPU:          1,
-		Memory:       10000,
+		Name:       runtime,
+		Type:       consts.PluginRuntimeTypeContainer,
+		OCIRuntime: ociRuntime,
 	}
 
 	// Register the plugin runtime
 	req := logical.TestRequest(t, logical.UpdateOperation, fmt.Sprintf("plugins/runtimes/catalog/%s/%s", conf.Type.String(), conf.Name))
 	req.Data = map[string]interface{}{
-		"oci_runtime":   conf.OCIRuntime,
-		"cgroup_parent": conf.CgroupParent,
-		"cpu_nanos":     conf.CPU,
-		"memory_bytes":  conf.Memory,
+		"oci_runtime": conf.OCIRuntime,
 	}
 
+	const pluginVersion = "v1.16.0"
+	plugin := pluginhelpers.CompilePlugin(t, consts.PluginTypeDatabase, pluginVersion, c.pluginDirectory)
+	plugin.Image, plugin.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, plugin, c.pluginDirectory)
+
 	resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 	if err != nil {
 		t.Fatalf("err: %v %#v", err, resp)
@@ -6726,18 +6729,17 @@ func TestSystemBackend_pluginRuntime_CannotDeleteRuntimeWithReferencingPlugins(t
 
 	// Register the plugin referencing the runtime.
 	req = logical.TestRequest(t, logical.UpdateOperation, "plugins/catalog/database/test-plugin")
-	req.Data["version"] = "v0.16.0"
-	req.Data["sha_256"] = hex.EncodeToString([]byte{'1'})
-	req.Data["command"] = ""
-	req.Data["oci_image"] = "hashicorp/vault-plugin-auth-jwt"
-	req.Data["runtime"] = "foo"
+	req.Data["version"] = pluginVersion
+	req.Data["sha_256"] = plugin.ImageSha256
+	req.Data["oci_image"] = plugin.Image
+	req.Data["runtime"] = runtime
 	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
 	if err != nil || resp.Error() != nil {
 		t.Fatalf("err: %v %v", err, resp.Error())
 	}
 
 	// Expect to fail to delete the plugin runtime
-	req = logical.TestRequest(t, logical.DeleteOperation, "plugins/runtimes/catalog/container/foo")
+	req = logical.TestRequest(t, logical.DeleteOperation, fmt.Sprintf("plugins/runtimes/catalog/container/%s", runtime))
 	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
 	if resp == nil || !resp.IsError() || resp.Error() == nil {
 		t.Errorf("expected logical error but got none, resp: %#v", resp)
@@ -6748,14 +6750,14 @@ func TestSystemBackend_pluginRuntime_CannotDeleteRuntimeWithReferencingPlugins(t
 
 	// Delete the plugin.
 	req = logical.TestRequest(t, logical.DeleteOperation, "plugins/catalog/database/test-plugin")
-	req.Data["version"] = "v0.16.0"
+	req.Data["version"] = pluginVersion
 	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
 	if err != nil || resp.Error() != nil {
 		t.Fatalf("err: %v %v", err, resp.Error())
 	}
 
 	// This time deleting the runtime should work.
-	req = logical.TestRequest(t, logical.DeleteOperation, "plugins/runtimes/catalog/container/foo")
+	req = logical.TestRequest(t, logical.DeleteOperation, fmt.Sprintf("plugins/runtimes/catalog/container/%s", runtime))
 	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
 	if err != nil || resp.Error() != nil {
 		t.Fatalf("err: %v %v", err, resp.Error())

vault/plugincatalog/plugin_catalog.go

@@ -41,6 +41,8 @@ var (
 	ErrPluginConnectionNotFound = errors.New("plugin connection not found for client")
 	ErrPluginBadType            = errors.New("unable to determine plugin type")
 	ErrPinnedVersion            = errors.New("cannot delete a pinned version")
+	ErrPluginVersionMismatch    = errors.New("plugin version mismatch")
+	ErrPluginUnableToRun        = errors.New("unable to run plugin")
 )
 
 // PluginCatalog keeps a record of plugins known to vault. External plugins need
@@ -631,7 +633,7 @@ func (c *PluginCatalog) getBackendRunningVersion(ctx context.Context, pluginRunn
 			}
 			return logical.EmptyPluginVersion, nil
 		}
-		merr = multierror.Append(merr, fmt.Errorf("failed to dispense plugin as backend v5: %w", err))
+		merr = multierror.Append(merr, err)
 	}
 	c.logger.Debug("failed to dispense v5 backend plugin", "name", pluginRunner.Name, "error", err)
 	config.AutoMTLS = false
@@ -639,8 +641,11 @@ func (c *PluginCatalog) getBackendRunningVersion(ctx context.Context, pluginRunn
 	// attempt to run as a v4 backend plugin
 	client, err = backendplugin.NewPluginClient(ctx, c.wrapper, pluginRunner, log.NewNullLogger(), true)
 	if err != nil {
-		merr = multierror.Append(merr, fmt.Errorf("failed to dispense v4 backend plugin: %w", err))
-		c.logger.Debug("failed to dispense v4 backend plugin", "name", pluginRunner.Name, "error", merr)
+		merr = multierror.Append(merr, err)
+		c.logger.Debug("failed to dispense v4 backend plugin", "name", pluginRunner.Name, "error", err)
+		if pluginRunner.OCIImage != "" {
+			return logical.EmptyPluginVersion, fmt.Errorf("%w: %w", ErrPluginUnableToRun, merr.ErrorOrNil())
+		}
 		return logical.EmptyPluginVersion, merr.ErrorOrNil()
 	}
 	c.logger.Debug("successfully dispensed v4 backend plugin", "name", pluginRunner.Name)
@@ -696,7 +701,7 @@ func (c *PluginCatalog) getDatabaseRunningVersion(ctx context.Context, pluginRun
 		}
 		return logical.EmptyPluginVersion, nil
 	}
-	merr = multierror.Append(merr, fmt.Errorf("failed to load plugin as database v5: %w", err))
+	merr = multierror.Append(merr, err)
 
 	c.logger.Debug("attempting to load database plugin as v4", "name", pluginRunner.Name)
 	v4Client, err := v4.NewPluginClient(ctx, c.wrapper, pluginRunner, log.NewNullLogger(), true)
@@ -715,8 +720,13 @@ func (c *PluginCatalog) getDatabaseRunningVersion(ctx context.Context, pluginRun
 
 		return logical.EmptyPluginVersion, nil
 	}
-	merr = multierror.Append(merr, fmt.Errorf("failed to load plugin as database v4: %w", err))
-	return logical.EmptyPluginVersion, merr
+
+	merr = multierror.Append(merr, err)
+	if pluginRunner.OCIImage != "" {
+		return logical.EmptyPluginVersion, fmt.Errorf("%w: %w", ErrPluginUnableToRun, merr.ErrorOrNil())
+	}
+
+	return logical.EmptyPluginVersion, merr.ErrorOrNil()
 }
 
 // isDatabasePlugin returns an error if the plugin is not a database plugin.
@@ -1014,16 +1024,20 @@ func (c *PluginCatalog) setInternal(ctx context.Context, plugin pluginutil.SetPl
 	}
 	if versionErr != nil {
 		c.logger.Warn("Error determining plugin version", "error", versionErr)
+		if errors.Is(versionErr, ErrPluginUnableToRun) {
+			return nil, versionErr
+		}
 	} else if plugin.Version != "" && runningVersion.Version != "" && plugin.Version != runningVersion.Version {
-		c.logger.Warn("Plugin self-reported version did not match requested version", "plugin", plugin.Name, "requestedVersion", plugin.Version, "reportedVersion", runningVersion.Version)
-		return nil, fmt.Errorf("plugin version mismatch: %s reported version (%s) did not match requested version (%s)", plugin.Name, runningVersion.Version, plugin.Version)
+		c.logger.Error("Plugin self-reported version did not match requested version",
+			"plugin", plugin.Name, "requestedVersion", plugin.Version, "reportedVersion", runningVersion.Version)
+		return nil, fmt.Errorf("%w: %s reported version (%s) did not match requested version (%s)",
+			ErrPluginVersionMismatch, plugin.Name, runningVersion.Version, plugin.Version)
 	} else if plugin.Version == "" && runningVersion.Version != "" {
 		plugin.Version = runningVersion.Version
 		_, err := semver.NewVersion(plugin.Version)
 		if err != nil {
 			return nil, fmt.Errorf("plugin self-reported version %q is not a valid semantic version: %w", plugin.Version, err)
 		}
-
 	}
 
 	entry := &pluginutil.PluginRunner{

vault/plugincatalog/plugin_catalog_test.go

@@ -843,12 +843,18 @@ func TestPluginCatalog_ErrDirectoryNotConfigured(t *testing.T) {
 			}
 		},
 		"set container plugin": func(t *testing.T) {
+			if runtime.GOOS != "linux" {
+				t.Skip("Containerized plugins only supported on Linux")
+			}
+
 			// Should never error.
-			const image = "does-not-exist"
+			plugin := pluginhelpers.CompilePlugin(t, consts.PluginTypeDatabase, "", tempDir)
+			plugin.Image, plugin.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, plugin, tempDir)
+
 			err := catalog.Set(context.Background(), pluginutil.SetPluginInput{
 				Name:     "container",
 				Type:     consts.PluginTypeDatabase,
-				OCIImage: image,
+				OCIImage: plugin.Image,
 			})
 			if err != nil {
 				t.Fatal(err)
@@ -858,8 +864,8 @@ func TestPluginCatalog_ErrDirectoryNotConfigured(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			if p.OCIImage != image {
-				t.Fatalf("Expected %s, got %s", image, p.OCIImage)
+			if p.OCIImage != plugin.Image {
+				t.Fatalf("Expected %s, got %s", plugin.Image, p.OCIImage)
 			}
 			// Make sure we can still get builtins too
 			_, err = catalog.Get(context.Background(), "mysql-database-plugin", consts.PluginTypeDatabase, "")
@@ -888,20 +894,27 @@ func TestPluginCatalog_ErrDirectoryNotConfigured(t *testing.T) {
 // are returned with their container runtime config populated if it was
 // specified.
 func TestRuntimeConfigPopulatedIfSpecified(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("Containerized plugins only supported on Linux")
+	}
+
 	pluginCatalog := testPluginCatalog(t)
-	const image = "does-not-exist"
+
+	plugin := pluginhelpers.CompilePlugin(t, consts.PluginTypeDatabase, "", pluginCatalog.directory)
+	plugin.Image, plugin.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, plugin, pluginCatalog.directory)
+
 	const runtime = "custom-runtime"
 	err := pluginCatalog.Set(context.Background(), pluginutil.SetPluginInput{
 		Name:     "container",
 		Type:     consts.PluginTypeDatabase,
-		OCIImage: image,
+		OCIImage: plugin.Image,
 		Runtime:  runtime,
 	})
 	if err == nil {
 		t.Fatal("specified runtime doesn't exist yet, should have failed")
 	}
 
-	const ociRuntime = "some-other-oci-runtime"
+	const ociRuntime = "runc"
 	err = pluginCatalog.runtimeCatalog.Set(context.Background(), &pluginruntimeutil.PluginRuntimeConfig{
 		Name:       runtime,
 		Type:       consts.PluginRuntimeTypeContainer,
@@ -915,7 +928,7 @@ func TestRuntimeConfigPopulatedIfSpecified(t *testing.T) {
 	err = pluginCatalog.Set(context.Background(), pluginutil.SetPluginInput{
 		Name:     "container",
 		Type:     consts.PluginTypeDatabase,
-		OCIImage: image,
+		OCIImage: plugin.Image,
 		Runtime:  runtime,
 	})
 	if err != nil {
@@ -1142,12 +1155,17 @@ func TestExternalPlugin_getBackendTypeVersion(t *testing.T) {
 
 func TestExternalPluginInContainer_GetBackendTypeVersion(t *testing.T) {
 	if runtime.GOOS != "linux" {
-		t.Skip("Running plugins in containers is only supported on linux")
+		t.Skip("Containerized plugins only supported on Linux")
 	}
 
 	pluginCatalog := testPluginCatalog(t)
 
-	var plugins []pluginhelpers.TestPlugin
+	type testCase struct {
+		plugin      pluginhelpers.TestPlugin
+		expectedErr error
+	}
+	var testCases []testCase
+
 	for _, pluginType := range []consts.PluginType{
 		consts.PluginTypeCredential,
 		consts.PluginTypeSecrets,
@@ -1155,45 +1173,68 @@ func TestExternalPluginInContainer_GetBackendTypeVersion(t *testing.T) {
 	} {
 		plugin := pluginhelpers.CompilePlugin(t, pluginType, "v1.2.3", pluginCatalog.directory)
 		plugin.Image, plugin.ImageSha256 = pluginhelpers.BuildPluginContainerImage(t, plugin, pluginCatalog.directory)
-		plugins = append(plugins, plugin)
+
+		testCases = append(testCases, testCase{
+			plugin:      plugin,
+			expectedErr: nil,
+		})
+
+		plugin.Image += "-will-not-be-found"
+		testCases = append(testCases, testCase{
+			plugin:      plugin,
+			expectedErr: ErrPluginUnableToRun,
+		})
 	}
 
-	for _, plugin := range plugins {
-		t.Run(plugin.Typ.String(), func(t *testing.T) {
-			for _, ociRuntime := range []string{"runc", "runsc"} {
-				t.Run(ociRuntime, func(t *testing.T) {
-					if _, err := exec.LookPath(ociRuntime); err != nil {
-						t.Skipf("Skipping test as %s not found on path", ociRuntime)
-					}
-
-					shaBytes, _ := hex.DecodeString(plugin.ImageSha256)
-					entry := &pluginutil.PluginRunner{
-						Name:     plugin.Name,
-						OCIImage: plugin.Image,
-						Args:     nil,
-						Sha256:   shaBytes,
-						Builtin:  false,
-						Runtime:  ociRuntime,
-						RuntimeConfig: &pluginruntimeutil.PluginRuntimeConfig{
-							OCIRuntime: ociRuntime,
-						},
-					}
-
-					var version logical.PluginVersion
-					var err error
-					if plugin.Typ == consts.PluginTypeDatabase {
-						version, err = pluginCatalog.getDatabaseRunningVersion(context.Background(), entry)
-					} else {
-						version, err = pluginCatalog.getBackendRunningVersion(context.Background(), entry)
-					}
-					if err != nil {
-						t.Fatal(err)
-					}
-					if version.Version != plugin.Version {
-						t.Errorf("Expected to get version %v but got %v", plugin.Version, version.Version)
-					}
-				})
+	for _, tc := range testCases {
+		t.Run(tc.plugin.Typ.String(), func(t *testing.T) {
+			expectedErrTestName := "nil err"
+			if tc.expectedErr != nil {
+				expectedErrTestName = tc.expectedErr.Error()
 			}
+
+			t.Run(expectedErrTestName, func(t *testing.T) {
+				for _, ociRuntime := range []string{"runc", "runsc"} {
+					t.Run(ociRuntime, func(t *testing.T) {
+						if _, err := exec.LookPath(ociRuntime); err != nil {
+							t.Skipf("Skipping test as %s not found on path", ociRuntime)
+						}
+
+						shaBytes, _ := hex.DecodeString(tc.plugin.ImageSha256)
+						entry := &pluginutil.PluginRunner{
+							Name:     tc.plugin.Name,
+							OCIImage: tc.plugin.Image,
+							Args:     nil,
+							Sha256:   shaBytes,
+							Builtin:  false,
+							Runtime:  ociRuntime,
+							RuntimeConfig: &pluginruntimeutil.PluginRuntimeConfig{
+								OCIRuntime: ociRuntime,
+							},
+						}
+
+						var version logical.PluginVersion
+						var err error
+						if tc.plugin.Typ == consts.PluginTypeDatabase {
+							version, err = pluginCatalog.getDatabaseRunningVersion(context.Background(), entry)
+						} else {
+							version, err = pluginCatalog.getBackendRunningVersion(context.Background(), entry)
+						}
+
+						if tc.expectedErr == nil {
+							if err != nil {
+								t.Fatalf("Expected successful get backend type version but got: %v", err)
+							}
+							if version.Version != tc.plugin.Version {
+								t.Errorf("Expected to get version %v but got %v", tc.plugin.Version, version.Version)
+							}
+
+						} else if !errors.Is(err, tc.expectedErr) {
+							t.Errorf("Expected to get err %s but got %s", tc.expectedErr, err)
+						}
+					})
+				}
+			})
 		})
 	}
 }