From 5c70213f9f1d2eb5c326e22f1945c92e7f73fa69 Mon Sep 17 00:00:00 2001 From: Paul Gier Date: Wed, 13 Jun 2018 10:34:59 -0500 Subject: [PATCH 1/7] config: set target group source index during unmarshalling (#4245) * config: set target group source index during unmarshalling Fixes issue #4214 where the scrape pool is unnecessarily reloaded for a config reload where the config hasn't changed. Previously, the discovery manager changed the static config after loading which caused the in-memory config to differ from a freshly reloaded config. Signed-off-by: Paul Gier * [issue #4214] Test that static targets are not modified by discovery manager Signed-off-by: Paul Gier --- config/config.go | 14 ++++++++++++++ config/config_test.go | 4 ++++ discovery/manager.go | 11 +---------- discovery/manager_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index 746aae3bf..b7a566764 100644 --- a/config/config.go +++ b/config/config.go @@ -370,6 +370,13 @@ func (c *ScrapeConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { } } } + + // Add index to the static config target groups for unique identification + // within scrape pool. + for i, tg := range c.ServiceDiscoveryConfig.StaticConfigs { + tg.Source = fmt.Sprintf("%d", i) + } + return nil } @@ -432,6 +439,13 @@ func (c *AlertmanagerConfig) UnmarshalYAML(unmarshal func(interface{}) error) er } } } + + // Add index to the static config target groups for unique identification + // within scrape pool. + for i, tg := range c.ServiceDiscoveryConfig.StaticConfigs { + tg.Source = fmt.Sprintf("%d", i) + } + return nil } diff --git a/config/config_test.go b/config/config_test.go index 8caaebd55..549a4f909 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -128,6 +128,7 @@ var expectedConf = &Config{ "my": "label", "your": "label", }, + Source: "0", }, }, @@ -484,6 +485,7 @@ var expectedConf = &Config{ Targets: []model.LabelSet{ {model.AddressLabel: "localhost:9090"}, }, + Source: "0", }, }, }, @@ -503,6 +505,7 @@ var expectedConf = &Config{ Targets: []model.LabelSet{ {model.AddressLabel: "localhost:9090"}, }, + Source: "0", }, }, }, @@ -548,6 +551,7 @@ var expectedConf = &Config{ {model.AddressLabel: "1.2.3.5:9093"}, {model.AddressLabel: "1.2.3.6:9093"}, }, + Source: "0", }, }, }, diff --git a/discovery/manager.go b/discovery/manager.go index 669a91dc5..97468a549 100644 --- a/discovery/manager.go +++ b/discovery/manager.go @@ -285,7 +285,7 @@ func (m *Manager) providersFromConfig(cfg sd_config.ServiceDiscoveryConfig) map[ app("triton", i, t) } if len(cfg.StaticConfigs) > 0 { - app("static", 0, NewStaticProvider(cfg.StaticConfigs)) + app("static", 0, &StaticProvider{cfg.StaticConfigs}) } return providers @@ -296,15 +296,6 @@ type StaticProvider struct { TargetGroups []*targetgroup.Group } -// NewStaticProvider returns a StaticProvider configured with the given -// target groups. -func NewStaticProvider(groups []*targetgroup.Group) *StaticProvider { - for i, tg := range groups { - tg.Source = fmt.Sprintf("%d", i) - } - return &StaticProvider{groups} -} - // Run implements the Worker interface. func (sd *StaticProvider) Run(ctx context.Context, ch chan<- []*targetgroup.Group) { // We still have to consider that the consumer exits right away in which case diff --git a/discovery/manager_test.go b/discovery/manager_test.go index c57351270..19efb7d62 100644 --- a/discovery/manager_test.go +++ b/discovery/manager_test.go @@ -774,6 +774,45 @@ scrape_configs: verifyPresence(discoveryManager.targets, poolKey{setName: "prometheus", provider: "static/0"}, "{__address__=\"bar:9090\"}", false) } +func TestApplyConfigDoesNotModifyStaticProviderTargets(t *testing.T) { + cfgText := ` +scrape_configs: + - job_name: 'prometheus' + static_configs: + - targets: ["foo:9090"] + - targets: ["bar:9090"] + - targets: ["baz:9090"] +` + originalConfig := &config.Config{} + if err := yaml.UnmarshalStrict([]byte(cfgText), originalConfig); err != nil { + t.Fatalf("Unable to load YAML config cfgYaml: %s", err) + } + origScrpCfg := originalConfig.ScrapeConfigs[0] + + processedConfig := &config.Config{} + if err := yaml.UnmarshalStrict([]byte(cfgText), processedConfig); err != nil { + t.Fatalf("Unable to load YAML config cfgYaml: %s", err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + discoveryManager := NewManager(ctx, nil) + go discoveryManager.Run() + + c := make(map[string]sd_config.ServiceDiscoveryConfig) + for _, v := range processedConfig.ScrapeConfigs { + c[v.JobName] = v.ServiceDiscoveryConfig + } + discoveryManager.ApplyConfig(c) + <-discoveryManager.SyncCh() + + for _, sdcfg := range c { + if !reflect.DeepEqual(origScrpCfg.ServiceDiscoveryConfig.StaticConfigs, sdcfg.StaticConfigs) { + t.Fatalf("discovery manager modified static config \n expected: %v\n got: %v\n", + origScrpCfg.ServiceDiscoveryConfig.StaticConfigs, sdcfg.StaticConfigs) + } + } +} + type update struct { targetGroups []targetgroup.Group interval time.Duration From dacb6c530a2b36d81542df7cf4868d27d45346a3 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 12 Jun 2018 13:45:59 +0200 Subject: [PATCH 2/7] discovery/file: fix logging (#4178) Signed-off-by: Simon Pasquier --- discovery/file/file.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/discovery/file/file.go b/discovery/file/file.go index 780e25819..be6337822 100644 --- a/discovery/file/file.go +++ b/discovery/file/file.go @@ -279,7 +279,7 @@ func (d *Discovery) deleteTimestamp(filename string) { // stop shuts down the file watcher. func (d *Discovery) stop() { - level.Debug(d.logger).Log("msg", "Stopping file discovery...", "paths", d.paths) + level.Debug(d.logger).Log("msg", "Stopping file discovery...", "paths", fmt.Sprintf("%v", d.paths)) done := make(chan struct{}) defer close(done) @@ -299,10 +299,10 @@ func (d *Discovery) stop() { } }() if err := d.watcher.Close(); err != nil { - level.Error(d.logger).Log("msg", "Error closing file watcher", "paths", d.paths, "err", err) + level.Error(d.logger).Log("msg", "Error closing file watcher", "paths", fmt.Sprintf("%v", d.paths), "err", err) } - level.Debug(d.logger).Log("File discovery stopped", "paths", d.paths) + level.Debug(d.logger).Log("msg", "File discovery stopped") } // refresh reads all files matching the discovery's patterns and sends the respective From db9dbeeaec8a1e32615cdc6ff7efaf63f55645ed Mon Sep 17 00:00:00 2001 From: Corentin Chary Date: Wed, 13 Jun 2018 09:19:17 +0200 Subject: [PATCH 3/7] 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 d68c442e5..cf43c390f 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -91,10 +91,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 e518f51a999647f77a2245114825083ad8e60632 Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Sat, 16 Jun 2018 18:26:37 +0100 Subject: [PATCH 4/7] 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 b8217720ac5c0a8f270d49147e91bd0e8e932b3d Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 18 Jun 2018 16:33:04 +0100 Subject: [PATCH 5/7] 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 cf43c390f..3063b64f9 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -91,8 +91,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 4e4f0d4e41dd7b44cd91dbeea06762451ed2db7e Mon Sep 17 00:00:00 2001 From: Tom Wilkie Date: Mon, 18 Jun 2018 17:32:44 +0100 Subject: [PATCH 6/7] 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() From 141799da6e1a251774da2c06554c5366a3f889ce Mon Sep 17 00:00:00 2001 From: Brian Brazil Date: Tue, 19 Jun 2018 13:12:11 +0100 Subject: [PATCH 7/7] Release 2.3.1 Signed-off-by: Brian Brazil --- CHANGELOG.md | 10 ++++++++++ VERSION | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eeebc1a2f..11699d053 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +## 2.3.1 / 2018-06-19 + +* [BUGFIX] Avoid infinite loop on duplicate NaN values. #4275 +* [BUGFIX] Fix nil pointer deference when using various API endpoints #4282 +* [BUGFIX] config: set target group source index during unmarshalling #4245 +* [BUGFIX] discovery/file: fix logging #4178 +* [BUGFIX] kubernetes_sd: fix namespace filtering #4285 +* [BUGFIX] web: restore old path prefix behavior #4273 +* [BUGFIX] web: remove security headers added in 2.3.0 #4259 + ## 2.3.0 / 2018-06-05 * [CHANGE] `marathon_sd`: use `auth_token` and `auth_token_file` for token-based authentication instead of `bearer_token` and `bearer_token_file` respectively. diff --git a/VERSION b/VERSION index 276cbf9e2..2bf1c1ccf 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3.0 +2.3.1