Loki: Add multi-tenancy support based off labels in stream #2587

Champ-Goblem · 2020-09-01T13:52:54Z

What this PR does / why we need it:
Allows a user to specify a label in order to base org ID off its value, continues to support the two original methods (auth over http header and fake-auth).

I propose this design change so that a cluster running multiple user workloads for different tenants with a single set of collectors can classify the logs better than current implementation where the tenant ID (org ID) can only be statically set in the config for promtail (similar to a suggestion from a previous issue).

I'm happy to make adjustments based on your verdicts.

Which issue(s) this PR fixes:
None

Special notes for your reviewer:
I require some advice on altering the documentation in order to add the new schema for multi-tenancy:

multi_tenancy:
  enabled: <bool>
  type: <auth|label>
  label: <label name> (can be nil for type auth)
  undefined: <fallback org ID for streams without label> (can be nil for type auth)

This schema is designed to replace the original auth_enabled: <bool>
Checklist

Tests updated - New tests added for multi-tenancy package as well
Documentation added

- Added new logic to get org/user ID from stream labels

- Add tests

CLAassistant · 2020-09-01T13:52:58Z

All committers have signed the CLA.

slim-bean · 2020-09-01T14:49:28Z

Thanks for the PR @Champ-Goblem! we are a bit backed up at the moment and there is quite a bit to unpack here so it may take us a little bit to get a good response to you on this!

codecov-commenter · 2020-09-14T09:51:59Z

Codecov Report

Merging #2587 into master will decrease coverage by 1.50%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##           master    #2587      +/-   ##
==========================================
- Coverage   62.87%   61.37%   -1.51%     
==========================================
  Files         170      172       +2     
  Lines       15051    13239    -1812     
==========================================
- Hits         9464     8126    -1338     
+ Misses       4827     4369     -458     
+ Partials      760      744      -16

Impacted Files	Coverage Δ
pkg/loki/loki.go	`0.00% <0.00%> (ø)`
pkg/loki/modules.go	`3.81% <0.00%> (-0.34%)`	⬇️
pkg/multitenancy/multitenancy_config.go	`53.84% <53.84%> (ø)`
pkg/multitenancy/multitenancy_middleware.go	`73.33% <73.33%> (ø)`
pkg/distributor/distributor.go	`77.94% <86.04%> (-0.87%)`	⬇️
pkg/logql/step_evaluator.go	`57.14% <0.00%> (-9.53%)`	⬇️
pkg/logql/marshal/labels.go	`66.66% <0.00%> (-8.34%)`	⬇️
pkg/logproto/timestamp.go	`40.00% <0.00%> (-6.81%)`	⬇️
pkg/logentry/stages/labeldrop.go	`53.33% <0.00%> (-6.67%)`	⬇️
pkg/chunkenc/encoding_helpers.go	`59.25% <0.00%> (-6.37%)`	⬇️
... and 167 more

midnightconman · 2020-09-26T01:19:31Z

I have a question @Champ-Goblem ...

My use case is mostly around limiting ingestion (having the capability to throttle customers at the distributor layer), while allowing for a pretty open query (all tenants can query all tenants logs). This is not possible today, as the auth header can only include one tenant per request.

Would these changes enable something like that?

owen-d

Hey, I was finally able to take a look at this in depth -- thanks for the PR! I think what you're trying to do seems reasonable (collect logs for multiple tenants from a single collector).

After I thought about it for a bit, I'm not sure this is the way that we should do this. The agents will already need to be tenant-aware in order to assign some tenant label. Wouldn't it make more sense to reduce the coordination burdens between agent<->loki by adding a promtail stage to choose which tenant a log line is assigned to? That would allow the same functionality without the need to complicate the distributor code or expose these extra configs server side (which would require coordination with the agent anyway).

What do you think?

owen-d · 2020-09-29T19:35:27Z

pkg/multitenancy/multitenancy_middleware.go

