-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Run all builtins as plugins #5536
Conversation
74a052f
to
97c24d7
Compare
// backend is a thin wrapper around plugin.BackendPluginClient | ||
type backend struct { | ||
// PluginBackend is a thin wrapper around plugin.BackendPluginClient | ||
type PluginBackend struct { |
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.
Does this have to be exported? The func Backend
factory func above returns an interface so having this as backend
seems to be fine (and follows the pattern on how other backends are laid out, e.g. credentials/aws
having its own internal type backend struct
).
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.
This is exported so it can be used in the plugin_test
package residing in the same directory, here.
The reason that was changed to its own package was to resolve an extremely persistent import cycle. This is the import cycle that results if I revert it:
$ make test
==> Checking that build is using go version >= 1.10...
==> Using go version 1.11...
# github.com/hashicorp/vault/builtin/plugin
import cycle not allowed in test
package github.com/hashicorp/vault/builtin/plugin (test)
imports github.com/hashicorp/vault/http
imports github.com/hashicorp/vault/vault
imports github.com/hashicorp/vault/builtin/plugin
FAIL github.com/hashicorp/vault/builtin/plugin [setup failed]
I experimented with multiple ways of breaking this import cycle, ultimately selecting this one because it was the least code change.
I'm open to taking other approaches, or adding a comment to that effect.
@@ -1,4 +1,4 @@ | |||
package plugin | |||
package plugin_test |
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.
Any reason why this has to be broken out to its own package?
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.
Replied to this one in the above comment.
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.
I think we need to think through some of the upgrade cases still, i worry that existing plugins mounts wont upgrade correctly. Also i think the catalog's storage needs to be broken out by type.
command/base_predict.go
Outdated
i := 0 | ||
for pluginName := range pluginNames { | ||
plugins[i] = pluginName | ||
i++ |
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.
Feels like the dedup should be happening in the API
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.
I agree - and the sorting of the list too. So, I added this commit and what that does is re-add the ability to list all plugins at sys/plugins/catalog
. At that endpoint, it sorts and dedupes them.
There's a catch, though. I can't move all the sorting and deduping to the API because when I re-add path prediction for the secret mounts, I need to predict all database and secrets plugins. So there need to be two API calls - one listing each type, and then some logic sorting and deduping them.
vault/plugin_reload.go
Outdated
@@ -77,8 +75,7 @@ func (c *Core) reloadMatchingPlugin(ctx context.Context, pluginName string) erro | |||
if ns.ID != entry.Namespace().ID { | |||
continue | |||
} | |||
|
|||
if entry.Config.PluginName == pluginName && entry.Type == "plugin" { | |||
if entry.Type == pluginName { |
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.
What does entry.Type equal now?
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.
In the command, $ vault auth enable my-custom-plugin
, the entry.Type == "my-custom-plugin".
Co-Authored-By: tyrannosaurus-becks <[email protected]>
Co-Authored-By: tyrannosaurus-becks <[email protected]>
Co-Authored-By: tyrannosaurus-becks <[email protected]>
Co-Authored-By: tyrannosaurus-becks <[email protected]>
"database": logicalDb.Factory, | ||
// This is also available in the plugin catalog, but is here due to the need to | ||
// automatically mount it. | ||
"kv": logicalKv.Factory, |
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.
Was this necessary to add?
* Determine plugin type if using the old untyped API * remove logger * add comment * Consolidate plugin paths
* Reuse the same path spec * Re-arrange paths to proper order
This reverts commit f6d2ee9.
Try list if get fails
This PR is to allow Vault to run all builtins as plugins. This PR is ready for review.
Overview
To understand how this is helpful, consider this scenario:
aws
auth builtinTo achieve this given this code, you would:
aws
builtin$ go build
onmain.go
in its directory to make a binary$ auth enable aws
aws
pluginThis hopefully illustrates the new behavior this PR introduces:
Because of possible naming clashes across plugins, we had to introduce the concept of a plugin type. This prevents the
aws
secrets plugin's name from clashing with theaws
auth plugin's name.Other side effects of these actions include:
/sys/plugins/catalog
which is DEPRECATED by this PR/sys/plugins/catalog/database
,/sys/plugins/catalog/auth
, and/sys/plugins/catalog/secret
/sys/plugins/catalog
which, under "data", returns a map of plugin type to the names of the plugins that fall into that type$ vault secrets enable -plugin-name='ad' plugin
will now be DEPRECATED (though still supported) and now all plugins will be enabled via$ vault secrets enable ad
TODO