Clean up subprocess handling and make shell use optional

agent/agent.go

@@ -535,16 +535,40 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
 	var watchPlans []*watch.Plan
 	for _, params := range cfg.Watches {
 		// Parse the watches, excluding the handler
-		wp, err := watch.ParseExempt(params, []string{"handler"})
+		wp, err := watch.ParseExempt(params, []string{"handler", "args"})
 		if err != nil {
 			return fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
 		}
 
-		// Get the handler
-		h := wp.Exempt["handler"]
-		if _, ok := h.(string); h == nil || !ok {
+		// Get the handler and subprocess arguments
+		handler, hasHandler := wp.Exempt["handler"]
+		args, hasArgs := wp.Exempt["args"]
+		if hasHandler {
+			a.logger.Printf("[WARN] agent: The 'handler' field in watches has been deprecated " +
+				"and replaced with the 'args' field. See https://www.consul.io/docs/agent/watches.html")
+		}
+		if _, ok := handler.(string); hasHandler && !ok {
 			return fmt.Errorf("Watch handler must be a string")
 		}
+		if raw, ok := args.([]interface{}); hasArgs && ok {
+			var parsed []string
+			for _, arg := range raw {
+				if v, ok := arg.(string); !ok {
+					return fmt.Errorf("Watch args must be a list of strings")
+				} else {
+					parsed = append(parsed, v)
+				}
+			}
+			wp.Exempt["args"] = parsed
+		} else if hasArgs && !ok {
+			return fmt.Errorf("Watch args must be a list of strings")
+		}
+		if hasHandler && hasArgs {
+			return fmt.Errorf("Cannot define both watch handler and args")
+		}
+		if !hasHandler && !hasArgs {
+			return fmt.Errorf("Must define either watch handler or args")
+		}
 
 		// Store the watch plan
 		watchPlans = append(watchPlans, wp)
@@ -566,7 +590,13 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
 	for _, wp := range watchPlans {
 		a.watchPlans = append(a.watchPlans, wp)
 		go func(wp *watch.Plan) {
-			wp.Handler = makeWatchHandler(a.LogOutput, wp.Exempt["handler"])
+			var handler interface{}
+			if h, ok := wp.Exempt["handler"]; ok {
+				handler = h
+			} else {
+				handler = wp.Exempt["args"]
+			}
+			wp.Handler = makeWatchHandler(a.LogOutput, handler)
 			wp.LogOutput = a.LogOutput
 			if err := wp.Run(addr); err != nil {
 				a.logger.Printf("[ERR] Failed to run watch: %v", err)
@@ -1681,6 +1711,7 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *structs.CheckType,
 				DockerContainerID: chkType.DockerContainerID,
 				Shell:             chkType.Shell,
 				Script:            chkType.Script,
+				ScriptArgs:        chkType.ScriptArgs,
 				Interval:          chkType.Interval,
 				Logger:            a.logger,
 				client:            a.dockerClient,
@@ -1694,18 +1725,24 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *structs.CheckType,
 				delete(a.checkMonitors, check.CheckID)
 			}
 			if chkType.Interval < MinInterval {
-				a.logger.Println(fmt.Sprintf("[WARN] agent: check '%s' has interval below minimum of %v",
-					check.CheckID, MinInterval))
+				a.logger.Printf("[WARN] agent: check '%s' has interval below minimum of %v",
+					check.CheckID, MinInterval)
 				chkType.Interval = MinInterval
 			}
+			if chkType.Script != "" {
+				a.logger.Printf("[WARN] agent: check %q has the 'script' field, which has been deprecated "+
+					"and replaced with the 'args' field. See https://www.consul.io/docs/agent/checks.html",
+					check.CheckID)
+			}
 
 			monitor := &CheckMonitor{
-				Notify:   a.state,
-				CheckID:  check.CheckID,
-				Script:   chkType.Script,
-				Interval: chkType.Interval,
-				Timeout:  chkType.Timeout,
-				Logger:   a.logger,
+				Notify:     a.state,
+				CheckID:    check.CheckID,
+				Script:     chkType.Script,
+				ScriptArgs: chkType.ScriptArgs,
+				Interval:   chkType.Interval,
+				Timeout:    chkType.Timeout,
+				Logger:     a.logger,
 			}
 			monitor.Start()
 			a.checkMonitors[check.CheckID] = monitor

agent/agent_test.go

@@ -1928,9 +1928,9 @@ func TestAgent_reloadWatches(t *testing.T) {
 	newConf := *a.config
 	newConf.Watches = []map[string]interface{}{
 		{
-			"type":    "key",
-			"key":     "asdf",
-			"handler": "ls",
+			"type": "key",
+			"key":  "asdf",
+			"args": []interface{}{"ls"},
 		},
 	}
 	if err := a.reloadWatches(&newConf); err != nil {
@@ -1942,9 +1942,9 @@ func TestAgent_reloadWatches(t *testing.T) {
 	newConf.HTTPAddrs = make([]net.Addr, 0)
 	newConf.Watches = []map[string]interface{}{
 		{
-			"type":    "key",
-			"key":     "asdf",
-			"handler": "ls",
+			"type": "key",
+			"key":  "asdf",
+			"args": []interface{}{"ls"},
 		},
 	}
 	if err := a.reloadWatches(&newConf); err != nil {
@@ -1955,9 +1955,9 @@ func TestAgent_reloadWatches(t *testing.T) {
 	newConf.HTTPSAddrs = make([]net.Addr, 0)
 	newConf.Watches = []map[string]interface{}{
 		{
-			"type":    "key",
-			"key":     "asdf",
-			"handler": "ls",
+			"type": "key",
+			"key":  "asdf",
+			"args": []interface{}{"ls"},
 		},
 	}
 	if err := a.reloadWatches(&newConf); err == nil || !strings.Contains(err.Error(), "watch plans require an HTTP or HTTPS endpoint") {

agent/check.go

@@ -49,12 +49,13 @@ type CheckNotifier interface {
 // determine the health of a given check. It is compatible with
 // nagios plugins and expects the output in the same format.
 type CheckMonitor struct {
-	Notify   CheckNotifier
-	CheckID  types.CheckID
-	Script   string
-	Interval time.Duration
-	Timeout  time.Duration
-	Logger   *log.Logger
+	Notify     CheckNotifier
+	CheckID    types.CheckID
+	Script     string
+	ScriptArgs []string
+	Interval   time.Duration
+	Timeout    time.Duration
+	Logger     *log.Logger
 
 	stop     bool
 	stopCh   chan struct{}
@@ -101,7 +102,13 @@ func (c *CheckMonitor) run() {
 // check is invoked periodically to perform the script check
 func (c *CheckMonitor) check() {
 	// Create the command
-	cmd, err := ExecScript(c.Script)
+	var cmd *exec.Cmd
+	var err error
+	if len(c.ScriptArgs) > 0 {
+		cmd, err = ExecSubprocess(c.ScriptArgs)
+	} else {
+		cmd, err = ExecScript(c.Script)
+	}
 	if err != nil {
 		c.Logger.Printf("[ERR] agent: failed to setup invoke '%s': %s", c.Script, err)
 		c.Notify.UpdateCheck(c.CheckID, api.HealthCritical, err.Error())
@@ -524,6 +531,7 @@ type CheckDocker struct {
 	Notify            CheckNotifier
 	CheckID           types.CheckID
 	Script            string
+	ScriptArgs        []string
 	DockerContainerID string
 	Shell             string
 	Interval          time.Duration
@@ -599,7 +607,13 @@ func (c *CheckDocker) check() {
 }
 
 func (c *CheckDocker) doCheck() (string, *circbuf.Buffer, error) {
-	cmd := []string{c.Shell, "-c", c.Script}
+	var cmd []string
+	if len(c.ScriptArgs) > 0 {
+		cmd = c.ScriptArgs
+	} else {
+		cmd = []string{c.Shell, "-c", c.Script}
+	}
+
 	execID, err := c.client.CreateExec(c.DockerContainerID, cmd)
 	if err != nil {
 		return api.HealthCritical, nil, err

agent/check_test.go

@@ -20,7 +20,7 @@ import (
 	"github.com/hashicorp/consul/types"
 )
 
-func TestCheckMonitor(t *testing.T) {
+func TestCheckMonitor_Script(t *testing.T) {
 	tests := []struct {
 		script, status string
 	}{
@@ -54,16 +54,51 @@ func TestCheckMonitor(t *testing.T) {
 	}
 }
 
+func TestCheckMonitor_Args(t *testing.T) {
+	tests := []struct {
+		args   []string
+		status string
+	}{
+		{[]string{"sh", "-c", "exit 0"}, "passing"},
+		{[]string{"sh", "-c", "exit 1"}, "warning"},
+		{[]string{"sh", "-c", "exit 2"}, "critical"},
+		{[]string{"foobarbaz"}, "critical"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.status, func(t *testing.T) {
+			notif := mock.NewNotify()
+			check := &CheckMonitor{
+				Notify:     notif,
+				CheckID:    types.CheckID("foo"),
+				ScriptArgs: tt.args,
+				Interval:   25 * time.Millisecond,
+				Logger:     log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
+			}
+			check.Start()
+			defer check.Stop()
+			retry.Run(t, func(r *retry.R) {
+				if got, want := notif.Updates("foo"), 2; got < want {
+					r.Fatalf("got %d updates want at least %d", got, want)
+				}
+				if got, want := notif.State("foo"), tt.status; got != want {
+					r.Fatalf("got state %q want %q", got, want)
+				}
+			})
+		})
+	}
+}
+
 func TestCheckMonitor_Timeout(t *testing.T) {
 	// t.Parallel() // timing test. no parallel
 	notif := mock.NewNotify()
 	check := &CheckMonitor{
-		Notify:   notif,
-		CheckID:  types.CheckID("foo"),
-		Script:   "sleep 1 && exit 0",
-		Interval: 50 * time.Millisecond,
-		Timeout:  25 * time.Millisecond,
-		Logger:   log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
+		Notify:     notif,
+		CheckID:    types.CheckID("foo"),
+		ScriptArgs: []string{"sh", "-c", "sleep 1 && exit 0"},
+		Interval:   50 * time.Millisecond,
+		Timeout:    25 * time.Millisecond,
+		Logger:     log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
 	}
 	check.Start()
 	defer check.Stop()
@@ -83,11 +118,11 @@ func TestCheckMonitor_RandomStagger(t *testing.T) {
 	// t.Parallel() // timing test. no parallel
 	notif := mock.NewNotify()
 	check := &CheckMonitor{
-		Notify:   notif,
-		CheckID:  types.CheckID("foo"),
-		Script:   "exit 0",
-		Interval: 25 * time.Millisecond,
-		Logger:   log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
+		Notify:     notif,
+		CheckID:    types.CheckID("foo"),
+		ScriptArgs: []string{"sh", "-c", "exit 0"},
+		Interval:   25 * time.Millisecond,
+		Logger:     log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
 	}
 	check.Start()
 	defer check.Stop()
@@ -108,11 +143,11 @@ func TestCheckMonitor_LimitOutput(t *testing.T) {
 	t.Parallel()
 	notif := mock.NewNotify()
 	check := &CheckMonitor{
-		Notify:   notif,
-		CheckID:  types.CheckID("foo"),
-		Script:   "od -N 81920 /dev/urandom",
-		Interval: 25 * time.Millisecond,
-		Logger:   log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
+		Notify:     notif,
+		CheckID:    types.CheckID("foo"),
+		ScriptArgs: []string{"od", "-N", "81920", "/dev/urandom"},
+		Interval:   25 * time.Millisecond,
+		Logger:     log.New(ioutil.Discard, UniqueID(), log.LstdFlags),
 	}
 	check.Start()
 	defer check.Stop()
@@ -775,7 +810,7 @@ func TestCheck_Docker(t *testing.T) {
 			check := &CheckDocker{
 				Notify:            notif,
 				CheckID:           id,
-				Script:            "/health.sh",
+				ScriptArgs:        []string{"/health.sh"},
 				DockerContainerID: "123",
 				Interval:          25 * time.Millisecond,
 				client:            c,

agent/config/builder.go

@@ -879,6 +879,7 @@ func (b *Builder) checkVal(v *CheckDefinition) *structs.CheckDefinition {
 		Token:             b.stringVal(v.Token),
 		Status:            b.stringVal(v.Status),
 		Script:            b.stringVal(v.Script),
+		ScriptArgs:        v.ScriptArgs,
 		HTTP:              b.stringVal(v.HTTP),
 		Header:            v.Header,
 		Method:            b.stringVal(v.Method),

agent/config/config.go

@@ -312,6 +312,7 @@ type CheckDefinition struct {
 	Token                          *string             `json:"token,omitempty" hcl:"token" mapstructure:"token"`
 	Status                         *string             `json:"status,omitempty" hcl:"status" mapstructure:"status"`
 	Script                         *string             `json:"script,omitempty" hcl:"script" mapstructure:"script"`
+	ScriptArgs                     []string            `json:"args,omitempty" hcl:"args" mapstructure:"args"`
 	HTTP                           *string             `json:"http,omitempty" hcl:"http" mapstructure:"http"`
 	Header                         map[string][]string `json:"header,omitempty" hcl:"header" mapstructure:"header"`
 	Method                         *string             `json:"method,omitempty" hcl:"method" mapstructure:"method"`