-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review83322
-----------------------------------------------------------
Thanks for the patch. A few comments below. The following unit tests seem to
fail.
org.apache.kafka.common.network.SSLFactoryTest > testSSLFactoryConfiguration
FAILED
java.lang.IllegalArgumentException: TLSv1.2
at
com.sun.net.ssl.internal.ssl.ProtocolVersion.valueOf(ProtocolVersion.java:133)
at
com.sun.net.ssl.internal.ssl.ProtocolList.<init>(ProtocolList.java:38)
at
com.sun.net.ssl.internal.ssl.SSLEngineImpl.setEnabledProtocols(SSLEngineImpl.java:1821)
at
org.apache.kafka.common.network.SSLFactory.createSSLEngine(SSLFactory.java:123)
at
org.apache.kafka.common.network.SSLFactoryTest.testSSLFactoryConfiguration(SSLFactoryTest.java:39)
org.apache.kafka.common.network.SSLFactoryTest > testClientMode FAILED
java.lang.IllegalArgumentException: TLSv1.2
at
com.sun.net.ssl.internal.ssl.ProtocolVersion.valueOf(ProtocolVersion.java:133)
at
com.sun.net.ssl.internal.ssl.ProtocolList.<init>(ProtocolList.java:38)
at
com.sun.net.ssl.internal.ssl.SSLEngineImpl.setEnabledProtocols(SSLEngineImpl.java:1821)
at
org.apache.kafka.common.network.SSLFactory.createSSLEngine(SSLFactory.java:123)
at
org.apache.kafka.common.network.SSLFactoryTest.testClientMode(SSLFactoryTest.java:52)
org.apache.kafka.common.network.SSLSelectorTest > testServerDisconnect FAILED
java.lang.NullPointerException
at org.apache.kafka.common.network.Selector.connect(Selector.java:167)
at
org.apache.kafka.common.network.SSLSelectorTest.blockingConnect(SSLSelectorTest.java:128)
at
org.apache.kafka.common.network.SSLSelectorTest.testServerDisconnect(SSLSelectorTest.java:77)
org.apache.kafka.common.network.SSLSelectorTest > testSendLargeRequest FAILED
java.lang.NullPointerException
at org.apache.kafka.common.network.Selector.connect(Selector.java:167)
at
org.apache.kafka.common.network.SSLSelectorTest.blockingConnect(SSLSelectorTest.java:128)
at
org.apache.kafka.common.network.SSLSelectorTest.testSendLargeRequest(SSLSelectorTest.java:64)
org.apache.kafka.common.network.SSLSelectorTest > testLargeMessageSequence
FAILED
java.lang.NullPointerException
at org.apache.kafka.common.network.Selector.connect(Selector.java:167)
at
org.apache.kafka.common.network.SSLSelectorTest.testLargeMessageSequence(SSLSelectorTest.java:101)
build.gradle
<https://reviews.apache.org/r/33620/#comment134268>
This seems to be just a test dependency?
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
<https://reviews.apache.org/r/33620/#comment134299>
Are these two actually used?
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
<https://reviews.apache.org/r/33620/#comment134301>
Those properties will be used by broker config as well, right? Should we
put them in a separate SslConfig so that it can be reused in both client and
server?
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
<https://reviews.apache.org/r/33620/#comment134304>
Typos:
prover -> provide
releated -> related
client need -> client needs
clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java
<https://reviews.apache.org/r/33620/#comment134308>
It seems that the provider is never set.
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134506>
We need to deal with other statuses, right?
BUFFER_OVERFLOW: We need to expand the netOutBuffer
BUFFER_UNDERFLOW: Throw an IllegalStateException since this shouldn't
happen.
CLOSED: Throw an EOFException.
clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java
<https://reviews.apache.org/r/33620/#comment134309>
Does that bind the port? If so, we will need to select a random port.
checkstyle/import-control.xml
<https://reviews.apache.org/r/33620/#comment134605>
Indentation.
checkstyle/import-control.xml
<https://reviews.apache.org/r/33620/#comment134606>
Why would network depend on clients?
checkstyle/import-control.xml
<https://reviews.apache.org/r/33620/#comment134607>
Indentation.
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
<https://reviews.apache.org/r/33620/#comment134608>
This is also optional for the client, right?
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java
<https://reviews.apache.org/r/33620/#comment134611>
CSV format expects no space after comma.
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java
<https://reviews.apache.org/r/33620/#comment134612>
Should we give those fields a default? The user only needs to configure
them if ssl is enabled.
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java
<https://reviews.apache.org/r/33620/#comment134614>
Same here. Should we give those fields a default?
clients/src/main/java/org/apache/kafka/common/network/Authenticator.java
<https://reviews.apache.org/r/33620/#comment134620>
Could you add a comment on the input parameters?
clients/src/main/java/org/apache/kafka/common/network/Channel.java
<https://reviews.apache.org/r/33620/#comment134622>
This is a general question. For ssl and sasl, does authentication only
happen at connection time for ssl and sasl? What happens when a
certificate/credential is removed? Do we have to re-authenticate on the
existing connection?
clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java
<https://reviews.apache.org/r/33620/#comment134541>
The test should be principal == null, right? If we cache the principal
here, does it work with renegotiation?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134520>
unused
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134525>
Do we need the while loop to wait until the netInBuffer is drained? It
seems that within the loop, we can return before netInBuffer is drained. So
read() already needs to work when netInBuffer is not drained.
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134546>
So, we want to support rehandshake? If so, shouldn't we handle other
handshakeStatus too? Could we reuse the logic in handshake()? In general,
should we deal with the handshakeStatus before the status or the reverse?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134518>
than -> then
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
<https://reviews.apache.org/r/33620/#comment134519>
Do we need the try clause? Could we just do compact before returning?
clients/src/main/java/org/apache/kafka/common/network/Selector.java
<https://reviews.apache.org/r/33620/#comment134540>
These are details about the channel implementation. Instead of putting them
directly in Selector, I am wondering if it's better to have a ChannelBuilder
interface for instantiating new channels. Then we can put the ssl details in a
SslChannelBuilder.
clients/src/main/java/org/apache/kafka/common/network/Selector.java
<https://reviews.apache.org/r/33620/#comment134545>
I am a bit confused how this works during the handshake. During the
handshake, we will need to read and write data from the socket. However, the
read/write part will be routed to connect() and then
SSLTransportLayer.handshake(), not SSLTransportLayer.read/write. So how would
the handshake complete?
clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java
<https://reviews.apache.org/r/33620/#comment134624>
This seems to be ssl specific. Is this needed in the interface? It doesn't
seem to be used.
clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java
<https://reviews.apache.org/r/33620/#comment134625>
We probably need to make this interface configurable and closable. Also, we
will need to expose this in the client config.
clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java
<https://reviews.apache.org/r/33620/#comment134547>
This seems redundant.
- Jun Rao
On May 12, 2015, 11:20 p.m., Sriharsha Chintalapani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33620/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 11:20 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.
>
>
> Diffs
> -----
>
> build.gradle fef515b3b2276b1f861e7cc2e33e74c3ce5e405b
> checkstyle/checkstyle.xml a215ff36e9252879f1e0be5a86fef9a875bb8f38
> 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
> cf32e4e7c40738fe6d8adc36ae0cfad459ac5b0b
> clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java
> bdff518b732105823058e6182f445248b45dc388
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
> d301be4709f7b112e1f3a39f3c04cfa65f00fa60
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
> 8e336a3aa96c73f52beaeb56b931baf4b026cf21
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
> 187d0004c8c46b6664ddaffecc6166d4b47351e5
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
> c4fa058692f50abb4f47bd344119d805c60123f5
> clients/src/main/java/org/apache/kafka/common/network/Authenticator.java
> PRE-CREATION
> clients/src/main/java/org/apache/kafka/common/network/Channel.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/PlainTextTransportLayer.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
> b5f8d83e89f9026dc0853e5f92c00b2d7f043e22
> clients/src/main/java/org/apache/kafka/common/network/Selector.java
> 57de0585e5e9a53eb9dcd99cac1ab3eb2086a302
> 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/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
> d5b306b026e788b4e5479f3419805aa49ae889f3
> clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
> 2ebe3c21f611dc133a2dbb8c7dfb0845f8c21498
> clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33620/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sriharsha Chintalapani
>
>
