Query Frontend: forward tenant information downstream #6595

douglascamata · 2023-08-08T10:55:37Z

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

It's not relevant to end users (yet) because it has no impact, just like #6530.

Changes

This PR continues the work from #6530, which started to forward tenant information down from Querier to Store API endpoints.

Here the tenant information is propagated from the Query Frontend to the Queriers. These are the change done by this PR:

Query Frontend has now 3 extra configuration options regarding tenancy, which are matching Receive's and Querier's:
- query-frontend.tenant-header: indicates name of the tenant header, defaults to THANOS-TENANT.
  When this option is provided at the same time as query-frontend.org-id-header: an error is thrown, the Query Frontend won't start, the user will then be warned to stop using the org id header configuration and migrate to the tenant header one.
- query-frontend.default-tenant-id: indicates the default tenant ID, defaults to default-tenant. This is the tenant that will be used when the request doesn't specify one.
- query-frontend.tenant-certificate-field: use the TLS client's certificate field to identify the tenant.
When a tenant is identified using the previously mentioned logic, it's forwarded to Queriers using the internal tenant header (THANOS-TENANT).
- The tenancy.GetTenantFromHTTP helper has been updated to match this behavior.
- The X-Scope-Orgid header from Cortex is kept for compatibility with request middlewares and tripperwares imported from Cortex' queryfrontend package. If and/or when Cortex allows the org id header name to be customized, the duplication can be removed in favor of properly configuring Cortex.

Verification

Added e2e tests and did manual tests.

Signed-off-by: Douglas Camata <[email protected]>

* Add tenant forwarding based on tenant cert. * Fix bugs in the header management. Signed-off-by: Douglas Camata <[email protected]>

Signed-off-by: Douglas Camata <[email protected]>

…enant

Signed-off-by: Douglas Camata <[email protected]>

philipgough · 2023-08-09T09:31:31Z

cmd/thanos/query_frontend.go

