Add multi tenant query federation by simonswine · Pull Request #3250 · cortexproject/cortex

simonswine · 2020-09-29T17:48:34Z

What this PR does:

This intends to allow cross tenant querying. The implemenation follows this proposal

Which issue(s) this PR fixes:

Fixes #923

Checklist/TODO

Merge Add tenant resolver #3486 first
Provide a design document with the details of the the form of the implementation (esp. what this would mean for configured tenant limits and metrics exposed about queries)
Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

docs/configuration/config-file-reference.md

integration/querier_tenant_federation_test.go

simonswine · 2020-12-01T09:36:01Z

pkg/api/middlewares.go

I am not too sure if I can cacheGenNumber like that

I don't think it's likely it will lead to collisions. However, it will increase the number of characters that prefix the Memcached item key string. This means that for multi-tenant queries across enough tenants caching could break due to the item key being too long. The existing limit is not very high at 150 characters. Multi-tenant queries will already have larger cache item keys since it will contain the concatenation of multiple tenant IDs. The cachegen number will exacerbate this issue further.

There are two options for dealing with this:

Ignore the cache generation for multi-tenant queries in this PR. It's not a commonly used feature and IMO it would be fine to ignore it for this PR. It

Consolidate the cachegen number into a small, consistent, unique string. One option off the top of my head is to convert the strings to integers and adding them together then use a string of the resulting value. The

My preference would be option 1 since it would reduce the scope of the PR a bit more and make it a bit easier to review. Cache generation handling can be added in a future PR.

I have deferred that to a future PR as suggested and added a TODO comment

pkg/cortex/cortex.go

jtlisi

This looks great, I added a few questions and nits. I want to go through it one more time before I add an approval. However, I didn't any major issues in my first pass.

docs/configuration/config-file-reference.md

pkg/querier/merge_queryable.go

pkg/cortex/cortex.go

pkg/querier/merge_queryable.go

jtlisi

LGTM

Most of the nits/suggestions are very minor except for the cachegen topic. Once that is sorted I'll approve and see if we can get a second reviewer on this.

jtlisi · 2020-12-04T16:54:38Z

pkg/api/middlewares.go

I don't think it's likely it will lead to collisions. However, it will increase the number of characters that prefix the Memcached item key string. This means that for multi-tenant queries across enough tenants caching could break due to the item key being too long. The existing limit is not very high at 150 characters. Multi-tenant queries will already have larger cache item keys since it will contain the concatenation of multiple tenant IDs. The cachegen number will exacerbate this issue further.

There are two options for dealing with this:

Ignore the cache generation for multi-tenant queries in this PR. It's not a commonly used feature and IMO it would be fine to ignore it for this PR. It

Consolidate the cachegen number into a small, consistent, unique string. One option off the top of my head is to convert the strings to integers and adding them together then use a string of the resulting value. The

My preference would be option 1 since it would reduce the scope of the PR a bit more and make it a bit easier to review. Cache generation handling can be added in a future PR.

pkg/cortex/cortex.go

jtlisi · 2020-12-04T16:58:54Z

integration/querier_tenant_federation_test.go

thought: We should be able to enable this feature for every e2e test and not have any failures. Doing so could provide a good smoke test for the feature.

@jtlisi Not entirely sure if you meant to do this locally once or do you want to have that as part of the integration test run by CI?

This is still a TODO, will look into this tomorrow

No need to commit any changes here. Just smoke testing that the tests pass if you enable it for globally on your local should be enough.

I have done that today and it didn't not come up with any unexpected failure (apart from the Backward_compatibility tests, which is expected as the feature flag is not recognised by them)

pkg/querier/tenantfederation/tenant_federation.go

csmarchbanks

Nice work!

Do we want to have an entirely new package for tenantfederation? It seems like all that is there is a new type of queryable which could also just be added to the querier package. Similarly, should the configuration be a new top level, or nested under querier? One thing that would help me decide is if there is more cross cutting flags that will be needed? Right now the flag is only used in querier related code.

pkg/frontend/v1/frontend.go

This experimental feature allows queries to cover data from more than a single Cortex tenant and can be activated by supplying `-tenant-federation.enabled` to all cortex instances. To query multiple tenants a `|` separated tenant list can be specified in the `X-Scope-OrgID` header. The source tenant of a metric will be exposed in the label `__tenant_id__`. Signed-off-by: Christian Simon <[email protected]>

This ensures the limit is aggregated correctly of the setting of each individual tenant. It also implements the logic for the v2 query frontend. Signed-off-by: Christian Simon <[email protected]>

Signed-off-by: Christian Simon <[email protected]>

Regexp matcher need to be initialized, this adds a test for regexp matcher and fixes the underlying issue. Signed-off-by: Christian Simon <[email protected]>

