Dan Burkert has posted comments on this change. Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: PS5, Line 56: client_nego > rename arg (also below) Done PS5, Line 175: NotAuthorized(" > InvalidArgument doesn't seem quite right, since really it's an error with t Done Line 188: RequestHeader header; > can you add a TRACE here? Done Line 202: RETURN_NOT_OK(ReceiveFramedMessageBlocking(socket(), buffer, &header, ¶m_buf, deadline_)); > and maybe a TRACE here that the negotiate response was received? Done Line 227: uint8_t buf[buflen]; > does this guarantee it sent the whole buffer? it seems not, given it has th Yes, from the docs (and I checked the impl as well): // Blocking Write call, returns IOError unless full buffer is sent. Line 237: unsigned secflags = 0; > I think the old code which saved the status and converted to RuntimeError w Changed it to RETURN_NOT_OK_PREPEND. PS5, Line 267: ation::HandleNe > same as comment elsewhere Done Line 277: RpcFeatureFlag feature_flag = RpcFeatureFlag_IsValid(flag) ? > I think the intention of having the separate kSupportedClientFeatureFlags h The issue is that the set of supported feature flags is no longer statically known, so it's not sufficient to check against the static supported set. In particular the TLS flag is only 'supported' on the server side if the TLS context field is set (e.g. there is an available TLS cert). I don't see the issue of having the 'remote_feature_flags' set contain feature flags which are parseable but not supported - what issues could this cause? http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: Line 47: // Creates a new client negotiation instance, taking ownership of the > this line doesn't seem right anymore Done Line 51: explicit ClientNegotiation(std::unique_ptr<Socket> socket); > I think Init() is gone now? Done Line 85: void set_remote_addr(const Sockaddr& addr); > same Done http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: PS5, Line 238: kSaslAppName > we should check with Impala folks whether this will cause a problem. I'm no It was only 1/2 plumbed through; I don't think you could set it in practice because the messenger didn't expose the correct hook. Impala isn't currently setting it. If impala needs it to be configurable, we can go back and add it back in, but right now it makes it considerably simpler to just use the constant (no per-negotiatiator field, less method params) -- To view, visit http://gerrit.cloudera.org:8080/5760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
