Support tenant header propagation in query service #4141
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
force-pushed
the
query-tenant-header
branch
from
e8fe0bb to
6373e19
Compare
Codecov ReportBase: 97.19% // Head: 97.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4141 +/- ##
==========================================
- Coverage 97.19% 97.14% -0.05%
==========================================
Files 295 295
Lines 17413 17436 +23
==========================================
+ Hits 16924 16938 +14
- Misses 394 400 +6
- Partials 95 98 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -93,6 +96,7 @@ func AddFlags(flagSet *flag.FlagSet) { | |||
| flagSet.String(queryStaticFiles, "", "The directory path override for the static assets for the UI") | |||
| flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format") | |||
| flagSet.Bool(queryTokenPropagation, false, "Allow propagation of bearer token to be used by storage plugins") | |||
| flagSet.String(queryTenantPropagation, "", "Enables propagation of a tenant header") | |||
There was a problem hiding this comment.
unclear description - is this a boolean or the name of the header?
| @@ -44,6 +44,7 @@ const ( | |||
| queryStaticFiles = "query.static-files" | |||
| queryUIConfig = "query.ui-config" | |||
| queryTokenPropagation = "query.bearer-token-propagation" | |||
| queryTenantPropagation = "query.tenant-propagation" | |||
There was a problem hiding this comment.
Please see https://github.com/jaegertracing/jaeger/pull/3688/files, there is already flag defined for the tenancy, can we not reuse that?
| const contextKey = contextKeyType(iota) | ||
| const ( | ||
| bearerTokenContextKey = contextKeyType(iota) | ||
| tenantHeaderContextKey |
There was a problem hiding this comment.
We have pkg/config/tenancy, I would rather consolidate all tenancy-related code there, not mix with bearer token.
There was a problem hiding this comment.
It's pkg/tenancy . I totally forgot about #3688. I will try to adjust the PR to use the tenancy package. It seems that only the ingestion part is using it for now.
|
Personal opinion is that this change should not be tied to tenancy. It would be more straightforward to add a flag that takes a list of headers and then propagate anything that matches. |
+1, the more we can generalize the better. Could the same be done with bearer token though? Or would it need some kind of mapping when headers are transitioned from HTTP to gRPC? |
|
superseded by #4151 |
Support tenant header propagation in the query HTTP APIs.
Our use case is to properly propagate tenant name (set in a configurable header) through the query service to the Tempo grpc plugin (right only the authorization header is propagated which is a bit of hack https://github.com/grafana/tempo/blob/main/cmd/tempo-query/tempo/plugin.go#L329-LL330)