From 979a3d284475df3d77fe07daec7d22c7ad25abf8 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 10 Jul 2020 14:19:12 -0400 Subject: [PATCH] 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 | 43 +++++++++----- agent/watch_handler_test.go | 114 +++++++++++++++++++++++++++++++++++- 2 files changed, 141 insertions(+), 16 deletions(-) diff --git a/agent/watch_handler.go b/agent/watch_handler.go index 82431a43d..643d4afb1 100644 --- a/agent/watch_handler.go +++ b/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") + } +} diff --git a/agent/watch_handler_test.go b/agent/watch_handler_test.go index 1a0b7a2dd..33c0909fe 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) + }) + } +}