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

Ability to require tenant in queries #3822

Closed
brancz opened this issue Feb 22, 2021 · 24 comments
Closed

Ability to require tenant in queries #3822

brancz opened this issue Feb 22, 2021 · 24 comments

Comments

@brancz
Copy link
Member

brancz commented Feb 22, 2021

Is your proposal related to a problem?

The most questions we get about Thanos are around multi tenancy, and I think it's worth considering that we could have more first class multi tenancy features. This is about the query path.

Describe the solution you'd like

Allow (optionally via a flag) requiring the THANOS_TENANT header on queries against the querier, that enforces the query towards that tenant, rejecting queries that do not specify it.

Describe alternatives you've considered

Just do it entirely outside of Thanos, but as I said in the first sentence, that does not seem like a real possibility to people outside of the maintainer group.

@thanos-io/thanos-maintainers

@kakkoyun
Copy link
Member

One other thing to consider is to have a special configurable tenant label. So we can support multi-tenancy more "natively". Store APIs can advertise these. And depending on the level of isolation you require, you can use querier for multiple tenants at the same time or you can create dedicated queries. This special label needs to be considered by any component that exposes StoreAPI.

@brancz
Copy link
Member Author

brancz commented Feb 22, 2021

That's actually really interesting, I had not thought about that! That way we wouldn't even have to parse the PromQL query but just do it at selection time. That indeed makes it much more efficient. I love it! More secure and more efficient! :D

@roidelapluie
Copy link

I think it is much easier to use a proxy for authentication that adds a http header than that alters a promql query to add a label.

@brancz
Copy link
Member Author

brancz commented Feb 22, 2021

Yep that's the idea of the first class feature! :)

@bwplotka
Copy link
Member

So essentially this proposes embedding prom-label-proxy. 🤔

So I don't think it's needed technically, it works just fine outside. However, agree that there is this perception and configuration complexity that makes people think Thanos is not multi-tenant. I think I would be fine to consider this.

I like the HTTP parameter idea more than fuzzy header. Parameter feels more "typed". I would only suggest looking on other aspects too: Injecting tenant isolation is one thing, but we also need other things like instrumentation (metrics, logs, traces) and limits per tenant, which might be orthogonal to what data tenant is asking for and have access to 🤔 I would vote on defining some strategy for those things.

cc @prmsrswt who is working on cross tenant views for prom-label proxy nowadays.

@roidelapluie
Copy link

roidelapluie commented Feb 24, 2021

I think that it is the opposite of the original request, which is about requiring a http header. It would then work with any proxy, and not prom-label-proxy.

@brancz
Copy link
Member Author

brancz commented Feb 24, 2021

As @kakkoyun mentioned, with a tighter integration we could actually push it further down into the store API request and ensure it there, without even having to parse the query.

@bwplotka
Copy link
Member

Thoughts on contributor hours from @brancz We can make it more efficient with selectors on already parsed queries etc, no need to work around. More efficient, simpler and more secure.

Other potentially orthogonal discussion:

  • Separating data from the environment. metadata, tenancy.
    • Potentially leaking abstractions: It hasn't bitten us much, but it might in the future!
    • List pros and cons
    • What about tenant? Is is infra or data? Kind of both! Meta one!
  • Zone aware strict discovery, maybe just file discovery

@bwplotka
Copy link
Member

Open question: Header vs Param?

@bwplotka
Copy link
Member

As agreed offline, looks like header is what people consistently use for this kind of metadata - we can make sure it's versioned and somehow typed (has proto for it?) as well.

@tomasfreund
Copy link

tomasfreund commented Mar 8, 2021

Would it also be possible to expose the tenant/labelset information in the query UI somehow? A drop-down or multi-select perhaps?

One problem I have with Thanos is that while I sometimes want to query data across multiple clusters, I'd prefer if that was not the default and you had to select one/multiple/all tenants/labelsets explicitly to avoid accidentally evaluating the queries in all clusters.

It would also be great if the query UI supported a url like https://my-thanos-instance/{some-tenant-or-labelset-segment}/query as this would allow me to set prometheus --web.external-url to this value and get a useful source url in alertmanager alerts. When I set the parameter to the url of my Thanos instance, I get a working url but it is not filtered by the cluster external_labels and I can't add those to the query because external_labels are applied only when the data is uploaded to object storage and the query is evaluated before that happens.

Anyway, thanks everyone for all the work on this project, aside from these small issues, it's been quite easy to set up and pleasant to work with.

@brancz
Copy link
Member Author

brancz commented Apr 7, 2021

I think exposing it in the UI would be reasonable.

@stale
Copy link

stale bot commented Jun 6, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 6, 2021
@kakkoyun kakkoyun removed the stale label Jun 11, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Aug 13, 2021
@stale
Copy link

stale bot commented Aug 28, 2021

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Aug 28, 2021
@onprem onprem removed the stale label Sep 11, 2021
@onprem onprem reopened this Sep 11, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jan 9, 2022
@GiedriusS GiedriusS removed the stale label Jan 10, 2022
@stale
Copy link

stale bot commented Apr 17, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@shaardie
Copy link

This would be a great feature. I am a bit confused that the PR tackling this is closed #4141

@jasonhao518
Copy link

How is going here? looking forward for the same feature also. As grafana stack support X-Scope-OrgID header, I think it's good to use header for Thanos to make it compatible to other tools.

@stale
Copy link

stale bot commented Oct 1, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 1, 2022
@carlosrmendes
Copy link

carlosrmendes commented Mar 16, 2023

Any news regarding this feature? This is still relevant for me.

Open question: Header vs Param?

I would prefer query parameter

@stale stale bot removed the stale label Mar 16, 2023
@GiedriusS
Copy link
Member

Any news regarding this feature? This is still relevant for me.

Open question: Header vs Param?

I would prefer query parameter

Help wanted 🤗

@NotAFile
Copy link
Contributor

NotAFile commented Apr 2, 2024

It looks like this has been completed in #6756, or is there anything else missing?

@GiedriusS
Copy link
Member

GOod point @NotAFile, let's close this. I think all requests have been implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.