From 530107f8ef413ba83121cc55e8ddc96a12f36b99 Mon Sep 17 00:00:00 2001 From: Corentin Chary Date: Wed, 13 Jun 2018 09:19:17 +0200 Subject: [PATCH 1/4] federation: nil pointer deference when using remove read ``` level=error ts=2018-06-13T07:19:04.515149169Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56202: runtime error: invalid memory address or nil pointer dereference" level=error ts=2018-06-13T07:19:04.516199547Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56204: runtime error: invalid memory address or nil pointer dereference" level=error ts=2018-06-13T07:19:04.51717692Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56206: runtime error: invalid memory address or nil pointer dereference" level=error ts=2018-06-13T07:19:04.564952878Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56208: runtime error: invalid memory address or nil pointer dereference" level=error ts=2018-06-13T07:19:04.566575791Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56210: runtime error: invalid memory address or nil pointer dereference" level=error ts=2018-06-13T07:19:04.567106063Z caller=stdlib.go:89 component=web caller="http: panic serving [::1" msg="]:56212: runtime error: invalid memory address or nil pointer dereference" ``` When remove read is enabled, federation will call `q.Select(nil, mset...)` which will break remote reads because it currently doesn't handle empty SelectParams. Signed-off-by: Corentin Chary --- storage/remote/codec.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/storage/remote/codec.go b/storage/remote/codec.go index d3858de74..89faffb99 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -95,10 +95,13 @@ func ToQuery(from, to int64, matchers []*labels.Matcher, p *storage.SelectParams if err != nil { return nil, err } + var rp *prompb.ReadHints = nil - rp := &prompb.ReadHints{ - StepMs: p.Step, - Func: p.Func, + if p != nil { + rp = &prompb.ReadHints{ + StepMs: p.Step, + Func: p.Func, + } } return &prompb.Query{ From ae295124446824f0f7443531f7dd9c6d71bee3ff Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Sat, 16 Jun 2018 18:26:37 +0100 Subject: [PATCH 2/4] Extend API tests to cover remote read API. Signed-off-by: Tom Wilkie --- web/api/v1/api_test.go | 192 +++++++++++++++++++++++++++++++---------- 1 file changed, 147 insertions(+), 45 deletions(-) diff --git a/web/api/v1/api_test.go b/web/api/v1/api_test.go index 06e1a4ac6..35e27d5e9 100644 --- a/web/api/v1/api_test.go +++ b/web/api/v1/api_test.go @@ -31,7 +31,9 @@ import ( "github.com/gogo/protobuf/proto" "github.com/golang/snappy" + config_util "github.com/prometheus/common/config" "github.com/prometheus/common/model" + "github.com/prometheus/common/promlog" "github.com/prometheus/common/route" "github.com/prometheus/prometheus/config" @@ -128,30 +130,125 @@ func TestEndpoints(t *testing.T) { now := time.Now() - var tr testTargetRetriever + t.Run("local", func(t *testing.T) { + api := &API{ + Queryable: suite.Storage(), + QueryEngine: suite.QueryEngine(), + targetRetriever: testTargetRetriever{}, + alertmanagerRetriever: testAlertmanagerRetriever{}, + now: func() time.Time { return now }, + config: func() config.Config { return samplePrometheusCfg }, + flagsMap: sampleFlagMap, + ready: func(f http.HandlerFunc) http.HandlerFunc { return f }, + } - var ar testAlertmanagerRetriever + testEndpoints(t, api, true) + }) - api := &API{ - Queryable: suite.Storage(), - QueryEngine: suite.QueryEngine(), - targetRetriever: tr, - alertmanagerRetriever: ar, - now: func() time.Time { return now }, - config: func() config.Config { return samplePrometheusCfg }, - flagsMap: sampleFlagMap, - ready: func(f http.HandlerFunc) http.HandlerFunc { return f }, - } + // Run all the API tests against a API that is wired to forward queries via + // the remote read client to a test server, which in turn sends them to the + // date from the test suite. + t.Run("remote", func(t *testing.T) { + server := setupRemote(suite.Storage()) + defer server.Close() + + u, err := url.Parse(server.URL) + if err != nil { + t.Fatal(err) + } + + al := promlog.AllowedLevel{} + al.Set("debug") + remote := remote.NewStorage(promlog.New(al), func() (int64, error) { + return 0, nil + }, 1*time.Second) + + err = remote.ApplyConfig(&config.Config{ + RemoteReadConfigs: []*config.RemoteReadConfig{ + { + URL: &config_util.URL{URL: u}, + RemoteTimeout: model.Duration(1 * time.Second), + ReadRecent: true, + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + api := &API{ + Queryable: remote, + QueryEngine: suite.QueryEngine(), + targetRetriever: testTargetRetriever{}, + alertmanagerRetriever: testAlertmanagerRetriever{}, + now: func() time.Time { return now }, + config: func() config.Config { return samplePrometheusCfg }, + flagsMap: sampleFlagMap, + ready: func(f http.HandlerFunc) http.HandlerFunc { return f }, + } + + testEndpoints(t, api, false) + }) +} + +func setupRemote(s storage.Storage) *httptest.Server { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + req, err := remote.DecodeReadRequest(r) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + resp := prompb.ReadResponse{ + Results: make([]*prompb.QueryResult, len(req.Queries)), + } + for i, query := range req.Queries { + from, through, matchers, err := remote.FromQuery(query) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + querier, err := s.Querier(r.Context(), from, through) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + defer querier.Close() + + set, err := querier.Select(nil, matchers...) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + resp.Results[i], err = remote.ToQueryResult(set) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + } + + if err := remote.EncodeReadResponse(&resp, w); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + }) + + return httptest.NewServer(handler) +} + +func testEndpoints(t *testing.T, api *API, testLabelAPI bool) { start := time.Unix(0, 0) - var tests = []struct { + type test struct { endpoint apiFunc params map[string]string query url.Values response interface{} errType errorType - }{ + } + + var tests = []test{ { endpoint: api.query, query: url.Values{ @@ -203,7 +300,7 @@ func TestEndpoints(t *testing.T) { ResultType: promql.ValueTypeScalar, Result: promql.Scalar{ V: 0.333, - T: timestamp.FromTime(now), + T: timestamp.FromTime(api.now()), }, }, }, @@ -309,34 +406,6 @@ func TestEndpoints(t *testing.T) { }, errType: errorBadData, }, - { - endpoint: api.labelValues, - params: map[string]string{ - "name": "__name__", - }, - response: []string{ - "test_metric1", - "test_metric2", - }, - }, - { - endpoint: api.labelValues, - params: map[string]string{ - "name": "foo", - }, - response: []string{ - "bar", - "boo", - }, - }, - // Bad name parameter. - { - endpoint: api.labelValues, - params: map[string]string{ - "name": "not!!!allowed", - }, - errType: errorBadData, - }, { endpoint: api.series, query: url.Values{ @@ -500,6 +569,39 @@ func TestEndpoints(t *testing.T) { }, } + if testLabelAPI { + tests = append(tests, []test{ + { + endpoint: api.labelValues, + params: map[string]string{ + "name": "__name__", + }, + response: []string{ + "test_metric1", + "test_metric2", + }, + }, + { + endpoint: api.labelValues, + params: map[string]string{ + "name": "foo", + }, + response: []string{ + "bar", + "boo", + }, + }, + // Bad name parameter. + { + endpoint: api.labelValues, + params: map[string]string{ + "name": "not!!!allowed", + }, + errType: errorBadData, + }, + }...) + } + methods := func(f apiFunc) []string { fp := reflect.ValueOf(f).Pointer() if fp == reflect.ValueOf(api.query).Pointer() || fp == reflect.ValueOf(api.queryRange).Pointer() { @@ -517,14 +619,14 @@ func TestEndpoints(t *testing.T) { return http.NewRequest(m, fmt.Sprintf("http://example.com?%s", q.Encode()), nil) } - for _, test := range tests { + for i, test := range tests { for _, method := range methods(test.endpoint) { // Build a context with the correct request params. ctx := context.Background() for p, v := range test.params { ctx = route.WithParam(ctx, p, v) } - t.Logf("run %s\t%q", method, test.query.Encode()) + t.Logf("run %d\t%s\t%q", i, method, test.query.Encode()) req, err := request(method, test.query) if err != nil { From 0b189b2da93d0859d0842ee512967a45e5f5c455 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 18 Jun 2018 16:33:04 +0100 Subject: [PATCH 3/4] Review feedback. Signed-off-by: Tom Wilkie --- storage/remote/codec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/remote/codec.go b/storage/remote/codec.go index 89faffb99..66037a9b5 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -95,8 +95,8 @@ func ToQuery(from, to int64, matchers []*labels.Matcher, p *storage.SelectParams if err != nil { return nil, err } - var rp *prompb.ReadHints = nil + var rp *prompb.ReadHints if p != nil { rp = &prompb.ReadHints{ StepMs: p.Step, From fcc3f43acd6ea560ac833c907cacc5f4d96b04a3 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 18 Jun 2018 17:32:44 +0100 Subject: [PATCH 4/4] spelling. Signed-off-by: Tom Wilkie --- web/api/v1/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/api/v1/api_test.go b/web/api/v1/api_test.go index 35e27d5e9..1cb7fdd02 100644 --- a/web/api/v1/api_test.go +++ b/web/api/v1/api_test.go @@ -147,7 +147,7 @@ func TestEndpoints(t *testing.T) { // Run all the API tests against a API that is wired to forward queries via // the remote read client to a test server, which in turn sends them to the - // date from the test suite. + // data from the test suite. t.Run("remote", func(t *testing.T) { server := setupRemote(suite.Storage()) defer server.Close()