Propagate the bearer token to the gRPC plugin server #1822

radekg · 2019-09-28T19:52:10Z

This PR attempts to resolve the problem described in #1821.

Problem summary

Query UI has an option to forward Bearer token to the storage back end using --query.bearer-token-propagation=true flag. However, when using the gRPC storage plugin, the context created with spanstore.ContextWithBearerToken(ctx, token) in https://github.com/jaegertracing/jaeger/blob/master/cmd/query/app/token_propagation_handler.go#L48 is not passed to the gRPC plugin.

Currently, the last place in the storage where the context with the bearer.token can be reached, is the gRPC client: https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/grpc/shared/grpc_client.go.

This can be tested by adding these lines of code:

  str, ok := spanstore.GetBearerToken(ctx)
  fmt.Println(fmt.Sprintf(" =====================> CLIENT: The context is: %v ::: %b", str, ok))

to any of the following methods:

func (c *grpcClient) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error)
func (c *grpcClient) GetServices(ctx context.Context) ([]string, error)
func (c *grpcClient) GetOperations(ctx context.Context, service string) ([]string, error)
func (c *grpcClient) FindTraces(ctx context.Context, query *spanstore.TraceQueryParameters) ([]*model.Trace, error)
func (c *grpcClient) FindTraceIDs(ctx context.Context, query *spanstore.TraceQueryParameters) ([]model.TraceID, error)

However, on the server part of the plugin, the context is a new instance because the plugin is running as a separate process.

Solution summary

This pull request adds support for bearer token forwarding to the gRPC spanstore.Reader methods of the gRPC storage. Similarly to the solution from the ES storage provider, only forward the bearer token when the --query.bearer-token-propagation flag has been set to true.

plugin/storage/grpc/proto/storage.proto

yurishkuro · 2019-09-29T20:14:30Z

plugin/storage/grpc/shared/grpc_client.go

-	readerClient     storage_v1.SpanReaderPluginClient
-	writerClient     storage_v1.SpanWriterPluginClient
-	depsReaderClient storage_v1.DependenciesReaderPluginClient
+	allowTokenFromContext bool


is the config option actually required? If the query-service is configured to pass the bearer token, it would include it in the context, and this code just needs to respect that.

We do want a way to tell our components that the token shouldn't be propagated.

There is already configuration to that effect in the query service, which controls token handoff from query to storage. I don't think yet another flag for storage-storagePlugin is needed.

yurishkuro · 2019-09-29T20:17:18Z

plugin/storage/grpc/shared/grpc_server.go

