grpc: combine client and server SecurityInfo - #2798
Conversation
Also, rename SecurityContext to ValidateAuthority since that's what it does.
ee1c883 to
d6fe1e1
Compare
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM, with few minor comments.
| Box::new(MockConnectionSecurityContext), | ||
| Attributes::new(), | ||
| ), | ||
| security_info: SecurityInfo::new("mock").with_security_level(SecurityLevel::NoSecurity), |
There was a problem hiding this comment.
nit: Since NoSecurity is the default level, the call to with_security_level can be omitted.
There was a problem hiding this comment.
Thanks for catching this.
| } | ||
|
|
||
| impl SecurityInfo { | ||
| /// Creates a new SecurityInfo with ajdfskjklfdsjklsdfajklsfdajk |
There was a problem hiding this comment.
ajdfskjklfdsjklsdfajklsfdajk seems like a keymash, please remove.
There was a problem hiding this comment.
Argh, sorry, I forgot to fix this.
| use std::sync::Mutex; | ||
| use std::sync::atomic::AtomicU64; | ||
| use std::sync::atomic::Ordering; | ||
| use std::task::{Context, Poll}; |
There was a problem hiding this comment.
nit: The imports can be ungrouped for consistency.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the gRPC security credentials layer by consolidating client and server security info into a unified SecurityInfo struct and renaming ChannelSecurityContext to ValidateAuthority. The review feedback is highly constructive, pointing out a placeholder gibberish string in the documentation of SecurityInfo::new and outdated doc comments on LocalConnectionAuthorityValidator and ClientTlsAuthorityValidator that still refer to the old security context naming.
| /// connections. | ||
| #[derive(Debug, Clone)] | ||
| pub struct LocalConnectionSecurityContext; | ||
| pub struct LocalConnectionAuthorityValidator; |
There was a problem hiding this comment.
The doc comment above LocalConnectionAuthorityValidator still refers to ClientConnectionSecurityContext (which was renamed/removed). Consider updating the doc comment to refer to ValidateAuthority or LocalConnectionAuthorityValidator.
| /// Security context for [`rustls`]-based gRPC [`ChannelCredentials`]. | ||
| pub struct ClientTlsSecurityContext { | ||
| #[derive(Debug)] | ||
| pub struct ClientTlsAuthorityValidator { |
There was a problem hiding this comment.
The doc comment above ClientTlsAuthorityValidator still refers to it as a 'Security context' instead of an authority validator. Consider updating the doc comment to match the new name and purpose.
2 participants
Also, rename
SecurityContexttoValidateAuthoritysince that's what it does.My plan is to add the common
SecurityInfointo the upcomingPeerInfostruct.