Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix plugin register cli error on unfound oci images #24990

Merged
merged 19 commits into from
Feb 13, 2024
Merged

fix plugin register cli error on unfound oci images #24990

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
21e64a1
fix plugin register cli error on unfound oci images
thyton Jan 23, 2024
16bf7f5
unwrap error before creating logical error response on all plugin loa…
thyton Jan 23, 2024
aa2f841
return all loads failed err when oci image is specified
thyton Jan 23, 2024
f2cee44
add another t.Run for expected err
thyton Jan 23, 2024
596fd9c
Merge branch 'main' into VAULT-20736-fix-plugin-register-cli-error-on…
thyton Jan 23, 2024
501b42b
fix plugincatalog test failures
thyton Feb 1, 2024
4cb6dca
Merge branch 'main' into VAULT-20736-fix-plugin-register-cli-error-on…
thyton Feb 1, 2024
d69e5bb
make fmt
thyton Feb 1, 2024
fde0a3b
fix acceptance test TestSystemBackend_PluginCatalog_ContainerCRUD fai…
thyton Feb 1, 2024
30e2259
fix TestSystemBackend_pluginRuntime_CannotDeleteRuntimeWithReferencin…
thyton Feb 1, 2024
44f8793
revert changing field name
thyton Feb 1, 2024
c5163c1
merge 2 sentinel errors to unable to run containerized plugin
thyton Feb 1, 2024
048d91b
Update vault/logical_system.go
thyton Feb 9, 2024
8fa6f10
Update vault/plugincatalog/plugin_catalog.go
thyton Feb 9, 2024
7b89089
rename error and using %w verb for merr
thyton Feb 9, 2024
d80d0dc
Merge branch 'VAULT-20736-fix-plugin-register-cli-error-on-unfound-oc…
thyton Feb 9, 2024
e3c5e67
skip containized plugin test if not run on linux
thyton Feb 9, 2024
c4be808
add 24990 changelog
thyton Feb 9, 2024
2c17522
Merge branch 'main' into VAULT-20736-fix-plugin-register-cli-error-on…
thyton Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions helper/testhelpers/pluginhelpers/pluginhelpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (

type TestPlugin struct {
Name string
Typ consts.PluginType
Type consts.PluginType
Version string
FileName string
Sha256 string
Expand Down Expand Up @@ -142,7 +142,7 @@ func CompilePlugin(t testing.T, typ consts.PluginType, pluginVersion string, plu
}
return TestPlugin{
Name: pluginName,
Typ: typ,
Type: typ,
Version: pluginVersion,
FileName: path.Base(pluginPath),
Sha256: fmt.Sprintf("%x", sha.Sum(nil)),
Expand Down
10 changes: 5 additions & 5 deletions vault/external_plugin_container_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestExternalPluginInContainer_MountAndUnmount(t *testing.T) {
})

for _, plugin := range plugins {
t.Run(plugin.Typ.String(), func(t *testing.T) {
t.Run(plugin.Type.String(), func(t *testing.T) {
t.Run("default runtime", func(t *testing.T) {
if _, err := exec.LookPath("runsc"); err != nil {
t.Skip("Skipping test as runsc not found on path")
Expand Down Expand Up @@ -85,13 +85,13 @@ func mountAndUnmountContainerPlugin_WithRuntime(t *testing.T, c *Core, plugin pl
if ociRuntime != "" {
registerPluginRuntime(t, c.systemBackend, ociRuntime, rootless)
}
registerContainerPlugin(t, c.systemBackend, plugin.Name, plugin.Typ.String(), "1.0.0", plugin.ImageSha256, plugin.Image, ociRuntime)
registerContainerPlugin(t, c.systemBackend, plugin.Name, plugin.Type.String(), "1.0.0", plugin.ImageSha256, plugin.Image, ociRuntime)

mountPlugin(t, c.systemBackend, plugin.Name, plugin.Typ, "v1.0.0", "")
mountPlugin(t, c.systemBackend, plugin.Name, plugin.Type, "v1.0.0", "")

routeRequest := func(expectMatch bool) {
pluginPath := "foo/bar"
if plugin.Typ == consts.PluginTypeCredential {
if plugin.Type == consts.PluginTypeCredential {
pluginPath = "auth/foo/bar"
}
match := c.router.MatchingMount(namespace.RootContext(context.Background()), pluginPath)
Expand All @@ -104,7 +104,7 @@ func mountAndUnmountContainerPlugin_WithRuntime(t *testing.T, c *Core, plugin pl
}

routeRequest(true)
unmountPlugin(t, c.systemBackend, plugin.Typ, "foo")
unmountPlugin(t, c.systemBackend, plugin.Type, "foo")
routeRequest(false)
}

Expand Down
24 changes: 12 additions & 12 deletions vault/external_tests/plugin/external_plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,15 @@ func testRegisterAndEnable(t *testing.T, client *api.Client, plugin pluginhelper
t.Helper()
if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Command: plugin.Name,
SHA256: plugin.Sha256,
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
}

switch plugin.Typ {
switch plugin.Type {
case consts.PluginTypeSecrets:
if err := client.Sys().Mount(plugin.Name, &api.MountInput{
Type: plugin.Name,
Expand Down Expand Up @@ -304,7 +304,7 @@ func TestExternalPlugin_AuthMethod(t *testing.T) {
// Register
if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Command: plugin.Name,
SHA256: plugin.Sha256,
Version: plugin.Version,
Expand Down Expand Up @@ -403,7 +403,7 @@ func TestExternalPlugin_AuthMethod(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -478,7 +478,7 @@ func TestExternalPlugin_AuthMethodReload(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand All @@ -498,7 +498,7 @@ func TestExternalPlugin_SecretsEngine(t *testing.T) {
// Register
if err := client.Sys().RegisterPlugin(&api.RegisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Command: plugin.Name,
SHA256: plugin.Sha256,
Version: plugin.Version,
Expand Down Expand Up @@ -558,7 +558,7 @@ func TestExternalPlugin_SecretsEngine(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -624,7 +624,7 @@ func TestExternalPlugin_SecretsEngineReload(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -760,7 +760,7 @@ func TestExternalPlugin_Database(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -848,7 +848,7 @@ func TestExternalPlugin_DatabaseReload(t *testing.T) {
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -944,7 +944,7 @@ func TestExternalPlugin_AuditEnabled_ShouldLogPluginMetadata_Auth(t *testing.T)
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -1017,7 +1017,7 @@ func TestExternalPlugin_AuditEnabled_ShouldLogPluginMetadata_Secret(t *testing.T
// Deregister
if err := client.Sys().DeregisterPlugin(&api.DeregisterPluginInput{
Name: plugin.Name,
Type: api.PluginType(plugin.Typ),
Type: api.PluginType(plugin.Type),
Version: plugin.Version,
}); err != nil {
t.Fatal(err)
Expand Down
9 changes: 8 additions & 1 deletion vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,9 +590,16 @@ 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) {
return logical.ErrorResponse(err.Error()), nil
}

if errors.Is(err, plugincatalog.ErrAllContainerizedBackendPluginLoadsFailed) ||
errors.Is(err, plugincatalog.ErrAllContainerizedDatabasePluginLoadsFailed) {
return logical.ErrorResponse(errors.Unwrap(err).Error()), nil
thyton marked this conversation as resolved.
Show resolved Hide resolved
}

return nil, err
}

Expand Down
41 changes: 28 additions & 13 deletions vault/plugincatalog/plugin_catalog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@ import (
)

var (
ErrDirectoryNotConfigured = errors.New("could not set plugin, plugin directory is not configured")
ErrPluginNotFound = errors.New("plugin not found in the catalog")
ErrPluginConnectionNotFound = errors.New("plugin connection not found for client")
ErrPluginBadType = errors.New("unable to determine plugin type")
ErrDirectoryNotConfigured = errors.New("could not set plugin, plugin directory is not configured")
ErrPluginNotFound = errors.New("plugin not found in the catalog")
ErrPluginConnectionNotFound = errors.New("plugin connection not found for client")
ErrPluginBadType = errors.New("unable to determine plugin type")
ErrPluginVersionMismatch = errors.New("plugin version mismatch")
ErrAllContainerizedBackendPluginLoadsFailed = errors.New("failed to dispense all containerized backend plugins v4 through v5")
ErrAllContainerizedDatabasePluginLoadsFailed = errors.New("failed to load all containerized database plugins v4 through v5")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need 2 sentinel errors. We should just need a sentinel error along the lines of "unable to run plugin", and we can return the same error from both db and backend functions.

)

// PluginCatalog keeps a record of plugins known to vault. External plugins need
Expand Down Expand Up @@ -585,16 +588,19 @@ 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
config.IsMetadataMode = true
// 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 != "" {
tomhjp marked this conversation as resolved.
Show resolved Hide resolved
return logical.EmptyPluginVersion, fmt.Errorf("%w: %s", ErrAllContainerizedBackendPluginLoadsFailed, merr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems too early to return? What if running the plugin as a v5 plugin would have succeeded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me we'd have actually returned the plugin version early if running the plugin as a v5 plugin succeeded. I think we attempt to run v4 plugin only when we fail to run v5 plugin. Let me know if I missed something in your question.

vault/vault/plugincatalog/plugin_catalog.go

Lines 575 to 588 in 2acac70

// dispense the plugin so we can get its version
client, err = backendplugin.Dispense(pc.ClientProtocol, pc)
if err == nil {
c.logger.Debug("successfully dispensed v5 backend plugin", "name", pluginRunner.Name)
err = client.Setup(ctx, &logical.BackendConfig{})
if err != nil {
return logical.EmptyPluginVersion, nil
}
if versioner, ok := client.(logical.PluginVersioner); ok {
return versioner.PluginVersion(), nil
}
return logical.EmptyPluginVersion, nil
}

}
return logical.EmptyPluginVersion, merr.ErrorOrNil()
}
c.logger.Debug("successfully dispensed v4 backend plugin", "name", pluginRunner.Name)
Expand Down Expand Up @@ -650,7 +656,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)
Expand All @@ -669,8 +675,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: %s", ErrAllContainerizedDatabasePluginLoadsFailed, merr)
}

return logical.EmptyPluginVersion, merr.ErrorOrNil()
}

// isDatabasePlugin returns an error if the plugin is not a database plugin.
Expand Down Expand Up @@ -963,16 +974,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, ErrAllContainerizedBackendPluginLoadsFailed) || errors.Is(versionErr, ErrAllContainerizedDatabasePluginLoadsFailed) {
return nil, versionErr
}
thyton marked this conversation as resolved.
Show resolved Hide resolved
} 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.Warn("Plugin self-reported version did not match requested version",
thyton marked this conversation as resolved.
Show resolved Hide resolved
"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{
Expand Down
Loading
Loading