RATIS-1541. Add SslContext to netty ChannelPipeline.#615
RATIS-1541. Add SslContext to netty ChannelPipeline.#615lokeshj1703 merged 2 commits intoapache:masterfrom
Conversation
|
Tested similar changes in https://issues.apache.org/jira/browse/RATIS-1546. It worked fine. I will if we can add some unit tests in https://issues.apache.org/jira/browse/RATIS-1542 . |
captainzmc
left a comment
captainzmc
left a comment
There was a problem hiding this comment.
Thanks @szetszwo for the PR. LGTM overall, just two minor comment.
| @@ -98,26 +97,25 @@ public GrpcTlsConfig(PrivateKey privateKey, X509Certificate certChain, | |||
|
|
|||
| public GrpcTlsConfig(File privateKeyFile, File certChainFile, | |||
There was a problem hiding this comment.
This method is not used anywhere, can we delete it?
There was a problem hiding this comment.
@captainzmc , thanks for reviewing this. This method is used in Ozone.
| @@ -38,7 +38,6 @@ | |||
| import org.apache.ratis.protocol.exceptions.LeaderNotReadyException; | |||
| import org.apache.ratis.protocol.exceptions.TimeoutIOException; | |||
| import org.apache.ratis.thirdparty.io.grpc.netty.GrpcSslContexts; | |||
There was a problem hiding this comment.
This line can also be moved below.
There was a problem hiding this comment.
Sure, let's sort the imports.
captainzmc
left a comment
captainzmc
left a comment
There was a problem hiding this comment.
+1 Thanks @szetszwo update this.
lokeshj1703
left a comment
lokeshj1703
left a comment
There was a problem hiding this comment.
@szetszwo Thanks for working on this! The changes look good to me. +1.
| b.keyManager(certificates.getFile(), privateKey.getFile()); | ||
| } else { | ||
| b.keyManager(privateKey.get(), certificates.get()); |
There was a problem hiding this comment.
Unrelated to PR. It would be better to have same order of private key and certificates in the constructors for SslContextBuilder.
|
@szetszwo Thanks for the contribution! I have committed the PR to master branch. |
|
@lokeshj1703 , thanks for reviewing and merging this! |
3 participants
See https://issues.apache.org/jira/browse/RATIS-1541