Sailesh Mukil 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 between the different versions. Also, I know it's tedious, but were you able to test it with OpenSSL versions > 1.1 and < 1.1 ? PS1, Line 341: // AES256 is v1.2+ only. Do we know if thrift bubbles up sensible errors for cipher-SSL version incompatibility? 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? PS1, Line 32: #include "util/metrics.h" Same as above Line 165: /// is used only for password-protected .PEM files. Should have caught this in the other review, but please add a comment for the 'ciphers' argument too. 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 versions are? Or do we expect users to find that from the documentation? -- 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: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
