[grpc storage]: Propagate tenant to grpc backend #6030

frzifus · 2024-09-30T21:48:45Z

Which problem is this PR solving?

Missing tenant information in a gRPC remote backend.

Description of the changes

Before the gRPC plugins were removed in Jaeger 1.58, tenant information was distributed in via the internal context.
With this change, the tenant information is also propagated to a removed grpc backend.

How was this change tested?

Manually with jaeger-query, tempo-query and wireshark.

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

cc @albertteoh do you think we can get this into the next jaeger release tomorrow? ^^

codecov · 2024-09-30T21:56:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.90%. Comparing base (99e9e0e) to head (20a31ba).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6030   +/-   ##
=======================================
  Coverage   96.90%   96.90%           
=======================================
  Files         349      349           
  Lines       16599    16600    +1     
=======================================
+ Hits        16086    16087    +1     
  Misses        329      329           
  Partials      184      184

Flag	Coverage Δ
badger_v1	`7.98% <0.00%> (-0.01%)`	⬇️
badger_v2	`1.81% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1	`15.74% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2	`1.74% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1	`15.74% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2	`1.74% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.69% <0.00%> (-0.01%)`	⬇️
elasticsearch-7.x-v1	`18.74% <0.00%> (-0.01%)`	⬇️
elasticsearch-8.x-v1	`18.93% <0.00%> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`1.80% <0.00%> (-0.01%)`	⬇️
grpc_v1	`9.51% <100.00%> (-0.01%)`	⬇️
grpc_v2	`7.10% <0.00%> (-0.01%)`	⬇️
kafka-v1	`9.69% <0.00%> (-0.01%)`	⬇️
kafka-v2	`1.81% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.80% <0.00%> (-0.02%)`	⬇️
opensearch-1.x-v1	`18.80% <0.00%> (-0.01%)`	⬇️
opensearch-2.x-v1	`18.78% <0.00%> (-0.01%)`	⬇️
opensearch-2.x-v2	`1.80% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.46% <0.00%> (-0.01%)`	⬇️
unittests	`95.70% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus · 2024-10-01T12:39:42Z

plugin/storage/grpc/shared/grpc_client.go

+	// BearerTokenKey is the key name for the bearer token context value.
+	BearerTokenKey = "bearer.token"
+	// TenantKey is the key name for the x-tenant context value.
+	TenantKey = "x.tenant"


Does it make sense to reuse the configured tenant header key?
Coming from --multi-tenancy.header=x-scope-orgid?

Well.. That should already be the case. Since tenancy.NewClientUnaryInterceptor and tenancy.NewClientStreamInterceptor are registered.

Running Jaeger query without this patch:

SPAN_STORAGE_TYPE=grpc ./query --query.base-path=/ --grpc-storage.server=127.0.0.1:7777 --query.bearer-token-propagation=true --multi-tenancy.enabled=true --multi-tenancy.header=x-scope-orgid --query.grpc-server.host-port=localhost:16685 --query.http-server.host-port=localhost:16686 --admin.http.host-port=localhost:16687

Then going a request:

curl -X GET "http://localhost:16686/api/traces?service=dummy" \ -H "Authorization: Bearer abc" \ -H "x-scope-orgid: 124"

Will only propagate the following metadata:

Stream Request Metadata: map[:authority:[127.0.0.1:7777] bearer.token:[abc] content-type:[application/grpc] grpc-accept-encoding:[snappy,zstd,gzip] user-agent:[grpc-go/1.65.0]]

Using this patch x.tenant:[124] will be part of it.

Well.. That should already be the case. Since tenancy.NewClientUnaryInterceptor and tenancy.NewClientStreamInterceptor are registered.

Seems the problem was that this branch was never executed. Since the tenancy options from config_v1 have not been translated into config_v2.

jaeger/plugin/storage/grpc/factory.go

Lines 134 to 136 in 19386cf

if tenancyMgr.Enabled {

opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr)))

opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr)))

After adding the tenancy options in TranslateToConfigV2 the tenant infos x-scope-orgid:[124] are propagated.

Stream Request Metadata: map[:authority:[127.0.0.1:7777] bearer.token:[abc] content-type:[application/grpc] grpc-accept-encoding:[snappy,zstd,gzip] user-agent:[grpc-go/1.67.0] x-scope-orgid:[124]]

cc @yurishkuro @pavolloffay

plugin/storage/grpc/shared/grpc_client.go

plugin/storage/grpc/shared/grpc_client_test.go

yurishkuro · 2024-10-01T14:58:17Z

Is the header name x.tenant in any way standard in gRPC?
I noticed that we don't have much in the way of documentation for these two headers, bearer.token and x.tenant - they both seem to be non-standard. Should we add a paragraph to https://www.jaegertracing.io/docs/1.61/deployment/#remote-storage explaining their use?
Do we also need a symmetrical header extractor in grpc/shared/grpc_handler?

This reverts commit 090962d. Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus · 2024-10-01T20:40:20Z

Is the header name x.tenant in any way standard in gRPC?

No, the header name x.tenant is not a standard header in gRPC. I followed the bearer token implementation with the naming.

I noticed that we don't have much in the way of documentation for these two headers, bearer.token and x.tenant - they both seem to be non-standard. Should we add a paragraph to https://www.jaegertracing.io/docs/1.61/deployment/#remote-storage explaining their use?

Sure, I will document the outcome of this PR afterwards.

Do we also need a symmetrical header extractor in grpc/shared/grpc_handler?

I think no. I'm also not sure if we should keep the bearer.token key for the authorization token. From my point of view it makes sense to provide gRPC server & client interceptor for a list of headers. That should work with any header key you want to forward.

yurishkuro · 2024-10-01T20:44:24Z

From my point of view it makes sense to provide gRPC server & client interceptor for a list of headers.

What would those interceptors do with the headers? In case of outbound ones they read the values from designated keys in the Context, so I am not sure what a "generic" interceptor for "headers" would do.

I do think we need an inbound interceptor for use in cmd/remote-storage, because it can be used to run storage backends natively supported by Jaeger which would be able to read those values from the Context.

yurishkuro · 2024-10-01T20:46:31Z

plugin/storage/grpc/config.go

