-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Query Frontend: forward tenant information downstream #6595
Changes from all commits
f4506f5
4b2ca14
aa33d04
6e8ed12
ffcbed6
92e3d71
e4ea657
85506ab
399a365
6e9ffeb
aa9f3d3
f955f71
500d29b
08dd0c4
dd619e3
531ded7
9d62032
01f8071
60e093e
4e8ef60
8cf2ba0
f5c5cba
82a7e3a
f1667e2
1033baf
41c3f6e
fec5df9
d6ec416
6daddf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,10 @@ func GetTenantFromHTTP(r *http.Request, tenantHeader string, defaultTenantID str | |
var err error | ||
tenant := r.Header.Get(tenantHeader) | ||
if tenant == "" { | ||
tenant = defaultTenantID | ||
tenant = r.Header.Get(DefaultTenantHeader) | ||
if tenant == "" { | ||
tenant = defaultTenantID | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think its worth logging down below if we end up overwriting based on the cert? or maybe we could just handle this on startup with a log or an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be too spammy. We had similar logs when parsing the tenant information from the request in the Store GW and they were removed: 510c054 |
||
} | ||
|
||
if certTenantField != "" { | ||
|
@@ -65,6 +68,28 @@ func GetTenantFromHTTP(r *http.Request, tenantHeader string, defaultTenantID str | |
return tenant, nil | ||
} | ||
|
||
type roundTripperFunc func(*http.Request) (*http.Response, error) | ||
|
||
func (r roundTripperFunc) RoundTrip(request *http.Request) (*http.Response, error) { | ||
return r(request) | ||
} | ||
|
||
// InternalTenancyConversionTripper is a tripperware that rewrites the configurable tenancy header in the request into | ||
// the hardcoded tenancy header that is used for internal communication in Thanos components. If any custom tenant | ||
// header is configured and present in the request, it will be stripped out. | ||
func InternalTenancyConversionTripper(customTenantHeader, certTenantField string, next http.RoundTripper) http.RoundTripper { | ||
return roundTripperFunc(func(r *http.Request) (*http.Response, error) { | ||
tenant, _ := GetTenantFromHTTP(r, customTenantHeader, DefaultTenant, certTenantField) | ||
r.Header.Set(DefaultTenantHeader, tenant) | ||
// If the custom tenant header is not the same as the default internal header, we want to exclude the custom | ||
// one from the request to keep things simple. | ||
if customTenantHeader != DefaultTenantHeader { | ||
r.Header.Del(customTenantHeader) | ||
} | ||
return next.RoundTrip(r) | ||
}) | ||
} | ||
|
||
// getTenantFromCertificate extracts the tenant value from a client's presented certificate. The x509 field to use as | ||
// value can be configured with Options.TenantField. An error is returned when the extraction has not succeeded. | ||
func getTenantFromCertificate(r *http.Request, certTenantField string) (string, error) { | ||
|
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.
I'm not sure if this is the right place to discuss this, but the query frontend also has a
query-frontend.org-id-header
arg. Thanos side, I believe this is only used when logging slow queries, but probably has some more use for Cortex.Do we want to keep both the
org-id-header
and thetenant-header
long term ? Could perhaps be nice to unify that into one cmd line arg if possible.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.
Long term we should indeed converge on only one to avoid confusion for users and ourselves, and for consistency.
As part of the migration process, I can mark the
query-frontend.org-id-header
as deprecated for now, mention the new one, and store the configuredorg-id-header
in the tenant header config. This way someone already using this will have a warning about the change and can easily adapt to the new CLI arg, allowing us to completely drop the org ID config in a future breaking change.Internally and more technically speaking, I am copying the tenant header field's value to the Org ID one because of some middlewares we are importing/reusing from Cortex, but I think over time we can adapt them to avoid this and clean things up.
What do you think, @jacobbaungard?
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.
Sounds like a good plan to me. What would happen if one define both, can we error out in that case?
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.
@jacobbaungard we could error out or print out a warning & give priority to the tenant header config, similar to what happens if you configure the header and the cert field in Receive. I personally have no preference. Will update to error out when both are provided. 👍
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.
Ok, I think the
orgID
was introduced only as a method of identifying sources of slow queries and passing that information to Cortex. It's also a repeatable flag, with significance in how headers are ordered, afaiu reading original issue/PR.Thus I don't think it is mutually exclusive with this
tenant
header, and both have different semantics as well, as you can have multipleorgID
headers, but only a singletenant
one. So let's not error or warn the user, and keep it for now.cc: @yeya24 in case he has some other thoughts on this! 🙂