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.

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

Helm: enable multi-tenancy in OSS by default #2117

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Jun 16, 2022

What this PR does

Disabled multi tenancy makes it look like OSS does not support it.
Also it makes it harder to migrate to multi tenancy and/or GEM.

Enable multi tenancy by default, but at the same time make sure that
Nginx sets the X-Scope-OrgID to anonymous if it's missing. This is done
for backwards compatibility and ease of use.

I've removed the setting, because enabled is the default, but maybe we'd want to be explicit?

Which issue(s) this PR fixes or relates to

Fixes grafana/helm-charts#1334

Checklist

  • Tests updated
  • [N/A] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@krajorama krajorama marked this pull request as draft June 16, 2022 08:20
@krajorama krajorama force-pushed the krajo/20220616-multitenancy-by-default branch 5 times, most recently from 810a502 to 710e96b Compare June 16, 2022 08:33
@krajorama krajorama marked this pull request as ready for review June 16, 2022 08:45
@krajorama krajorama added the helm label Jun 16, 2022
Disabled multi tenancy makes it look like OSS does not support it.
Also it makes it harder to migrate to multi tenancy and/or GEM.

Enable multi tenancy by default, but at the same time make sure that
Nginx sets the X-Scope-OrgID to anonymous if it's missing. This is done
for backwards compatibility and ease of use.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama force-pushed the krajo/20220616-multitenancy-by-default branch from 710e96b to 5baa9d8 Compare June 16, 2022 10:08
Copy link
Contributor

@Logiraptor Logiraptor left a comment

Choose a reason for hiding this comment

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

LGTM, I was able to send metrics both with and without the tenant header. Those without were sent to the anonymous tenant and were queryable with other metrics from before enabling multi-tenancy.

@krajorama krajorama merged commit 9616b2b into main Jun 17, 2022
@krajorama krajorama deleted the krajo/20220616-multitenancy-by-default branch June 17, 2022 05:22
@09jvilla
Copy link
Contributor

This is great @krajorama ! I think you made the right call to remove the setting. Less options makes things easier to maintain and is less to take in for the user.

@Logiraptor @krajorama -- did you validate that reading metrics back still works? I'm assuming nginx will also set the header on my read requests to anonymous if I send a read without a header?

@Logiraptor
Copy link
Contributor

@09jvilla Yes, in my tests the query results were identical whether setting the tenant to anonymous or leaving it out, and queries for other tenants correctly returned only data for those tenants.

masonmei pushed a commit to udmire/mimir that referenced this pull request Jul 11, 2022
Disabled multi tenancy makes it look like OSS does not support it.
Also it makes it harder to migrate to multi tenancy and/or GEM.

Enable multi tenancy by default, but at the same time make sure that
Nginx sets the X-Scope-OrgID to anonymous if it's missing. This is done
for backwards compatibility and ease of use.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mimir-distributed] multitenancy and enterprise relationship
3 participants