@@ -38,6 +38,7 @@ func DefaultConfigV2() ConfigV2 {

 func (c *Configuration) TranslateToConfigV2() *ConfigV2 {
 	return &ConfigV2{
+		Tenancy: c.TenancyOpts,


@mahadzaryab1 I think this is another example of divergence between v1/v2 configs - this is why my preference is always NOT to have separate configs, so that omissions like this won't happen. Do you want to take a look at that as part of #5229?

@frzifus what kind of unit or e2e test could we have to catch this bug?

I was going to add a unittest for this translate config function. But when I started to re-type this 4 line logic, I thought it doesnt make much sense.

An e2e test that runs jaeger-query with the SPAN_STORAGE_TYPE=grpc storage and the --multi-tenancy.header=x-scope-orgid flag and checks the metadata of the outgoing gRPC request would be perfect to avoid that issue in the future.
Similar to this: #6030 (comment)

@yurishkuro Would it be ok, creating an issue to keep track and submit something afterwards?

will do @yurishkuro

An e2e test that runs jaeger-query with the SPAN_STORAGE_TYPE=grpc storage and the --multi-tenancy.header=x-scope-orgid flag and checks the metadata of the outgoing gRPC request would be perfect to avoid that issue in the future.

We have a grpc storage e2e test today. We can add a test there that will run the storage with tenancy enabled and store a span with tenant1 then try to retrieve it with tenant1 (found) and tenant2 (not found). The memory storage used in that e2e test already supports separation of data by tenant.

It would be even better to do this with jaeger-v2 e2e tests, but one thing that I don't know about is how we can thread the tenant ID through the OTEL Collector pipeline, from OTLP receiver to jaeger-storage-extension.

frzifus · 2024-10-01T20:55:38Z

What would those interceptors do with the headers? In case of outbound ones they read the values from designated keys in the Context, so I am not sure what a "generic" interceptor for "headers" would do.

mh.. y, I think you are right. In that case, we would need to attach a map as value and build something custom. Probably not worth it.

I do think we need an inbound interceptor for use in cmd/remote-storage, because it can be used to run storage backends natively supported by Jaeger which would be able to read those values from the Context.

Its the case here, right?

jaeger/cmd/remote-storage/app/server.go

Lines 99 to 104 in 19386cf

    
           if tm.Enabled { 
        
           	grpcOpts = append(grpcOpts, 
        
           		grpc.StreamInterceptor(tenancy.NewGuardingStreamInterceptor(tm)), 
        
           		grpc.UnaryInterceptor(tenancy.NewGuardingUnaryInterceptor(tm)), 
        
           	) 
        
           }

yurishkuro · 2024-10-01T21:04:45Z

Its the case here, right?

ah, so it handles tenancy but not the bearer token? I actually don't see anything grpc-related in pkg/bearertoken

yurishkuro · 2024-10-01T21:06:32Z

we should move upgradeContextWithBearerToken from plugin/storage/grpc/shared/grpc_client.go to be implemented as interceptor in pkg/bearertoken

frzifus · 2024-10-01T21:22:03Z

ah, so it handles tenancy but not the bearer token? I actually don't see anything grpc-related in pkg/bearertoken

y, I did not go through the code yet. It might be handled somewhere. But having a pkg with the gRPC impl. (similar to the tenant thing) would simplify it.

) ## Which problem is this PR solving? - Resolves #6041 ## Description of the changes - We were currently maintaining two entirely separate configurations for v1 and v2 for the GRPC Storage Component and were initializing v1 configurations and then translating them to the v2 configurations which were the ones actually being used. This caused an issue where one configuration field was left out of the translation method (see #6030 for more details). - In this PR, we consolidate the v1 and v2 configurations into a single config type that is directly initialized to avoid having configurations that diverge or running into issues like the one described above. ## How was this change tested? - Unit tests were updated - Integration tests were not touched but should still pass since this was just an internal change and the interface was not touched ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

frzifus closed this Sep 30, 2024

frzifus reopened this Oct 1, 2024

frzifus marked this pull request as ready for review October 1, 2024 11:25

frzifus requested a review from a team as a code owner October 1, 2024 11:25

frzifus requested a review from pavolloffay October 1, 2024 11:25

storage: propagate tenant to grpc backend

090962d

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

dosubot bot added the area/storage label Oct 1, 2024

frzifus force-pushed the grpc-tenancy branch from 7d59d3c to 090962d Compare October 1, 2024 11:26

frzifus commented Oct 1, 2024

View reviewed changes

yurishkuro reviewed Oct 1, 2024

View reviewed changes

plugin/storage/grpc/shared/grpc_client.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 1, 2024

View reviewed changes

plugin/storage/grpc/shared/grpc_client_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 1, 2024

View reviewed changes

plugin/storage/grpc/shared/grpc_client_test.go Outdated Show resolved Hide resolved

frzifus added 2 commits October 1, 2024 22:22

Revert "storage: propagate tenant to grpc backend"

37fb0e1

This reverts commit 090962d. Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

fix: translate tenancy config from v1 to v2

20a31ba

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus requested a review from yurishkuro October 1, 2024 20:31

yurishkuro reviewed Oct 1, 2024

View reviewed changes

yurishkuro mentioned this pull request Oct 1, 2024

[jaeger-v2] Rethink components configuration to align with OTEL #5229

Closed

16 tasks

frzifus mentioned this pull request Oct 1, 2024

[Feature]: Implement bearer token auth interceptor #6035

Closed

frzifus requested a review from yurishkuro October 1, 2024 23:08

yurishkuro approved these changes Oct 3, 2024

View reviewed changes

yurishkuro changed the title ~~storage: propagate tenant to grpc backend~~ [grpc storage]: Propagate tenant to grpc backend Oct 3, 2024

yurishkuro added the changelog:bugfix-or-minor-feature label Oct 3, 2024

yurishkuro merged commit d6631f5 into jaegertracing:main Oct 3, 2024
49 of 50 checks passed

mahadzaryab1 mentioned this pull request Oct 4, 2024

[jaeger-v2] Consolidate v1 and v2 Configurations for GRPC Storage #6042

Merged

4 tasks

frzifus deleted the grpc-tenancy branch October 4, 2024 14:06

frzifus mentioned this pull request Oct 4, 2024

Release 0.14.0 grafana/tempo-operator#1046

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grpc storage]: Propagate tenant to grpc backend #6030

[grpc storage]: Propagate tenant to grpc backend #6030

frzifus commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

frzifus Oct 1, 2024 •

edited

Loading

frzifus Oct 1, 2024

frzifus Oct 1, 2024

frzifus Oct 1, 2024 •

edited

Loading

yurishkuro commented Oct 1, 2024

frzifus commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

yurishkuro Oct 1, 2024

yurishkuro Oct 1, 2024

frzifus Oct 1, 2024

mahadzaryab1 Oct 2, 2024

yurishkuro Oct 2, 2024

yurishkuro Oct 2, 2024

frzifus commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

frzifus commented Oct 1, 2024

	if tenancyMgr.Enabled {
	opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr)))
	opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr)))

[grpc storage]: Propagate tenant to grpc backend #6030

[grpc storage]: Propagate tenant to grpc backend #6030

Conversation

frzifus commented Sep 30, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

frzifus Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frzifus Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro commented Oct 1, 2024

frzifus commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frzifus commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

yurishkuro commented Oct 1, 2024

frzifus commented Oct 1, 2024

frzifus commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

frzifus Oct 1, 2024 •

edited

Loading

frzifus Oct 1, 2024 •

edited

Loading