Signed-off-by: Christian Simon <[email protected]>

…ication Signed-off-by: Christian Simon <[email protected]>

Signed-off-by: Christian Simon <[email protected]>

To avoid any future regressions in the multi tenant support within the query path, a faillint command tests if TenantID is used and if it finds one, it suggestest using TenantIDs instead> Signed-off-by: Christian Simon <[email protected]>

Signed-off-by: Christian Simon <[email protected]>

simonswine · 2020-12-23T12:32:22Z

* `pkg/frontend/transport/handler.go` is still usig `tenant.TenantID`. How should it behave there?

* What do you think about adding a `faillint` rule to fail on `tenant.TenantID` used in `pkg/frontend/...`, `pkg/scheduler/...` and `pkg/querier/tenantfederation/...` to make sure we handle it everywhere (in the future)?

* What's about `tenant.TenantID`  usage in `pkg/querier/queryrange`? Have you checked where we should handle it? For example, I think `limitsMiddleware` should be tenants federation aware at least. This is another package where having a `faillint` may help to prevent future regressions.

@pracucci I think I have addressed all of the remaining points (I think 🙂 ).

Esp those ones had the biggest changes, so certainly are in need for another review:

The result cache gets skipped once there are more than one tenant b41a4df
limitsMiddleware is now using the "strictest" of the tenant's limits 0667242
Fail lint is checking the packages in the query path: e312687

pracucci · 2020-12-23T14:43:14Z

Thanks a lot @simonswine for your hard work and patiently address all comments! I agree on skipping the query results caching in this PR and address it in a follow up PR (given we have to design how to deal with cache invalidation done for deleted series).

This PR looks in a good shape, considering it's an experimental feature. We can merge this PR, so that we begin testing it, and in the meanwhile you can work on follow up work (eg. query results caching).

Signed-off-by: Christian Simon <[email protected]>

bhiravabhatla · 2021-01-20T10:35:26Z

Is this released as part of 1.6. Could not find this in release notes. Its already present in configuration documentation though.

pstibrany · 2021-01-20T10:36:52Z

Is this released as part of 1.6. Could not find this in release notes. Its already present in configuration documentation though.

It will be part of Cortex 1.7.

Documentation on the website currently reflects master branch, with latest (unreleased) changes.

cabrinha · 2021-02-11T02:24:20Z

In order to add Cortex as a datasource for grafana, when using auth and tenants... Whats the config for the datasource need to look like in order to see every tenant in one datasource?

simonswine · 2021-02-22T12:15:52Z

In order to add Cortex as a datasource for grafana, when using auth and tenants... Whats the config for the datasource need to look like in order to see every tenant in one datasource?

@cabrinha unfortunately there is no way to specify this currently. You need to list the tenants explicitly (separated by a |). If you care about this enough it would be good to create a new issue with a feature request and detailing your use case a bit more.

* Add tenant query federation This experimental feature allows queries to cover data from more than a single Cortex tenant and can be activated by supplying `-tenant-federation.enabled` to all cortex instances. To query multiple tenants a `|` separated tenant list can be specified in the `X-Scope-OrgID` header. The source tenant of a metric will be exposed in the label `__tenant_id__`. Signed-off-by: Christian Simon <[email protected]> * Aggregate the limit of maxQueriers correctly This ensures the limit is aggregated correctly of the setting of each individual tenant. It also implements the logic for the v2 query frontend. Signed-off-by: Christian Simon <[email protected]> * Fix tenant labels and make LabelNames more efficient Signed-off-by: Christian Simon <[email protected]> * Use tsdb_errors for error handling Signed-off-by: Christian Simon <[email protected]> * Fix uninitialized label matcher Regexp matcher need to be initialized, this adds a test for regexp matcher and fixes the underlying issue. Signed-off-by: Christian Simon <[email protected]> * Use map for filterValuesByMatchers to reduce allocations Signed-off-by: Christian Simon <[email protected]> * Address review suggestions Signed-off-by: Christian Simon <[email protected]> * Add validation.SmallestPositiveNonZeroIntPerTenant to avoid code duplication Signed-off-by: Christian Simon <[email protected]> * Add limitations and experimental status to docs Signed-off-by: Christian Simon <[email protected]> * Remove unnecessary cast of defaultTenantLabel Signed-off-by: Christian Simon <[email protected]> * Handle query-range limits for multi tenant queries Signed-off-by: Christian Simon <[email protected]> * Skip results cache, if multi tenants query Signed-off-by: Christian Simon <[email protected]> * Add failint to ensure query path supports multiple tenants To avoid any future regressions in the multi tenant support within the query path, a faillint command tests if TenantID is used and if it finds one, it suggestest using TenantIDs instead> Signed-off-by: Christian Simon <[email protected]> * Align CHANGELOG line with the flag description Signed-off-by: Christian Simon <[email protected]> * Add a limitation about missing results cache Signed-off-by: Christian Simon <[email protected]>

