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

Tenant package relies on mutable global state #443

Closed
56quarters opened this issue Nov 27, 2023 · 1 comment · Fixed by #445
Closed

Tenant package relies on mutable global state #443

56quarters opened this issue Nov 27, 2023 · 1 comment · Fixed by #445
Assignees
@56quarters

Comments

@56quarters
Copy link
Contributor

56quarters commented Nov 27, 2023 •
edited

The tenant package is used for (among other things) extracting a tenant ID from a context.Context.

There are two separate implementations of this logic:

  • Logic that understands tenant IDs separated by a | and so can return multiple tenant IDs from a context.
  • Logic that does not understand tenant IDs separate by a | and so returns an error in this case.

There is a WithDefaultResolver that changes which logic is used, via mutating a global. Mimir and Loki do this in response to some form of multi-tenant queries being enabled or disabled by configuration.

This is a problem because the behavior of huge portions of each database can change by changes made far away from the code in question. It also makes it more difficult to unit test the code in question since adjusting the global state for a single test affects the behavior of every other test.

I propose:

  • Remove the WithDefaultResolver method to prevent mutation of global state
  • Remove all global state since it's unnecessary. The logic of MultiResolver would move to the global tenant.TenantID() and tenant.TenantIDs() methods.
  • This would also make SingleResolver, MultiResolver, and Resolver unnecessary

This would require changes in Mimir and Loki to reject multi-tenant queries when they aren't enabled. However, this would result in a better design anyway since the databases are checking for and enforcing exactly what they intend to enforce.
@56quarters
Copy link
Contributor Author

56quarters commented Nov 29, 2023 •
edited

A few things of note as I've been testing this locally with Mimir:

  • The SingleResolver doesn't enforce any limit on the length of tenant IDs while the multi-tenant logic (which will become the default) does (150 chars).
  • The ID team-a|team-b is a valid single tenant ID to read and write with tenant federation disabled. This will become invalid after this change and the corresponding upgrade in Mimir.

Per the Cortex documentation referenced in the code and the Mimir documentation, both of these case should have been disallowed to begin with: tenant IDs can't be longer than 150 characters, | isn't allowed in tenant IDs.

56quarters added a commit that referenced this issue Nov 29, 2023
@56quarters
Remove global state from tenant ID parsing package
12d64df 
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 opptionally support multiple tenant
IDs should validate incoming input to ensure only a single ID is present.

Fixes #443

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters mentioned this issue Nov 29, 2023
Merged
2 tasks
56quarters added a commit that referenced this issue Nov 29, 2023
@56quarters
Remove global state from tenant ID parsing package
98ae518 
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 divided 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 added a commit that referenced this issue Nov 29, 2023
@56quarters
Remove global state from tenant ID parsing package
1ae07a7 
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 added a commit that referenced this issue Dec 15, 2023
@56quarters
Remove global state from tenant ID parsing package
8851d48 
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 added a commit that referenced this issue Dec 18, 2023
@56quarters
Remove global state from tenant ID parsing package
3bad405 
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 56quarters self-assigned this Dec 18, 2023
56quarters added a commit that referenced this issue Dec 19, 2023
@56quarters
Remove global state from tenant ID parsing package (#445)
2bfd679 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@56quarters 56quarters

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove global state from tenant ID parsing package grafana/dskit
1 participant
@56quarters