@@ -217,6 +223,13 @@ func runQueryFrontend(
 	cfg *queryFrontendConfig,
 	comp component.Component,
 ) error {
+	cfg.ForwardHeaders = append(cfg.ForwardHeaders, tenancy.DefaultTenantHeader)
+	cfg.orgIdHeaders = append(cfg.orgIdHeaders, tenancy.DefaultTenantHeader)
+	if cfg.TenantHeader != "" && cfg.TenantHeader != tenancy.DefaultTenantHeader {


should this be an if else? or is it a case that we always want to forward the default tenant header since it is appended above?

I prefer to use only if with a good initialization instead of if else. Makes it easier to read: "we always forward the default tenant header and if there's a custom one, we forward it too".

Buuuut, I think I can remove cfg.ForwardHeaders = append(cfg.ForwardHeaders, cfg.TenantHeader), as a custom tenant header doesn't need to be forwarded as-is downstream. It'll be converted to the internal one (the default), before being forwarded.

But I still have to add it to the list of orgIdHeaders.

yes, don't disagree, was just wondering if it was a case of always passing on the default but that answers it.

pkg/queryfrontend/roundtrip.go

philipgough · 2023-08-09T09:35:06Z

pkg/tenancy/tenancy.go

+		tenant = r.Header.Get(DefaultTenantHeader)
+		if tenant == "" {
+			tenant = defaultTenantID
+		}


do you think its worth logging down below if we end up overwriting based on the cert? or maybe we could just handle this on startup with a log or an error?

It might be too spammy. We had similar logs when parsing the tenant information from the request in the Store GW and they were removed: 510c054

test/e2e/query_frontend_test.go

Signed-off-by: Douglas Camata <[email protected]>

We should always and only forward the default (internal) one. Signed-off-by: Douglas Camata <[email protected]>

jacobbaungard · 2023-08-15T08:44:27Z

cmd/thanos/query_frontend.go

@@ -146,6 +147,10 @@ func registerQueryFrontend(app *extkingpin.App) {

 	cmd.Flag("query-frontend.forward-header", "List of headers forwarded by the query-frontend to downstream queriers, default is empty").PlaceHolder("<http-header-name>").StringsVar(&cfg.ForwardHeaders)

+	cmd.Flag("query-frontend.tenant-header", "HTTP header to determine tenant").Default(tenancy.DefaultTenantHeader).Hidden().StringVar(&cfg.TenantHeader)
+	cmd.Flag("query-frontend.default-tenant", "Default tenant to use if tenant header is not present").Default(tenancy.DefaultTenant).Hidden().StringVar(&cfg.DefaultTenant)
+	cmd.Flag("query-frontend.tenant-certificate-field", "Use TLS client's certificate field to determine tenant for requests. Must be one of "+tenancy.CertificateFieldOrganization+", "+tenancy.CertificateFieldOrganizationalUnit+" or "+tenancy.CertificateFieldCommonName+". This setting will cause the query-frontend.tenant-header flag value to be ignored.").Default("").EnumVar(&cfg.TenantCertField, "", tenancy.CertificateFieldOrganization, tenancy.CertificateFieldOrganizationalUnit, tenancy.CertificateFieldCommonName)


This one should probably be hidden as the other two, until this functionality is ready.

jacobbaungard · 2023-08-15T09:41:40Z

cmd/thanos/query_frontend.go

@@ -146,6 +147,10 @@ func registerQueryFrontend(app *extkingpin.App) {

 	cmd.Flag("query-frontend.forward-header", "List of headers forwarded by the query-frontend to downstream queriers, default is empty").PlaceHolder("<http-header-name>").StringsVar(&cfg.ForwardHeaders)

+	cmd.Flag("query-frontend.tenant-header", "HTTP header to determine tenant").Default(tenancy.DefaultTenantHeader).Hidden().StringVar(&cfg.TenantHeader)


I'm not sure if this is the right place to discuss this, but the query frontend also has a query-frontend.org-id-header arg. Thanos side, I believe this is only used when logging slow queries, but probably has some more use for Cortex.

Do we want to keep both the org-id-header and the tenant-header long term ? Could perhaps be nice to unify that into one cmd line arg if possible.

Long term we should indeed converge on only one to avoid confusion for users and ourselves, and for consistency.

As part of the migration process, I can mark the query-frontend.org-id-header as deprecated for now, mention the new one, and store the configured org-id-header in the tenant header config. This way someone already using this will have a warning about the change and can easily adapt to the new CLI arg, allowing us to completely drop the org ID config in a future breaking change.

Internally and more technically speaking, I am copying the tenant header field's value to the Org ID one because of some middlewares we are importing/reusing from Cortex, but I think over time we can adapt them to avoid this and clean things up.

What do you think, @jacobbaungard?

Sounds like a good plan to me. What would happen if one define both, can we error out in that case?

@jacobbaungard we could error out or print out a warning & give priority to the tenant header config, similar to what happens if you configure the header and the cert field in Receive. I personally have no preference. Will update to error out when both are provided. 👍

Ok, I think the orgID was introduced only as a method of identifying sources of slow queries and passing that information to Cortex. It's also a repeatable flag, with significance in how headers are ordered, afaiu reading original issue/PR.

Thus I don't think it is mutually exclusive with this tenant header, and both have different semantics as well, as you can have multiple orgID headers, but only a single tenant one. So let's not error or warn the user, and keep it for now.

cc: @yeya24 in case he has some other thoughts on this! 🙂

Signed-off-by: Douglas Camata <[email protected]>

…enant

Signed-off-by: Douglas Camata <[email protected]>

jacobbaungard · 2023-08-16T15:25:38Z

cmd/thanos/query_frontend.go

@@ -217,6 +224,19 @@ func runQueryFrontend(
 	cfg *queryFrontendConfig,
 	comp component.Component,
 ) error {
+	if len(cfg.orgIdHeaders) != 0 && cfg.TenantHeader != "" {


Actually, won't this always evaluate true when setting the orgIdHeader, as the TenantHeader has a non-empty default value?

Hummmm, true. I'm starting to think it will be much simpler to output a warning and give priority to the tenant header configuration. 🤔

Let me see what I can do!

Signed-off-by: Douglas Camata <[email protected]>

jacobbaungard

Great, looks good to me!

Signed-off-by: Douglas Camata <[email protected]>

…enant

moadz · 2023-09-27T14:37:31Z

cc: @yeya24 as a reviewer with some cortex knowledge :)

yeya24 · 2023-09-27T15:58:57Z

pkg/queryfrontend/roundtrip.go

-	queriesCount *prometheus.CounterVec
+	queriesCount  *prometheus.CounterVec
+	tenantHeader  string
+	defaultTenant string


Where are the two new fields used in the roundTripper struct?

Nice catch, @yeya24. I think they are some leftovers of previous approaches I tried. I'm removing them.

…enant

Signed-off-by: Douglas Camata <[email protected]>

…enant

douglascamata · 2023-10-02T11:30:47Z

@yeya24 took care of the comment and rebased, please have another look when you have some time. Thanks! 🙇

saswatamcode

Thanks for the great work! Generally LGTM, but cc: @yeya24 on the point of orgID and tenant headers.

saswatamcode · 2023-10-05T08:05:19Z

cmd/thanos/query_frontend.go

@@ -146,6 +147,10 @@ func registerQueryFrontend(app *extkingpin.App) {

 	cmd.Flag("query-frontend.forward-header", "List of headers forwarded by the query-frontend to downstream queriers, default is empty").PlaceHolder("<http-header-name>").StringsVar(&cfg.ForwardHeaders)

+	cmd.Flag("query-frontend.tenant-header", "HTTP header to determine tenant").Default(tenancy.DefaultTenantHeader).Hidden().StringVar(&cfg.TenantHeader)


Ok, I think the orgID was introduced only as a method of identifying sources of slow queries and passing that information to Cortex. It's also a repeatable flag, with significance in how headers are ordered, afaiu reading original issue/PR.

Thus I don't think it is mutually exclusive with this tenant header, and both have different semantics as well, as you can have multiple orgID headers, but only a single tenant one. So let's not error or warn the user, and keep it for now.

cc: @yeya24 in case he has some other thoughts on this! 🙂

douglascamata · 2023-10-05T09:46:43Z

@saswatamcode from the Thanos perspective, the headers do the same thing. In theory, you can send multiple values for any HTTP header (internally in the stdlib they are map[string][]string), we only decide to ignore anything being the 1st value for now because we're not implementing anything cross-tenant related (yet).

HTTP headers ordering is not a concern we need to have, stdlib takes care of this.

I could have completely removed OrgID from the codebase and keep the slow query log working fine with the tenant ID. Only didn't do it because I wanted this PR to be small. Eventually, we can get rid of this concept of "OrgID" in Thanos, as in this context we talk about tenants and not "orgs".

Note that nothing here makes Thanos incompatible with Cortex and vice-versa. On this context. the only change is the rename of the cli arg from query-frontend.org-id-header to query-frontend.tenant-header. All behavior should be preserved when moving between them.

douglascamata · 2023-10-05T10:00:10Z

Even added this in the feature's e2e test to ensure the OrgID header keeps being passed downstream: https://github.com/thanos-io/thanos/pull/6595/files#diff-e84e75cb190ebf54e47774afa03e68ed6cbc3f03e6216ae31e79c58509b0acbeR984-R985

…enant Signed-off-by: Douglas Camata <[email protected]>

Signed-off-by: Douglas Camata <[email protected]>

saswatamcode

LGTM! I think this is good to go.

For the orgID stuff, I believe it's there as a separate header due to certain cortex feature, but we can make it more in-line with Thanos tenancy model or remove it completely as Thanos has its own slow query log. 🙂

* Make query frontend forward tenant info downstream Signed-off-by: Douglas Camata <[email protected]> * Add TODO for client certificate Signed-off-by: Douglas Camata <[email protected]> * Finish qfe tenant forward features * Add tenant forwarding based on tenant cert. * Fix bugs in the header management. Signed-off-by: Douglas Camata <[email protected]> * Test more scenarios Signed-off-by: Douglas Camata <[email protected]> * Goimports files Signed-off-by: Douglas Camata <[email protected]> * Rerun CI Signed-off-by: Douglas Camata <[email protected]> * Rerun goimports without any line breaks Signed-off-by: Douglas Camata <[email protected]> * Resort imports in groups (std, 3rd-party, local) Signed-off-by: Douglas Camata <[email protected]> * No need to forward a custom tenant header downstream We should always and only forward the default (internal) one. Signed-off-by: Douglas Camata <[email protected]> * Make query frontend tenant cert config hidden Signed-off-by: Douglas Camata <[email protected]> * Error out when org ig and tenant header are provided at the same time Signed-off-by: Douglas Camata <[email protected]> * Fix typo Signed-off-by: Douglas Camata <[email protected]> * Update docs Signed-off-by: Douglas Camata <[email protected]> * Improve comments around forward and org id headers for tenancy Signed-off-by: Douglas Camata <[email protected]> * Add some test comments Signed-off-by: Douglas Camata <[email protected]> * Improve logic for detecting tenant and org id headers were provided Signed-off-by: Douglas Camata <[email protected]> * Fix order of check for org id headers Signed-off-by: Douglas Camata <[email protected]> * Add some TODO comments for the future Signed-off-by: Douglas Camata <[email protected]> * Rerun CI Signed-off-by: Douglas Camata <[email protected]> * Make default tenant flag in qfe match receive's Signed-off-by: Douglas Camata <[email protected]> * Remove unnecessary struct fields Signed-off-by: Douglas Camata <[email protected]> * Add instant query to tenant forward tests Signed-off-by: Douglas Camata <[email protected]> * Rerun build Signed-off-by: Douglas Camata <[email protected]> --------- Signed-off-by: Douglas Camata <[email protected]>

douglascamata added 4 commits August 3, 2023 19:56

Make query frontend forward tenant info downstream

f4506f5

Signed-off-by: Douglas Camata <[email protected]>

Add TODO for client certificate

4b2ca14

Signed-off-by: Douglas Camata <[email protected]>

Finish qfe tenant forward features

aa33d04

* Add tenant forwarding based on tenant cert. * Fix bugs in the header management. Signed-off-by: Douglas Camata <[email protected]>

Test more scenarios

6e8ed12

Signed-off-by: Douglas Camata <[email protected]>

pull-request-size bot added the size/L label Aug 8, 2023

douglascamata added the component: query-frontend label Aug 8, 2023

douglascamata added 2 commits August 8, 2023 13:12

Goimports files

ffcbed6

Signed-off-by: Douglas Camata <[email protected]>

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

92e3d71

…enant

douglascamata marked this pull request as ready for review August 8, 2023 15:08

Rerun CI

e4ea657

Signed-off-by: Douglas Camata <[email protected]>

philipgough reviewed Aug 9, 2023

View reviewed changes

pkg/queryfrontend/roundtrip.go Show resolved Hide resolved

philipgough reviewed Aug 9, 2023

View reviewed changes

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

douglascamata added 3 commits August 9, 2023 12:29

Rerun goimports without any line breaks

85506ab

Signed-off-by: Douglas Camata <[email protected]>

Resort imports in groups (std, 3rd-party, local)

399a365

Signed-off-by: Douglas Camata <[email protected]>

No need to forward a custom tenant header downstream

6e9ffeb

We should always and only forward the default (internal) one. Signed-off-by: Douglas Camata <[email protected]>

jacobbaungard reviewed Aug 15, 2023

View reviewed changes

douglascamata added 5 commits August 16, 2023 12:45

Make query frontend tenant cert config hidden

aa9f3d3

Signed-off-by: Douglas Camata <[email protected]>

Error out when org ig and tenant header are provided at the same time

f955f71

Signed-off-by: Douglas Camata <[email protected]>

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

500d29b

…enant

Fix typo

08dd0c4

Signed-off-by: Douglas Camata <[email protected]>

Update docs

dd619e3

Signed-off-by: Douglas Camata <[email protected]>

douglascamata requested review from philipgough and jacobbaungard August 16, 2023 14:42

jacobbaungard reviewed Aug 16, 2023

View reviewed changes

douglascamata added 4 commits August 16, 2023 18:08

Improve comments around forward and org id headers for tenancy

531ded7

Signed-off-by: Douglas Camata <[email protected]>

Add some test comments

9d62032

Signed-off-by: Douglas Camata <[email protected]>

Improve logic for detecting tenant and org id headers were provided

01f8071

Signed-off-by: Douglas Camata <[email protected]>

Fix order of check for org id headers

60e093e

Signed-off-by: Douglas Camata <[email protected]>

douglascamata added 2 commits August 16, 2023 21:51

Add some TODO comments for the future

4e8ef60

Signed-off-by: Douglas Camata <[email protected]>

Rerun CI

8cf2ba0

Signed-off-by: Douglas Camata <[email protected]>

jacobbaungard approved these changes Aug 17, 2023

View reviewed changes

jacobbaungard mentioned this pull request Sep 1, 2023

Store: Add tenant label to exported metrics #6690

Merged

2 tasks

douglascamata added 2 commits September 13, 2023 16:15

Make default tenant flag in qfe match receive's

f5c5cba

Signed-off-by: Douglas Camata <[email protected]>

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

82a7e3a

…enant

douglascamata added the feature request/improvement label Sep 13, 2023

yeya24 reviewed Sep 27, 2023

View reviewed changes

douglascamata added 4 commits September 27, 2023 18:36

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

f1667e2

…enant

Remove unnecessary struct fields

1033baf

Signed-off-by: Douglas Camata <[email protected]>

Add instant query to tenant forward tests

41c3f6e

Signed-off-by: Douglas Camata <[email protected]>

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

fec5df9

…enant

saswatamcode previously approved these changes Oct 5, 2023

View reviewed changes

Merge branch 'main' of github.com:thanos-io/thanos into qfe-forward-t…

d6ec416

…enant Signed-off-by: Douglas Camata <[email protected]>

douglascamata dismissed saswatamcode’s stale review via d6ec416 October 24, 2023 09:19

Rerun build

6daddf7

Signed-off-by: Douglas Camata <[email protected]>

saswatamcode approved these changes Oct 25, 2023

View reviewed changes

saswatamcode enabled auto-merge (squash) October 25, 2023 03:19

saswatamcode merged commit df48504 into thanos-io:main Oct 25, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query Frontend: forward tenant information downstream #6595

Query Frontend: forward tenant information downstream #6595

douglascamata commented Aug 8, 2023 •

edited

Loading

philipgough Aug 9, 2023

douglascamata Aug 9, 2023

philipgough Aug 9, 2023

philipgough Aug 9, 2023

douglascamata Aug 9, 2023

jacobbaungard Aug 15, 2023 •

edited

Loading

jacobbaungard Aug 15, 2023

douglascamata Aug 16, 2023

jacobbaungard Aug 16, 2023

douglascamata Aug 16, 2023

saswatamcode Oct 5, 2023

jacobbaungard Aug 16, 2023

douglascamata Aug 16, 2023

jacobbaungard left a comment

moadz commented Sep 27, 2023

yeya24 Sep 27, 2023

douglascamata Sep 27, 2023

douglascamata commented Oct 2, 2023

saswatamcode left a comment

saswatamcode Oct 5, 2023

douglascamata commented Oct 5, 2023

douglascamata commented Oct 5, 2023

saswatamcode left a comment

		@@ -146,6 +147,10 @@ func registerQueryFrontend(app *extkingpin.App) {

		cmd.Flag("query-frontend.forward-header", "List of headers forwarded by the query-frontend to downstream queriers, default is empty").PlaceHolder("<http-header-name>").StringsVar(&cfg.ForwardHeaders)

		cmd.Flag("query-frontend.tenant-header", "HTTP header to determine tenant").Default(tenancy.DefaultTenantHeader).Hidden().StringVar(&cfg.TenantHeader)

Query Frontend: forward tenant information downstream #6595

Query Frontend: forward tenant information downstream #6595

Conversation

douglascamata commented Aug 8, 2023 • edited Loading

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobbaungard Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobbaungard left a comment

Choose a reason for hiding this comment

moadz commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglascamata commented Oct 2, 2023

saswatamcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglascamata commented Oct 5, 2023

douglascamata commented Oct 5, 2023

saswatamcode left a comment

Choose a reason for hiding this comment

douglascamata commented Aug 8, 2023 •

edited

Loading

jacobbaungard Aug 15, 2023 •

edited

Loading