Add tenant resolver

[simonswine]

What this PR does:

Which issue(s) this PR fixes:

This implements the multi tenant resolver as described by the proposal for multi tenant query-federation.

By default it behaves like before, but it's implementation can be swapped out.

This replaces:

• user.ExtractOrgID by either TenantID or UserID depending on if in this context a multi tenant usage should be allowed
• user.ExtractOrgIDFromHTTPRequest with ExtractTenantIDFromHTTPRequest to gather exactly one tenant id.

I have also added lint test to check for usage of the upstream methods directly

Checklist

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

[simonswine]

Using a global variable here is not something I am particularly happy about.

I have tried to pass a tenantResolver reference around that would itself be instantiated in a Cortex module. But this approach has a massive changeset, as its touching so many areas of the codebase and I decided to go with this approach.

[jtlisi]

I think a global is definitely acceptable in this case. A package could always instantiate it's own resolver down the line if there was a reason to go down that path.

[jtlisi]

LGTM

[jtlisi]

I think a global is definitely acceptable in this case. A package could always instantiate it's own resolver down the line if there was a reason to go down that path.

[jtlisi]

Cool, so we are going to continue using the weaveworks/common library as is? It just won't be aware of how the header value will be resolved.

[simonswine]

Yes being able intercept headers would require us to do something that I have trailed #3416, which came with a massive change set across the project.

[jtlisi]

suggestion(if-minor): Instead of retrieving the default resolver and calling these functions, what if you expose the functions of the Resolver interface as top level functions in the tenant package and just assume they will utilize the default in the package. That could make the syntax a bit cleaner.

Suggested change

	userID, err := tenant.DefaultResolver().TenantID(r.Context())
	userID, err := tenant.TenantID(r.Context())

[simonswine]

I have done that, golint complained about the stuttering issue: tenant.TenantID[s], but I think it's important to have it there to make it clear UserID vs TenantID.

[csmarchbanks]

I would love some more detail around when UserID will be used vs TenantID/TenantIDs. I don't see any info in the proposal around that (or I am blind which is totally possible). Is this meant to be something like a username? Just an extra header that will be passed?

The comment "used to identify a user in log messages" scares me a bit as we could be getting into PII which I certainly do not want to encourage people to log.

[simonswine]

I would love some more detail around when UserID will be used vs TenantID/TenantIDs. I don't see any info in the proposal around that (or I am blind which is totally possible). Is this meant to be something like a username? Just an extra header that will be passed?

@csmarchbanks agreed. This is something where the current comments on the interface tenant.Resolver are not enough and we potentially need to update the proposal to define that properly.

What I think we should be trying to solve is how to move away from the overloaded meaning that is currently derived from user.ExtractOrgID. It is the tenant ID, but it's also used for identifying limits and the origin of certain requests in the logs and metrics. What I tried to do in the scope of this PR is to ensure we call the correct method depending on which of those things the intent is at the point where currently user.ExtractOrgID is called.

While I think using the default settings cortex will always behave TentantID() == TenantIDs[0] == UserID() (and everything else would break compatibility expectations), it would be a good opportunity to evolve the tenant.Resolver by the experiences we gain from adding more experimental features like this one.

As you are aware we are using Cortex as a basis of Grafana Metric Enterprise and we have some plans that involve distinguishing more details between different actors using the API within the same tenant. I see that as a first step in getting a good understanding what is involved there.

I hope that makes sense and wonder what if you think this could go forward, with some of that baked into the comments of the tenant.Resolver?

[pstibrany]

What I think we should be trying to solve is how to move away from the overloaded meaning that is currently derived from user.ExtractOrgID. It is the tenant ID, but it's also used for identifying limits and the origin of certain requests in the logs and metrics.

I don't see this as overloaded. Introducing new userID concept is confusing, since in Cortex we don't deal with individual users in any way.

While I think using the default settings cortex will always behave TentantID() == TenantIDs[0] == UserID()