+// InjectLabelForID injects a field into the context that specifies using a label rather than orgID from header
+func InjectLabelForID(ctx context.Context, label string, undefined string) context.Context {
+	if label == "" || undefined == "" {
+		return nil


This can't return nil or the subsequent WithContext will panic.

owen-d · 2020-09-29T19:36:03Z

pkg/multitenancy/multitenancy_middleware.go

+	if label == "" || undefined == "" {
+		return nil
+	}
+	newCtx := context.WithValue(ctx, interface{}("useLabelAsOrgID"), label)


The WithValue calls should use an unexported key type (see https://golang.org/pkg/context/#WithValue) as a precaution.

owen-d · 2020-09-29T19:40:05Z

pkg/multitenancy/multitenancy_middleware.go

+	}
+	// Try get the associated label from the stream, otherwise we can use undefined
+	if label != "" {
+		re := regexp.MustCompile(label + "=\"([^ ]+)\"")


~~This should probably be stored so that it doesn't recompile on every request.~~

This should use the existing label parsing functions instead (https://github.com/grafana/loki/blob/master/pkg/logql/parser.go#L50).

owen-d · 2020-09-29T19:45:40Z

pkg/multitenancy/multitenancy_config.go

+	f.BoolVar(&c.Enabled, "multitenancy.enabled", false, "Enable multi-tenancy mode")
+	f.StringVar(&c.Type, "multitenancy.type", "auth", "Where to get the Org ID for multi-tenancy ")
+	f.StringVar(&c.Label, "multitenancy.label", "", "Specifies the label to use for Org ID (in label mode)")
+	f.StringVar(&c.Undefined, "multitenancy.undefined", "unlabeled", "Sepcifies the name to use when log doesnt contain the label (in label mode)")


I'd rather default this to fake for consistency with our defaults when auth is not enabled.

owen-d · 2020-09-29T19:55:34Z

pkg/multitenancy/multitenancy_middleware.go

+// designed as a drop in upgrade to the original user.ExtractOrgID
+func GetUserIDFromContextAndStringLabels(ctx context.Context, labels string) (string, error) {
+	userID, err := user.ExtractOrgID(ctx)
+	label, undefined := GetLabelFromContext(ctx)


It might be more readable to encode the validation into the GetLabelFromContext code such that it returns label, undefined, ok where ok is true when both label and undefined are non-empty.

Champ-Goblem · 2020-09-30T10:29:26Z

Hi @midnightconman, unfortunately this change currently doesn't support something like that, although it was another idea that I would like to see implemented as well if this PR or a similar PR gets accepted. Currently the label processing is only done on the api/push endpoint and defaults to the original implementation of setting the org ID via a header for the query endpoints.

Champ-Goblem · 2020-09-30T22:56:34Z

@owen-d Thanks for the review, I see where you are coming from and it was also an alternative idea when I thought about viable solutions. I ended up choosing to implement it server side in order to cut down on the number of network requests that would potentially be made. Splitting the logs by tenant on the collector side we could only group logs of same tenant together each time we pushed, that means at each push interval there could be at most a request per tenant and if we had a large multi-tenant system, this could start to amount to a lot of requests. Wheres by implementing the logic server side we can bundle all of the log lines together and then separate them out once they reach loki, theoretically only needing one network request to send logs for a set of tenants at each push interval.
My other reason for implementing it server side is that the functionality then isnt collector dependent, if someone wanted to use a different collector to promtail, but still wanted the multi-tenancy feature, all they would need to do is enable it in the loki config.

Other than that, I am happy to make those suggest code changes once we deside on the best path moving forward.

stale · 2020-10-31T04:02:28Z

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

travis-sobeck · 2020-12-08T19:55:38Z

whatever came of this? The need still exists for multi-tenancy within a single cluster

kavin-kr · 2023-05-23T08:19:46Z

Is there any plan to reopen this PR?

Champ-Goblem added 9 commits August 29, 2020 15:08

Add multitennancy package

b47f11d

- Added new middleware for multi-tennancy

f243fdf

- Added new logic to get org/user ID from stream labels

Add GRPC Middleware

5866136

Update distributor to record userID for streams

4236f86

Separate middleware for multi-tenancy in label mode

047e07d

Fix nil map error

034319d

Revert to using origional auth middleware for grpc

fe69ade

- Remove unused functions

a422853

- Add tests

Merge branch 'master' of github.com:grafana/loki into master

1341fa1

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

Champ-Goblem force-pushed the master branch from 746b875 to 1341fa1 Compare September 14, 2020 09:49

Champ-Goblem added 3 commits September 14, 2020 13:03

Added ci config

79b5eda

Update query frontend to use query middleware

d899084

Merge branch 'master' of github.com:grafana/loki into master

54f0962

pull-request-size bot added size/XL and removed size/L labels Sep 21, 2020

Remove ci

93e3550

pull-request-size bot added size/L and removed size/XL labels Sep 21, 2020

owen-d reviewed Sep 29, 2020

View reviewed changes

stale bot added the stale A stale issue or PR that will automatically be closed. label Oct 31, 2020

stale bot closed this Nov 7, 2020

kavin-kr mentioned this pull request May 23, 2023

Support multi-tenancy using labels #9499

Open

ankeshh pushed a commit to ankeshh/loki that referenced this pull request May 25, 2023

PR(grafana#2587) changes of champ-goblem

8be378f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Add multi-tenancy support based off labels in stream #2587

Loki: Add multi-tenancy support based off labels in stream #2587

Champ-Goblem commented Sep 1, 2020 •

edited

Loading

CLAassistant commented Sep 1, 2020 •

edited

Loading

slim-bean commented Sep 1, 2020

codecov-commenter commented Sep 14, 2020 •

edited

Loading

midnightconman commented Sep 26, 2020

owen-d left a comment

owen-d Sep 29, 2020

owen-d Sep 29, 2020

owen-d Sep 29, 2020 •

edited

Loading

owen-d Sep 29, 2020

owen-d Sep 29, 2020

Champ-Goblem commented Sep 30, 2020

Champ-Goblem commented Sep 30, 2020

stale bot commented Oct 31, 2020

travis-sobeck commented Dec 8, 2020

kavin-kr commented May 23, 2023

Loki: Add multi-tenancy support based off labels in stream #2587

Loki: Add multi-tenancy support based off labels in stream #2587

Conversation

Champ-Goblem commented Sep 1, 2020 • edited Loading

CLAassistant commented Sep 1, 2020 • edited Loading

slim-bean commented Sep 1, 2020

codecov-commenter commented Sep 14, 2020 • edited Loading

Codecov Report

midnightconman commented Sep 26, 2020

owen-d left a comment

Choose a reason for hiding this comment

owen-d Sep 29, 2020

Choose a reason for hiding this comment

owen-d Sep 29, 2020

Choose a reason for hiding this comment

owen-d Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

owen-d Sep 29, 2020

Choose a reason for hiding this comment

owen-d Sep 29, 2020

Choose a reason for hiding this comment

Champ-Goblem commented Sep 30, 2020

Champ-Goblem commented Sep 30, 2020

stale bot commented Oct 31, 2020

travis-sobeck commented Dec 8, 2020

kavin-kr commented May 23, 2023

Champ-Goblem commented Sep 1, 2020 •

edited

Loading

CLAassistant commented Sep 1, 2020 •

edited

Loading

codecov-commenter commented Sep 14, 2020 •

edited

Loading

owen-d Sep 29, 2020 •

edited

Loading