Querier: Added native multi-tenancy support#4141
Querier: Added native multi-tenancy support#4141Abhishek357 wants to merge 11 commits intothanos-io:mainfrom
Conversation
kakkoyun
left a comment
There was a problem hiding this comment.
Looking good so far.
It'd be amazing to create a wrapper around existing querier, such as QuerierWithTenant. It'd make the code cleaner and more maintainable.
bwplotka
left a comment
There was a problem hiding this comment.
High-level questions (maybe even proposal)
-
Injecting identifier what tenant is making the call (to any API)
-
Injecting identifier of data that we have to scope to (single-tenant, multiple in future) - I guess this is the purpose of this PR?
-
Headers have to be configured, the same tenant label (avoid hardcoded strings)
-
Can we reuse promlabel. It's used mainly for PromQL mangling, however, we could decouple injection and have the same injection code for PromQL and raw matches, so the injection and multi-tenancy / cross tenancy are coming from the single, well-tested library. That we can use in Thanos, Observatorium, PromLabel proxy sidecar it. One source of truth.
|
Blocked by #66 |
yashrsharma44
left a comment
There was a problem hiding this comment.
Looks good, I need to test it out to cross-check the findings :)
We can not test it until we merge PR in prom-label-proxy because only then we will be able to use those enforcing helper functions. |
9fa939c to
4ef4624
Compare
| defaultRangeQueryStep time.Duration, | ||
| defaultInstantQueryMaxSourceResolution time.Duration, | ||
| defaultMetadataTimeRange time.Duration, | ||
| tenantHeader string, |
There was a problem hiding this comment.
Why don't we initiate TenantInfo where we call NewQueryAPI?
There was a problem hiding this comment.
Because we want to propagate only valuable TenantInfo, we get this when an HTTP request comes and we extract the tenant access according to the configurable tenant Header name (it's not necessary to propagate this label name).
|
Hey @Abhishek357 could you please address the review comments and resolve them afterwards? Also it'd be nice to rebase this PR. |
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
…dcoded strings Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
Signed-off-by: Abhishek357 <abhisheksinghchauhan357@gmail.com>
|
This sonar type lift thingy is misleading. Those transient versions are not used in most cases, so there is no action required here. |
|
Can you look how we can configure this to ignore those kind of false alarms? @Abhishek357 ? |
@Abhishek357 and @bwplotka you can add a Which should screen out these results. You can also look at the full configuration reference for more options. |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
@bwplotka @Abhishek357 Looks like this should be merged in? Definitely a useful feature and the conversation above seems to indicate no problems/blockers. |
|
I think this is still relevant. I am also a bit confused this is closed and not merged?! |
Signed-off-by: Abhishek357 abhisheksinghchauhan357@gmail.com
Changes
Fixes #3822 Related to Proposal
We take the tenant header and replace the tenant label if specified in the query, if not just append it in prometheus label matches using enforceMatcher(helper function defined in prom-label proxy). Then these labels are further used to fan out a query to store API which advertises these external tenant label. Therefore achieving tenant isolation.
We use the same injection code for raw matches from the prom-label proxy, so the injection and multi-tenancy / cross tenancy are coming from the single, well-tested library.
Alternative considered
Using complete prom-label proxy as a library, importing it, and using it to modify the query itself. In Prom-label proxy we extract expression using parser from the query and then convert to matchers inside EnforceNode, then enforces label in enforceMatcher, then convert again to query. and these queries are again converted to matchers in Thanos and then further processed. Why such overhead if we are finally going to convert queries to the matcher inside Thanos and let's enforce the labels there itself using enforceMatcher.
Having a dedicated thanos field in Info method in Store API.
Verification
Wrote a e2e test.