In the proposal, multiple tenants were sorted. I am bit confused by this statement (esp. by "TenantIDs[0]" part, since that will always be tenant with lowest ID??)

[pstibrany]

Using userID without tenantID is misleading. Different tenants may have same set of users. For log messages, metrics or fairness, both need to be used.

For metrics especially, using user can lead to additional cardinality explosion, and is explicitly called out as bad practice.

From the code it seems like what you're calling "user ID" is basically a sorted set of tenant IDs. This mix of terminology is confusing.

This code replaces user.ExtractOrgID calls, but not user.InjectOrgID calls. Furthermore, there are components from weaveworks library, that we use, that still use original user.ExtractOrgID (instrumentation, tenant propagation for grpc calls). I think this needs to be fixed, preferably in the weaveworks common library. (Otherwise we need to reimplement these helper components on top of new "tenant" package).

[simonswine]

Using userID without tenantID is misleading. Different tenants may have same set of users. For log messages, metrics or fairness, both need to be used.

Totally agreed, yes eventually we will need information in the context.Context that will tell us both the user and the tenant, so we can consider both for those decisions.

For metrics especially, using user can lead to additional cardinality explosion, and is explicitly called out as bad practice.

From the code it seems like what you're calling "user ID" is basically a sorted set of tenant IDs. This mix of terminology is confusing.

Both of those are correct and I know this is far from ideal, but that was called out as part of the proposal to be a step-gap solution, that really only affects you if you enabled multi-tenant support and ideally log both and so on

This code replaces user.ExtractOrgID calls, but not user.InjectOrgID calls.

I don't think this is a problem as we ourselves in the tenant package call ExtractOrgID and derive our values from it.

While I think using the default settings cortex will always behave TentantID() == TenantIDs[0] == UserID()

In the proposal, multiple tenants were sorted. I am bit confused by this statement (esp. by "TenantIDs[0]" part, since that will always be tenant with lowest ID??)

Sorry I have really phrased that in aquite confusing way, what I meant to say. Using the default resolver, which is defaultResolver = NewSingleResolver(), all three methods which replace ExtractOrgID basically return the same. The string returned in UserID/TenantID is the same and it also matches with the first and only returned element in the []string in TenantIDs

Furthermore, there are components from weaveworks library, that we use, that still use original user.ExtractOrgID (instrumentation, tenant propagation for grpc calls). I think this needs to be fixed, preferably in the weaveworks common library. (Otherwise we need to reimplement these helper components on top of new "tenant" package).

I think that's how my vision for the future work would look like, either making weaveworks/common more adaptable so that we can reuse our own context extraction/injection from HTTP/GRPC or we have to move them into the project. Especially code which is quite specific to how Cortex works is probably better to live within here.

I am right now not yet in a position to predict how we would achieve that and how interfaces and more specific would look like, but by gradually carving parts out (like this PR) I think we can make this transition work.

[simonswine]

I am wondering if this PR would have it easier to get consensus if I would remove the UserID() string method from the Resolver interface:

• TenantID() (which will only ever return a single tenant), replaces all usages of ExtractOrgID.
• TenantIDs(), would then only be used on the read path where it's necessary for multi-tenant querying.

wdyt @pstibrany?

Withdrawing my request in case someone else wants to approve. (I plan to take another look but not sure when)

[pstibrany]

I am wondering if this PR would have it easier to get consensus if I would remove the UserID() string method from the Resolver interface:

• TenantID() (which will only ever return a single tenant), replaces all usages of ExtractOrgID.
• TenantIDs(), would then only be used on the read path where it's necessary for multi-tenant querying.

These suggestions would make it less controversial

[pracucci]

Very good job, LGTM!

[pracucci]

Performance wise a couple of things:

1. The input ids is just fine if there are no duplicates (which I would expect to be the common case)
2. result make be pre-allocated with the length of ids

[pstibrany]

Thank you for addressing the feedback, LGTM. Currently it's a noop in Cortex, and I'm looking forward to see more support for multitenant.