feat: plugin multiplexing: handle plugin client cleanup #13896

fairclothjm · 2022-02-04T00:09:22Z

Description

This is a follow up to the PR #13734 which added multiplexing support for DB plugins.

This PR makes the following changes

Ensure plugin processes are cleaned up for v5 and multiplexed plugins
- this is handled by a cleanup function that is stored on the PluginClient instance
Move the context ID injection logic from the SDK to Core

To test multiplexing

build a v5 db plugin from main branch
build v6 multiplexed plugin from the feat-mux branch
- can use postgres or oracle db plugin
build a vault binary from the multiplexing branch feat-mux
start vault and register the plugins
write the database configs and perform credential operations as normal

vault/plugin_catalog.go

sdk/helper/pluginutil/runner.go

vault/plugin_catalog.go

sdk/database/dbplugin/v5/plugin_client.go

vault/plugin_catalog.go

This reverts commit 73d420e.

vault/plugin_catalog.go

This reverts commit f755270.

fairclothjm · 2022-02-09T20:05:11Z

sdk/helper/pluginutil/runner.go

@@ -98,6 +99,8 @@ func CtxCancelIfCanceled(f context.CancelFunc, ctxCanceler context.Context) chan
 	return quitCh
 }

+// MultiplexingSupport returns true if a plugin supports multiplexing.
+// Currently this is hardcoded for database plugins.
 func MultiplexingSupport(version int) bool {


This will likely need a Plugin Type arg in the future but I think it is ok to leave out for now.

vault/plugin_catalog.go

calvn · 2022-02-09T20:20:54Z

sdk/logical/system_view.go

-	NewPluginClient(ctx context.Context, pluginRunner *pluginutil.PluginRunner, config pluginutil.PluginClientConfig) (pluginutil.Multiplexer, error)
+	// NewPluginClient returns a client for managing the lifecycle of plugin
+	// processes
+	NewPluginClient(ctx context.Context, config pluginutil.PluginClientConfig) (pluginutil.PluginInstance, error)


This makes me think whether we should be embedding pluginutil.RunnerUtil instead of duplicating the method signatures...

Would it be more appropriate to name this NewPluginClientInstance now that the interface is renamed?

…nto feat-mux-plugin-cleanup

* use closure for plugin client cleanup * log and return errors; add comments * move rpcClient wrapping to core for ID injection * refactor core plugin client and sdk * remove unused ID method * refactor and only wrap clientConn on multiplexed plugins * rename structs and do not export types * Slight refactor of system view interface * Revert "Slight refactor of system view interface" This reverts commit 73d420e. * Revert "Revert "Slight refactor of system view interface"" This reverts commit f755270. * only provide pluginRunner arg to the internal newPluginClient method * embed ClientProtocol in pluginClient and name logger * Add back MLock support * remove enableMlock arg from setupPluginCatalog * rename plugin util interface to PluginClient Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com>

* feat: DB plugin multiplexing (#13734) * WIP: start from main and get a plugin runner from core * move MultiplexedClient map to plugin catalog - call sys.NewPluginClient from PluginFactory - updates to getPluginClient - thread through isMetadataMode * use go-plugin ClientProtocol interface - call sys.NewPluginClient from dbplugin.NewPluginClient * move PluginSets to dbplugin package - export dbplugin HandshakeConfig - small refactor of PluginCatalog.getPluginClient * add removeMultiplexedClient; clean up on Close() - call client.Kill from plugin catalog - set rpcClient when muxed client exists * add ID to dbplugin.DatabasePluginClient struct * only create one plugin process per plugin type * update NewPluginClient to return connection ID to sdk - wrap grpc.ClientConn so we can inject the ID into context - get ID from context on grpc server * add v6 multiplexing protocol version * WIP: backwards compat for db plugins * Ensure locking on plugin catalog access - Create public GetPluginClient method for plugin catalog - rename postgres db plugin * use the New constructor for db plugins * grpc server: use write lock for Close and rlock for CRUD * cleanup MultiplexedClients on Close * remove TODO * fix multiplexing regression with grpc server connection * cleanup grpc server instances on close * embed ClientProtocol in Multiplexer interface * use PluginClientConfig arg to make NewPluginClient plugin type agnostic * create a new plugin process for non-muxed plugins * feat: plugin multiplexing: handle plugin client cleanup (#13896) * use closure for plugin client cleanup * log and return errors; add comments * move rpcClient wrapping to core for ID injection * refactor core plugin client and sdk * remove unused ID method * refactor and only wrap clientConn on multiplexed plugins * rename structs and do not export types * Slight refactor of system view interface * Revert "Slight refactor of system view interface" This reverts commit 73d420e. * Revert "Revert "Slight refactor of system view interface"" This reverts commit f755270. * only provide pluginRunner arg to the internal newPluginClient method * embed ClientProtocol in pluginClient and name logger * Add back MLock support * remove enableMlock arg from setupPluginCatalog * rename plugin util interface to PluginClient Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com> * feature: multiplexing: fix unit tests (#14007) * fix grpc_server tests and add coverage * update run_config tests * add happy path test case for grpc_server ID from context * update test helpers * feat: multiplexing: handle v5 plugin compiled with new sdk * add mux supported flag and increase test coverage * set multiplexingSupport field in plugin server * remove multiplexingSupport field in sdk * revert postgres to non-multiplexed * add comments on grpc server fields * use pointer receiver on grpc server methods * add changelog * use pointer for grpcserver instance * Use a gRPC server to determine if a plugin should be multiplexed * Apply suggestions from code review Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com> * add lock to removePluginClient * add multiplexingSupport field to externalPlugin struct * do not send nil to grpc MultiplexingSupport * check err before logging * handle locking scenario for cleanupFunc * allow ServeConfigMultiplex to dispense v5 plugin * reposition structs, add err check and comments * add comment on locking for cleanupExternalPlugin Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com> Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com>

* feat: DB plugin multiplexing (#13734) * WIP: start from main and get a plugin runner from core * move MultiplexedClient map to plugin catalog - call sys.NewPluginClient from PluginFactory - updates to getPluginClient - thread through isMetadataMode * use go-plugin ClientProtocol interface - call sys.NewPluginClient from dbplugin.NewPluginClient * move PluginSets to dbplugin package - export dbplugin HandshakeConfig - small refactor of PluginCatalog.getPluginClient * add removeMultiplexedClient; clean up on Close() - call client.Kill from plugin catalog - set rpcClient when muxed client exists * add ID to dbplugin.DatabasePluginClient struct * only create one plugin process per plugin type * update NewPluginClient to return connection ID to sdk - wrap grpc.ClientConn so we can inject the ID into context - get ID from context on grpc server * add v6 multiplexing protocol version * WIP: backwards compat for db plugins * Ensure locking on plugin catalog access - Create public GetPluginClient method for plugin catalog - rename postgres db plugin * use the New constructor for db plugins * grpc server: use write lock for Close and rlock for CRUD * cleanup MultiplexedClients on Close * remove TODO * fix multiplexing regression with grpc server connection * cleanup grpc server instances on close * embed ClientProtocol in Multiplexer interface * use PluginClientConfig arg to make NewPluginClient plugin type agnostic * create a new plugin process for non-muxed plugins * feat: plugin multiplexing: handle plugin client cleanup (#13896) * use closure for plugin client cleanup * log and return errors; add comments * move rpcClient wrapping to core for ID injection * refactor core plugin client and sdk * remove unused ID method * refactor and only wrap clientConn on multiplexed plugins * rename structs and do not export types * Slight refactor of system view interface * Revert "Slight refactor of system view interface" This reverts commit 73d420e. * Revert "Revert "Slight refactor of system view interface"" This reverts commit f755270. * only provide pluginRunner arg to the internal newPluginClient method * embed ClientProtocol in pluginClient and name logger * Add back MLock support * remove enableMlock arg from setupPluginCatalog * rename plugin util interface to PluginClient Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com> * feature: multiplexing: fix unit tests (#14007) * fix grpc_server tests and add coverage * update run_config tests * add happy path test case for grpc_server ID from context * update test helpers * feat: multiplexing: handle v5 plugin compiled with new sdk * add mux supported flag and increase test coverage * set multiplexingSupport field in plugin server * remove multiplexingSupport field in sdk * revert postgres to non-multiplexed * add comments on grpc server fields * use pointer receiver on grpc server methods * add changelog * use pointer for grpcserver instance * Use a gRPC server to determine if a plugin should be multiplexed * Apply suggestions from code review Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com> * add lock to removePluginClient * add multiplexingSupport field to externalPlugin struct * do not send nil to grpc MultiplexingSupport * check err before logging * handle locking scenario for cleanupFunc * allow ServeConfigMultiplex to dispense v5 plugin * reposition structs, add err check and comments * add comment on locking for cleanupExternalPlugin * feature: multiplexing: handle error plugin shutdown Co-authored-by: Brian Kassouf <bkassouf@hashicorp.com> Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com>

use closure for plugin client cleanup

4ff8b08

fairclothjm requested review from a team and removed request for a team February 4, 2022 00:09

briankassouf reviewed Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

kalafut reviewed Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Show resolved Hide resolved

kalafut reviewed Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

log and return errors; add comments

8adfd95

vercel bot temporarily deployed to Preview – vault February 4, 2022 15:10 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 4, 2022 15:10 Inactive

move rpcClient wrapping to core for ID injection

e9da3b4

vercel bot temporarily deployed to Preview – vault-storybook February 4, 2022 17:23 Inactive

vercel bot temporarily deployed to Preview – vault February 4, 2022 17:23 Inactive

fairclothjm commented Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

fairclothjm commented Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

fairclothjm changed the title ~~feat: plugin multiplexing: use closure for plugin client cleanup~~ feat: plugin multiplexing: handle plugin client cleanup Feb 4, 2022

calvn reviewed Feb 4, 2022

View reviewed changes

sdk/helper/pluginutil/runner.go Outdated Show resolved Hide resolved

calvn reviewed Feb 4, 2022

View reviewed changes

vault/plugin_catalog.go Show resolved Hide resolved

refactor core plugin client and sdk

7c1e08b

vercel bot temporarily deployed to Preview – vault February 7, 2022 18:06 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 18:06 Inactive

kalafut reviewed Feb 7, 2022

View reviewed changes

sdk/database/dbplugin/v5/plugin_client.go Outdated Show resolved Hide resolved

briankassouf reviewed Feb 7, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

briankassouf reviewed Feb 7, 2022

View reviewed changes

vault/plugin_catalog.go Show resolved Hide resolved

remove unused ID method

6a72cf8

vercel bot temporarily deployed to Preview – vault February 7, 2022 21:10 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 21:10 Inactive

calvn reviewed Feb 7, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

kalafut reviewed Feb 8, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

fairclothjm added 2 commits February 8, 2022 17:26

refactor and only wrap clientConn on multiplexed plugins

3afa088

rename structs and do not export types

c6a2e28

vercel bot temporarily deployed to Preview – vault February 9, 2022 01:47 Inactive

Revert "Slight refactor of system view interface"

f755270

This reverts commit 73d420e.

vercel bot temporarily deployed to Preview – vault February 9, 2022 01:48 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 01:48 Inactive

briankassouf reviewed Feb 9, 2022

View reviewed changes

vault/plugin_catalog.go Outdated Show resolved Hide resolved

vault/plugin_catalog.go Outdated Show resolved Hide resolved

vault/plugin_catalog.go Outdated Show resolved Hide resolved

vault/plugin_catalog.go Outdated Show resolved Hide resolved

briankassouf and others added 3 commits February 8, 2022 17:55

Revert "Revert "Slight refactor of system view interface""

5bfe889

This reverts commit f755270.

only provide pluginRunner arg to the internal newPluginClient method

7365970

embed ClientProtocol in pluginClient and name logger

e5c57a3

vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 20:02 Inactive

vercel bot temporarily deployed to Preview – vault February 9, 2022 20:02 Inactive

fairclothjm requested review from calvn and briankassouf February 9, 2022 20:02

fairclothjm commented Feb 9, 2022

View reviewed changes

calvn reviewed Feb 9, 2022

View reviewed changes

calvn approved these changes Feb 9, 2022

View reviewed changes

briankassouf and others added 3 commits February 9, 2022 12:43

Add back MLock support

414bc11

remove enableMlock arg from setupPluginCatalog

dd7e52c

Merge remote-tracking branch 'origin/feat-mux-plugin-cleanup-mlock' i…

73077e2

…nto feat-mux-plugin-cleanup

vercel bot temporarily deployed to Preview – vault February 9, 2022 20:48 Inactive

vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 20:48 Inactive

rename plugin util interface to PluginClient

35af94e

vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 20:57 Inactive

vercel bot temporarily deployed to Preview – vault February 9, 2022 20:57 Inactive

fairclothjm requested a review from calvn February 10, 2022 15:21

calvn approved these changes Feb 10, 2022

View reviewed changes

fairclothjm merged commit 269df8e into feat-mux Feb 10, 2022

fairclothjm deleted the feat-mux-plugin-cleanup branch February 10, 2022 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: plugin multiplexing: handle plugin client cleanup #13896

feat: plugin multiplexing: handle plugin client cleanup #13896

fairclothjm commented Feb 4, 2022 •

edited

Loading

fairclothjm Feb 9, 2022

calvn Feb 9, 2022

calvn Feb 9, 2022 •

edited

Loading

feat: plugin multiplexing: handle plugin client cleanup #13896

feat: plugin multiplexing: handle plugin client cleanup #13896

Conversation

fairclothjm commented Feb 4, 2022 • edited Loading

Description

To test multiplexing

fairclothjm Feb 9, 2022

Choose a reason for hiding this comment

calvn Feb 9, 2022

Choose a reason for hiding this comment

calvn Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

fairclothjm commented Feb 4, 2022 •

edited

Loading

calvn Feb 9, 2022 •

edited

Loading