credentials: add RequestInfo to context passed to GetRequestMetadata #3057
Conversation
… request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data. This is a fix for grpc#3019
… request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data. This is a fix for grpc#3019
… request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data. This is a fix for grpc#3019
… request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data. This is a fix for grpc#3019
|
"Continued" from #3032 |
internal/transport/http2_client.go
Outdated
| ri := credentials.RequestInfo{ | ||
| Method: callHdr.Method, | ||
| } | ||
| ctx = internal.NewRequestInfoContext.(func(context.Context, credentials.RequestInfo) context.Context)(ctx, ri) |
There was a problem hiding this comment.
I didn't consider this initially, but.. This will leave the RequestInfo in the context for the life of the stream. Since RequestInfo is in the credentials package, let's not keep it around except when calling the credentials. If it's needed elsewhere, we might want to move RequestInfo somewhere else. I would say: let's create a new context the same place where createAudience is called, and only pass this to the get*AuthData functions.
There was a problem hiding this comment.
Done. Though I had to remove the unit test that tested via NewStream. So the only test coverage is via the e2e test. That said it appears that is the case for everything in the method that calls createAudience. ¯_(ツ)_/¯
There was a problem hiding this comment.
I prefer API-level tests to unit tests for most things, so that's totally fine by me.
credentials/credentials.go
Outdated
| @@ -334,3 +335,27 @@ func cloneTLSConfig(cfg *tls.Config) *tls.Config { | |||
|
|
|||
| return cfg.Clone() | |||
| } | |||
|
|
|||
| // RequestInfo contains request data to be attached to the context passed along to GetRequestMetadata calls. This API is experimental. | |||
credentials/credentials.go
Outdated
| @@ -334,3 +335,27 @@ func cloneTLSConfig(cfg *tls.Config) *tls.Config { | |||
|
|
|||
| return cfg.Clone() | |||
| } | |||
|
|
|||
| // RequestInfo contains request data to be attached to the context passed along to GetRequestMetadata calls. This API is experimental. | |||
There was a problem hiding this comment.
Please put "This API is experimental" on a new line, with a blank comment line between it and the rest of the comments. (And below; see Bundle in this file for an example.)
credentials/credentials.go
Outdated
| @@ -334,3 +335,27 @@ func cloneTLSConfig(cfg *tls.Config) *tls.Config { | |||
|
|
|||
| return cfg.Clone() | |||
| } | |||
|
|
|||
| // RequestInfo contains request data to be attached to the context passed along to GetRequestMetadata calls. This API is experimental. | |||
There was a problem hiding this comment.
Let's also document on GetRequestMetadata in the PerRPCCredentials interface that the context will contain a RequestInfo retrievable via RequestInfoFromContext.
There was a problem hiding this comment.
Done. Let me know if you would like to change the wording.
credentials/credentials.go
Outdated
| // RequestInfoFromContext extracts the RequestInfo from the context. | ||
| // | ||
| // This API is experimental. | ||
| func RequestInfoFromContext(ctx context.Context) RequestInfo { |
There was a problem hiding this comment.
Bah, I missed this before, sorry: it's typical to also return an ok bool here, in case the RequestInfo is not present in ctx. Please add this. The comment can be updated to match peer.FromContext's wording by simply adding "if it exists".
dfawley
changed the title
Add a RequestInfo struct which initially is used for passing the full request method (though could later be expanded to pass more info) so that things like GetRequestMetadata can be used to apply logic based on that data.
This is a fix for #3019