Query: add optional tenancy enforcement #6756
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -220,6 +220,8 @@ func registerQuery(app *extkingpin.App) { | |||||
| tenantHeader := cmd.Flag("query.tenant-header", "HTTP header to determine tenant.").Default(tenancy.DefaultTenantHeader).String() | ||||||
| defaultTenant := cmd.Flag("query.default-tenant-id", "Default tenant ID to use if tenant header is not present").Default(tenancy.DefaultTenant).String() | ||||||
| tenantCertField := cmd.Flag("query.tenant-certificate-field", "Use TLS client's certificate field to determine tenant for write requests. Must be one of "+tenancy.CertificateFieldOrganization+", "+tenancy.CertificateFieldOrganizationalUnit+" or "+tenancy.CertificateFieldCommonName+". This setting will cause the query.tenant-header flag value to be ignored.").Default("").Enum("", tenancy.CertificateFieldOrganization, tenancy.CertificateFieldOrganizationalUnit, tenancy.CertificateFieldCommonName) | ||||||
| enforceTenancy := cmd.Flag("query.enforce-tenancy", "Enforce tenancy on Query APIs. Only responses where the value of the configured tenant-label-name and value of the tenant header matches are returned.").Default("false").Bool() | ||||||
|
There was a problem hiding this comment. Maybe we can clarify a bit here?
Suggested change
|
||||||
| tenantLabel := cmd.Flag("query.tenant-label-name", "Label name to use when enforce tenancy when -querier.tenancy is enabled").Default(tenancy.DefaultTenantLabel).String() | ||||||
|
There was a problem hiding this comment. Do you mean There was a problem hiding this comment.
I don't follow. Isn't that exactly what is there (except the dashes
Perhaps sometime later if we implement cross-tenancy there would need to be multiple options (disabled/single-tenant/cross-tenant), but not sure exactly how such an implementation would play out. It would be nice to have one flag less, but I also think it's a little unclear from the flag name if All minor details though, and I don't have a strong opinion, pros/cons with both. Mostly tried to make the implementation follow the approved proposal (as far as I can see there wasn't any specific discussion on these flags at the proposal stage). Any thoughts on this @douglascamata / @saswatamcode ? There was a problem hiding this comment. I think the argumentation regarding lack of clarity that a feature flag named They are separate features:
I must confess that I also like the insight that in the future the
+1 to this. There was a problem hiding this comment.
We don't currently use this information in any other place than for tenancy enforcement purposes. With #6794 we gather metrics about which tenant requests data, but we don't gather metrics about which tenants data is being accessed (i.e if enforcement is off tenant a could query tenant b's data). Perhaps that could be useful to do in the future though. There was a problem hiding this comment. A bit off-topic: kind of wishing all the config flags in any Thanos component could be loaded from a properly structured file (yaml, toml, json, whatever). 😅 There was a problem hiding this comment. Probably flag name needs to be enforce-tenancy?
Suggested change
|
||||||
|
|
||||||
| var storeRateLimits store.SeriesSelectLimits | ||||||
| storeRateLimits.RegisterFlags(cmd) | ||||||
|
|
@@ -343,6 +345,8 @@ func registerQuery(app *extkingpin.App) { | |||||
| *tenantHeader, | ||||||
| *defaultTenant, | ||||||
| *tenantCertField, | ||||||
| *enforceTenancy, | ||||||
| *tenantLabel, | ||||||
| ) | ||||||
| }) | ||||||
| } | ||||||
|
|
@@ -422,6 +426,8 @@ func runQuery( | |||||
| tenantHeader string, | ||||||
| defaultTenant string, | ||||||
| tenantCertField string, | ||||||
| enforceTenancy bool, | ||||||
| tenantLabel string, | ||||||
| ) error { | ||||||
| if alertQueryURL == "" { | ||||||
| lastColon := strings.LastIndex(httpBindAddr, ":") | ||||||
|
|
@@ -759,6 +765,8 @@ func runQuery( | |||||
| tenantHeader, | ||||||
| defaultTenant, | ||||||
| tenantCertField, | ||||||
| enforceTenancy, | ||||||
| tenantLabel, | ||||||
| ) | ||||||
|
|
||||||
| api.Register(router.WithPrefix("/api/v1"), tracer, logger, ins, logMiddleware) | ||||||
|
|
||||||
jacobbaungard marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. This feature is a bit tricky to setup correctly, so I'd really appreciate if you could add a section in query docs on how to enable tenancy enforcement, with this PR! :) |
|
There was a problem hiding this comment. We probably also want to enforce other endpoints like /api/v1/rules at some point, can be done as a follow-up. There was a problem hiding this comment. Also, I noticed the There was a problem hiding this comment. Sorry for the delay, but yes I think we need it there too. But can be done in follow-ups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's mention ecosystem integration as well. :)