* Add tenant resolver (cortexproject/cortex#3486) * Add tenant resolver package This implements the multi tenant resolver as described by the [proposal] for multi tenant query-federation. By default it behaves like before, but it's implementation can be swapped out. [proposal]: cortexproject/cortex#3364 Signed-off-by: Christian Simon <[email protected]> * Replace usages of `ExtractOrgID` Use TenantID or UserID depending on which of the methods are meant to be used. Signed-off-by: Christian Simon <[email protected]> * Replace usages of `ExtractOrgIDFromHTTPRequest` This is replaced by ExtractTenantIDFromHTTPRequest, which makes sure that exactly one tenant ID is set. Signed-off-by: Christian Simon <[email protected]> * Add methods to `tenant` package to use resolver directly Signed-off-by: Christian Simon <[email protected]> * Remove UserID method from Resolver interface We need a better definition for what we are trying to achieve with UserID before we can add it to the interface Signed-off-by: Christian Simon <[email protected]> * Update comment on the TenantID/TenantIDs Signed-off-by: Christian Simon <[email protected]> * Improve performance of NormalizeTenantIDs - reduce allocations by reusing the input slice during de-duplication Signed-off-by: Christian Simon <[email protected]> * Add multi tenant query federation (cortexproject/cortex#3250) * Add tenant query federation This experimental feature allows queries to cover data from more than a single Cortex tenant and can be activated by supplying `-tenant-federation.enabled` to all cortex instances. To query multiple tenants a `|` separated tenant list can be specified in the `X-Scope-OrgID` header. The source tenant of a metric will be exposed in the label `__tenant_id__`. Signed-off-by: Christian Simon <[email protected]> * Aggregate the limit of maxQueriers correctly This ensures the limit is aggregated correctly of the setting of each individual tenant. It also implements the logic for the v2 query frontend. Signed-off-by: Christian Simon <[email protected]> * Fix tenant labels and make LabelNames more efficient Signed-off-by: Christian Simon <[email protected]> * Use tsdb_errors for error handling Signed-off-by: Christian Simon <[email protected]> * Fix uninitialized label matcher Regexp matcher need to be initialized, this adds a test for regexp matcher and fixes the underlying issue. Signed-off-by: Christian Simon <[email protected]> * Use map for filterValuesByMatchers to reduce allocations Signed-off-by: Christian Simon <[email protected]> * Address review suggestions Signed-off-by: Christian Simon <[email protected]> * Add validation.SmallestPositiveNonZeroIntPerTenant to avoid code duplication Signed-off-by: Christian Simon <[email protected]> * Add limitations and experimental status to docs Signed-off-by: Christian Simon <[email protected]> * Remove unnecessary cast of defaultTenantLabel Signed-off-by: Christian Simon <[email protected]> * Handle query-range limits for multi tenant queries Signed-off-by: Christian Simon <[email protected]> * Skip results cache, if multi tenants query Signed-off-by: Christian Simon <[email protected]> * Add failint to ensure query path supports multiple tenants To avoid any future regressions in the multi tenant support within the query path, a faillint command tests if TenantID is used and if it finds one, it suggestest using TenantIDs instead> Signed-off-by: Christian Simon <[email protected]> * Align CHANGELOG line with the flag description Signed-off-by: Christian Simon <[email protected]> * Add a limitation about missing results cache Signed-off-by: Christian Simon <[email protected]> * Restrict path segments in TenantIDs (cortexproject/cortex#4375) (cortexproject/cortex#4376) This prevents paths generated from TenantIDs to become vulnerable to path traversal attacks. CVE-2021-36157 Signed-off-by: Christian Simon <[email protected]> * Update nolint statement Co-authored-by: Bryan Boreham <[email protected]>

pull-request-size bot added the size/L label Sep 29, 2020

simonswine force-pushed the crosstenant branch from a72bd88 to 761a1d4 Compare October 23, 2020 11:03

pull-request-size bot added size/XXL and removed size/L labels Oct 23, 2020

simonswine force-pushed the crosstenant branch 3 times, most recently from fc84860 to 023f1f1 Compare October 23, 2020 11:29

simonswine mentioned this pull request Oct 23, 2020

Support multiple org IDs weaveworks/common#201

Closed

simonswine force-pushed the crosstenant branch 2 times, most recently from 1d43854 to 3ceeb5e Compare October 26, 2020 16:07

simonswine force-pushed the crosstenant branch from 3ceeb5e to 62363b0 Compare November 13, 2020 17:52

simonswine changed the title ~~Multi tenant queries proof-of-concept~~ WIP: Multi tenant query federation Nov 13, 2020

simonswine force-pushed the crosstenant branch 2 times, most recently from 2a41025 to d815857 Compare November 26, 2020 18:06

pull-request-size bot added size/XL and removed size/XXL labels Nov 26, 2020

simonswine force-pushed the crosstenant branch from d815857 to c3dfd31 Compare November 27, 2020 17:19

simonswine commented Nov 27, 2020

View reviewed changes

docs/configuration/config-file-reference.md Outdated Show resolved Hide resolved

simonswine commented Dec 1, 2020

View reviewed changes

integration/querier_tenant_federation_test.go Outdated Show resolved Hide resolved

simonswine commented Dec 1, 2020

View reviewed changes

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

simonswine changed the title ~~WIP: Multi tenant query federation~~ Add multi tenant query federation Dec 1, 2020

simonswine force-pushed the crosstenant branch from c3dfd31 to c3432b1 Compare December 1, 2020 10:10

jtlisi reviewed Dec 2, 2020

View reviewed changes

simonswine force-pushed the crosstenant branch from c3432b1 to a6211ef Compare December 3, 2020 18:15

simonswine marked this pull request as ready for review December 3, 2020 18:20

jtlisi reviewed Dec 4, 2020

View reviewed changes

csmarchbanks reviewed Dec 4, 2020

View reviewed changes

pkg/frontend/v1/frontend.go Outdated Show resolved Hide resolved

simonswine force-pushed the crosstenant branch from a6211ef to 3bc1204 Compare December 8, 2020 17:51

pull-request-size bot removed the size/XL label Dec 8, 2020

simonswine added 14 commits December 23, 2020 12:18

Aggregate the limit of maxQueriers correctly

f047968

This ensures the limit is aggregated correctly of the setting of each individual tenant. It also implements the logic for the v2 query frontend. Signed-off-by: Christian Simon <[email protected]>

Fix tenant labels and make LabelNames more efficient

64c3918

Signed-off-by: Christian Simon <[email protected]>

Use tsdb_errors for error handling

42663d1

Signed-off-by: Christian Simon <[email protected]>

Fix uninitialized label matcher

7b94e53

Regexp matcher need to be initialized, this adds a test for regexp matcher and fixes the underlying issue. Signed-off-by: Christian Simon <[email protected]>

Use map for filterValuesByMatchers to reduce allocations

b22a22c

Signed-off-by: Christian Simon <[email protected]>

Address review suggestions

52b53a6

Signed-off-by: Christian Simon <[email protected]>

Add validation.SmallestPositiveNonZeroIntPerTenant to avoid code dupl…

9826281

…ication Signed-off-by: Christian Simon <[email protected]>

Add limitations and experimental status to docs

9bfc3af

Signed-off-by: Christian Simon <[email protected]>

Remove unnecessary cast of defaultTenantLabel

e51291b

Signed-off-by: Christian Simon <[email protected]>

Handle query-range limits for multi tenant queries

0667242

Signed-off-by: Christian Simon <[email protected]>

Skip results cache, if multi tenants query

b41a4df

Signed-off-by: Christian Simon <[email protected]>

Align CHANGELOG line with the flag description

54b9981

Signed-off-by: Christian Simon <[email protected]>

simonswine force-pushed the crosstenant branch from 891e692 to 54b9981 Compare December 23, 2020 12:20

Add a limitation about missing results cache

298c214

Signed-off-by: Christian Simon <[email protected]>

pracucci merged commit 6614def into cortexproject:master Dec 23, 2020

simonswine mentioned this pull request Dec 31, 2020

Implement result cache for tenant query federation #3640

Merged

3 tasks

midnightconman mentioned this pull request Mar 10, 2021

Tenant Federation grafana/loki#3462

Closed

simonswine mentioned this pull request Mar 22, 2022

Add tenant package grafana/dskit#143

Merged

clubanderson mentioned this pull request Mar 17, 2026

🌱 cortex: Query across multiple users / tenants / instances kubestellar/console-kb#1324

Merged

Conversation

simonswine commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonswine commented Dec 23, 2020

Uh oh!

pracucci commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhiravabhatla commented Jan 20, 2021

Uh oh!

pstibrany commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cabrinha commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonswine commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

simonswine commented Sep 29, 2020 •

edited

Loading

pracucci commented Dec 23, 2020 •

edited

Loading

pstibrany commented Jan 20, 2021 •

edited

Loading

cabrinha commented Feb 11, 2021 •

edited

Loading