credentials: expose NewRequestInfoContext publicly #8198
Conversation
The primary use case is to allow testing PerRPCCredentials implementations that live outside of google.golang.org/grpc. The secondary (somewhat weirded) use case is to allow reusing existing PerRPCCredentials implementations (as long as they don't depend on handshake details) outside of the standard gRPC client environment (i.e. with alternative implementations of grpc.ClientConnInterface that reuse gRPC code generator machinery and APIs, but don't use the actual implementations).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8198 +/- ##
==========================================
+ Coverage 81.91% 82.01% +0.10%
==========================================
Files 410 412 +2
Lines 40216 40475 +259
==========================================
+ Hits 32942 33195 +253
- Misses 5894 5905 +11
+ Partials 1380 1375 -5
🚀 New features to boost your workflow:
|
|
Updated. PTAL. Let me know if you want to me to revert other changes to icredentials.NewRequestInfoContext |
|
looks good to me , adding @dfawley for another review. |
|
Looks like the presubmit check wants "RELEASE NOTES". Please review/modify the line I've added to the description. Not sure how prominently this change should be highlighted in the notes (if at all). |
credentials/credentials.go
Outdated
| // | ||
| // This API is experimental. | ||
| func NewRequestInfoContext(ctx context.Context, ri RequestInfo) context.Context { | ||
| return icredentials.NewRequestInfoContext(ctx, ri) |
There was a problem hiding this comment.
Can we move the internal functions here instead of passing calls through?
There was a problem hiding this comment.
Done, but internal/credentials/credentials.go still remains since it still has ClientHandshakeInfoFromContext and NewClientHandshakeInfoContext. Removing it will require making NewClientHandshakeInfoContext public as well.
Do you want me to do this?
Strictly speaking it may also be useful for testing PerRPCCredentials.
There was a problem hiding this comment.
I would be fine with this, too, if you are interested in doing it. The same caveats would apply: it should only be used for testing. But if you don't need it, then nobody else has asked, so I'm fine with waiting until someone needs it, instead.
There was a problem hiding this comment.
I looked into how ClientHandshakeInfoFromContext is used (by actual prod code) and it is only used by xDS credentials implementation. I think for xds-related tests outside of grpc package (if any) to be useful, they will need access to other xDS internal parts (internal/xds package). For that reason I think it makes little sense to make it public now (also no one really needs it public).
credentials/credentials.go
Outdated
| // NewRequestInfoContext creates a new context with the given RequestInfo | ||
| // attached to it. |
There was a problem hiding this comment.
NewContextWithRequestInfo attaches the RequestInfo to ctx?
There was a problem hiding this comment.
I chose "creates a new context with ... attached" because many other existing functions are documented that way:
// NewIncomingContext creates a new context with incoming md attached.
// NewOutgoingContext creates a new context with outgoing md attached.
// NewContext creates a new context with peer information attached.
// NewContextWithServerTransportStream creates a new context from ctx and attaches stream to it.
There was a problem hiding this comment.
Of these, the last one is most accurate. I'm also fine with creates a new context from ctx and attaches ri to it, too.
There was a problem hiding this comment.
Changed it to NewContextWithRequestInfo creates a new context from ctx and attaches ri to it.
The primary use case is to allow testing PerRPCCredentials implementations that live outside of google.golang.org/grpc.
The secondary (somewhat weirder) use case is to allow reusing existing PerRPCCredentials implementations (as long as they don't depend on handshake details) outside of the standard gRPC client environment (i.e. with alternative implementations of grpc.ClientConnInterface that reuse gRPC code generator machinery and APIs, but don't use the actual implementations).
Fixes #8080.
RELEASE NOTES:
NewContextWithRequestInfoto allow testingPerRPCCredentialsimplementations.