From 3247dcca253002626cc42216dad8196ff6803beb Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 6 Nov 2020 14:29:10 +0200 Subject: [PATCH 1/7] [dtest] Fix coordinator API incompatibilities in docker harness --- .../dtest/docker/harness/resources/coordinator.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go index 00da24e3e1..dfcfea1059 100644 --- a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go +++ b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go @@ -31,7 +31,8 @@ import ( "github.com/m3db/m3/src/query/generated/proto/admin" - dockertest "github.com/ory/dockertest" + "github.com/gogo/protobuf/jsonpb" + "github.com/ory/dockertest" "go.uber.org/zap" ) @@ -112,7 +113,7 @@ func (c *coordinator) GetNamespace() (admin.NamespaceGetResponse, error) { return admin.NamespaceGetResponse{}, errClosed } - url := c.resource.getURL(7201, "api/v1/namespace") + url := c.resource.getURL(7201, "api/v1/services/m3db/namespace") logger := c.resource.logger.With( zapMethod("getNamespace"), zap.String("url", url)) @@ -135,7 +136,7 @@ func (c *coordinator) GetPlacement() (admin.PlacementGetResponse, error) { return admin.PlacementGetResponse{}, errClosed } - url := c.resource.getURL(7201, "api/v1/placement") + url := c.resource.getURL(7201, "api/v1/services/m3db/placement") logger := c.resource.logger.With( zapMethod("getPlacement"), zap.String("url", url)) @@ -273,13 +274,15 @@ func (c *coordinator) AddNamespace( zapMethod("addNamespace"), zap.String("url", url), zap.String("request", addRequest.String())) - b, err := json.Marshal(addRequest) + marshaller := jsonpb.Marshaler{EmitDefaults: true} + data := bytes.NewBuffer(nil) + err := marshaller.Marshal(data, &addRequest) if err != nil { logger.Error("failed to marshal", zap.Error(err)) return admin.NamespaceGetResponse{}, err } - resp, err := http.Post(url, "application/json", bytes.NewReader(b)) + resp, err := http.Post(url, "application/json", data) if err != nil { logger.Error("failed post", zap.Error(err)) return admin.NamespaceGetResponse{}, err From f867b1c1c9180d81cd90e10a9f154c95e1e4bcde Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 6 Nov 2020 15:49:56 +0200 Subject: [PATCH 2/7] fix linter errors --- .../docker/harness/resources/coordinator.go | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go index dfcfea1059..034c271ce2 100644 --- a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go +++ b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go @@ -22,7 +22,7 @@ package resources import ( "bytes" - "encoding/json" + "context" "fmt" "io/ioutil" "net" @@ -32,6 +32,7 @@ import ( "github.com/m3db/m3/src/query/generated/proto/admin" "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" "github.com/ory/dockertest" "go.uber.org/zap" ) @@ -240,13 +241,7 @@ func (c *coordinator) CreateDatabase( zapMethod("createDatabase"), zap.String("url", url), zap.String("request", addRequest.String())) - b, err := json.Marshal(addRequest) - if err != nil { - logger.Error("failed to marshal", zap.Error(err)) - return admin.DatabaseCreateResponse{}, err - } - - resp, err := http.Post(url, "application/json", bytes.NewReader(b)) + resp, err := makePostRequest(logger, url, &addRequest) if err != nil { logger.Error("failed post", zap.Error(err)) return admin.DatabaseCreateResponse{}, err @@ -274,15 +269,7 @@ func (c *coordinator) AddNamespace( zapMethod("addNamespace"), zap.String("url", url), zap.String("request", addRequest.String())) - marshaller := jsonpb.Marshaler{EmitDefaults: true} - data := bytes.NewBuffer(nil) - err := marshaller.Marshal(data, &addRequest) - if err != nil { - logger.Error("failed to marshal", zap.Error(err)) - return admin.NamespaceGetResponse{}, err - } - - resp, err := http.Post(url, "application/json", data) + resp, err := makePostRequest(logger, url, &addRequest) if err != nil { logger.Error("failed post", zap.Error(err)) return admin.NamespaceGetResponse{}, err @@ -333,6 +320,26 @@ func (c *coordinator) WriteCarbon( // return nil } +func makePostRequest(logger *zap.Logger, url string, body proto.Message) (*http.Response, error) { + data := bytes.NewBuffer(nil) + if err := (&jsonpb.Marshaler{}).Marshal(data, body); err != nil { + logger.Error("failed to marshal", zap.Error(err)) + + return &http.Response{}, fmt.Errorf("failed to marshal: %w", err) + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, url, data) + if err != nil { + logger.Error("failed to construct request", zap.Error(err)) + + return &http.Response{}, fmt.Errorf("failed to construct request: %w", err) + } + + req.Header.Add("Content-Type", "application/json") + + return http.DefaultClient.Do(req) +} + func (c *coordinator) query( verifier GoalStateVerifier, query string, ) error { From e17ef5d61245fa2f165e375ba5a1cf896b671ea3 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Wed, 11 Nov 2020 14:30:44 +0200 Subject: [PATCH 3/7] allow verifying HTTP status codes --- .../tools/dtest/docker/harness/carbon_test.go | 16 ++++++++++++---- .../docker/harness/resources/coordinator.go | 19 ++++++++----------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/carbon_test.go b/src/cmd/tools/dtest/docker/harness/carbon_test.go index 199a770a43..93827cd091 100644 --- a/src/cmd/tools/dtest/docker/harness/carbon_test.go +++ b/src/cmd/tools/dtest/docker/harness/carbon_test.go @@ -31,12 +31,16 @@ import ( "github.com/stretchr/testify/assert" ) -func findVerifier(expected string) resources.GoalStateVerifier { - return func(s string, err error) error { +func findVerifier(expected string) resources.ResponseVerifier { + return func(status int, s string, err error) error { if err != nil { return err } + if status/100 != 2 { + return fmt.Errorf("expected 200 status code, got %v", status) + } + if s == expected { return nil } @@ -45,17 +49,21 @@ func findVerifier(expected string) resources.GoalStateVerifier { } } -func renderVerifier(metric string, v float64) resources.GoalStateVerifier { +func renderVerifier(metric string, v float64) resources.ResponseVerifier { type graphiteRender struct { Target string `json:"target"` Datapoints [][]float64 `json:"datapoints"` } - return func(s string, err error) error { + return func(status int, s string, err error) error { if err != nil { return err } + if status/100 != 2 { + return fmt.Errorf("expected 200 status code, got %v", status) + } + var render []graphiteRender if err := json.Unmarshal([]byte(s), &render); err != nil { return err diff --git a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go index 034c271ce2..f34e4f72cf 100644 --- a/src/cmd/tools/dtest/docker/harness/resources/coordinator.go +++ b/src/cmd/tools/dtest/docker/harness/resources/coordinator.go @@ -54,6 +54,9 @@ var ( } ) +// ResponseVerifier is a function that checks if the query response is valid. +type ResponseVerifier func(int, string, error) error + // Coordinator is a wrapper for a coordinator. It provides a wrapper on HTTP // endpoints that expose cluster management APIs as well as read and write // endpoints for series data. @@ -64,7 +67,7 @@ type Coordinator interface { // WriteCarbon writes a carbon metric datapoint at a given time. WriteCarbon(port int, metric string, v float64, t time.Time) error // RunQuery runs the given query with a given verification function. - RunQuery(verifier GoalStateVerifier, query string) error + RunQuery(verifier ResponseVerifier, query string) error } // Admin is a wrapper for admin functions. @@ -341,7 +344,7 @@ func makePostRequest(logger *zap.Logger, url string, body proto.Message) (*http. } func (c *coordinator) query( - verifier GoalStateVerifier, query string, + verifier ResponseVerifier, query string, ) error { if c.resource.closed { return errClosed @@ -357,20 +360,14 @@ func (c *coordinator) query( return err } - defer resp.Body.Close() - if resp.StatusCode/100 != 2 { - logger.Error("status code not 2xx", - zap.Int("status code", resp.StatusCode), - zap.String("status", resp.Status)) - return fmt.Errorf("status code %d", resp.StatusCode) - } defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) - return verifier(string(b), err) + + return verifier(resp.StatusCode, string(b), err) } func (c *coordinator) RunQuery( - verifier GoalStateVerifier, query string, + verifier ResponseVerifier, query string, ) error { if c.resource.closed { return errClosed From b2bd2b8412de9200a17ad71de90ebd609f237f44 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Wed, 11 Nov 2020 14:31:16 +0200 Subject: [PATCH 4/7] add a test for /query and /query_range endpoints --- .../dtest/docker/harness/query_api_test.go | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 src/cmd/tools/dtest/docker/harness/query_api_test.go diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go new file mode 100644 index 0000000000..047bfbeca6 --- /dev/null +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -0,0 +1,131 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package harness + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/m3db/m3/src/cmd/tools/dtest/docker/harness/resources" +) + +var instantQueryBadRequestTest = []struct { + name string + q string +}{ + // {"missing query", queryURL("query", "")}, + // FAILING issue #2: invalid query string should result in 400 + // {"invalid query", queryURL("query", "@!")}, + {"invalid time", queryURL("time", "INVALID")}, + {"invalid timeout", queryURL("timeout", "INVALID")}, +} + +func TestInvalidInstantQueryReturns400(t *testing.T) { + coord := singleDBNodeDockerResources.Coordinator() + + for _, tt := range instantQueryBadRequestTest { + t.Run(tt.name, func(t *testing.T) { + assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.q), "for query '%v'", tt.q) + }) + } +} + +var queryBadRequestTest = []struct { + name string + q string +}{ + {"missing query", queryRangeURL("query", "")}, + // FAILING issue #2: invalid query string should result in 400 + // {"invalid query", queryRangeURL("query", "@!")}, + {"missing start", queryRangeURL("start", "")}, + {"invalid start", queryRangeURL("start", "INVALID")}, + {"missing end", queryRangeURL("end", "")}, + {"invalid end", queryRangeURL("end", "INVALID")}, + {"missing step", queryRangeURL("step", "")}, + {"invalid step", queryRangeURL("step", "INVALID")}, +} + +func TestInvalidRangeQueryReturns400(t *testing.T) { + coord := singleDBNodeDockerResources.Coordinator() + + for _, tt := range queryBadRequestTest { + t.Run(tt.name, func(t *testing.T) { + assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.q), "for query '%v'", tt.q) + }) + } +} + +func queryURL(key, value string) string { + params := map[string]string{ + "query": "foo", + } + + if value == "" { + delete(params, key) + } else { + params[key] = value + } + + return "api/v1/query?" + queryString(params) +} + +func queryRangeURL(key, value string) string { + params := map[string]string{ + "query": "foo", + "start": "now", + "end": "now", + "step": "1000", + } + + if value == "" { + delete(params, key) + } else { + params[key] = value + } + + return "api/v1/query_range?" + queryString(params) +} + +func queryString(params map[string]string) string { + p := make([]string, 0, len(params)) + for k, v := range params { + p = append(p, fmt.Sprintf("%v=%v", k, v)) + } + + return strings.Join(p, "&") +} + +func verifyStatus(expectedStatus int) resources.ResponseVerifier { + return func(status int, resp string, err error) error { + if err != nil { + return err + } + + if status != expectedStatus { + return fmt.Errorf("expeceted %v status code, got %v", expectedStatus, status) + } + + return nil + } +} From a9f76c232011236fd58cc3482c5755a1bb4c7fad Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Wed, 11 Nov 2020 17:51:57 +0200 Subject: [PATCH 5/7] fix lint errors --- src/cmd/tools/dtest/docker/harness/carbon_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/carbon_test.go b/src/cmd/tools/dtest/docker/harness/carbon_test.go index 93827cd091..6bc35a9ee7 100644 --- a/src/cmd/tools/dtest/docker/harness/carbon_test.go +++ b/src/cmd/tools/dtest/docker/harness/carbon_test.go @@ -49,7 +49,7 @@ func findVerifier(expected string) resources.ResponseVerifier { } } -func renderVerifier(metric string, v float64) resources.ResponseVerifier { +func renderVerifier(v float64) resources.ResponseVerifier { type graphiteRender struct { Target string `json:"target"` Datapoints [][]float64 `json:"datapoints"` @@ -111,7 +111,7 @@ func TestCarbon(t *testing.T) { read := func(metric string, expected float64) { assert.NoError(t, coord.RunQuery( - renderVerifier(metric, expected), + renderVerifier(expected), graphiteQuery(metric, timestamp))) } From 77f076c2efb862b562c7c349e6247d21a2ba78d3 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 12 Nov 2020 11:35:38 +0200 Subject: [PATCH 6/7] PR comments --- .../dtest/docker/harness/query_api_test.go | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index 047bfbeca6..24c2210447 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -30,48 +30,48 @@ import ( "github.com/m3db/m3/src/cmd/tools/dtest/docker/harness/resources" ) -var instantQueryBadRequestTest = []struct { +type URLTest struct { name string - q string -}{ - // {"missing query", queryURL("query", "")}, - // FAILING issue #2: invalid query string should result in 400 - // {"invalid query", queryURL("query", "@!")}, - {"invalid time", queryURL("time", "INVALID")}, - {"invalid timeout", queryURL("timeout", "INVALID")}, + url string } func TestInvalidInstantQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() + instantQueryBadRequestTest := []URLTest{ + // FAILING issue #2: invalid or missing query string should result in 400 + // {"missing query", queryURL("query", "")}, + // {"invalid query", queryURL("query", "@!")}, + {"invalid time", queryURL("time", "INVALID")}, + {"invalid timeout", queryURL("timeout", "INVALID")}, + } + for _, tt := range instantQueryBadRequestTest { t.Run(tt.name, func(t *testing.T) { - assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.q), "for query '%v'", tt.q) + assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.url), "for query '%v'", tt.url) }) } } -var queryBadRequestTest = []struct { - name string - q string -}{ - {"missing query", queryRangeURL("query", "")}, - // FAILING issue #2: invalid query string should result in 400 - // {"invalid query", queryRangeURL("query", "@!")}, - {"missing start", queryRangeURL("start", "")}, - {"invalid start", queryRangeURL("start", "INVALID")}, - {"missing end", queryRangeURL("end", "")}, - {"invalid end", queryRangeURL("end", "INVALID")}, - {"missing step", queryRangeURL("step", "")}, - {"invalid step", queryRangeURL("step", "INVALID")}, -} - func TestInvalidRangeQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() + queryBadRequestTest := []URLTest{ + {"missing query", queryRangeURL("query", "")}, + // FAILING issue #2: invalid query string should result in 400 + // {"invalid query", queryRangeURL("query", "@!")}, + {"missing start", queryRangeURL("start", "")}, + {"invalid start", queryRangeURL("start", "INVALID")}, + {"missing end", queryRangeURL("end", "")}, + {"invalid end", queryRangeURL("end", "INVALID")}, + {"missing step", queryRangeURL("step", "")}, + {"invalid step", queryRangeURL("step", "INVALID")}, + {"invalid timeout", queryRangeURL("timeout", "INVALID")}, + } + for _, tt := range queryBadRequestTest { t.Run(tt.name, func(t *testing.T) { - assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.q), "for query '%v'", tt.q) + assert.NoError(t, coord.RunQuery(verifyStatus(400), tt.url), "for query '%v'", tt.url) }) } } @@ -108,7 +108,7 @@ func queryRangeURL(key, value string) string { } func queryString(params map[string]string) string { - p := make([]string, 0, len(params)) + p := make([]string, 0) for k, v := range params { p = append(p, fmt.Sprintf("%v=%v", k, v)) } From 3468694ba5d38df0db951241bb47e940a8aab7ad Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Thu, 12 Nov 2020 12:03:18 +0200 Subject: [PATCH 7/7] make urlTest struct private --- src/cmd/tools/dtest/docker/harness/query_api_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cmd/tools/dtest/docker/harness/query_api_test.go b/src/cmd/tools/dtest/docker/harness/query_api_test.go index 24c2210447..289f5cff55 100644 --- a/src/cmd/tools/dtest/docker/harness/query_api_test.go +++ b/src/cmd/tools/dtest/docker/harness/query_api_test.go @@ -30,7 +30,7 @@ import ( "github.com/m3db/m3/src/cmd/tools/dtest/docker/harness/resources" ) -type URLTest struct { +type urlTest struct { name string url string } @@ -38,7 +38,7 @@ type URLTest struct { func TestInvalidInstantQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() - instantQueryBadRequestTest := []URLTest{ + instantQueryBadRequestTest := []urlTest{ // FAILING issue #2: invalid or missing query string should result in 400 // {"missing query", queryURL("query", "")}, // {"invalid query", queryURL("query", "@!")}, @@ -56,7 +56,7 @@ func TestInvalidInstantQueryReturns400(t *testing.T) { func TestInvalidRangeQueryReturns400(t *testing.T) { coord := singleDBNodeDockerResources.Coordinator() - queryBadRequestTest := []URLTest{ + queryBadRequestTest := []urlTest{ {"missing query", queryRangeURL("query", "")}, // FAILING issue #2: invalid query string should result in 400 // {"invalid query", queryRangeURL("query", "@!")},