Merge pull request #8290 from hashicorp/dnephin/watch-decode

watch: fix script watches with single arg

agent/agent.go

@@ -1332,44 +1332,10 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
 			}
 		}
 
-		// Parse the watches, excluding 'handler' and 'args'
-		wp, err := watch.ParseExempt(params, []string{"handler", "args"})
+		wp, err := makeWatchPlan(a.logger, params)
 		if err != nil {
-			return fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
+			return err
 		}
-
-		// Get the handler and subprocess arguments
-		handler, hasHandler := wp.Exempt["handler"]
-		args, hasArgs := wp.Exempt["args"]
-		if hasHandler {
-			a.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")
-		}
-		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 {
-				v, ok := arg.(string)
-				if !ok {
-					return fmt.Errorf("Watch args must be a list of strings")
-				}
-
-				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 || hasHandler && wp.HandlerType == "http" || hasArgs && wp.HandlerType == "http" {
-			return fmt.Errorf("Only one watch handler allowed")
-		}
-		if !hasHandler && !hasArgs && wp.HandlerType != "http" {
-			return fmt.Errorf("Must define a watch handler")
-		}
-
-		// Store the watch plan
 		watchPlans = append(watchPlans, wp)
 	}
 

agent/watch_handler.go

@@ -184,3 +184,59 @@ func makeHTTPWatchHandler(logger hclog.Logger, config *watch.HttpHandlerConfig)
 	}
 	return fn
 }
+
+// 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"})
+	if err != nil {
+		return nil, fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
+	}
+
+	handler, hasHandler := wp.Exempt["handler"]
+	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")
+	}
+	if _, ok := handler.(string); hasHandler && !ok {
+		return nil, fmt.Errorf("Watch handler must be a string")
+	}
+
+	args, hasArgs := wp.Exempt["args"]
+	if hasArgs {
+		wp.Exempt["args"], err = parseWatchArgs(args)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if hasHandler && hasArgs || hasHandler && wp.HandlerType == "http" || hasArgs && wp.HandlerType == "http" {
+		return nil, fmt.Errorf("Only one watch handler allowed")
+	}
+	if !hasHandler && !hasArgs && wp.HandlerType != "http" {
+		return nil, fmt.Errorf("Must define a watch handler")
+	}
+	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)
+		})
+	}
+}