diff --git a/agent/agent.go b/agent/agent.go index ab6bfac98d..a1958b6883 100644 --- a/agent/agent.go +++ b/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) } diff --git a/agent/watch_handler.go b/agent/watch_handler.go index a98acd68c8..643d4afb13 100644 --- a/agent/watch_handler.go +++ b/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") + } +} diff --git a/agent/watch_handler_test.go b/agent/watch_handler_test.go index 1a0b7a2dd6..33c0909fe5 100644 --- a/agent/watch_handler_test.go +++ b/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) + }) + } +}