You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/01/21 04:47:58 UTC
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Hello Adar Dembo, Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5760
to review the following change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,008 insertions(+), 1,159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/1
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 5:
(12 comments)
Seems like there is some kind of openssl-related leak as well:
Direct leak of 4848 byte(s) in 6 object(s) allocated from:
#0 0x53ca06 in __interceptor_malloc /home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
#1 0x7f45da9c5d72 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fd72)
Indirect leak of 38284 byte(s) in 578 object(s) allocated from:
#0 0x53ca06 in __interceptor_malloc /home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
#1 0x7f45da9c5d72 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fd72)
Indirect leak of 4608 byte(s) in 12 object(s) allocated from:
#0 0x53cdbd in realloc /home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:77
#1 0x7f45da9c5e28 in CRYPTO_realloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fe28)
SUMMARY: AddressSanitizer: 47740 byte(s) leaked in 596 allocation(s).
unfortunately the stack trace is truncated because openssl doesn't have frame pointers/debuginfo I guess?
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
PS5, Line 56: sasl_client
rename arg (also below)
PS5, Line 175: InvalidArgument
InvalidArgument doesn't seem quite right, since really it's an error with the other side, not an argument set by the caller
Line 188: RequestHeader header;
can you add a TRACE here?
Line 202: RETURN_NOT_OK(helper_.CheckSaslCallId(header.call_id()));
and maybe a TRACE here that the negotiate response was received?
Line 227: return socket()->BlockingWrite(buf, buflen, &nsent, deadline_);
does this guarantee it sent the whole buffer? it seems not, given it has the &nsent parameter. Maybe need something that loops to try to send all?
Line 237: RETURN_NOT_OK(WrapSaslCall(nullptr /* no conn */, [&]() {
I think the old code which saved the status and converted to RuntimeError was on purpose, to make sure the error type was correct? At least we should keep prepending 'unable to create new SASL client' so it's clear where the error came from (SASL error messages are typically inscrutable on their own so the extra context is helpful even if verbose)
PS5, Line 267: InvalidArgument
same as comment elsewhere
Line 277: if (feature_flag != UNKNOWN) {
I think the intention of having the separate kSupportedClientFeatureFlags here was that it allows for us to keep a flag in the .proto even if it stops being supported by either client or server in a future version. i.e we decouple the set of supported client flags from supported server flags.
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.h
File src/kudu/rpc/client_negotiation.h:
Line 47: // Does not take ownership of the socket indicated by the fd.
this line doesn't seem right anymore
Line 51: // Must be called after Init().
I think Init() is gone now?
Line 85: // Must be called before Init(). Required for some mechanisms.
same
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 not 100% sure what the "app name" ends up used for, but maybe they'll need a programattic way to set this to Impala rather than having to modify the code? though maybe it doesnt matter
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#4).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,006 insertions(+), 1,166 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/4
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 10:
(13 comments)
Overall looks great, just a few nits. Probably, I'll do another pass after digesting the information bit more.
I also have some a question about memory management in the code of ssl_socket.cc (not the subject of this change, though).
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS10, Line 159: take_socket
nit: release_socket ?
PS10, Line 163: set_socket
nit: adopt_socket ?
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/negotiation.h
File src/kudu/rpc/negotiation.h:
PS10, Line 25: class Socket;
Why is it necessary in here?
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
PS10, Line 345: std::unique_ptr<Socket> new_socket;
: if (reactor()->messenger()->ssl_enabled()) {
: new_socket = reactor()->messenger()->ssl_factory()->CreateSocket(sock.Release(), false);
: } else {
: new_socket.reset(new Socket(sock.Release()));
: }
:
: // Register the new connection in our map.
: *conn = new Connection(this, conn_id.remote(), std::move(new_socket), Connection::CLIENT);
:
nit: consider adding an embedded scope for this to signify that new_socket variable is no longer available for usage after std::move(new_socket) was called.
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/sasl_helper.h
File src/kudu/rpc/sasl_helper.h:
PS10, Line 30: namespace google {
: namespace protobuf {
: class MessageLite;
: } // namespace protobuf
: } // namespace google
nit: is this needed?
PS10, Line 38: class MonoTime;
nit: is this needed?
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS10, Line 20: #include <sasl/sasl.h>
nit: could you add a comment about the reason of putting this before the block of the system headers?
PS10, Line 56: *
nit: since the asterisk disposition for other pointer-like parameters has been updated for some parameters, consider doing so for the rest.
PS10, Line 61: *
ditto
PS10, Line 453: E
nit: I'm not sure whether we have already adopted the idea of starting those messages from non-capital letters for 'easy log chaining', but if so, consider replacing this lower-case letters.
You could talk to Todd about this. At least, in one of his reviews he asked me to change the messages. I think the convention is originated from the Golang code style guide.
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:
PS10, Line 101: take_
nit: consider renaming into 'release_socket'
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/security/ssl_socket.cc
File src/kudu/security/ssl_socket.cc:
PS10, Line 126: (*nwritten < frame_size)
Is it possible for in-the-middle Write() to return an error even if *nwritten == frame_size?
PS10, Line 167: SSL_free(ssl_);
Is it possible that an object of this type is destructed without prior call of the Close() method?
If yes, would it make sense to de-allocate ssl_ in the destructor?
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#12).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ssl_socket.cc
M src/kudu/security/ssl_socket.h
19 files changed, 1,057 insertions(+), 1,229 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/12
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 13: Code-Review+2
--
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: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
Line 196: void ReactorThread::AsyncHandler(ev::async& /*watcher*/, int /*revents*/) {
> warning: parameter 'watcher' is unused [misc-unused-parameters]
Done
Line 196: void ReactorThread::AsyncHandler(ev::async& /*watcher*/, int /*revents*/) {
> warning: parameter 'revents' is unused [misc-unused-parameters]
Done
Line 247: void ReactorThread::TimerHandler(ev::timer& /*watcher*/, int revents) {
> warning: parameter 'watcher' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:
Line 97: void Run(ReactorThread* thread) override;
> warning: function 'kudu::rpc::DelayedTask::Run' has a definition with diffe
Done
--
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: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#10).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ssl_socket.cc
M src/kudu/security/ssl_socket.h
19 files changed, 1,054 insertions(+), 1,213 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/10
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#3).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,004 insertions(+), 1,166 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/3
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Reviewed-on: http://gerrit.cloudera.org:8080/5760
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ssl_socket.cc
M src/kudu/security/ssl_socket.h
19 files changed, 1,057 insertions(+), 1,229 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#6).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,027 insertions(+), 1,189 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/6
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 277: RpcFeatureFlag feature_flag = RpcFeatureFlag_IsValid(flag) ?
> The issue is that the set of supported feature flags is no longer staticall
k, I see your point, thx.
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#2).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,006 insertions(+), 1,158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/2
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#7).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,033 insertions(+), 1,192 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/7
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 7:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5760/7/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
Line 464: when_(std::move(when)),
> warning: std::move of the variable of a trivially-copyable type has no effe
Done
Line 553: virtual void Run(ReactorThread* reactor) OVERRIDE {
> warning: parameter 'reactor' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/5760/7/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:
Line 97: virtual void Run(ReactorThread* reactor) OVERRIDE;
> warning: function 'kudu::rpc::DelayedTask::Run' has a definition with diffe
Done
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/5760/4/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
Line 29: #include <gflags/gflags.h>
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 72: remote_(std::move(remote)),
> warning: std::move of the variable of a trivially-copyable type has no effe
Done
http://gerrit.cloudera.org:8080/#/c/5760/4/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
Line 58: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
Line 59: using std::unique_ptr;
> warning: using decl 'unique_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/5760/4/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
Line 599: void Abort(const Status &status) override {
> warning: parameter 'status' is unused [misc-unused-parameters]
Done
http://gerrit.cloudera.org:8080/#/c/5760/4/src/kudu/rpc/sasl_helper.cc
File src/kudu/rpc/sasl_helper.cc:
Line 42: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/5760/4/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 23: #include <set>
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 264: // TODO: Support security flags.
> warning: missing username/bug in TODO [google-readability-todo]
Done
Line 418: } else if (s.IsIncomplete()) {
> warning: don't use else after return [readability-else-after-return]
Done
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#13).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ssl_socket.cc
M src/kudu/security/ssl_socket.h
19 files changed, 1,057 insertions(+), 1,229 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/13
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#8).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,033 insertions(+), 1,192 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/8
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#11).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ssl_socket.cc
M src/kudu/security/ssl_socket.h
M src/kudu/util/net/socket.cc
20 files changed, 1,059 insertions(+), 1,213 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/11
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5760
to look at the new patch set (#5).
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
TLS-negotiation [6/n]: Refactor RPC negotiation
The KRPC negotiation process now encompasses more than just SASL
authentication. Feature flags are negotiated, and soon TLS encryption
will be negotiated as well. In anticipation of TLS negotiation, this
commit refactors the SaslClient and SaslServer classes to better reflect
their role. Additionally, the Connection class now has less
responsibility and knowledge of negotiation. Instead, the existing logic
in negotiation.cc has been beefed up to serve as the glue between the
ClientNegotiation and ServerNegotiation classes (which have no knowledge
of Connection), and Connection (which now has no knowledge of
ClientNegotiation/ServerNegotiation). Hopefully this will lead to more
maintainable code in the long term. It is expected that this refactor
will make TLS-negotiation an easy add-on.
Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/negotiation.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_helper.cc
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
17 files changed, 1,005 insertions(+), 1,169 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/5760/5
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 10:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS10, Line 159: take_socket
> nit: release_socket ?
Done
PS10, Line 163: set_socket
> nit: adopt_socket ?
Done
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/negotiation.h
File src/kudu/rpc/negotiation.h:
PS10, Line 25: class Socket;
> Why is it necessary in here?
Done
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
PS10, Line 345: std::unique_ptr<Socket> new_socket;
: if (reactor()->messenger()->ssl_enabled()) {
: new_socket = reactor()->messenger()->ssl_factory()->CreateSocket(sock.Release(), false);
: } else {
: new_socket.reset(new Socket(sock.Release()));
: }
:
: // Register the new connection in our map.
: *conn = new Connection(this, conn_id.remote(), std::move(new_socket), Connection::CLIENT);
:
> nit: consider adding an embedded scope for this to signify that new_socket
This snippet is being significantly rewritten in another patch in the series, so I'm going to punt on this for now.
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/sasl_helper.h
File src/kudu/rpc/sasl_helper.h:
PS10, Line 30: namespace google {
: namespace protobuf {
: class MessageLite;
: } // namespace protobuf
: } // namespace google
> nit: is this needed?
Done
PS10, Line 38: class MonoTime;
> nit: is this needed?
Done
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS10, Line 20: #include <sasl/sasl.h>
> nit: could you add a comment about the reason of putting this before the bl
woops, that was a mistake.
PS10, Line 56: *
> nit: since the asterisk disposition for other pointer-like parameters has b
Done
PS10, Line 61: *
> ditto
Done
PS10, Line 453: E
> nit: I'm not sure whether we have already adopted the idea of starting thos
Changed to lowercase.
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:
PS10, Line 101: take_
> nit: consider renaming into 'release_socket'
Done
http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/security/ssl_socket.cc
File src/kudu/security/ssl_socket.cc:
PS10, Line 126: (*nwritten < frame_size)
> Is it possible for in-the-middle Write() to return an error even if *nwritt
I don't think so, and in case it does, I would expect the next loop through to break out with an error status.
PS10, Line 167: SSL_free(ssl_);
> Is it possible that an object of this type is destructed without prior call
I've recently updated it to call Close from the dtor.
--
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: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 13: Code-Review+2
--
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: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5760/12/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 25: #include <sasl/sasl.h>
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
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: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes