You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/04 22:43:19 UTC
[kudu-CR] rpc: move connection header send/receive into negotiation.cc
Hello Dan Burkert, Sailesh Mukil, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4959
to review the following change.
Change subject: rpc: move connection header send/receive into negotiation.cc
......................................................................
rpc: move connection header send/receive into negotiation.cc
These methods were being called as part of the SASL negotiation code,
but really have nothing to do with SASL. This patch refactors them into
negotiation.cc.
This is some clean-up in preparation for making SSL support negotiated
during connection establishment ("StartTLS" style) rather than
pre-determined as on or off prior to creating the connection.
This doesn't affect the wire protocol at all: I verified this by
connecting to an existing cluster from a patched client.
Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
5 files changed, 24 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/4959/1
--
To view, visit http://gerrit.cloudera.org:8080/4959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] rpc: move connection header send/receive into negotiation.cc
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: move connection header send/receive into negotiation.cc
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4959/1/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
PS1, Line 228: RETURN_NOT_OK(conn->InitSaslServer());
> I missed this in my patch, but I think it would be good to make both the SA
I think we're planning to wrap TLS around SASL, so the current order makes sense from that perspective.
--
To view, visit http://gerrit.cloudera.org:8080/4959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] rpc: move connection header send/receive into negotiation.cc
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: rpc: move connection header send/receive into negotiation.cc
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[kudu-CR] rpc: move connection header send/receive into negotiation.cc
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: rpc: move connection header send/receive into negotiation.cc
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4959/1//COMMIT_MSG
Commit Message:
PS1, Line 13: preparation for making SSL support negotiated
: during connection establishment
If this is the case, should the Send/ReceiveHeaderMessage() functions be called before InitSSL..() in this patch? Or would doing it later be fine?
http://gerrit.cloudera.org:8080/#/c/4959/1/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
Line 75: const uint8_t buflen = kMagicNumberLength + kHeaderFlagsLength;
DCHECK_EQ(conn->direction(), CLIENT) ?
Line 83: TRACE("Waiting for connection header");
DCHECK(conn->direction(), SERVER)
PS1, Line 228: RETURN_NOT_OK(conn->InitSaslServer());
I missed this in my patch, but I think it would be good to make both the SASL inits happen before the InitSSL..(). Just for readability purposes. Functionally I don't think it matters.
--
To view, visit http://gerrit.cloudera.org:8080/4959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] rpc: move connection header send/receive into negotiation.cc
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change.
Change subject: rpc: move connection header send/receive into negotiation.cc
......................................................................
Abandoned
Dan's patch series obviates this
--
To view, visit http://gerrit.cloudera.org:8080/4959
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I78a9b592b8cc139cc6db50cf9d0b48dc272d779a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>