-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(grpc) Add channel credentials API + Insecure credentials #2495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
7841232 to
954c3bc
Compare
954c3bc to
a0cbbf2
Compare
| @@ -0,0 +1,132 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file need unit test coverage to verify the blanket implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for the client creds wrapper, will add tests for the server side after making the required additions in the Runtime trait.
| @@ -0,0 +1,134 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for the client side. I realized that the Runtime trait lacks methods to create TCP listeners, which are required to test the server-side logic. I will add these methods in a follow-up PR, along with tests for insecure and type-erased server credentials.
grpc/src/credentials/dyn_wrapper.rs
Outdated
| String, | ||
| > { | ||
| let (stream, sec_info) = | ||
| SendFuture::send(self.connect(authority, source, info, runtime)).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Shouldn't we be able to do self.connect(...).send().await?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that works. Changed.
sauravzg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good overall. Please address the comments about unit test.
This change introduces a channel credentials API to abstract over TLS/Insecure/Local credentials. Implementations for TLS and local credentials will follow in future PRs.
Key Changes
rt::TcpStreamtoGrpcEndpointto reflect a more generic connection interface.GrpcEndpointis now sealed. This allows us to change the API in the future (e.g., to align with gRPC C++) without breaking changes. This restricts implementations ofrt::Runtimeand channel credentials to thegrpccrate.Other changes
pub(crate)as users can't to implement them due toGrpcEndpointtrait being sealed.rtmodule topub(crate)and exposedRuntimeaspubto resolveprivate_boundslints when using runtime types inpubAPIs.