From c97faa7a6e790fa632b6c32dea301e831a67a1c5 Mon Sep 17 00:00:00 2001 From: Tobiasz Heller Date: Thu, 19 Mar 2020 09:24:37 +0100 Subject: [PATCH 1/4] rule: fix query addr parsing Signed-off-by: Tobiasz Heller --- CHANGELOG.md | 1 + cmd/thanos/rule.go | 25 ++++++++-------- pkg/query/config.go | 26 ++++++++++++++++ pkg/query/config_test.go | 65 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 13 deletions(-) create mode 100644 pkg/query/config_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b1ac5c5d..014c15ee7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel ### Fixed +- [#2288](https://github.com/thanos-io/thanos/pull/2288) Ruler: Fixes issue #2281 bug in ruler with parsing query addr with path prefix - [#2238](https://github.com/thanos-io/thanos/pull/2238) Ruler: Fixed Issue #2204 bug in alert queue signalling filled up queue and alerts were dropped - [#2231](https://github.com/thanos-io/thanos/pull/2231) Bucket Web - Sort chunks by thanos.downsample.resolution for better grouping - [#2254](https://github.com/thanos-io/thanos/pull/2254) Bucket: Fix metrics registered multiple times in bucket replicate diff --git a/cmd/thanos/rule.go b/cmd/thanos/rule.go index cdcf3c3ae1..60da41a897 100644 --- a/cmd/thanos/rule.go +++ b/cmd/thanos/rule.go @@ -295,10 +295,10 @@ func runRule( return err } } else { - for _, addr := range queryAddrs { - if addr == "" { - return errors.New("static querier address cannot be empty") - } + var err error + queryCfg, err = query.BuildQueryConfig(queryAddrs) + if err != nil { + return err } // Build the query configuration from the legacy query flags. @@ -308,16 +308,15 @@ func runRule( Files: querySDFiles, RefreshInterval: model.Duration(querySDInterval), }) - } - queryCfg = append(queryCfg, - query.Config{ - EndpointsConfig: http_util.EndpointsConfig{ - Scheme: "http", - StaticAddresses: queryAddrs, - FileSDConfigs: fileSDConfigs, + queryCfg = append(queryCfg, + query.Config{ + EndpointsConfig: http_util.EndpointsConfig{ + Scheme: "http", + FileSDConfigs: fileSDConfigs, + }, }, - }, - ) + ) + } } queryProvider := dns.NewProvider( diff --git a/pkg/query/config.go b/pkg/query/config.go index 3787695410..01ad41391b 100644 --- a/pkg/query/config.go +++ b/pkg/query/config.go @@ -4,8 +4,12 @@ package query import ( + "fmt" + "net/url" + "gopkg.in/yaml.v2" + "github.com/pkg/errors" http_util "github.com/thanos-io/thanos/pkg/http" ) @@ -39,3 +43,25 @@ func LoadConfigs(confYAML []byte) ([]Config, error) { } return queryCfg, nil } + +// BuildQueryConfig initializes and returns an Query client configuration from a static address. +func BuildQueryConfig(queryAddrs []string) ([]Config, error) { + configs := make([]Config, 0, len(queryAddrs)) + for _, addr := range queryAddrs { + if addr == "" { + return nil, errors.New("static querier address cannot be empty") + } + u, err := url.Parse(fmt.Sprintf("http://%s", addr)) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse addr %q", addr) + } + configs = append(configs, Config{ + EndpointsConfig: http_util.EndpointsConfig{ + Scheme: u.Scheme, + StaticAddresses: []string{u.Host}, + PathPrefix: u.Path, + }, + }) + } + return configs, nil +} diff --git a/pkg/query/config_test.go b/pkg/query/config_test.go new file mode 100644 index 0000000000..52286a4be6 --- /dev/null +++ b/pkg/query/config_test.go @@ -0,0 +1,65 @@ +// Copyright (c) The Thanos Authors. +// Licensed under the Apache License 2.0. + +package query + +import ( + "testing" + + "github.com/thanos-io/thanos/pkg/http" + "github.com/thanos-io/thanos/pkg/testutil" +) + +func TestBuildQueryConfig(t *testing.T) { + for _, tc := range []struct { + desc string + addresses []string + err bool + expected []Config + }{ + { + desc: "signe addr without path", + addresses: []string{"localhost:9093"}, + expected: []Config{{ + EndpointsConfig: http.EndpointsConfig{ + StaticAddresses: []string{"localhost:9093"}, + Scheme: "http", + }, + }}, + }, + { + desc: "1st addr without path, 2nd with", + addresses: []string{"localhost:9093", "localhost:9094/prefix"}, + expected: []Config{ + { + EndpointsConfig: http.EndpointsConfig{ + StaticAddresses: []string{"localhost:9093"}, + Scheme: "http", + }, + }, + { + EndpointsConfig: http.EndpointsConfig{ + StaticAddresses: []string{"localhost:9094"}, + Scheme: "http", + PathPrefix: "/prefix", + }, + }, + }, + }, + { + desc: "invalid addr", + addresses: []string{"this is not a valid addr"}, + err: true, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + cfg, err := BuildQueryConfig(tc.addresses) + if tc.err { + testutil.NotOk(t, err) + return + } + + testutil.Equals(t, tc.expected, cfg) + }) + } +} From 91f51d19cc75d73754b44f86674703bdf716fa02 Mon Sep 17 00:00:00 2001 From: Tobiasz Heller Date: Thu, 19 Mar 2020 16:33:37 +0100 Subject: [PATCH 2/4] CR: support different schemas Signed-off-by: Tobiasz Heller --- pkg/query/config.go | 7 ++++++- pkg/query/config_test.go | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/pkg/query/config.go b/pkg/query/config.go index 01ad41391b..a0221e6563 100644 --- a/pkg/query/config.go +++ b/pkg/query/config.go @@ -6,6 +6,7 @@ package query import ( "fmt" "net/url" + "strings" "gopkg.in/yaml.v2" @@ -51,7 +52,11 @@ func BuildQueryConfig(queryAddrs []string) ([]Config, error) { if addr == "" { return nil, errors.New("static querier address cannot be empty") } - u, err := url.Parse(fmt.Sprintf("http://%s", addr)) + // If addr is missing schema, add http. + if !strings.Contains(addr, "://") { + addr = fmt.Sprintf("http://%s", addr) + } + u, err := url.Parse(addr) if err != nil { return nil, errors.Wrapf(err, "failed to parse addr %q", addr) } diff --git a/pkg/query/config_test.go b/pkg/query/config_test.go index 52286a4be6..6fa77b1c8b 100644 --- a/pkg/query/config_test.go +++ b/pkg/query/config_test.go @@ -46,6 +46,26 @@ func TestBuildQueryConfig(t *testing.T) { }, }, }, + { + desc: "signe addr with path and http scheme", + addresses: []string{"http://localhost:9093"}, + expected: []Config{{ + EndpointsConfig: http.EndpointsConfig{ + StaticAddresses: []string{"localhost:9093"}, + Scheme: "http", + }, + }}, + }, + { + desc: "signe addr with path and https scheme", + addresses: []string{"https://localhost:9093"}, + expected: []Config{{ + EndpointsConfig: http.EndpointsConfig{ + StaticAddresses: []string{"localhost:9093"}, + Scheme: "https", + }, + }}, + }, { desc: "invalid addr", addresses: []string{"this is not a valid addr"}, From 0e741cbc47c67aff774cde5b2dd08b3dc9d2268a Mon Sep 17 00:00:00 2001 From: Tobiasz Heller Date: Thu, 19 Mar 2020 16:44:09 +0100 Subject: [PATCH 3/4] CR: docs and err Signed-off-by: Tobiasz Heller --- cmd/thanos/rule.go | 3 +-- pkg/query/config.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/thanos/rule.go b/cmd/thanos/rule.go index 60da41a897..b044819078 100644 --- a/cmd/thanos/rule.go +++ b/cmd/thanos/rule.go @@ -288,14 +288,13 @@ func runRule( metrics := newRuleMetrics(reg) var queryCfg []query.Config + var err error if len(queryConfigYAML) > 0 { - var err error queryCfg, err = query.LoadConfigs(queryConfigYAML) if err != nil { return err } } else { - var err error queryCfg, err = query.BuildQueryConfig(queryAddrs) if err != nil { return err diff --git a/pkg/query/config.go b/pkg/query/config.go index a0221e6563..27367ff471 100644 --- a/pkg/query/config.go +++ b/pkg/query/config.go @@ -45,7 +45,7 @@ func LoadConfigs(confYAML []byte) ([]Config, error) { return queryCfg, nil } -// BuildQueryConfig initializes and returns an Query client configuration from a static address. +// BuildQueryConfig returns a query client configuration from a static address. func BuildQueryConfig(queryAddrs []string) ([]Config, error) { configs := make([]Config, 0, len(queryAddrs)) for _, addr := range queryAddrs { From fe6c6aaa0cfccbdcb77e59388caeaeeba3d314b2 Mon Sep 17 00:00:00 2001 From: Tobiasz Heller Date: Mon, 23 Mar 2020 16:09:44 +0100 Subject: [PATCH 4/4] CR: improve error handling and more TC Signed-off-by: Tobiasz Heller --- pkg/query/config.go | 7 +++++-- pkg/query/config_test.go | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/query/config.go b/pkg/query/config.go index 27367ff471..12918e614f 100644 --- a/pkg/query/config.go +++ b/pkg/query/config.go @@ -48,9 +48,9 @@ func LoadConfigs(confYAML []byte) ([]Config, error) { // BuildQueryConfig returns a query client configuration from a static address. func BuildQueryConfig(queryAddrs []string) ([]Config, error) { configs := make([]Config, 0, len(queryAddrs)) - for _, addr := range queryAddrs { + for i, addr := range queryAddrs { if addr == "" { - return nil, errors.New("static querier address cannot be empty") + return nil, errors.Errorf("static querier address cannot be empty at index %d", i) } // If addr is missing schema, add http. if !strings.Contains(addr, "://") { @@ -60,6 +60,9 @@ func BuildQueryConfig(queryAddrs []string) ([]Config, error) { if err != nil { return nil, errors.Wrapf(err, "failed to parse addr %q", addr) } + if u.Scheme != "http" && u.Scheme != "https" { + return nil, errors.Errorf("%q is not supported scheme for querier address", u.Scheme) + } configs = append(configs, Config{ EndpointsConfig: http_util.EndpointsConfig{ Scheme: u.Scheme, diff --git a/pkg/query/config_test.go b/pkg/query/config_test.go index 6fa77b1c8b..1169df0498 100644 --- a/pkg/query/config_test.go +++ b/pkg/query/config_test.go @@ -18,7 +18,7 @@ func TestBuildQueryConfig(t *testing.T) { expected []Config }{ { - desc: "signe addr without path", + desc: "single addr without path", addresses: []string{"localhost:9093"}, expected: []Config{{ EndpointsConfig: http.EndpointsConfig{ @@ -47,7 +47,7 @@ func TestBuildQueryConfig(t *testing.T) { }, }, { - desc: "signe addr with path and http scheme", + desc: "single addr with path and http scheme", addresses: []string{"http://localhost:9093"}, expected: []Config{{ EndpointsConfig: http.EndpointsConfig{ @@ -57,7 +57,7 @@ func TestBuildQueryConfig(t *testing.T) { }}, }, { - desc: "signe addr with path and https scheme", + desc: "single addr with path and https scheme", addresses: []string{"https://localhost:9093"}, expected: []Config{{ EndpointsConfig: http.EndpointsConfig{ @@ -66,11 +66,21 @@ func TestBuildQueryConfig(t *testing.T) { }, }}, }, + { + desc: "not supported scheme", + addresses: []string{"ttp://localhost:9093"}, + err: true, + }, { desc: "invalid addr", addresses: []string{"this is not a valid addr"}, err: true, }, + { + desc: "empty addr", + addresses: []string{""}, + err: true, + }, } { t.Run(tc.desc, func(t *testing.T) { cfg, err := BuildQueryConfig(tc.addresses)