*: support TLS and authentication for Thanos Ruler queries by simonpasquier · Pull Request #1939 · thanos-io/thanos

simonpasquier · 2020-01-06T11:26:55Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Similar to #1838, this PR adds TLS and authentication support to Thanos Ruler for the query API endpoints. The YAML configuration format is very similar to the one used for configuring Alertmanager.

Verification

I have adapted the end-to-end tests to use the new parameters. The tests already exercised static addresses and file SD.

bwplotka

Nice!

Some comments, but generally 👍 👍 👍

Thanks!

bwplotka · 2020-01-07T13:02:36Z

cmd/thanos/rule.go

+			return err
 		}
+		c.Transport = tracing.HTTPTripperware(logger, c.Transport)
+		queryClient, err := http_util.NewFanoutClient(logger, cfg.EndpointsConfig, c, queryProvider.Clone())


Fanout? I think the logic for rule evaluation might be different. It's fanout = broadcast approach.

Yea, the logic is ok, just the Name of client might be confusing. It's not Fanout for Querier.

bwplotka · 2020-01-07T13:03:12Z

cmd/thanos/rule.go

+		}
 		// Each Alertmanager client has a different list of targets thus each needs its own DNS provider.
-		am, err := alert.NewAlertmanager(logger, cfg, amProvider.Clone())
+		amClient, err := http_util.NewFanoutClient(logger, cfg.EndpointsConfig, c, amProvider.Clone())


We can add tracing Tripperware as well I think

bwplotka · 2020-01-07T13:06:25Z

cmd/thanos/rule.go

+	// Discover and resolve query addresses.
+	{
+		for _, c := range queryClients {
+			addToGroup(g, c, dnsSDInterval)


Not consistent with alertmanager addToGroup placement. Let's decide on one approach if we can (:

Reason: I spent some time trying to understand if we missed addToGroup for query or not (:

I've moved the call to addToGroup to the loop creating the query clients.

bwplotka · 2020-01-07T13:13:47Z

cmd/thanos/rule.go


+	queryConfig := extflag.RegisterPathOrContent(cmd, "query.config", "YAML file that contains query API servers configuration. See format details: https://thanos.io/components/rule.md/#configuration. If defined, it takes precedence over the '--query' and '--query.sd-files' flags.", false)
+
 	fileSDFiles := cmd.Flag("query.sd-files", "Path to file that contain addresses of query peers. The path can be a glob pattern (repeatable).").


Suggested change

fileSDFiles := cmd.Flag("query.sd-files", "Path to file that contain addresses of query peers. The path can be a glob pattern (repeatable).").

fileSDFiles := cmd.Flag("query.sd-files", "Path to file that contains addresses of query API servers. The path can be a glob pattern (repeatable).").

bwplotka · 2020-01-07T13:14:24Z

cmd/thanos/rule.go

-					// TODO(bwplotka): Propagate those to UI, probably requires changing rule manager code ):
-					level.Warn(logger).Log("warnings", strings.Join(warns, ", "), "query", q)
+				if err != nil {
+					level.Error(logger).Log("err", err, "query", q)


let's do continue instead of else - bit more readable

bwplotka · 2020-01-07T13:15:04Z

cmd/thanos/rule.go

-				return v, nil
 			}
 		}
 		return nil, errors.Errorf("no query peer reachable")


Suggested change

return nil, errors.Errorf("no query peer reachable")

return nil, errors.Errorf("no query API server reachable")

The peer name is obsolete: Came from gossip times. (:

bwplotka · 2020-01-07T13:16:18Z

cmd/thanos/rule.go

 	}
 }
+
+func addToGroup(g *run.Group, c *http_util.FanoutClient, interval time.Duration) {


It's not only add, it adds for DNS & file discovery, so maybe:

Suggested change

func addToGroup(g *run.Group, c *http_util.FanoutClient, interval time.Duration) {

func addDiscoveryGroups(g *run.Group, c *http_util.FanoutClient, interval time.Duration) {

bwplotka · 2020-01-07T13:18:20Z

cmd/thanos/rule.go

@@ -738,36 +719,49 @@ func queryFunc(
 	}

 	return func(ctx context.Context, q string, t time.Time) (promql.Vector, error) {


I hope we can proper timeout set on caller top this (:

bwplotka · 2020-01-07T13:19:25Z

pkg/http/http.go

+}
+
+// FanoutClient represents a client that can send requests to a cluster of HTTP-based endpoints.
+type FanoutClient struct {


Again, fanout might be bad wording here, why not just Client?

I'm bad at naming 😄 I think I dismissedClient because it would be confusing with http.Client but it 's indeed better than the incorrect FanoutClient.

bwplotka · 2020-01-07T13:21:15Z

cmd/thanos/rule.go

-			span.Finish()
+		for _, i := range rand.Perm(len(queriers)) {
+			querier := queriers[i]
+			c := promclient.NewClient(logger, querier)


Why not creating this at the start of ruler? (:

bwplotka

Thanks for doing this and addressing all comments! Super tiny nits and we can merge IMO 👍

pkg/http/http.go

test/e2e/rule_test.go

bwplotka · 2020-01-09T16:08:45Z

Rebase needed ):

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the tls-and-auth-for-query-rules branch 2 times, most recently from 97b6f73 to 51cb94e Compare January 6, 2020 12:40

bwplotka approved these changes Jan 7, 2020

View reviewed changes

simonpasquier force-pushed the tls-and-auth-for-query-rules branch 2 times, most recently from ee16698 to 8c177a6 Compare January 8, 2020 15:37

bwplotka approved these changes Jan 9, 2020

View reviewed changes

pkg/http/http.go Outdated Show resolved Hide resolved

test/e2e/rule_test.go Outdated Show resolved Hide resolved

simonpasquier force-pushed the tls-and-auth-for-query-rules branch from ce17a0c to d761ef6 Compare January 9, 2020 14:36

simonpasquier added 4 commits January 9, 2020 17:58

*: support TLS and authentication for Thanos Ruler queries

8307c85

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

cmd/thanos: apply Bartek's comments

b1cd767

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Rename FanoutClient to Client

946d63c

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Address a few more nits

2be9b4b

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the tls-and-auth-for-query-rules branch from d761ef6 to 2be9b4b Compare January 9, 2020 16:59

bwplotka merged commit 1a419c2 into thanos-io:master Jan 9, 2020

simonpasquier deleted the tls-and-auth-for-query-rules branch January 10, 2020 08:13

simonpasquier mentioned this pull request Jan 20, 2020

add thanos-ruler operator prometheus-operator/prometheus-operator#2943

Merged

eswdd mentioned this pull request Mar 10, 2020

query: support store config file #2226

Closed

2 tasks


		queryConfig := extflag.RegisterPathOrContent(cmd, "query.config", "YAML file that contains query API servers configuration. See format details: https://thanos.io/components/rule.md/#configuration. If defined, it takes precedence over the '--query' and '--query.sd-files' flags.", false)

		fileSDFiles := cmd.Flag("query.sd-files", "Path to file that contain addresses of query peers. The path can be a glob pattern (repeatable).").

	return nil, errors.Errorf("no query peer reachable")
	return nil, errors.Errorf("no query API server reachable")

	func addToGroup(g run.Group, c http_util.FanoutClient, interval time.Duration) {
	func addDiscoveryGroups(g run.Group, c http_util.FanoutClient, interval time.Duration) {

		@@ -738,36 +719,49 @@ func queryFunc(
		}

		return func(ctx context.Context, q string, t time.Time) (promql.Vector, error) {

Conversation

simonpasquier commented Jan 6, 2020

Changes

Verification

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bwplotka commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants