Proposal: query path tenancy

[douglascamata]

Yet another proposal for including multi-tenancy support in the query path. In the same base as outlined previously by #4055 (with implementation available at #4141).

[saswatamcode]

Thanks for great work on this proposal! This is definitely something we want to enable, and would be a cool feature to have! 🙂

Some questions/suggestions!

[fpetkovski]

We can achieve these goals by having separate query instances for individual tenants. A valid goal would be simplifying management for multi-tenant setups and pooling resources to reduce waste.

[friedrichg]

Don't see any mention of reusing some cortex components that already implement multi-tenancy using an HTTP header. 😦

I wouldn't recommend putting cortex as a dependency. But why not just copy the bits you like here?

we would be happy be deleting code from cortex if it makes it in Thanos 😄

[douglascamata]

Hey @friedrichg, can you give us some good pointers for this implementation on the Cortex side? I'm not familiar with Cortex and its features, so I'm very unaware of how it implements things.

If it aligns well with how we want to move forward with this, it'll be great to reuse some code (be it importing or copying) so that both projects benefit from it. I know there are some design differences between the two projects, so let's see what can be done and reused.

[friedrichg]

Thanos and Cortex are very similar, both CNCF projects, sort of rivals in the old days. But nowadays Cortex just reuses a bunch of components from thanos. The compactor, the store-gateway, etc. There is too many things in common.

The thing cortex added from the beginning was the multi-tenancy, so we it adds header to all requests, so all components know what tenant is it about.

And like I said importing cortex into thanos is not a good idea, because it was a thing that happened in the past that created circular dependencies. I think is better you just copy what you need and we reuse it in Cortex (As we already do 😃 )

https://cortexmetrics.io/docs/architecture/

Find me in slack cncf #cortex if you want to chat more about it.

[douglascamata]

Awesome. I'll get back to this next week and reach out if I have questions or ideas. Thanks. 🙇

[bwplotka]

Yea, I think it's good point to mention Cortex way of dealing with this as inspiration, but there is nothing wrong in revisiting this feature and checking if there is anything we could improve before implementing.

Ideally, we could have the same solution, so the tools that works on top of Cortex will work on top of Thanos. That's fair advantage to adopting similar patterns. Another advantage is easier migrations between projects, helpful if we will try to unify projects at some point.

Can we mention this somewhere in the doc (advantage and Cortex pattern)? Thanks!

[fpetkovski]

Configuring the tenant in multiple places can be a big source of confusion for users. Can query frontend simply propagate all headers? I would prefer it we enabled tenancy on the query path using one flag in one component.

[douglascamata]

We need it to be configurable in multiple places because this header has to go all the way down to the Thanos Store, not only from Query Frontend to Query. It also has to be transmitted from Querier to Store Gateway in setups with, but also without, the Query Frontend and from Querier to Querier in setups with multiple Querier trees.

We need every component in the query path to be able to identify the header containing the tenant name, so there's no escape configuring this everywhere. If we configure it only in the Query Frontend, how does the Querier know which header includes the tenant's name?

I'm against doing a blanket forwarding of all headers. It seems like a potential source of security issues or unknown unintended consequences. WDYT?

[douglascamata]

Basically, every component has to agree on which header is being used to identify the tenant and we have nothing to facilitate centralized configuration at the moment.

Our only option is to hardcode the tenant header name to be always the same and impose it on all the users of Thanos that want to use this feature.

There would still be the need to enable a flag in the Query Frontend and Queriers.

[douglascamata]

If we hardcode the header name, we could even be compatible with Cortex and Mimir by only using X-Scope-OrgID.

[fpetkovski]

The way I see it, the querier is the only component that needs to know which header is used for tenancy. It can then take the value of the header and add it to a store api request.

[fpetkovski]

Actually now that I think about this, label injection would happen in the Querier, so only the Querier needs a flag to indicate the tenant label. If we go with @bwplotka's proposal to use pre-defined header names, Query Frontend just needs to forward the header to the Querier, so we won't need additional configuration options.

[douglascamata]

