Lazy-load plugin mounts #3255
Merged
Lazy-load plugin mounts #3255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…test-setup-plugins
Tests for setup plugin errors
…ting to bootstrap
jefferai
reviewed
Aug 31, 2017
|
|
||
| // Ensure proper cleanup of the backend (i.e. call client.Kill()) | ||
| b.Backend.Cleanup() | ||
|
|
||
| nb, err := bplugin.NewBackend(pluginName, b.config.System, b.config.Logger) | ||
| nb, err := bplugin.NewBackend(pluginName, b.config.System, b.config.Logger, false) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
At this point, please check the type and special paths against the values retrieved from when it was run in metadata mode and make sure they match.
There was a problem hiding this comment.
This should be addressed. It is checked further down in the b.loaded conditional.
jefferai
reviewed
Aug 31, 2017
helper/pluginutil/runner.go
Outdated
| @@ -98,6 +102,39 @@ func (r *PluginRunner) Run(wrapper RunnerUtil, pluginMap map[string]plugin.Plugi | |||
| return client, nil | |||
| } | |||
|
|
|||
| func (r *PluginRunner) RunMetadataMode(wrapper RunnerUtil, pluginMap map[string]plugin.Plugin, hs plugin.HandshakeConfig, env []string, logger log.Logger) (*plugin.Client, error) { | |||
| cmd := exec.Command(r.Command, r.Args...) | |||
There was a problem hiding this comment.
This appears to be, other than whether metadatamode env var is enabled or not, the same exact logic as the latter part of Run. Any chance they can be combined into a runCommon that Run and RunMetadataMode call with appropriate bool var?
There was a problem hiding this comment.
@briankassouf reverted this on 5526108#diff-217b9ed237f303ae16a69462dfcc6bf3. I think it's because he wanted to keep the behavior separate explicitly. Brian correct me if otherwise :)
There was a problem hiding this comment.
That's correct, but i'm okay with RunMetadataMode, Run, and runCommon. Other plugin types shouldn't have to know about metadata mode IMO.
jefferai
previously approved these changes
Aug 31, 2017
|
@calvn can you merge with master, the tests wont run on travis otherwise |
logical/plugin/plugin.go
Outdated
| // The backend is returned as a logical.Backend interface. | ||
| func NewBackend(pluginName string, sys pluginutil.LookRunnerUtil, logger log.Logger) (logical.Backend, error) { | ||
| // The backend is returned as a logical.Backend interface. The isMetadataMode param determines whether | ||
| // the plugin should run with the -metadata flag. |
jefferai
previously approved these changes
Aug 31, 2017
calvn
added a commit
that referenced
this pull request
Sep 1, 2017
* Lazy load plugins to avoid setup-unwrap cycle * Remove commented blocks * Refactor NewTestCluster, use single core cluster on basic plugin tests * Set c.pluginDirectory in TestAddTestPlugin for setupPluginCatalog to work properly * Add special path to mock plugin * Move ensureCoresSealed to vault/testing.go * Use same method for EnsureCoresSealed and Cleanup * Bump ensureCoresSealed timeout to 60s * Correctly handle nil opts on NewTestCluster * Add metadata flag to APIClientMeta, use meta-enabled plugin when mounting to bootstrap * Check metadata flag directly on the plugin process * Plumb isMetadataMode down to PluginRunner * Add NOOP shims when running in metadata mode * Remove unused flag from the APIMetadata object * Remove setupSecretPlugins and setupCredentialPlugins functions * Move when we setup rollback manager to after the plugins are initialized * Fix tests * Fix merge issue * start rollback manager after the credential setup * Add guards against running certain client and server functions while in metadata mode * Call initialize once a plugin is loaded on the fly * Add more tests, update basic secret/auth plugin tests to trigger lazy loading * Skip mount if plugin removed from catalog * Fixup * Remove commented line on LookupPlugin * Fail on mount operation if plugin is re-added to catalog and mount is on existing path * Check type and special paths on startBackend * Fix merge conflicts * Refactor PluginRunner run methods to use runCommon, fix TestSystemBackend_Plugin_auth
chrishoffman
pushed a commit
that referenced
this pull request
Sep 1, 2017
* oss/master: Plugin Version Update (#3275) Lazy-load plugin mounts (#3255) changelog++ changelog++ Add pki/root/sign-self-issued. (#3274) Travis, be happier please changelog++ Change auth helper interface to api.Secret. (#3263) changelog++ Try reconnecting Mongo on EOF (#3269) Don't append a trailing slash to the request path if it doesn't actually help find something (#3271) changelog++ Use TypeDurationSecond for TTL values in PKI. (#3270) changelog++ changelog++ Use net.SplitHostPort on Consul address (#3268) Normalize plugin_name option for mount and enable-auth (#3202) Updating Okta lib for credential backend (#3245) Explicitly mention that aws/aws-ec2 were unified under aws.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3241
Closes #3266