You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Johny Rufus John <jr...@cloudera.com> on 2015/01/27 02:54:26 UTC
Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-2574
https://issues.apache.org/jira/browse/FLUME-2574
Repository: flume-git
Description
-------
Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
Diff: https://reviews.apache.org/r/30301/diff/
Testing
-------
mvn clean install -Dhadoop.profile=hbase-98
Thanks,
Johny Rufus John
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review72441
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java
<https://reviews.apache.org/r/30301/#comment118496>
Again, this code can be way simpler no:
server = getTThreadSelectorServer()
if (server == null) server = getTThreadPoolServer();
getTThreadSelectorServer() {
if (enableSSL) return null;
...
}
getTThreadPoolServer() {
if (enableSSL) socket = getSSLSocker()
else socket = getNormalSocket()
...
}
- Hari Shreedharan
On Feb. 13, 2015, 8:30 p.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 8:30 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review73058
-----------------------------------------------------------
Ship it!
Ship It!
- Hari Shreedharan
On Feb. 13, 2015, 11:04 p.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 11:04 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Johny Rufus John <jr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 11:04 p.m.)
Review request for Flume.
Bugs: FLUME-2574
https://issues.apache.org/jira/browse/FLUME-2574
Repository: flume-git
Description
-------
Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
Diff: https://reviews.apache.org/r/30301/diff/
Testing
-------
mvn clean install -Dhadoop.profile=hbase-98
Thanks,
Johny Rufus John
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review72437
-----------------------------------------------------------
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment118493>
Is this caught and wrapped in a Flume exception somewhere. We should not be throwing Thrift exceptions, we should be throwing FlumeExceptions.
- Hari Shreedharan
On Feb. 13, 2015, 8:30 p.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 8:30 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Johny Rufus John <jr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 8:30 p.m.)
Review request for Flume.
Bugs: FLUME-2574
https://issues.apache.org/jira/browse/FLUME-2574
Repository: flume-git
Description
-------
Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
Diff: https://reviews.apache.org/r/30301/diff/
Testing
-------
mvn clean install -Dhadoop.profile=hbase-98
Thanks,
Johny Rufus John
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Johny Rufus John <jr...@cloudera.com>.
> On Feb. 13, 2015, 6:32 p.m., Hari Shreedharan wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 657
> > <https://reviews.apache.org/r/30301/diff/1-2/?file=835477#file835477line657>
> >
> > Why do you need to create a new socket? Wouldn't the old one just work, since you just changed the enabled protocols on that one.
The TFastFramedTransport needs a TSocket, hence the wrapper.
- Johny Rufus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review72407
-----------------------------------------------------------
On Feb. 13, 2015, 8:30 p.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 8:30 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review72407
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java
<https://reviews.apache.org/r/30301/#comment118465>
Why do you need this code to be so convoluted?
Wouldn't this work:
if(!enableSSL) server = getTThreadSelectorServer()
if (server == null) getThreadpoolServer()
I feel this expresses the intent much clearer here than the current code
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment118467>
Can you rename this method to something like createSSLSocket, to make it clear that this creates only SSL-enabled sockets and not regular ones?
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment118468>
Why do you need to create a new socket? Wouldn't the old one just work, since you just changed the enabled protocols on that one.
- Hari Shreedharan
On Feb. 13, 2015, 6:02 p.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 6:02 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Johny Rufus John <jr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 6:02 p.m.)
Review request for Flume.
Bugs: FLUME-2574
https://issues.apache.org/jira/browse/FLUME-2574
Repository: flume-git
Description
-------
Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
Diff: https://reviews.apache.org/r/30301/diff/
Testing
-------
mvn clean install -Dhadoop.profile=hbase-98
Thanks,
Johny Rufus John
Re: Review Request 30301: FLUME-2574 SSL Support for Thrift Rpc
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30301/#review70457
-----------------------------------------------------------
This generally looks good. I posted some comments through the code. Once those are fixed, I will review the tests and we can take it from there.
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java
<https://reviews.apache.org/r/30301/#comment115657>
This whole thing should happen inside if(enableSSL). There is no point trying to read conf values that would never be used if ssl is false.
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java
<https://reviews.apache.org/r/30301/#comment115658>
Is this in seconds or millis? Also, why was 10000 chosen? If it is in millis we should probably bump it up.
flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java
<https://reviews.apache.org/r/30301/#comment115659>
This code is the same as the one in the catch block below. This should be abstracted out into a method which takes the serverTransport instance as a parameter
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115660>
Same as above. These lookups need to happen only if ssl is enabled.
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115661>
Taken or Lifted? ;-)
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115663>
Again, we should have a higher timeout
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115664>
if(..) {
..
}
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115667>
This should be FlumeException not a TTransportException
flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/30301/#comment115669>
This entire class seems like overkill. Since we are creating the socket from the SSLSocketFactory ourselves and then passing it on to thrift, why not simply create the socket and then exclude the protocols and then pass the socket to the TSocket constructor?
- Hari Shreedharan
On Jan. 27, 2015, 1:54 a.m., Johny Rufus John wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30301/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 1:54 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2574
> https://issues.apache.org/jira/browse/FLUME-2574
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Current Thrift Source/Sink does not have support for SSL. Similar to Avro we should start supporting SSL for Thrift based communication
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java 551fe13
> flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java fccaede
> flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java 357965f
> flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION
> flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java 6382a0e
> flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java 63d2fc3
>
> Diff: https://reviews.apache.org/r/30301/diff/
>
>
> Testing
> -------
>
> mvn clean install -Dhadoop.profile=hbase-98
>
>
> Thanks,
>
> Johny Rufus John
>
>