xds: implement requireClientCertificate semantics #6948
Conversation
xds/src/main/java/io/grpc/xds/internal/sds/SslContextProvider.java
Outdated
Show resolved
Hide resolved
xds/src/test/java/io/grpc/xds/internal/sds/CommonTlsContextTestsUtil.java
Show resolved
Hide resolved
xds/src/test/java/io/grpc/xds/internal/sds/SecretVolumeSslContextProviderTest.java
Show resolved
Hide resolved
| 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
No description provided.