From 55cbbded6cab2bddeecbd9be65551b72e60e50d2 Mon Sep 17 00:00:00 2001
From: Michael Schurter <mschurter@hashicorp.com>
Date: Thu, 21 Feb 2019 12:02:41 -0800
Subject: [PATCH] logmon: fix reattach configuration

There were multiple bugs here:

1. Reattach unmarshalling always returned an error because you can't
   unmarshal into a nil pointer.
2. The hook data wasn't being saved because it was put on the request
   struct, not the response struct.
3. The plugin configuration should only have reattach *or* a command
   set. Not both.
4. Setting Done=true meant the hook was never re-run on agent restart so
   reattaching was never attempted.
---
 client/allocrunner/taskrunner/logmon_hook.go | 19 +++++++++++--------
 client/logmon/plugin.go                      | 17 ++++++++++++-----
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/client/allocrunner/taskrunner/logmon_hook.go b/client/allocrunner/taskrunner/logmon_hook.go
index 06617f686..93d8ef37d 100644
--- a/client/allocrunner/taskrunner/logmon_hook.go
+++ b/client/allocrunner/taskrunner/logmon_hook.go
@@ -15,6 +15,12 @@ import (
 	pstructs "github.com/hashicorp/nomad/plugins/shared/structs"
 )
 
+const (
+	// logmonReattachKey is the HookData key where logmon's reattach config
+	// is stored.
+	logmonReattachKey = "reattach_config"
+)
+
 // logmonHook launches logmon and manages task logging
 type logmonHook struct {
 	// logmon is the handle to the log monitor process for the task.
@@ -72,17 +78,17 @@ func (h *logmonHook) launchLogMon(reattachConfig *plugin.ReattachConfig) error {
 }
 
 func reattachConfigFromHookData(data map[string]string) (*plugin.ReattachConfig, error) {
-	if data == nil || data["reattach_config"] == "" {
+	if data == nil || data[logmonReattachKey] == "" {
 		return nil, nil
 	}
 
-	var cfg *pstructs.ReattachConfig
-	err := json.Unmarshal([]byte(data["reattach_config"]), cfg)
+	var cfg pstructs.ReattachConfig
+	err := json.Unmarshal([]byte(data[logmonReattachKey]), &cfg)
 	if err != nil {
 		return nil, err
 	}
 
-	return pstructs.ReattachConfigToGoPlugin(cfg)
+	return pstructs.ReattachConfigToGoPlugin(&cfg)
 }
 
 func (h *logmonHook) Prestart(ctx context.Context,
@@ -122,14 +128,11 @@ func (h *logmonHook) Prestart(ctx context.Context,
 	if err != nil {
 		return err
 	}
-	req.HookData = map[string]string{"reattach_config": string(jsonCfg)}
-
-	resp.Done = true
+	resp.HookData = map[string]string{logmonReattachKey: string(jsonCfg)}
 	return nil
 }
 
 func (h *logmonHook) Stop(context.Context, *interfaces.TaskStopRequest, *interfaces.TaskStopResponse) error {
-
 	if h.logmon != nil {
 		h.logmon.Stop()
 	}
diff --git a/client/logmon/plugin.go b/client/logmon/plugin.go
index 6f8577807..07c8dfbf1 100644
--- a/client/logmon/plugin.go
+++ b/client/logmon/plugin.go
@@ -12,7 +12,7 @@ import (
 	"google.golang.org/grpc"
 )
 
-// LaunchLogMon an instance of logmon
+// LaunchLogMon launches a new logmon or reattaches to an existing one.
 // TODO: Integrate with base plugin loader
 func LaunchLogMon(logger hclog.Logger, reattachConfig *plugin.ReattachConfig) (LogMon, *plugin.Client, error) {
 	logger = logger.Named("logmon")
@@ -21,18 +21,25 @@ func LaunchLogMon(logger hclog.Logger, reattachConfig *plugin.ReattachConfig) (L
 		return nil, nil, err
 	}
 
-	client := plugin.NewClient(&plugin.ClientConfig{
+	conf := &plugin.ClientConfig{
 		HandshakeConfig: base.Handshake,
-		Reattach:        reattachConfig,
 		Plugins: map[string]plugin.Plugin{
 			"logmon": &Plugin{},
 		},
-		Cmd: exec.Command(bin, "logmon"),
 		AllowedProtocols: []plugin.Protocol{
 			plugin.ProtocolGRPC,
 		},
 		Logger: logger,
-	})
+	}
+
+	// Only set one of Cmd or Reattach
+	if reattachConfig == nil {
+		conf.Cmd = exec.Command(bin, "logmon")
+	} else {
+		conf.Reattach = reattachConfig
+	}
+
+	client := plugin.NewClient(conf)
 
 	rpcClient, err := client.Client()
 	if err != nil {