@@ -53,7 +64,8 @@ func (s *grpcServer) WriteSpan(ctx context.Context, r *storage_v1.WriteSpanReque

 // GetTrace takes a traceID and streams a Trace associated with that traceID
 func (s *grpcServer) GetTrace(r *storage_v1.GetTraceRequest, stream storage_v1.SpanReaderPlugin_GetTraceServer) error {
-	trace, err := s.Impl.SpanReader().GetTrace(stream.Context(), r.TraceID)
+	opCtx := s.getMaybeBearerTokenContext(stream.Context(), r.BearerToken)


this may not be needed per my earlier comments, but if needed then it would be better to rename to a more general method of "adapting" the context from request, so that we could, in the future, support different types of middleware.

yurishkuro · 2019-09-29T20:18:28Z

plugin/storage/grpc/shared/interface.go

-var PluginMap = map[string]plugin.Plugin{
-	StoragePluginIdentifier: &StorageGRPCPlugin{},
+// GetPluginMap returns a plugin map.
+func GetPluginMap(allowTokenFromContext bool) map[string]plugin.Plugin {


if we can get away without explicit configuration, please undo this

radekg · 2019-09-30T09:44:06Z

Hi @yurishkuro, thank you for a quick review. I have updated the code according to your request / suggestions. Indeed, this is a way cleaner solution than the original one.

radekg · 2019-09-30T09:46:19Z

plugin/storage/grpc/shared/grpc_client.go

@@ -34,6 +35,18 @@ type grpcClient struct {
 	depsReaderClient storage_v1.DependenciesReaderPluginClient
 }

+// updateContextWithBearerToken updates the outgoing context with a bearer token extracted from the source
+func (c *grpcClient) updateContextWithBearerToken(outgoing context.Context, source context.Context) context.Context {


I am not sure what would be a cleaner way to handle this. The problem here is that FindTraces and FindTraceIDs uses context.Background() so this function has to operate on two contexts.

radekg · 2019-09-30T09:50:41Z

plugin/storage/grpc/shared/grpc_client.go

+	bearerToken, hasToken := spanstore.GetBearerToken(source)
+	if hasToken {
+		requestMetadata := metadata.New(map[string]string{
+			"bearerToken": bearerToken,


A suggestion here. There is a place in the code where the property identifying this setting is already defined. It is: https://github.com/jaegertracing/jaeger/blob/master/storage/spanstore/token_propagation.go#L21.
Maybe we could do something like:

const BearerTokenKey = "bearer.token" const bearerToken = contextKey("bearer.token")

and use the spanstore.BearerTokenKey here and then in the gRPC plugin server?

Will do. Final question: should I squash before it is merged?

No need, we squash on merge

plugin/storage/grpc/shared/grpc_client.go

yurishkuro

It looks like one of the earlier commits did not have a DCO sign-off, you may need to squash them to fix that.

storage/spanstore/token_propagation.go

yurishkuro · 2019-10-01T14:51:08Z

plugin/storage/grpc/shared/grpc_client.go

+// upgradeContextWithBearerToken turns the context into a gRPC outgoing context with bearer token
+// in the request metadata, if the original context has bearer token attached.
+// Otherwise returns original context.
+func (c *grpcClient) upgradeContextWithBearerToken(ctx context.Context) context.Context {


this function does not need a receiver

Signed-off-by: radekg <radek@gruchalski.com>

…uctions Signed-off-by: radekg <radek@gruchalski.com>

…plugin, FindTraceIDs client context. Signed-off-by: radekg <radek@gruchalski.com>

…unction Signed-off-by: radekg <radek@gruchalski.com>

Signed-off-by: radekg <radek@gruchalski.com>

jpkrohling · 2019-10-02T09:36:38Z

Looks like there's one or more files that need formatting. The CI job failed with:

Go fmt, license check, or import ordering failures, run 'make fmt'

Signed-off-by: radekg <radek@gruchalski.com>

radekg · 2019-10-02T10:05:47Z

All fixed and signed off.

codecov · 2019-10-02T10:24:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2957d89). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1822   +/-   ##
=========================================
  Coverage          ?   98.22%           
=========================================
  Files             ?      195           
  Lines             ?     9611           
  Branches          ?        0           
=========================================
  Hits              ?     9440           
  Misses            ?      134           
  Partials          ?       37

Impacted Files	Coverage Δ
storage/spanstore/token_propagation.go	`71.42% <ø> (ø)`
plugin/storage/grpc/shared/grpc_client.go	`83.67% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2957d89...8fb5e57. Read the comment docs.

no tests

yurishkuro

@radekg could you please add a test somewhere verifying that the token is indeed accessible in the plugin implementation?

yurishkuro · 2019-10-02T21:33:07Z

This code is not covered:

Signed-off-by: radekg <radek@gruchalski.com>

radekg · 2019-10-03T10:47:41Z

Hi @yurishkuro, I'm not sure if the failing tests are related to my changes. I can see cql related fatal errors in the Travis job.

pavolloffay · 2019-10-09T11:24:00Z

thanks @radekg

radekg · 2019-10-09T13:33:04Z

Thank you.

@yurishkuro

) * Add bearer token forward to the GRPC storage plugin Signed-off-by: radekg <radek@gruchalski.com> * Apply the changes suggested by @yurishkuro in the review Signed-off-by: radekg <radek@gruchalski.com> * Update gRPC storage plugin readme with bearer token propagation instructions Signed-off-by: radekg <radek@gruchalski.com> * Expose a literal from token propagation context, use literal in gRPC plugin, FindTraceIDs client context. Signed-off-by: radekg <radek@gruchalski.com> * gRPC plugin client, uses original context everywhere, adapt upgrade function Signed-off-by: radekg <radek@gruchalski.com> * Reuse the bearer token key string in the context key Signed-off-by: radekg <radek@gruchalski.com> * upgradeContextWithBearerToken does not need a receiver Signed-off-by: radekg <radek@gruchalski.com> * Apply make fmt Signed-off-by: radekg <radek@gruchalski.com> * Add test coverage for context upgrades Signed-off-by: radekg <radek@gruchalski.com> * D'oh, fmt for tests... Signed-off-by: radekg <radek@gruchalski.com> Signed-off-by: Jonah Back <jonah@jonahback.com>

radekg requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners September 28, 2019 19:52

radekg force-pushed the grpc-storage-bearer-token-propagation branch from a088b29 to 9ce0549 Compare September 28, 2019 19:53

yurishkuro requested changes Sep 29, 2019

View reviewed changes

radekg commented Sep 30, 2019

View reviewed changes

yurishkuro reviewed Sep 30, 2019

View reviewed changes

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

jpkrohling approved these changes Oct 1, 2019

View reviewed changes

yurishkuro reviewed Oct 1, 2019

View reviewed changes

radekg added 7 commits October 1, 2019 18:33

Add bearer token forward to the GRPC storage plugin

7f28edf

Signed-off-by: radekg <radek@gruchalski.com>

Apply the changes suggested by @yurishkuro in the review

69d25d5

Signed-off-by: radekg <radek@gruchalski.com>

Update gRPC storage plugin readme with bearer token propagation instr…

670d6a9

…uctions Signed-off-by: radekg <radek@gruchalski.com>

Expose a literal from token propagation context, use literal in gRPC …

372aab9

…plugin, FindTraceIDs client context. Signed-off-by: radekg <radek@gruchalski.com>

gRPC plugin client, uses original context everywhere, adapt upgrade f…

c35211c

…unction Signed-off-by: radekg <radek@gruchalski.com>

Reuse the bearer token key string in the context key

3e82f81

Signed-off-by: radekg <radek@gruchalski.com>

upgradeContextWithBearerToken does not need a receiver

308d113

Signed-off-by: radekg <radek@gruchalski.com>

radekg force-pushed the grpc-storage-bearer-token-propagation branch from f650352 to 308d113 Compare October 1, 2019 16:33

Apply make fmt

2b8bb78

Signed-off-by: radekg <radek@gruchalski.com>

radekg force-pushed the grpc-storage-bearer-token-propagation branch from d10a84b to 2b8bb78 Compare October 2, 2019 10:04

yurishkuro previously approved these changes Oct 2, 2019

View reviewed changes

yurishkuro reviewed Oct 2, 2019

View reviewed changes

radekg added 2 commits October 3, 2019 11:38

Add test coverage for context upgrades

4a00c03

Signed-off-by: radekg <radek@gruchalski.com>

D'oh, fmt for tests...

0831497

Signed-off-by: radekg <radek@gruchalski.com>

yurishkuro approved these changes Oct 3, 2019

View reviewed changes

Merge branch 'master' into grpc-storage-bearer-token-propagation

8fb5e57

pavolloffay merged commit 9a701bc into jaegertracing:master Oct 9, 2019

pavolloffay mentioned this pull request Oct 9, 2019

gRPC plugin framework does not respect --query.bearer-token-propagation flag #1821

Closed

radekg deleted the grpc-storage-bearer-token-propagation branch October 9, 2019 17:47

yurishkuro added this to the Release 1.15 milestone Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate the bearer token to the gRPC plugin server #1822

Propagate the bearer token to the gRPC plugin server #1822

radekg commented Sep 28, 2019 •

edited

Loading

yurishkuro Sep 29, 2019

jpkrohling Oct 1, 2019

yurishkuro Oct 1, 2019

yurishkuro Sep 29, 2019

yurishkuro Sep 29, 2019

radekg commented Sep 30, 2019

radekg Sep 30, 2019

radekg Sep 30, 2019

yurishkuro Sep 30, 2019

radekg Sep 30, 2019

yurishkuro Oct 1, 2019

yurishkuro left a comment

yurishkuro Oct 1, 2019

jpkrohling commented Oct 2, 2019

radekg commented Oct 2, 2019

codecov bot commented Oct 2, 2019 •

edited

Loading

yurishkuro left a comment

yurishkuro commented Oct 2, 2019

radekg commented Oct 3, 2019

pavolloffay commented Oct 9, 2019

radekg commented Oct 9, 2019

Propagate the bearer token to the gRPC plugin server #1822

Propagate the bearer token to the gRPC plugin server #1822

Conversation

radekg commented Sep 28, 2019 • edited Loading

Problem summary

Solution summary

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

radekg commented Sep 30, 2019

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

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Oct 2, 2019

radekg commented Oct 2, 2019

codecov bot commented Oct 2, 2019 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Oct 2, 2019

radekg commented Oct 3, 2019

pavolloffay commented Oct 9, 2019

radekg commented Oct 9, 2019

radekg commented Sep 28, 2019 •

edited

Loading

codecov bot commented Oct 2, 2019 •

edited

Loading