Remove global state from tenant ID parsing package #445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56quarters
force-pushed
the
56quarters/rm-tenant-state
branch
2 times, most recently
from
98ae518
to
1ae07a7
Compare
56quarters
force-pushed
the
56quarters/rm-tenant-state
branch
from
1ae07a7
to
8851d48
Compare
Remove the ability to set a global "default" parser for tenant IDs since this makes uses of this package fragile and bug prone. This also removes the `SingleResolver` struct which does not support multiple tenant IDs and was previously the default logic. All code for parsing tenant IDs is now aware of multiple tenant IDs separated by a `|` character. Consumers that don't want to support multiple tenant IDs at all should use the `TenantID()` method which returns an error if there are multiple tenant IDs. Consumers that wish to optionally support multiple tenant IDs should validate incoming input to ensure only a single ID is present. This changes the default behavior of `TenantID()` and `TenantIDs()` in two ways: * `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. Fixes #443 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters
force-pushed
the
56quarters/rm-tenant-state
branch
from
8851d48
to
3bad405
Compare
4 tasks
|
An example of how multi-tenant queries could be rejected when tenant-federation is not enabled in Mimir: https://github.com/grafana/mimir/pull/6959/files#diff-2a3eea939d04482548f6edf1e004ecc720e1bb218963865cf9e9ad706f5fd5b8 |
dimitarvdimitrov
approved these changes
Dec 19, 2023
tenant/resolver.go
Outdated
| defaultResolver = r | ||
| } | ||
| var ( | ||
| errInvalidTenantID = errors.New("invalid tenant ID") |
There was a problem hiding this comment.
can we add some details about what's invalid ("tenant ID is ., .. or contains / or \")
There was a problem hiding this comment.
I'm realizing now that it doesn't make sense to have containsUnsafePathSegments separate from the logic in ValidTenantID. WDYT about moving the logic from containsUnsafePathSegments to ValidTenantID?
There was a problem hiding this comment.
Fixed in cb00525 and moved that logic into ValidTenantID()
|
I was concerned about places that under the SingleResolver used to call But you said this would have only happened when multi-tenant is disabled and the tenant ID contains |
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters
commented
Dec 19, 2023
56quarters
added a commit
to grafana/mimir
that referenced
this pull request
Dec 20, 2023
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
added a commit
to grafana/mimir
that referenced
this pull request
Dec 21, 2023
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
added a commit
to grafana/mimir
that referenced
this pull request
Jan 2, 2024
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. See grafana/dskit#445
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
Remove the ability to set a global "default" parser for tenant IDs since this makes uses of this package fragile and bug prone. This also removes the
SingleResolverstruct which does not support multiple tenant IDs and was previously the default logic.All code for parsing tenant IDs is now aware of multiple tenant IDs separated by a
|character. Consumers that don't want to support multiple tenant IDs at all should use theTenantID()method which returns an error if there are multiple tenant IDs. Consumers that wish to optionally support multiple tenant IDs should validate incoming input to ensure only a single ID is present.This changes the default behavior of
TenantID()andTenantIDs()in twoways:
SingleResolverdid not previously enforce a limit on the length of a tenantID. 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.
SingleResolverpreviously 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:
Fixes #443
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]