watch: Allow args from different types

Fixes a bug where specifying a slice of args with a single item was being converted to
a string when config was loaded, causing an error.

agent/watch_handler.go

@@ -185,7 +185,7 @@ func makeHTTPWatchHandler(logger hclog.Logger, config *watch.HttpHandlerConfig)
 	return fn
 }
 
-// TODO: return a fully constructed watch.Plan with a Plan.Handler, so that Except
+// TODO: return a fully constructed watch.Plan with a Plan.Handler, so that Exempt
 // can be ignored by the caller.
 func makeWatchPlan(logger hclog.Logger, params map[string]interface{}) (*watch.Plan, error) {
 	wp, err := watch.ParseExempt(params, []string{"handler", "args"})
@@ -193,9 +193,7 @@ func makeWatchPlan(logger hclog.Logger, params map[string]interface{}) (*watch.P
 		return nil, fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
 	}
 
-	// Get the handler and subprocess arguments
 	handler, hasHandler := wp.Exempt["handler"]
-	args, hasArgs := wp.Exempt["args"]
 	if hasHandler {
 		logger.Warn("The 'handler' field in watches has been deprecated " +
 			"and replaced with the 'args' field. See https://www.consul.io/docs/agent/watches.html")
@@ -203,20 +201,15 @@ func makeWatchPlan(logger hclog.Logger, params map[string]interface{}) (*watch.P
 	if _, ok := handler.(string); hasHandler && !ok {
 		return nil, fmt.Errorf("Watch handler must be a string")
 	}
-	if raw, ok := args.([]interface{}); hasArgs && ok {
-		var parsed []string
-		for _, arg := range raw {
-			v, ok := arg.(string)
-			if !ok {
-				return nil, fmt.Errorf("Watch args must be a list of strings")
-			}
 
-			parsed = append(parsed, v)
+	args, hasArgs := wp.Exempt["args"]
+	if hasArgs {
+		wp.Exempt["args"], err = parseWatchArgs(args)
+		if err != nil {
+			return nil, err
 		}
-		wp.Exempt["args"] = parsed
-	} else if hasArgs && !ok {
-		return nil, fmt.Errorf("Watch args must be a list of strings")
 	}
+
 	if hasHandler && hasArgs || hasHandler && wp.HandlerType == "http" || hasArgs && wp.HandlerType == "http" {
 		return nil, fmt.Errorf("Only one watch handler allowed")
 	}
@@ -225,3 +218,25 @@ func makeWatchPlan(logger hclog.Logger, params map[string]interface{}) (*watch.P
 	}
 	return wp, nil
 }
+
+func parseWatchArgs(args interface{}) ([]string, error) {
+	switch args := args.(type) {
+	case string:
+		return []string{args}, nil
+	case []string:
+		return args, nil
+	case []interface{}:
+		result := make([]string, 0, len(args))
+		for _, arg := range args {
+			v, ok := arg.(string)
+			if !ok {
+				return nil, fmt.Errorf("Watch args must be a list of strings")
+			}
+
+			result = append(result, v)
+		}
+		return result, nil
+	default:
+		return nil, fmt.Errorf("Watch args must be a list of strings")
+	}
+}

agent/watch_handler_test.go

@@ -10,10 +10,11 @@ import (
 
 	"github.com/hashicorp/consul/api/watch"
 	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/go-hclog"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMakeWatchHandler(t *testing.T) {
-	t.Parallel()
 	defer os.Remove("handler_out")
 	defer os.Remove("handler_index_out")
 	script := "bash -c 'echo $CONSUL_INDEX >> handler_index_out && cat >> handler_out'"
@@ -36,7 +37,6 @@ func TestMakeWatchHandler(t *testing.T) {
 }
 
 func TestMakeHTTPWatchHandler(t *testing.T) {
-	t.Parallel()
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		idx := r.Header.Get("X-Consul-Index")
 		if idx != "100" {
@@ -65,3 +65,113 @@ func TestMakeHTTPWatchHandler(t *testing.T) {
 	handler := makeHTTPWatchHandler(testutil.Logger(t), &config)
 	handler(100, []string{"foo", "bar", "baz"})
 }
+
+type raw map[string]interface{}
+
+func TestMakeWatchPlan(t *testing.T) {
+	type testCase struct {
+		name        string
+		params      map[string]interface{}
+		expected    func(t *testing.T, plan *watch.Plan)
+		expectedErr string
+	}
+	fn := func(t *testing.T, tc testCase) {
+		plan, err := makeWatchPlan(hclog.New(nil), tc.params)
+		if tc.expectedErr != "" {
+			require.Error(t, err)
+			require.Contains(t, err.Error(), tc.expectedErr)
+			return
+		}
+		require.NoError(t, err)
+		tc.expected(t, plan)
+	}
+	var testCases = []testCase{
+		{
+			name: "handler_type script, with deprecated handler field",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"handler":      "./script.sh",
+			},
+			expected: func(t *testing.T, plan *watch.Plan) {
+				require.Equal(t, plan.HandlerType, "script")
+				require.Equal(t, plan.Exempt["handler"], "./script.sh")
+			},
+		},
+		{
+			name: "handler_type script, with single arg",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"args":         "./script.sh",
+			},
+			expected: func(t *testing.T, plan *watch.Plan) {
+				require.Equal(t, plan.HandlerType, "script")
+				require.Equal(t, plan.Exempt["args"], []string{"./script.sh"})
+			},
+		},
+		{
+			name: "handler_type script, with multiple args from slice of interface",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"args":         []interface{}{"./script.sh", "arg1"},
+			},
+			expected: func(t *testing.T, plan *watch.Plan) {
+				require.Equal(t, plan.HandlerType, "script")
+				require.Equal(t, plan.Exempt["args"], []string{"./script.sh", "arg1"})
+			},
+		},
+		{
+			name: "handler_type script, with multiple args from slice of strings",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"args":         []string{"./script.sh", "arg1"},
+			},
+			expected: func(t *testing.T, plan *watch.Plan) {
+				require.Equal(t, plan.HandlerType, "script")
+				require.Equal(t, plan.Exempt["args"], []string{"./script.sh", "arg1"})
+			},
+		},
+		{
+			name: "handler_type script, with not string args",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"args":         []interface{}{"./script.sh", true},
+			},
+			expectedErr: "Watch args must be a list of strings",
+		},
+		{
+			name: "conflicting handler",
+			params: raw{
+				"type":         "key",
+				"key":          "foo",
+				"handler_type": "script",
+				"handler":      "./script.sh",
+				"args":         []interface{}{"arg1"},
+			},
+			expectedErr: "Only one watch handler allowed",
+		},
+		{
+			name: "no handler_type",
+			params: raw{
+				"type": "key",
+				"key":  "foo",
+			},
+			expectedErr: "Must define a watch handler",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fn(t, tc)
+		})
+	}
+}