----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review89063 -----------------------------------------------------------
Thanks for the patch, Sriharsha. A few comments below. clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java (lines 52 - 53) <https://reviews.apache.org/r/33620/#comment141700> I read some java doc but still not quite understand how are those two parameteres going to be used. Can you elaborate the usage of that a little bit? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 40) <https://reviews.apache.org/r/33620/#comment141672> Nit pick, keyStore and trustStore? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 111) <https://reviews.apache.org/r/33620/#comment141673> We already have a default algorithm set in config, do we still need to set default here? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (lines 135 - 138) <https://reviews.apache.org/r/33620/#comment141674> Should we mention in the doc that if NeedClientAuth is set, it will be used, otherwise WantClientAuth is used. So if user knows what they get when set both to be true. clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 138) <https://reviews.apache.org/r/33620/#comment141686> setWantClientAuth? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (lines 140 - 143) <https://reviews.apache.org/r/33620/#comment141685> Any reason why we only set the endpoint identification algorithm on client side? Also, it looks the SSLParameters actually contains: enabledCipherSuites, enabledProtocols, need clientAuth, wantClientAuth, can we just use one SSLParameter instead of calling different methods? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 156) <https://reviews.apache.org/r/33620/#comment141668> Can we put these checks in checkKeyStoreConfigs()? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 167) <https://reviews.apache.org/r/33620/#comment141671> ditto above. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java (lines 75 - 78) <https://reviews.apache.org/r/33620/#comment141703> Can they be replaced by clear() method? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java (line 101) <https://reviews.apache.org/r/33620/#comment141706> It seems we just removed the OP_CONNECT operation. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java (lines 127 - 139) <https://reviews.apache.org/r/33620/#comment141794> Not sure if it matters, but JSSE reference guide suggested to have a loop checking handshake state when shutdown. - Jiangjie Qin On June 23, 2015, 8:18 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated June 23, 2015, 8:18 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1690 > https://issues.apache.org/jira/browse/KAFKA-1690 > > > Repository: kafka > > > Description > ------- > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. SSLFactory tests. > > > KAFKA-1690. new java producer needs ssl support as a client. Added > PrincipalBuilder. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Fixed minor > issues with the patch. > > > KAFKA-1690. new java producer needs ssl support as a client. Fixed minor > issues with the patch. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > Merge remote-tracking branch 'refs/remotes/origin/trunk' into KAFKA-1690-V1 > > > KAFKA-1690. Broker side ssl changes. > > > KAFKA-1684. SSL for socketServer. > > > KAFKA-1690. Added SSLProducerSendTest and fixes to get right port for SSL. > > > Merge branch 'trunk' into KAFKA-1690-V1 > > > KAFKA-1690. Post merge fixes. > > > KAFKA-1690. Added SSLProducerSendTest. > > > KAFKA-1690. Minor fixes based on patch review comments. > > > Diffs > ----- > > build.gradle 30d1cf2f1ff9ed3f86a060da8099bb0774b4cf91 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > 2c421f42ed3fc5d61cf9c87a7eaa7bb23e26f63b > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > 48fe7961e2215372d8033ece4af739ea06c6457b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > daff34db5bf2144e9dc274b23dc56b88f4efafdc > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > 951c34c92710fc4b38d656e99d2a41255c60aeb7 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 5a37580ec69af08b97cf5b43b241790ba8c129dd > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > aa264202f2724907924985a5ecbe74afc4c6c04b > clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java > bae528d31516679bed88ee61b408f209f185a8cc > clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Authenticator.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java > df0e6d5105ca97b7e1cb4d334ffb7b443506bd0b > clients/src/main/java/org/apache/kafka/common/network/Channel.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/ChannelBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java > 3ca0098b8ec8cfdf81158465b2d40afc47eb6f80 > > clients/src/main/java/org/apache/kafka/common/network/PlainTextChannelBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Selectable.java > 618a0fa53848ae6befea7eba39c2f3285b734494 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 4aee214b24fd990be003adc36d675f015bf22fe6 > clients/src/main/java/org/apache/kafka/common/network/Send.java > 8f6daadf6b67c3414911cda77765512131e56fd3 > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java > dab1a94dd29563688b6ecf4eeb0e180b06049d3f > > clients/src/main/java/org/apache/kafka/common/security/auth/DefaultPrincipalBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > f73eedb030987f018d8446bb1dcd98d19fa97331 > clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java > 13ce519f03d13db041e1f2dbcd6b59414d2775b8 > > clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java > f3f8334f848be4cc043d5a573975609a3681fe7e > clients/src/test/java/org/apache/kafka/common/network/EchoServer.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > 158f9829ff64a969008f699e40c51e918287859e > clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java > 2ebe3c21f611dc133a2dbb8c7dfb0845f8c21498 > clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION > core/src/main/scala/kafka/network/SocketServer.scala > 91319fa010b140cca632e5fa8050509bd2295fc9 > core/src/main/scala/kafka/server/KafkaConfig.scala > c1f0ccad4900d74e41936fae4c6c2235fe9314fe > core/src/main/scala/kafka/server/KafkaServer.scala > 52dc728bb1ab4b05e94dc528da1006040e2f28c9 > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 > core/src/test/scala/integration/kafka/api/SSLProducerSendTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala > df5c6ba20f01e497ce896af790cbab40369f1776 > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala > e4bf2df48dd59a251b646b7f96d63ec4b924fc0b > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 7dc2fad542ea553ee888543dd215eb41ea57d509 > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 98a5b042a710d3c1064b0379db1d152efc9eabee > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 17e9fe4c159a29033fe9a287db6ced2fdc3fa9d1 > > Diff: https://reviews.apache.org/r/33620/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
