feat(grpc): Call credentials API#2529
Conversation
arjan-bal
force-pushed
the
call-credentials
branch
from
124ad58 to
b0ca952
Compare
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
I'm not sure whether I like the impl Into<CallCredentials>. It is nice in some ways, but:
- It adds an extra type (
CallCredentialsvs.CallCredentialsProvider) to the API surface. - It requires the user to know that there is a blanket
impl<T: Provider> From<T> for CC. - It only saves the user needing to do an
Arc::new()(or.clone()) when configuring the channel. - It means if the user already
ArcstheirProvider, e.g. if they're going to use the same call creds for multiple channels, then this arcs their arc, which is wasteful in the same ways you're trying to avoid.
grpc/src/credentials/call.rs
Outdated
| &self.service_url | ||
| } | ||
|
|
||
| /// The method name suffix (e.g., `Method` or `package.Service/Method`). |
There was a problem hiding this comment.
This example lost me. Why would the method suffix include the service and a slash? Or did you typo of->or?
There was a problem hiding this comment.
The "or" should have been "in". Fixed that now.
While gRPC Go only sends the service URL, after stripping the method to the call credentials, gRPC C++ also sends the method name separately. I've gone with the C++ approach here.
grpc/src/credentials/call.rs
Outdated
| use crate::attributes::Attributes; | ||
| use crate::credentials::common::SecurityLevel; | ||
|
|
||
| /// Details regarding the RPC call. |
There was a problem hiding this comment.
Nit: "RPC call" -> "RPC" or just "call".
(x3)
| ) -> Result<(), Status>; | ||
|
|
||
| /// Indicates the minimum transport security level required to send | ||
| /// these credentials. |
There was a problem hiding this comment.
Does the docstring need to call out the default value here?
There was a problem hiding this comment.
Ping - I'm not sure what the documentation norms are for this kind of situation (default impls in traits) off the top of my head.
There was a problem hiding this comment.
Added a section in the rustdoc.
grpc/src/credentials/call.rs
Outdated
| SecurityLevel::IntegrityOnly => 1, | ||
| SecurityLevel::PrivacyAndIntegrity => 2, | ||
| }) | ||
| .unwrap_or(SecurityLevel::NoSecurity) |
There was a problem hiding this comment.
PrivacyAndIntegrity?
Although, why would unwrap fail here? Only if there were zero elements in self.creds (which is impossible)?
There was a problem hiding this comment.
Because the CompositeCallCredentials constructor requires two call credentials, the vector is guaranteed to be non-empty. I've replaced this with an expect() call and a clear message.
I don't have strong opinions on this, and I see the benefit of having a simpler API surface. The double Arcing can be avoided if the user creates the wrapper object (CallCredentials) once, and passed it to the channel constructor. I've replaced the wrapper type with |
| ) -> Result<(), Status>; | ||
|
|
||
| /// Indicates the minimum transport security level required to send | ||
| /// these credentials. |
There was a problem hiding this comment.
Ping - I'm not sure what the documentation norms are for this kind of situation (default impls in traits) off the top of my head.
grpc/src/credentials/client.rs
Outdated
| fn get_call_credentials(&self) -> Option<Arc<dyn CallCredentials>> { | ||
| Some(self.call_creds.clone()) | ||
| } |
There was a problem hiding this comment.
I'm not sure if it's worth it but this could still return Option<&Arc<dyn CallCredentials>> to delay the clone as much as possible. (That's effectively what you were doing before, too.)
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Looks like we're waiting for a new version of semver (w/ obi1kenobi/trustfall-rustdoc-adapter#1044), to work around a bug in rust (rust-lang/rust#153465), which should be available soon.
3 participants
This change introduces a Call Credentials API for gRPC, allowing users to attach authentication metadata (such as OAuth or JWT tokens) to individual RPCs.
API Design
CallCredentials: The base trait that specific credential implementations must implement.Arc<dyn CallCredentials>.Why Reference Counting?
Call credentials must be reference-counted (
Arc) to support sharing across out-of-band gRPC channels, such as those used by Load Balancers for load reporting.Functional Changes
fn get_call_credentials(&self) -> Option<&CallCredentials>to theChannelCredentialstrait. This enables attaching call credentials to channel-level credentials that apply to every RPC.CompositeCallCredentials: A new utility for combining multiple call credentials.CompositeChannelCredentials: A new utility for attaching call credentials to an existing channel credentials object.