Add auth token propagation for metrics reader #3341

albertteoh · 2021-10-27T11:37:56Z

Signed-off-by: albertteoh [email protected]

Which problem is this PR solving?

Resolves Authorization header does not propagate to /api/metrics/* endpoints #3340

Short description of the changes

Adds propagation of authorization token from the inbound request to the outbound request to a metrics server.
This is achieve by wrapping the http.Transport into a tokenAuthTransport struct that implements the http.RoundTripper's RoundTrip function, which injects the Authorization bearer token header into the outbound request.
Follows the existing pattern used by the ES reader: https://github.com/jaegertracing/jaeger/blob/master/pkg/es/config/config.go#L532

Signed-off-by: albertteoh <[email protected]>

codecov · 2021-10-27T11:43:20Z

Codecov Report

Merging #3341 (2742a11) into master (c4a3904) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3341      +/-   ##
==========================================
+ Coverage   96.46%   96.49%   +0.03%     
==========================================
  Files         259      260       +1     
  Lines       15178    15196      +18     
==========================================
+ Hits        14642    14664      +22     
+ Misses        452      449       -3     
+ Partials       84       83       -1

Impacted Files	Coverage Δ
cmd/query/app/server.go	`95.58% <100.00%> (ø)`
pkg/bearertoken/context.go	`100.00% <100.00%> (ø)`
pkg/bearertoken/http.go	`100.00% <100.00%> (ø)`
pkg/bearertoken/transport.go	`100.00% <100.00%> (ø)`
plugin/metrics/prometheus/metricsstore/reader.go	`100.00% <100.00%> (ø)`
plugin/storage/es/options.go	`100.00% <100.00%> (ø)`
plugin/storage/grpc/shared/grpc_client.go	`83.43% <100.00%> (ø)`
cmd/collector/app/server/zipkin.go	`70.73% <0.00%> (+2.43%)`	⬆️

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 c4a3904...2742a11. Read the comment docs.

th3M1ke

🔥

yurishkuro · 2021-10-27T13:49:38Z

plugin/metrics/prometheus/metricsstore/reader.go

+
+// tokenAuthTransport wraps an instance of http.Transport for the purpose of
+// propagating an Authorization token from inbound to outbound HTTP requests.
+type tokenAuthTransport struct {


Nit: could we take this opportunity and refactor/cleanup bearer token functionality into a standalone package? We have ./cmd/query/app/token_propagation_handler.go and ./storage/spanstore/token_propagation.go, which should really be in a single package like ./pkg/bearertoken.

Addressed in d1c96ad.

Signed-off-by: albertteoh <[email protected]>

albertteoh · 2021-10-28T11:10:23Z

pkg/bearertoken/propagation.go

+// the Authorization token from the http.Request and inserts it into the http.Request
+// context for easier access to the request token via GetBearerToken for bearer token
+// propagation use cases.
+func PropagationHandler(logger *zap.Logger, h http.Handler) http.Handler {


Renamed bearerTokenPropagationHandler -> PropagationHandler to reduce stutter.

I considered renaming ContextWithBearerToken and GetBearerToken for similar reasons but on further thought, I felt it reduces readability. Let me know if you disagree.

albertteoh · 2021-10-28T11:16:45Z

plugin/metrics/prometheus/metricsstore/reader.go

+
+// tokenAuthTransport wraps an instance of http.Transport for the purpose of
+// propagating an Authorization token from inbound to outbound HTTP requests.
+type tokenAuthTransport struct {


Addressed in d1c96ad.

yurishkuro · 2021-10-28T16:44:09Z

pkg/bearertoken/propagation.go

+func GetBearerToken(ctx context.Context) (string, bool) {
+	val, ok := ctx.Value(bearerToken).(string)
+	return val, ok
+}


I would split this file:

up to this point it's context.go

below it's http.go

you may also want to add the http transport implementation instead of using private types in different storages

Addressed in f0774b2.

yurishkuro · 2021-10-28T16:45:48Z

pkg/bearertoken/propagation_test.go

 )

-func Test_bearTokenPropagationHandler(t *testing.T) {
+func Test_GetBearerToken(t *testing.T) {
+	token := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI"


Couldn't this string just be "blah"? Currently it looks like there's special meaning in the value

Addressed in 8e5f1b9.

plugin/storage/es/options.go

Signed-off-by: albertteoh <[email protected]>

plugin/storage/es/options.go

albertteoh · 2021-10-29T11:50:16Z

plugin/metrics/prometheus/metricsstore/reader_test.go

@@ -331,12 +332,27 @@ func TestGetRoundTripper(t *testing.T) {
 				},
 			}, logger)
 			require.NoError(t, err)
-			assert.IsType(t, &http.Transport{}, rt)
-			if tc.tlsEnabled {
-				assert.NotNil(t, rt.(*http.Transport).TLSClientConfig)


Moving the common transport into the bearertoken package meant that we can't inspect the wrapped http.Transport.
The original test didn't feel quite right in the first place when it had to make type assertions.

Suggestions also welcome.

albertteoh · 2021-10-29T11:57:44Z

pkg/bearertoken/transport.go

+// request's Authorization bearer token header.
+// If the WithAllowOverrideFromCtx(true) option is provided, the request context's
+// bearer token, will be used in preference to this token.
+func WithToken(token string) Option {


I took the functional options approach because the defaultToken wasn't mandatory and didn't feel right to force callers to pass a sentinel token value, nor make callers guess what they should be setting it to if we exported this value in the struct, especially given it's in a separate package.

Let me know if you disagree with the approach.

We started to move away from the use of functional option pattern, it just adds unnecessary mental overhead and has poor discoverability in the docs. I feel like this type can simply be declared with public fields and instantiated directly, without a constructor function. The optionality of the fields is naturally available via struct zero values.

albertteoh · 2021-10-29T12:04:48Z

pkg/bearertoken/transport_test.go

+		{
+			name: "No options provided and request context set should have empty Bearer token",
+			roundTripper: roundTripFunc(func(r *http.Request) (*http.Response, error) {
+				assert.Equal(t, "Bearer ", r.Header.Get("Authorization"))


I wasn't sure if this is an acceptable Auth header value or if an error should be returned. This was the original behaviour so I kept it that way.

albertteoh · 2021-10-29T12:06:01Z

pkg/bearertoken/transport.go

+// NewTransport returns a new bearer token transport that wraps the given
+// http.RoundTripper, forwarding the authorization token from inbound to
+// outbound HTTP requests.
+func NewTransport(roundTripper http.RoundTripper, opts ...Option) http.RoundTripper {


I didn't feel like it was necessary to expose the internals of the transport, hence why http.RoundTripper is returned, given that's the only use case right now.

Signed-off-by: albertteoh <[email protected]>

albertteoh · 2021-10-29T13:26:26Z

It's not clear to me why the CIT Crossdock job is failing on:

✗ [endtoend] test_driver→ (services=go) ⇒ Fail: could not retrieve traces from query service

yurishkuro · 2021-10-29T15:41:38Z

pkg/bearertoken/context.go

+// Key is the string literal used internally in the implementation of this context.
+const Key = "bearer.token"
+const bearerToken = contextKey(Key)


I don't see Key being used outside of this package

Suggested change

// Key is the string literal used internally in the implementation of this context.

const Key = "bearer.token"

const bearerToken = contextKey(Key)

type contextKeyType string

const contextKey = contextKeyType("bearer.token")

yurishkuro · 2021-10-29T15:47:39Z

pkg/bearertoken/transport.go

+// request's Authorization bearer token header.
+// If the WithAllowOverrideFromCtx(true) option is provided, the request context's
+// bearer token, will be used in preference to this token.
+func WithToken(token string) Option {


We started to move away from the use of functional option pattern, it just adds unnecessary mental overhead and has poor discoverability in the docs. I feel like this type can simply be declared with public fields and instantiated directly, without a constructor function. The optionality of the fields is naturally available via struct zero values.

yurishkuro · 2021-10-29T15:48:39Z

pkg/bearertoken/transport.go

+			token = headerToken
+		}
+	}
+	r.Header.Set("Authorization", "Bearer "+token)


Do we want to check for token != "" before setting the header?

Yeah, I considered it but was technically a breaking change; I don't know enough about Auth headers to determine if this is a bug or correct behaviour, though sounds like it's a bug given your suggestion.

pkg/bearertoken/transport.go

plugin/storage/es/options.go

Signed-off-by: albertteoh <[email protected]>

albertteoh · 2021-10-29T20:14:42Z

pkg/bearertoken/transport.go

+			token = headerToken
+		}
+	}
+	r.Header.Set("Authorization", "Bearer "+token)


Yeah, I considered it but was technically a breaking change; I don't know enough about Auth headers to determine if this is a bug or correct behaviour, though sounds like it's a bug given your suggestion.

albertteoh · 2021-10-29T20:43:35Z

pkg/bearertoken/context.go

-const bearerToken = contextKey(BearerTokenKey)
+const (
+	// Key is the key name for the bearer token context value.
+	Key = "bearer.token"


Key is used outside of this package here.

Strictly speaking, I think it would've been better if grpc plugin just defined its own public constant instead of relying on this one, since they are completely independent.

$ grx BearerTokenKey . ./plugin/storage/grpc/shared/grpc_client.go:76: md.Set(spanstore.BearerTokenKey, bearerToken) ./plugin/storage/grpc/shared/grpc_client_test.go:115: bearerTokenFromMetadata := md.Get(spanstore.BearerTokenKey) ./plugin/storage/grpc/README.md:191: values := md.Get(spanstore.BearerTokenKey)

If we remove that external dependency, then this key can be made private, and doesn't need to be a string, just this:

type contextKeyType int const contextKey = iota

Addressed in 696cb5f.

yurishkuro · 2021-10-30T14:48:11Z

pkg/bearertoken/context.go

-const bearerToken = contextKey(BearerTokenKey)
+const (
+	// Key is the key name for the bearer token context value.
+	Key = "bearer.token"


Strictly speaking, I think it would've been better if grpc plugin just defined its own public constant instead of relying on this one, since they are completely independent.

$ grx BearerTokenKey . ./plugin/storage/grpc/shared/grpc_client.go:76: md.Set(spanstore.BearerTokenKey, bearerToken) ./plugin/storage/grpc/shared/grpc_client_test.go:115: bearerTokenFromMetadata := md.Get(spanstore.BearerTokenKey) ./plugin/storage/grpc/README.md:191: values := md.Get(spanstore.BearerTokenKey)

If we remove that external dependency, then this key can be made private, and doesn't need to be a string, just this:

type contextKeyType int const contextKey = iota

pkg/bearertoken/http.go

Signed-off-by: albertteoh <[email protected]>

albertteoh · 2021-11-02T20:57:56Z

Thanks for the review!

Add auth token propagation for metrics reader

a5133cc

Signed-off-by: albertteoh <[email protected]>

albertteoh requested a review from a team as a code owner October 27, 2021 11:37

albertteoh requested a review from yurishkuro October 27, 2021 11:38

th3M1ke previously approved these changes Oct 27, 2021

View reviewed changes

pavolloffay previously approved these changes Oct 27, 2021

View reviewed changes

yurishkuro reviewed Oct 27, 2021

View reviewed changes

Consolidate token propagation code

fa128b3

Signed-off-by: albertteoh <[email protected]>

albertteoh dismissed stale reviews from pavolloffay and th3M1ke via fa128b3 October 28, 2021 11:06

Rename to use PropagationHandler for consistency

d1c96ad

Signed-off-by: albertteoh <[email protected]>

albertteoh commented Oct 28, 2021

View reviewed changes

yurishkuro reviewed Oct 28, 2021

View reviewed changes

albertteoh added 4 commits October 29, 2021 22:45

Split and move common transports

f0774b2

Signed-off-by: albertteoh <[email protected]>

Give bearerToken value less meaning

8e5f1b9

Signed-off-by: albertteoh <[email protected]>

Fix import grouping

73db361

Signed-off-by: albertteoh <[email protected]>

Give token value less meaning

787b07b

Signed-off-by: albertteoh <[email protected]>

albertteoh commented Oct 29, 2021

View reviewed changes

albertteoh added 2 commits October 29, 2021 23:09

make fmt

9af8681

Signed-off-by: albertteoh <[email protected]>

Merge branch 'master' into 3340-token-prop

bac98cc

yurishkuro reviewed Oct 29, 2021

View reviewed changes

albertteoh added 2 commits October 30, 2021 07:05

Remove functional options

56ac31b

Signed-off-by: albertteoh <[email protected]>

Fix build

f5e3801

Signed-off-by: albertteoh <[email protected]>

albertteoh commented Oct 29, 2021

View reviewed changes

albertteoh and others added 3 commits October 31, 2021 08:50

Merge branch 'master' into 3340-token-prop

65208c2

Merge branch 'master' into 3340-token-prop

696eb45

Merge branch 'master' into 3340-token-prop

4d7e1c5

yurishkuro reviewed Nov 1, 2021

View reviewed changes

Address review comments

696cb5f

Signed-off-by: albertteoh <[email protected]>

albertteoh added 2 commits November 2, 2021 22:32

Merge branch 'master' into 3340-token-prop

bf6a818

Fix staticcheck err

2742a11

Signed-off-by: albertteoh <[email protected]>

yurishkuro approved these changes Nov 2, 2021

View reviewed changes

yurishkuro merged commit 4aa436b into jaegertracing:master Nov 2, 2021

albertteoh deleted the 3340-token-prop branch November 2, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auth token propagation for metrics reader #3341

Add auth token propagation for metrics reader #3341

albertteoh commented Oct 27, 2021

codecov bot commented Oct 27, 2021 •

edited

Loading

th3M1ke left a comment

yurishkuro Oct 27, 2021

albertteoh Oct 28, 2021

albertteoh Oct 28, 2021

albertteoh Oct 28, 2021

yurishkuro Oct 28, 2021

albertteoh Oct 29, 2021

yurishkuro Oct 28, 2021

albertteoh Oct 29, 2021

albertteoh Oct 29, 2021

albertteoh Oct 29, 2021

yurishkuro Oct 29, 2021

albertteoh Oct 29, 2021

albertteoh Oct 29, 2021

albertteoh commented Oct 29, 2021

yurishkuro Oct 29, 2021

yurishkuro Oct 29, 2021

yurishkuro Oct 29, 2021

albertteoh Oct 29, 2021

albertteoh Oct 29, 2021

albertteoh Oct 29, 2021

yurishkuro Oct 30, 2021

albertteoh Nov 2, 2021

yurishkuro Oct 30, 2021

albertteoh commented Nov 2, 2021

Add auth token propagation for metrics reader #3341

Add auth token propagation for metrics reader #3341

Conversation

albertteoh commented Oct 27, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Oct 27, 2021 • edited Loading

Codecov Report

th3M1ke left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh commented Oct 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh commented Nov 2, 2021

codecov bot commented Oct 27, 2021 •

edited

Loading