MM-57018: support reattaching plugins (#26421)

* ProfileImageBytes for EnsureBotOptions

* leverage plugintest.NewAPI

* fix linting

* add UpdateUserRoles to plugin api

* MM-57018: support reattaching plugins

Expose a local-only API for reattaching plugins: instead of the server starting and managing the process itself, allow the plugin to be launched externally (eg within a unit test) and reattach to an existing server instance to provide the unit test with a fully functional RPC API, sidestepping the need for mocking the plugin API in most cases.

In the future, this may become the basis for running plugins in a sidecar container.

Fixes: https://mattermost.atlassian.net/browse/MM-57018

* drop unused supervisor.pid

* factor out checkMinServerVersion

* factor out startPluginServer

* restore missing setPluginState on successful reattach

* avoid passing around a stale registeredPlugin

* inline initializePluginImplementation

* have IsValid return an error

* explicitly close rpcClient

In the case of reattached plugins, the Unix socket won't necessarily disappear leaving the muxBrokers blocked indefinitely. And `Kill()` doesn't do anything if there's no process being managed.

* explicitly detachPlugin

* emphasize gRPC not being supported

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/public/plugin/client.go

@@ -4,6 +4,8 @@
 package plugin
 
 import (
+	"context"
+
 	"github.com/hashicorp/go-plugin"
 )
 
@@ -12,10 +14,51 @@ const (
 	BotUserKey        = InternalKeyPrefix + "botid"
 )
 
-// Starts the serving of a Mattermost plugin over net/rpc. gRPC is not yet supported.
+// WithTestContext provides a context typically used to terminate a plugin from a unit test.
+func WithTestContext(ctx context.Context) func(*plugin.ServeConfig) error {
+	return func(config *plugin.ServeConfig) error {
+		if config.Test == nil {
+			config.Test = &plugin.ServeTestConfig{}
+		}
+
+		config.Test.Context = ctx
+
+		return nil
+	}
+}
+
+// WithTestReattachConfigCh configures the channel to receive the ReattachConfig used to reattach
+// an externally launched plugin instance with the Mattermost server.
+func WithTestReattachConfigCh(reattachConfigCh chan<- *plugin.ReattachConfig) func(*plugin.ServeConfig) error {
+	return func(config *plugin.ServeConfig) error {
+		if config.Test == nil {
+			config.Test = &plugin.ServeTestConfig{}
+		}
+
+		config.Test.ReattachConfigCh = reattachConfigCh
+
+		return nil
+	}
+}
+
+// WithTestCloseCh provides a channel that signals when the plugin exits.
+func WithTestCloseCh(closeCh chan<- struct{}) func(*plugin.ServeConfig) error {
+	return func(config *plugin.ServeConfig) error {
+		if config.Test == nil {
+			config.Test = &plugin.ServeTestConfig{}
+		}
+
+		config.Test.CloseCh = closeCh
+
+		return nil
+	}
+}
+
+// Starts the serving of a Mattermost plugin over net/rpc. gRPC is not supported.
 //
-// Call this when your plugin is ready to start.
-func ClientMain(pluginImplementation any) {
+// Call this when your plugin is ready to start. Options allow configuring plugins for testing
+// scenarios.
+func ClientMain(pluginImplementation any, opts ...func(config *plugin.ServeConfig) error) {
 	impl, ok := pluginImplementation.(interface {
 		SetAPI(api API)
 		SetDriver(driver Driver)
@@ -30,10 +73,19 @@ func ClientMain(pluginImplementation any) {
 		"hooks": &hooksPlugin{hooks: pluginImplementation},
 	}
 
-	plugin.Serve(&plugin.ServeConfig{
+	serveConfig := &plugin.ServeConfig{
 		HandshakeConfig: handshake,
 		Plugins:         pluginMap,
-	})
+	}
+
+	for _, opt := range opts {
+		err := opt(serveConfig)
+		if err != nil {
+			panic("failed to start serving plugin: " + err.Error())
+		}
+	}
+
+	plugin.Serve(serveConfig)
 }
 
 type MattermostPlugin struct {

server/public/plugin/environment.go

@@ -11,6 +11,7 @@ import (
 	"sync"
 	"time"
 
+	plugin "github.com/hashicorp/go-plugin"
 	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost/server/public/model"
@@ -196,6 +197,15 @@ func (env *Environment) setPluginState(id string, state int) {
 	}
 }
 
+// setPluginSupervisor records the supervisor for a registered plugin.
+func (env *Environment) setPluginSupervisor(id string, supervisor *supervisor) {
+	if rp, ok := env.registeredPlugins.Load(id); ok {
+		p := rp.(registeredPlugin)
+		p.supervisor = supervisor
+		env.registeredPlugins.Store(id, p)
+	}
+}
+
 // PublicFilesPath returns a path and true if the plugin with the given id is active.
 // It returns an empty string and false if the path is not set or invalid
 func (env *Environment) PublicFilesPath(id string) (string, error) {
@@ -254,6 +264,46 @@ func (env *Environment) GetManifest(pluginId string) (*model.Manifest, error) {
 	return nil, ErrNotFound
 }
 
+func checkMinServerVersion(pluginInfo *model.BundleInfo) error {
+	if pluginInfo.Manifest.MinServerVersion == "" {
+		return nil
+	}
+
+	fulfilled, err := pluginInfo.Manifest.MeetMinServerVersion(model.CurrentVersion)
+	if err != nil {
+		return fmt.Errorf("%v: %v", err.Error(), pluginInfo.Manifest.Id)
+	}
+	if !fulfilled {
+		return fmt.Errorf("plugin requires Mattermost %v: %v", pluginInfo.Manifest.MinServerVersion, pluginInfo.Manifest.Id)
+	}
+
+	return nil
+}
+
+func (env *Environment) startPluginServer(pluginInfo *model.BundleInfo, opts ...func(*supervisor, *plugin.ClientConfig) error) error {
+	sup, err := newSupervisor(pluginInfo, env.newAPIImpl(pluginInfo.Manifest), env.dbDriver, env.logger, env.metrics, opts...)
+	if err != nil {
+		return errors.Wrapf(err, "unable to start plugin: %v", pluginInfo.Manifest.Id)
+	}
+
+	// We pre-emptively set the state to running to prevent re-entrancy issues.
+	// The plugin's OnActivate hook can in-turn call UpdateConfiguration
+	// which again calls this method. This method is guarded against multiple calls,
+	// but fails if it is called recursively.
+	//
+	// Therefore, setting the state to running prevents this from happening,
+	// and in case there is an error, the defer clause will set the proper state anyways.
+	env.setPluginState(pluginInfo.Manifest.Id, model.PluginStateRunning)
+
+	if err := sup.Hooks().OnActivate(); err != nil {
+		sup.Shutdown()
+		return err
+	}
+	env.setPluginSupervisor(pluginInfo.Manifest.Id, sup)
+
+	return nil
+}
+
 func (env *Environment) Activate(id string) (manifest *model.Manifest, activated bool, reterr error) {
 	defer func() {
 		if reterr != nil {
@@ -296,20 +346,16 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 		}
 	}()
 
-	if pluginInfo.Manifest.MinServerVersion != "" {
-		fulfilled, err := pluginInfo.Manifest.MeetMinServerVersion(model.CurrentVersion)
-		if err != nil {
-			return nil, false, fmt.Errorf("%v: %v", err.Error(), id)
-		}
-		if !fulfilled {
-			return nil, false, fmt.Errorf("plugin requires Mattermost %v: %v", pluginInfo.Manifest.MinServerVersion, id)
-		}
+	err = checkMinServerVersion(pluginInfo)
+	if err != nil {
+		return nil, false, err
 	}
 
 	componentActivated := false
 
 	if pluginInfo.Manifest.HasWebapp() {
-		updatedManifest, err := env.UnpackWebappBundle(id)
+		var updatedManifest *model.Manifest
+		updatedManifest, err = env.UnpackWebappBundle(id)
 		if err != nil {
 			return nil, false, errors.Wrapf(err, "unable to generate webapp bundle: %v", id)
 		}
@@ -319,27 +365,10 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 	}
 
 	if pluginInfo.Manifest.HasServer() {
-		sup, err := newSupervisor(pluginInfo, env.newAPIImpl(pluginInfo.Manifest), env.dbDriver, env.logger, env.metrics)
+		err = env.startPluginServer(pluginInfo, WithExecutableFromManifest(pluginInfo))
 		if err != nil {
-			return nil, false, errors.Wrapf(err, "unable to start plugin: %v", id)
-		}
-
-		// We pre-emptively set the state to running to prevent re-entrancy issues.
-		// The plugin's OnActivate hook can in-turn call UpdateConfiguration
-		// which again calls this method. This method is guarded against multiple calls,
-		// but fails if it is called recursively.
-		//
-		// Therefore, setting the state to running prevents this from happening,
-		// and in case there is an error, the defer clause will set the proper state anyways.
-		env.setPluginState(id, model.PluginStateRunning)
-
-		if err := sup.Hooks().OnActivate(); err != nil {
-			sup.Shutdown()
 			return nil, false, err
 		}
-		rp.supervisor = sup
-		env.registeredPlugins.Store(id, rp)
-
 		componentActivated = true
 	}
 
@@ -352,6 +381,64 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 	return pluginInfo.Manifest, true, nil
 }
 
+// Reattach allows the server to bind to an existing plugin instance launched elsewhere.
+func (env *Environment) Reattach(manifest *model.Manifest, pluginReattachConfig *model.PluginReattachConfig) (reterr error) {
+	id := manifest.Id
+
+	defer func() {
+		if reterr != nil {
+			env.SetPluginError(id, reterr.Error())
+		} else {
+			env.SetPluginError(id, "")
+		}
+	}()
+
+	// Check if we are already active
+	if env.IsActive(id) {
+		return nil
+	}
+
+	pluginInfo := &model.BundleInfo{
+		Path:          "",
+		Manifest:      manifest,
+		ManifestPath:  "",
+		ManifestError: nil,
+	}
+
+	rp := newRegisteredPlugin(pluginInfo)
+	env.registeredPlugins.Store(id, rp)
+
+	defer func() {
+		if reterr == nil {
+			env.setPluginState(id, model.PluginStateRunning)
+		} else {
+			env.setPluginState(id, model.PluginStateFailedToStart)
+		}
+	}()
+
+	err := checkMinServerVersion(pluginInfo)
+	if err != nil {
+		return nil
+	}
+
+	if !pluginInfo.Manifest.HasServer() {
+		return errors.New("cannot reattach plugin without server component")
+	}
+
+	if pluginInfo.Manifest.HasWebapp() {
+		env.logger.Warn("Ignoring webapp for reattached plugin", mlog.String("plugin_id", id))
+	}
+
+	err = env.startPluginServer(pluginInfo, WithReattachConfig(pluginReattachConfig))
+	if err != nil {
+		return nil
+	}
+
+	mlog.Debug("Plugin reattached", mlog.String("plugin_id", pluginInfo.Manifest.Id), mlog.String("version", pluginInfo.Manifest.Version))
+
+	return nil
+}
+
 func (env *Environment) RemovePlugin(id string) {
 	if _, ok := env.registeredPlugins.Load(id); ok {
 		env.registeredPlugins.Delete(id)

server/public/plugin/health_check_test.go

@@ -52,7 +52,7 @@ func testPluginHealthCheckSuccess(t *testing.T) {
 
 	bundle := model.BundleInfoForPath(dir)
 	logger := mlog.CreateConsoleTestLogger(t)
-	supervisor, err := newSupervisor(bundle, nil, nil, logger, nil)
+	supervisor, err := newSupervisor(bundle, nil, nil, logger, nil, WithExecutableFromManifest(bundle))
 	require.NoError(t, err)
 	require.NotNil(t, supervisor)
 	defer supervisor.Shutdown()
@@ -93,7 +93,7 @@ func testPluginHealthCheckPanic(t *testing.T) {
 
 	bundle := model.BundleInfoForPath(dir)
 	logger := mlog.CreateConsoleTestLogger(t)
-	supervisor, err := newSupervisor(bundle, nil, nil, logger, nil)
+	supervisor, err := newSupervisor(bundle, nil, nil, logger, nil, WithExecutableFromManifest(bundle))
 	require.NoError(t, err)
 	require.NotNil(t, supervisor)
 	defer supervisor.Shutdown()

server/public/plugin/plugintest/example_reattach_test.go

@@ -0,0 +1,85 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package plugintest_test
+
+import (
+	"context"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	goPlugin "github.com/hashicorp/go-plugin"
+	"github.com/mattermost/mattermost/server/public/model"
+	"github.com/mattermost/mattermost/server/public/plugin"
+)
+
+type UnitTestedPlugin struct {
+	plugin.MattermostPlugin
+}
+
+// This example demonstrates a plugin that's launched during a unit test and reattached to an
+// existing server instance to obtain a real PluginAPI.
+func Example_unitTestingPlugins() {
+	t := &testing.T{}
+
+	// The manifest is usually generated dynamically.
+	manifest := &model.Manifest{
+		Id: "reattach-plugin-test",
+	}
+
+	// ctx, and specifically cancel, gives us control over the plugin lifecycle
+	ctx, cancel := context.WithCancel(context.Background())
+
+	// reattachConfigCh is the means by which we get the Unix socket information to relay back
+	// to the server and finish the reattachment.
+	reattachConfigCh := make(chan *goPlugin.ReattachConfig)
+
+	// closeCh tells us when the plugin exits and allows for cleanup.
+	closeCh := make(chan struct{})
+
+	// plugin.ClientMain with options allows for reattachment.
+	go plugin.ClientMain(
+		&UnitTestedPlugin{},
+		plugin.WithTestContext(ctx),
+		plugin.WithTestReattachConfigCh(reattachConfigCh),
+		plugin.WithTestCloseCh(closeCh),
+	)
+
+	// Make sure the plugin shuts down normally with the test
+	t.Cleanup(func() {
+		cancel()
+
+		select {
+		case <-closeCh:
+		case <-time.After(5 * time.Second):
+			panic("plugin failed to close after 5 seconds")
+		}
+	})
+
+	// Wait for the plugin to start and then reattach to the server.
+	var reattachConfig *goPlugin.ReattachConfig
+	select {
+	case reattachConfig = <-reattachConfigCh:
+	case <-time.After(5 * time.Second):
+		t.Fatal("failed to get reattach config")
+	}
+
+	// Reattaching requires a local mode client.
+	socketPath := os.Getenv("MM_LOCALSOCKETPATH")
+	if socketPath == "" {
+		socketPath = model.LocalModeSocketPath
+	}
+
+	clientLocal := model.NewAPIv4SocketClient(socketPath)
+	_, err := clientLocal.ReattachPlugin(ctx, &model.PluginReattachRequest{
+		Manifest:             manifest,
+		PluginReattachConfig: model.NewPluginReattachConfig(reattachConfig),
+	})
+	require.NoError(t, err)
+
+	// At this point, the plugin is ready for unit testing and will be cleaned up automatically
+	// with the testing.T instance.
+}

server/public/plugin/supervisor.go

@@ -23,15 +23,58 @@ import (
 )
 
 type supervisor struct {
-	lock        sync.RWMutex
-	client      *plugin.Client
-	hooks       Hooks
-	implemented [TotalHooksID]bool
-	pid         int
-	hooksClient *hooksRPCClient
+	lock         sync.RWMutex
+	client       *plugin.Client
+	hooks        Hooks
+	implemented  [TotalHooksID]bool
+	hooksClient  *hooksRPCClient
+	isReattached bool
 }
 
-func newSupervisor(pluginInfo *model.BundleInfo, apiImpl API, driver Driver, parentLogger *mlog.Logger, metrics metricsInterface) (retSupervisor *supervisor, retErr error) {
+func WithExecutableFromManifest(pluginInfo *model.BundleInfo) func(*supervisor, *plugin.ClientConfig) error {
+	return func(_ *supervisor, clientConfig *plugin.ClientConfig) error {
+		executable := pluginInfo.Manifest.GetExecutableForRuntime(runtime.GOOS, runtime.GOARCH)
+		if executable == "" {
+			return fmt.Errorf("backend executable not found for environment: %s/%s", runtime.GOOS, runtime.GOARCH)
+		}
+
+		executable = filepath.Clean(filepath.Join(".", executable))
+		if strings.HasPrefix(executable, "..") {
+			return fmt.Errorf("invalid backend executable: %s", executable)
+		}
+
+		executable = filepath.Join(pluginInfo.Path, executable)
+
+		cmd := exec.Command(executable)
+
+		// This doesn't add more security than before
+		// but removes the SecureConfig is nil warning.
+		// https://mattermost.atlassian.net/browse/MM-49167
+		pluginChecksum, err := getPluginExecutableChecksum(executable)
+		if err != nil {
+			return errors.Wrapf(err, "unable to generate plugin checksum")
+		}
+
+		clientConfig.Cmd = cmd
+		clientConfig.SecureConfig = &plugin.SecureConfig{
+			Checksum: pluginChecksum,
+			Hash:     sha256.New(),
+		}
+
+		return nil
+	}
+}
+
+func WithReattachConfig(pluginReattachConfig *model.PluginReattachConfig) func(*supervisor, *plugin.ClientConfig) error {
+	return func(sup *supervisor, clientConfig *plugin.ClientConfig) error {
+		clientConfig.Reattach = pluginReattachConfig.ToHashicorpPluginReattachmentConfig()
+		sup.isReattached = true
+
+		return nil
+	}
+}
+
+func newSupervisor(pluginInfo *model.BundleInfo, apiImpl API, driver Driver, parentLogger *mlog.Logger, metrics metricsInterface, opts ...func(*supervisor, *plugin.ClientConfig) error) (retSupervisor *supervisor, retErr error) {
 	sup := supervisor{}
 	defer func() {
 		if retErr != nil {
@@ -54,49 +97,28 @@ func newSupervisor(pluginInfo *model.BundleInfo, apiImpl API, driver Driver, par
 		},
 	}
 
-	executable := pluginInfo.Manifest.GetExecutableForRuntime(runtime.GOOS, runtime.GOARCH)
-	if executable == "" {
-		return nil, fmt.Errorf("backend executable not found for environment: %s/%s", runtime.GOOS, runtime.GOARCH)
-	}
-
-	executable = filepath.Clean(filepath.Join(".", executable))
-	if strings.HasPrefix(executable, "..") {
-		return nil, fmt.Errorf("invalid backend executable: %s", executable)
-	}
-
-	executable = filepath.Join(pluginInfo.Path, executable)
-
-	cmd := exec.Command(executable)
-
-	// This doesn't add more security than before
-	// but removes the SecureConfig is nil warning.
-	// https://mattermost.atlassian.net/browse/MM-49167
-	pluginChecksum, err := getPluginExecutableChecksum(executable)
-	if err != nil {
-		return nil, errors.Wrapf(err, "unable to generate plugin checksum")
-	}
-
-	sup.client = plugin.NewClient(&plugin.ClientConfig{
+	clientConfig := &plugin.ClientConfig{
 		HandshakeConfig: handshake,
 		Plugins:         pluginMap,
-		Cmd:             cmd,
 		SyncStdout:      wrappedLogger.With(mlog.String("source", "plugin_stdout")).StdLogWriter(),
 		SyncStderr:      wrappedLogger.With(mlog.String("source", "plugin_stderr")).StdLogWriter(),
 		Logger:          hclogAdaptedLogger,
 		StartTimeout:    time.Second * 3,
-		SecureConfig: &plugin.SecureConfig{
-			Checksum: pluginChecksum,
-			Hash:     sha256.New(),
-		},
-	})
+	}
+	for _, opt := range opts {
+		err := opt(&sup, clientConfig)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to apply option")
+		}
+	}
+
+	sup.client = plugin.NewClient(clientConfig)
 
 	rpcClient, err := sup.client.Client()
 	if err != nil {
 		return nil, err
 	}
 
-	sup.pid = cmd.Process.Pid
-
 	raw, err := rpcClient.Dispense("hooks")
 	if err != nil {
 		return nil, err
@@ -126,6 +148,20 @@ func (sup *supervisor) Shutdown() {
 	sup.lock.RLock()
 	defer sup.lock.RUnlock()
 	if sup.client != nil {
+		// For reattached plugins, Kill() is mostly a no-op, so manually clean up the
+		// underlying rpcClient. This might be something to upstream unless we're doing
+		// something else wrong.
+		if sup.isReattached {
+			rpcClient, err := sup.client.Client()
+			if err != nil {
+				mlog.Warn("Failed to obtain rpcClient on Shutdown")
+			} else {
+				if err = rpcClient.Close(); err != nil {
+					mlog.Warn("Failed to close rpcClient on Shutdown")
+				}
+			}
+		}
+
 		sup.client.Kill()
 	}