@fpetkovski as we spoke yesterday, I think we need customizable header name because Receive's already offering that (through --receive.tenant-header="THANOS-TENANT") and using two different header names depending on whether it's a query or a remote write creates confusion through inconsistency.

I think it's valuable to keep the consistency here so that people that already know how Receive works will easily understand how the tenancy enforcement on the query path works.

The default header name will also match Receive's. So we will end up with:

• --querier.tenant-header="THANOS-TENANT".
• --querier.default-tenant="default-tenant".
• --querier.tenancy="false".

When using the default header name, you will have to only add --querier.tenancy="true" to toggle everything on. The simplest case is gracefully simple to handle.

And if you are using a custom header name in Receive, you may (or may not) make the query path consistent with it by customizing --querier.tenant-header. The same applies to the default tenant setting.

cc @bwplotka

[douglascamata]

As also mentioned by @bwplotka, the possibility of customizing the header name allows us to be more compatible with other tools, like Cortex, which facilitates interoperation and migration between tools.

[fpetkovski]

That makes sense to me.

[douglascamata]

@fpetkovski actually, we need that header name flag in the Query Frontend too. It cannot simply forward all headers. It needs to know what is the tenant header to include the information in its own metrics and for the per-tenant settings, as previously mentioned in our conversations.

[fpetkovski]

Can't we use the tenant from request headers?

[douglascamata]

How do we know the header in the request that includes the tenant's name if the header used to transmit this information can have its name changed?

i.e. Query Frontend configured to use X-User as tenant header. How will Store Gateway know that X-User is the header being used for tenant identification and not anything else?

[fpetkovski]

The querier knows which header is used for tenancy, and it can propagate this header in either the request or in a special gRPC/http header.

[douglascamata]

This overlaps with our conversation in the other comment, let's solve that one first and then we come back to this one if there will be anything left to discuss. 👍

[bwplotka]

Also be careful as gRPC does not expose HTTP2 headers - metadata and trailers can be used instead. So flag name for gRPC would need to be different.

[douglascamata]

@bwplotka aahhh, that's good to know. I'll learn more about it and update the proposal accordingly. Thanks for the tip.

[bwplotka]

Amazing! Good direction, and some good discussion. I would be supportive of consistent header/metadata, but I would start with hardcoded, opinionated header name, without flags.

I feel for this discussion it would be healthy to explain tenancy model we expect. For example I think read and write tenancy have a little bit different semantics here which are worth to mention.

[bwplotka]

Interesting. I would argue we might need to be careful with adding feature that will block important features like cross tenancy in future. However I feel we get into the point where we talk about different tenancy aspects:

One is tenancy of data, so who owns what. Second is tenancy of query, who should be billed for query. Those write/read tenancy does not need to be 1:1. For example cross tenancy to me means that tenant A can read data of tenant B, but the quotas, settings and billing should be on behalf of one tenant.

All of this to me means we could refine this cross tenancy point. I think cross-tenancy is a goal, just we don't want shared tenancy for reads (?)

[douglascamata]

Cross-tenancy is definitely a future goal of the project and a good "follow up", but I didn't plan this proposal to go deep into its details. I don't think it'll block in any way a cross-tenant query implementation. Cross-tenant queries will require changes to the logic behind the label enforcement part, but there's no architectural decision that will need to be undone when the time comes.

Totally we will need to develop more on these two aspects of tenancy you mention, @bwplotka. Plus we will need some configuration that will work kinda like an authorization for cross-tenant query (i.e. tenant A can query B and C; tenant B can only query C, etc).

[bwplotka]

Are we sure the tenant will be always represented by a single label not multiple labels?

[douglascamata]

Yup, our behavior should match the Receive's, which is using a single and configurable label, named tenant_id by default.

[bwplotka]

To be strict for gRPC - will we pass that tenant as metadata or HTTP2 header? I think the metadata is the answer.

[douglascamata]

Agreed, @bwplotka. GRPC metadata seems to be the answer for it. Will batch this into updates to the proposal for the next push.

[bwplotka]

