From ea795a53523cc7b286aa814fe87505880464ef8b Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 24 Aug 2023 20:54:41 +0100 Subject: [PATCH] Make config.Cmd and config.RunnerFunc mutually exclusive A set of small follow-ups to #270: * Make `config.Cmd` and `config.RunnerFunc` mutually exclusive. * Clients setting `RunnerFunc` can call `ReattachConfig` but need to supply their own `ReattachFunc` as it can't necessarily be derived from `RunnerFunc`. Exercise `ReattachConfig()` in tests in a way that previously panicked. * Improve 1 logger line for non-cmd implementations. * Add `ID()` function to client; allows clients to construct a `ReattachFunc` and also useful for client's debug log information. --- client.go | 52 +++++++++++++++++++++++++++++++++++++++++--------- client_test.go | 19 ++++++++++++++++-- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/client.go b/client.go index c81258cd..93b5b5fe 100644 --- a/client.go +++ b/client.go @@ -118,6 +118,19 @@ func (c *Client) NegotiatedVersion() int { return c.negotiatedVersion } +// ID returns a unique ID for the running plugin. By default this is the process +// ID (pid), but it could take other forms if RunnerFunc was provided. +func (c *Client) ID() string { + c.l.Lock() + defer c.l.Unlock() + + if c.runner != nil { + return c.runner.ID() + } + + return "" +} + // ClientConfig is the configuration used to initialize a new // plugin client. After being used to initialize a plugin client, // that configuration must not be modified again. @@ -539,14 +552,21 @@ func (c *Client) Start() (addr net.Addr, err error) { // this in a {} for scoping reasons, and hopeful that the escape // analysis will pop the stack here. { - cmdSet := c.config.Cmd != nil - attachSet := c.config.Reattach != nil - secureSet := c.config.SecureConfig != nil - if cmdSet == attachSet { - return nil, fmt.Errorf("exactly one of Cmd or Reattach must be set") + var mutuallyExclusiveOptions int + if c.config.Cmd != nil { + mutuallyExclusiveOptions += 1 + } + if c.config.Reattach != nil { + mutuallyExclusiveOptions += 1 + } + if c.config.RunnerFunc != nil { + mutuallyExclusiveOptions += 1 + } + if mutuallyExclusiveOptions != 1 { + return nil, fmt.Errorf("exactly one of Cmd, or Reattach, or RunnerFunc must be set") } - if secureSet && attachSet { + if c.config.SecureConfig != nil && c.config.Reattach != nil { return nil, ErrSecureConfigAndReattach } } @@ -582,6 +602,12 @@ func (c *Client) Start() (addr net.Addr, err error) { } cmd := c.config.Cmd + if cmd == nil { + // It's only possible to get here if RunnerFunc is non-nil, but we'll + // still use cmd as a spec to populate metadata for the external + // implementation to consume. + cmd = exec.Command("") + } if !c.config.SkipHostEnv { cmd.Env = append(cmd.Env, os.Environ()...) } @@ -731,7 +757,7 @@ func (c *Client) Start() (addr net.Addr, err error) { timeout := time.After(c.config.StartTimeout) // Start looking for the address - c.logger.Debug("waiting for RPC address", "path", cmd.Path) + c.logger.Debug("waiting for RPC address", "plugin", runner.Name()) select { case <-timeout: err = errors.New("timeout while waiting for plugin to start") @@ -939,6 +965,9 @@ func (c *Client) checkProtoVersion(protoVersion string) (int, PluginSet, error) // // If this returns nil then the process hasn't been started yet. Please // call Start or Client before calling this. +// +// Clients who specified a RunnerFunc will need to populate their own +// ReattachFunc in the returned ReattachConfig before it can be used. func (c *Client) ReattachConfig() *ReattachConfig { c.l.Lock() defer c.l.Unlock() @@ -956,11 +985,16 @@ func (c *Client) ReattachConfig() *ReattachConfig { return c.config.Reattach } - return &ReattachConfig{ + reattach := &ReattachConfig{ Protocol: c.protocol, Addr: c.address, - Pid: c.config.Cmd.Process.Pid, } + + if c.config.Cmd != nil && c.config.Cmd.Process != nil { + reattach.Pid = c.config.Cmd.Process.Pid + } + + return reattach } // Protocol returns the protocol of server on the remote end. This will diff --git a/client_test.go b/client_test.go index cde083c8..8def4aa7 100644 --- a/client_test.go +++ b/client_test.go @@ -320,7 +320,6 @@ func testClient_grpcSyncStdio(t *testing.T, useRunnerFunc bool) { process := helperProcess("test-grpc") cfg := &ClientConfig{ - Cmd: process, HandshakeConfig: testHandshake, Plugins: testGRPCPluginMap, AllowedProtocols: []Protocol{ProtocolGRPC}, @@ -330,8 +329,11 @@ func testClient_grpcSyncStdio(t *testing.T, useRunnerFunc bool) { if useRunnerFunc { cfg.RunnerFunc = func(l hclog.Logger, cmd *exec.Cmd, _ string) (runner.Runner, error) { - return cmdrunner.NewCmdRunner(l, cmd) + process.Env = append(process.Env, cmd.Env...) + return cmdrunner.NewCmdRunner(l, process) } + } else { + cfg.Cmd = process } c := NewClient(cfg) defer c.Kill() @@ -361,6 +363,18 @@ func testClient_grpcSyncStdio(t *testing.T, useRunnerFunc bool) { t.Fatalf("bad: %#v", raw) } + // Check reattach config is sensible. + reattach := c.ReattachConfig() + if useRunnerFunc { + if reattach.Pid != 0 { + t.Fatal(reattach.Pid) + } + } else { + if reattach.Pid == 0 { + t.Fatal(reattach.Pid) + } + } + // Print the data stdout := []byte("hello\nworld!") stderr := []byte("and some error\n messages!") @@ -870,6 +884,7 @@ func TestClient_SkipHostEnv(t *testing.T) { } { t.Run(tc.helper, func(t *testing.T) { process := helperProcess(tc.helper) + // Set env in the host process, which we'll look for in the plugin. t.Setenv("PLUGIN_TEST_SKIP_HOST_ENV", "foo") c := NewClient(&ClientConfig{ Cmd: process,