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.

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

Conversation

thyton
Copy link
Contributor

@thyton thyton commented Jan 23, 2024

The vault plugin register command doesn't return an error when the image isn't found. This PR refactors the logic in generating errors in getting backend type version and generating an error response through the system backend when both containerized plugin v4 and v5 loads fail.

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jan 23, 2024
@thyton thyton requested a review from tomhjp January 23, 2024 07:39
Copy link

Build Results:
All builds succeeded! ✅

Copy link

github-actions bot commented Jan 23, 2024

CI Results:
All Go tests succeeded! ✅

vault/plugincatalog/plugin_catalog.go Show resolved Hide resolved
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: %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.

// 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
}

Comment on lines 42 to 43
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.

vault/plugincatalog/plugin_catalog.go Outdated Show resolved Hide resolved
@thyton thyton requested a review from tomhjp February 1, 2024 06:44
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Looking good! I left a few comments, but it's mostly just nits and CI/test considerations.

vault/logical_system.go Outdated Show resolved Hide resolved
vault/logical_system_test.go Show resolved Hide resolved
vault/plugincatalog/plugin_catalog.go Outdated Show resolved Hide resolved
vault/plugincatalog/plugin_catalog.go Show resolved Hide resolved
vault/plugincatalog/plugin_catalog_test.go Show resolved Hide resolved
vault/plugincatalog/plugin_catalog_test.go Show resolved Hide resolved
vault/plugincatalog/plugin_catalog_test.go Outdated Show resolved Hide resolved
vault/plugincatalog/plugin_catalog.go Outdated Show resolved Hide resolved
vault/plugincatalog/plugin_catalog.go Outdated Show resolved Hide resolved
vault/plugincatalog/plugin_catalog.go Outdated Show resolved Hide resolved
@thyton
Copy link
Contributor Author

thyton commented Feb 9, 2024

Thank you very much for your feedback!

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants