-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Plugins: Auto version selection for auth/secrets + tune version #17167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Seems to work for me when I tested with multiple versions of a secret engine! Just left a couple stray observations.
I also noticed that vault read sys/mounts/<mount>
always shows running_sha
as n/a
even with an external plugin mounted (though I'm not sure what that's actually used for).
And another side note, I also tried mounting a builtin plugin, then registering a version of the same plugin, and tuning the mount for the version, then reloading, and vault read sys/mounts/<mount>
still showed running_version
as the builtin version. I think the versioned plugin started up since I see the process running, but I didn't verify anything else beyond that.
switch version { | ||
case "": | ||
var err error | ||
version, err = selectPluginVersion(ctx, b.System(), logicalType, consts.PluginTypeSecrets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like selectPluginVersion()
only returns plugins that aren't builtin, so I'm curious if it's possible to mount a builtin plugin if there's also a versioned instance of it registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently no, registering a versioned plugin with the same name overrides the built-in in the same way that registering an unversioned plugin of the same name does. However, I'm wondering whether it might be nice to also allow specifying -plugin-version=builtin
or -plugin-version=1.12.0+builtin
to explicitly select the builtin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, it just seemed strange to me that I could see the built-in plugin in vault plugin list -detailed
, but couldn't mount it. And I'd vote for using the version listed in the catalog (-plugin-version=1.12.0+builtin
) if we end up going that way.
@tvoran sorry I realised just after I merged I forgot to respond to the main review comment. I think it came up in another PR, but the empty
Thanks for testing that, sounds like a bug. I'll take a look and follow up on it in another PR. |
Two main features:
Other changes:
external_plugin_test.go
moves the calling code a level up the stack, using the system backend'sHandleRequest
instead of directly calling the implementation functions for each route. Moves the tests slightly closer to production call stacks.There's still some more follow-up work to do in future PRs, including:
-plugin-version
flag tovault auth tune
andvault secrets tune
ReplaceDone in Plugins: Consistently use plugin_version #17171version
withplugin_version
everywhere (I excluded that from this PR to avoid diff noise)