xds: implement requireClientCertificate semantics - #6948
Conversation
| protected void setClientAuthValues( | ||
| SslContextBuilder sslContextBuilder, CertificateValidationContext localCertValidationContext) | ||
| throws CertificateException, IOException, CertStoreException { | ||
| checkState(server); |
There was a problem hiding this comment.
nit: should have a meaningful message.
btw, this class is rather confusing (and errorprone). it should be split into 2 different classes (server and client). can it be done? how hard to change?
There was a problem hiding this comment.
We had this discussion before. UpstreamTlsContext and DownstreamTlsContext are quite similar but there is no common abstraction to use. So I ended up using the generic K to eliminate code duplication. Splitting into server & client classes is definitely possible but I suspect there will be code duplication. Do you have better ideas?
There was a problem hiding this comment.
Yeah i thought we discussed this before.
i think code dup is better than error prone / confusing nature of current structure. we can still share some code as static or protected in parent method.
There was a problem hiding this comment.
Okay I will add a TODO for this suggestion and in the list of pending PRs
creamsoup
left a comment
creamsoup
left a comment
There was a problem hiding this comment.
one nit otherwise LGTM.
2 participants
No description provided.