Also be careful as gRPC does not expose HTTP2 headers - metadata and trailers can be used instead. So flag name for gRPC would need to be different.

[bwplotka]

Cool discussion, but I feel we all miscommunicated here a bit - some answers were a bit too repetitive and signalling some frustration. Let's calm down a bit, we all want to have the best solution here 🙈

My suggestions about the essence (let me know if I understood all right):

1. I think we tend to agree on the top most component (Querier, frontend or both) using header not request field. I would vote however to NOT add any flag to configure this unless community will need that to adopt it. Let's just agree on some hardcoded, opinionated header name for now and go from there.

2. Header vs Fields on downstream components and APIs. I think @fpetkovski raised a valid concern about too many flags. One solution is to move information to request field e.g StoreAPI. I would argue the better solution is to simply stick to one opinionated header name (hardcoded) for all components. If in unlikely case somebody will need a flag for it, we can discuss later (we would rather not).

To truly uncover what's best for no 2, I would unpack this item in Alternatives section, which actually @douglascamata wrote down (request vs header). I agree with @douglascamata to stay with headers, but I don't feel the arguments you used @douglascamata are convincing (: Maybe this is why we have some communication issues here. Let's unpack your points in the proposal:

This isn't the preferred solution because the Store API isn't used all the way from the topmost components (e.g. Querier and Query Frontend) to the bottommost one (e.g. Store Gateway). The Query Frontend talks to the Querier via the Query API. A Querier can talk to other Queriers using the Query API via gRPC. Finally, a Querier talks to Store Gateways using the Store API. All these APIs would have to be modified and the transmission of this information would differ depending on the component and API being used.

Fair, but also all components would need to agree on the header name that is very loosely defined and implemented so I feel this is neither improvement or disadvantage of a header (a tie )

In addition to this complication, this implementation does not enable any capability that can't be achieved with HTTP header approach.

Is it really a good argument? The opposite is also true - you can add field to all requests.

Essentially I feel we need different argumentation here:

• Reference to Cortex (Pro header)
• What semantically it makes sense more? (Pro header IMO as tenant belongs more to auth domain really)
• Header consistency across components: Better code reusability and user understanding (Pro header)
• Header consistency across HTTP APIs and gRPC Services. We don't have just StoreAPIs, we have other APIs and calls. Instead of extending all request messages we can just use single, consistent header (HTTP) and metadata item (gRPC).
• Typed (Pro request)

[GiedriusS]

So, it won't be possible to query data from multiple tenants anymore? 😢

[douglascamata]

@GiedriusS: it will still be possible, by default, to keep backwards compatibility. When the tenancy-enforcement feature is turned on on a Querier, then it won't be possible for requests arriving there. But you can have another set of Queriers with the feature disabled and multi-tenant queries will work, just like today. o/

[douglascamata]

@GiedriusS: I am trying to ensure we keep backwards compatibility and the possibility of running queries with and without tenant-enforcement in a cluster. Maybe we can collaborate later on a proposal for multi-tenant query with the tenancy enforcement on? It's not a goal of this proposal to outline how this would work. What do you think? :D

[douglascamata]

@bwplotka @fpetkovski @saswatamcode: I believe I have handled and/or replied to all of your comments. Please do check the replies and the proposal in its current state when you have some time, we (me and the proposal) ready for a next round of feedback. :)

Thanks for your reviews 🙇

[douglascamata]

Could reviewers please give this a second look? Thank you for your time.

[saswatamcode]

Sorry for the delay on this! 🙏🏻

I think this proposal is in a good state, and this is a feature that we definitely want & need!

The proposal already covers a wide range of features across query path components, both for admin and end-user personas,

• Per tenant o11y & billing (admin)
• PromQL query tenancy (end-user)
• Query limits (end-user & admin)

As the devil is in the details, it'd be good to keep in mind that while implementing, there might be certain limitations that haven't been considered, due to the breadth of this proposal. So being flexible, and updating this proposal at that point would be a great way to move forward! 🙂

Approving as I'd like to unblock and move this stream of work forward!

[fpetkovski]

Looks good to me as well, we can polish out the details during the implementation phase.