Dan Burkert has posted comments on this change. Change subject: TLS-negotiation [8/n]: TLS negotiation ......................................................................
Patch Set 6: (9 comments) Not ready to review just yet http://gerrit.cloudera.org:8080/#/c/5762/5//COMMIT_MSG Commit Message: Line 9: This commit adds TLS negotiation to the KRPC protocol, and removes the > can you document this in rpc.md? Done http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: Line 338: NegotiatePB msg; > I suppose it could but I don't see it has a potential perf issue (compared I went ahead and implemented the TODO since it was just a few method call changes. PS5, Line 345: ation::HandleTl > would prefer NetworkError or something here (same below in a few places) Changed to NotAuthorized to match up with other invalid message error cases. http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: PS5, Line 93: anisms. > borrowed makes it sound like it's temporarily taken exclusive ownership of, Done http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/constants.cc File src/kudu/rpc/constants.cc: Line 28: set<RpcFeatureFlag> kSupportedServerRpcFeatureFlags = { APPLICATION_FEATURE_FLAGS }; > Add a comment why TLS isn't listed on the server too? Done http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: PS5, Line 305: tls_context_.reset(new security::TlsContext()); : RETURN_NOT_OK(tls_context_->Init()); > I think it's worth a comment saying something like "enable TLS unconditiona Done http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 135: if (tls_context_ && ContainsKey(client_features_, TLS)) { > also worth a TODO here about allowing the server to specify TLS-required? Done PS5, Line 365: est.step() != N > same as elsewhere Done http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.h File src/kudu/rpc/server_negotiation.h: Line 95: // Must be called before Negotiate(). Required for some mechanisms. > same comment as the equivalent in client_negotation Done -- To view, visit http://gerrit.cloudera.org:8080/5762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
