Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always validate tenant IDs and introduce max tenants setting #6959

Merged
merged 11 commits into from
Jan 2, 2024
Merged

Always validate tenant IDs and introduce max tenants setting #6959

merged 11 commits into from
Jan 2, 2024

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Dec 18, 2023 •
edited
Loading

What this PR does

This change updates dskit to a version that does not rely on global state for tenant ID parsing. Specifically it pulls in grafana/dskit#445. As part of this there are a few things changing:

  • We use multi-tenant parsing logic everywhere which actually enforces limits on the length of tenant IDs and the legal characters in them.
  • Instead of relying on single tenant parsing logic when tenant federation is disabled to reject multi-tenant queries, we add a query middleware that validates the number of expected tenants based on configuration.
  • We introduce a new setting to limit the max number of tenant IDs that may be included in a multi-tenant query.

This change will result in different behavior in a few cases. However, it brings the actual behavior of Mimir in line with the documented behavior. Specifically, the following behavior changes (copied from dskit PR):

  • SingleResolver did not previously enforce a limit on the length of a tenant ID. A limit of 150 characters is now enforced. This has always been the documented behavior as far back as Cortex, where this code originated. Not enforcing it was an oversight.
  • SingleResolver previously allowed tenant IDs to contain the | character. This is no longer allowed as part of a tenant ID and instead will be treated as a divider between multiple tenant IDs. This has always been the documented behavior as far back as Cortex, where this code originated. Not enforcing it was an oversight.

Which issue(s) this PR fixes or relates to

See grafana/dskit#445

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@56quarters 56quarters force-pushed the 56quarters/federation-rework branch 3 times, most recently from 7ecf10c to 48c864c Compare December 18, 2023 18:14
56quarters
56quarters commented Dec 18, 2023
View reviewed changes
pkg/api/error/error.go Outdated Show resolved Hide resolved
@56quarters 56quarters marked this pull request as ready for review December 19, 2023 18:41
@56quarters 56quarters requested review from grafanabot and a team as code owners December 19, 2023 18:41
leizor
leizor approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@leizor leizor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

dimitarvdimitrov
dimitarvdimitrov reviewed Dec 20, 2023
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally look good. I would be more explicit in the changelog entry because this can break users. Also not sure how to handle label values and cardinality requests.

pkg/frontend/querymiddleware/roundtrip.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/tenant_federation.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@56quarters
Always validate tenant IDs and introduce max tenants setting
a42be15 
This change updates dskit to a version that does _not_ rely on global
state for tenant ID parsing. Specifically it pulls in grafana/dskit#445.
As part of this there are a few things changing:

* We use multi-tenant parsing logic everywhere which actually enforces
  limits on the length of tenant IDs and the legal characters in them.
* Instead of relying on single tenant parsing logic when tenant federation
  is disabled to reject multi-tenant queries, we add a query middleware that
  validates the number of expected tenants based on configuration.
* We introduce a new setting to limit the max number of tenant IDs that may be
  included in a multi-tenant query.

This change will result in different behavior in a few cases. However, it
brings the actual behavior of Mimir in line with the documented behavior.
Specifically, the following behavior changes (copied from dskit PR):

* SingleResolver did not previously enforce a limit on the length of a tenant
  ID. A limit of 150 characters is now enforced. This has always been the
  documented behavior as far back as Cortex, where this code originated. Not
  enforcing it was an oversight.
* SingleResolver previously allowed tenant IDs to contain the | character.
  This is no longer allowed as part of a tenant ID and instead will be treated
  as a divider between multiple tenant IDs. This has always been the documented
  behavior as far back as Cortex, where this code originated. Not enforcing it
  was an oversight.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Code review changes
9f6a995 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
License header
b0797ee 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
CHANGELOG
521e287 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Unexport apiError since it isn't needed anymore
0bdc670 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Add tenant validation middleware in the API layer
11a795b
@56quarters
License header, AGAIN
bf14058 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters
Make changelog more explicit about potential breakage.
fd24fd6 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
dimitarvdimitrov
dimitarvdimitrov approved these changes Dec 26, 2023
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for addressing my comments! Happy holidays 🎄

pkg/api/tenant.go Outdated Show resolved Hide resolved
@pracucci pracucci mentioned this pull request Jan 2, 2024
Closed
1 task
pracucci
pracucci approved these changes Jan 2, 2024
View reviewed changes
pkg/api/tenant.go Outdated Show resolved Hide resolved
56quarters and others added 2 commits January 2, 2024 09:47
@56quarters @pracucci
Update pkg/api/tenant.go
862bcd9 
Co-authored-by: Marco Pracucci <marco@pracucci.com>
@56quarters
Don't export middleware constructor
fbd4049 
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters merged commit 41fd54e into main Jan 2, 2024
28 checks passed
@56quarters 56quarters deleted the 56quarters/federation-rework branch January 2, 2024 15:19
@56quarters 56quarters mentioned this pull request Apr 10, 2024
Closed
@eyenx eyenx mentioned this pull request May 28, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

@dimitarvdimitrov dimitarvdimitrov dimitarvdimitrov approved these changes

@leizor leizor leizor approved these changes

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@56quarters @pracucci @dimitarvdimitrov @leizor