Make config.Cmd and config.RunnerFunc mutually exclusive (#272)

A set of small follow-ups to #270:

* Make `ClientConfig.Cmd` and `ClientConfig.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; accommodates creating a `ReattachFunc` and also useful for client's debug log information.

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

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,