Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17189 )
Change subject: KUDU-2871 support TLSv1.3 in Kudu RPC (C++ part) ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/messenger.cc@148 PS2, Line 148: nal-facing > nit: can drop the std::s here and elsewhere I posted a separate patch to address these nits in messenger.{h,cc} http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/server_negotiation.cc@610 PS2, Line 610: // Check that the handshake step didn't produce an error. It also propagates : // any non-OK status. : RETURN_NOT_OK(s); > Wouldn't this return early if 's' is Incomplete in the needs_extra_step cas It's designed to return early from here if 's' is non-OK, both for Status::Incomplete() and other non-OK cases. At line 607 we just send back the information produced by TLS handshake, but the essence of the HandleTlsHandshake() method is to convey status from TlsHandshake::Continue() to the upper level. http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.h File src/kudu/security/tls_context.h: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.h@181 PS2, Line 181: // The cipher suite preferences to use for RPC connections secured with : // pre-TLSv1.3 protocols. Uses the OpenSSL cipher preference list format. > nit: I suppose it's implied from the below comment, but maybe also mention Done http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.cc@241 PS2, Line 241: > nit: add space Done http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc File src/kudu/security/tls_handshake-test.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@327 PS2, Line 327: // The client has a self-signed certificate, so the server has nothing to > nit: mind adding a comment whether/why this is important for this test? Done http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@358 PS2, Line 358: > nit: very Done http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@375 PS2, Line 375: a > nit: extra space Done -- To view, visit http://gerrit.cloudera.org:8080/17189 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia92a4d102c3c8cff76101e71ff71d24a9d78b672 Gerrit-Change-Number: 17189 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Mar 2021 03:33:46 +0000 Gerrit-HasComments: Yes
