Henry Robinson has posted comments on this change. Change subject: IMPALA-5743: Support TLS version configuration for Thrift servers ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: PS1, Line 257: TEST(SslTest, StringToProtocol) { > Please add a brief description explaining what this test does, especially b Not yet, but I'll run tests on CentOS 6 once the toolchain changes have been published. PS1, Line 341: // AES256 is v1.2+ only. > Do we know if thrift bubbles up sensible errors for cipher-SSL version inco Do you mean if the cipher is incompatible with the TLS version requested? In that case yes - it says that no matching cipher is found, which is not perfect but a good start. http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: PS1, Line 31: #include "rpc/auth-provider.h" > We can change this to a forward declare right? Done PS1, Line 32: #include "util/metrics.h" > Same as above This one is a bit harder because of the templated definitions that are trickier to forward-declare. Line 165: /// is used only for password-protected .PEM files. > Should have caught this in the other review, but please add a comment for t Done http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS1, Line 181: Supported versions are " : #if OPENSSL_VERSION_NUMBER >= 0x1000100L : "TLSv1.0, TLSv1.1 and TLSv1.2"); : #else : "TLSv1.0"); : #endif > Should we also mention what the strings representing these different versio These are the strings that represent those versions :) -- To view, visit http://gerrit.cloudera.org:8080/7606 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
