[kudu-CR] [master] CA management: added SysTable base class

[Alexey Serbin (Code Review)]

Alexey Serbin has abandoned this change.

Change subject: [master] CA management: added SysTable base class
..


Abandoned

Abandoned in favor of https://gerrit.cloudera.org/#/c/5793/

-- 
To view, visit http://gerrit.cloudera.org:8080/5063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I00ccb45cd01cc63b044e5ffe4b3e194ae68cdb66
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] store CA information in the system table

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5793

Change subject: WIP [master] store CA information in the system table
..

WIP [master] store CA information in the system table

Use CatalogManager::SetCAInfo() to set CA private key and certificate.

TBD: unit tests

Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 436 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5761

to look at the new patch set (#9).

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..

TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS 
handshakes

The new TLS negotiation protocol calls for tunneling the TLS handshake
through negotiation protobufs. The existing SSL socket helper classes
were not built with this in mind. This commit introduces new helpers,
largely based off the existing versions. A followup commit will switch
KRPC to use the new classes and remove the old.

Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/tls_context.cc
A src/kudu/security/tls_context.h
A src/kudu/security/tls_handshake-test.cc
A src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_handshake.h
A src/kudu/security/tls_socket.cc
A src/kudu/security/tls_socket.h
10 files changed, 713 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/5761/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5762

to look at the new patch set (#9).

Change subject: TLS-negotiation [8/n]: TLS negotiation
..

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
17 files changed, 368 insertions(+), 586 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5762/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
> 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5761

to look at the new patch set (#10).

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..

TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS 
handshakes

The new TLS negotiation protocol calls for tunneling the TLS handshake
through negotiation protobufs. The existing SSL socket helper classes
were not built with this in mind. This commit introduces new helpers,
largely based off the existing versions. A followup commit will switch
KRPC to use the new classes and remove the old.

Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/security-test-util.h
A src/kudu/security/tls_context.cc
A src/kudu/security/tls_context.h
A src/kudu/security/tls_handshake-test.cc
A src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_handshake.h
A src/kudu/security/tls_socket.cc
A src/kudu/security/tls_socket.h
12 files changed, 813 insertions(+), 74 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/5761/10
-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: KUDU-1848. in-memory dictionary for binary columns

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: WIP: KUDU-1848. in-memory dictionary for binary columns
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5780/1/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:

PS1, Line 119: arraysize(slice_dict_)
Can this just be kSliceDictSize?


PS1, Line 154: hash & (DICT_SIZE - 1)
This works correctly only if DICT_SIZE is a power of 2, right? Could you add a 
quick comment or check that DICT_SIZE is a power of two, just in case?


PS1, Line 176: //   <24 bits length>
 :   //   <48 bits pointer>
Isn't 24 + 48 = 72, so there's 1 too many bytes to store in 64 bits?


PS1, Line 190: kMaxSize = (1 << 24) - 1;
If my comment above isn't mistaken, max size should be (1 << 16) - 1


-- 
To view, visit http://gerrit.cloudera.org:8080/5780
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic03ef0473383b1dcf22ebefee029ff29d2dae813
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] security: initial work on token creation and verification

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/5796

to review the following change.

Change subject: security: initial work on token creation and verification
..

security: initial work on token creation and verification

This adds classes for managing Token Signing Keys (TSKs) on the
signer (masters) and verifiers (all servers). The new classes aren't
hooked up with the actual servers as of yet, nor do they actually use
real signature routines (blocked on another in-flight patch for that).

A unit test is included which drives the APIs using a stubbed-out
"signature" algorithm.

Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/token_signer.cc
A src/kudu/security/token_signer.h
A src/kudu/security/token_signing_key.cc
A src/kudu/security/token_signing_key.h
A src/kudu/security/token_verifier.cc
A src/kudu/security/token_verifier.h
7 files changed, 612 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5796/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

Line 318: const char *GenericCalculatorService::kSecondString =
> warning: variable 'kSecondString' defined in a header file; variable defini
ignoring...


http://gerrit.cloudera.org:8080/#/c/5761/8/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

Line 110:   if (!handshake->ssl_) {
> warning: redundant get() call on smart pointer [google-readability-redundan
Done


http://gerrit.cloudera.org:8080/#/c/5761/7/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

Line 20: #include 
> warning: #includes are not sorted properly [llvm-include-order]
Done


Line 76: 
> warning: don't use else after return [readability-else-after-return]
Done


Line 116:   return Status::OK();
> warning: don't use else after return [readability-else-after-return]
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Alexey Serbin (Code Review)]

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 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 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 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 
> 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1853. Tablet copy: Don't orphan blocks on failure

[Mike Percy (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5799

to look at the new patch set (#2).

Change subject: KUDU-1853. Tablet copy: Don't orphan blocks on failure
..

KUDU-1853. Tablet copy: Don't orphan blocks on failure

Previously, if a tablet copy failed we would orphan data blocks. This
patch makes it so that a failed tablet copy operation that does not
involve a process crash does not orphan data blocks.

Change-Id: Ifbc0720d460de8d912a241a5186ef6e8f524b830
---
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
3 files changed, 101 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5799/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifbc0720d460de8d912a241a5186ef6e8f524b830
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: KUDU-1713: add a client Partitioner API

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-1713: add a client Partitioner API
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5775/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2209:   /// @param [out] partition
> I think it would be nice for this to be a KuduTablet instead. Then each par
I was thinking of adding a separate API which is 'GetPartitionInfo()' which 
returns a KuduTablet or something of that sort. I left that out of the initial 
version because Impala doesn't need it for now, and APIs are forever. I don't 
want to speculate about what consumers need until there's a consumer.

The idea is that knowing up front the fixed number of tablets and returning 
ints makes it much easier for the consumer to be high performance about this 
(eg they can just index the result into an array if they're making buffers to 
send elsewhere, etc). Certainly they could build their own map 
but then they end up with more expensive hash lookups vs array lookups.


Line 2216:   Status PartitionRow(const KuduPartialRow& row, int* partition);
> How about forgoing KuduPartitionerBuilder and KuduPartitioner and making th
That wouldn't give us the ability to fetch the partition mapping up-front and 
cache it (it's important to be able to give a synchronous and simple API here 
for the target use case)


-- 
To view, visit http://gerrit.cloudera.org:8080/5775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: initial work on token creation and verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto
File src/kudu/security/token.proto:

PS2, Line 71: TokenSigningKeyPB
Is it just for verification or for signing?  If for both, then may be it makes 
sense to have separate ones for different operations?

TokenSigningKeyPB

TokenVerifyingKeyPB


PS2, Line 75:   optional bytes public_key_der = 2;
:   optional bytes private_key_der = 3;
If it's necessary to have public key only, why do we need the private part?  
From other other side, given the private key, if using the RSA keys, it's 
possible to extract the public part.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.h
File src/kudu/security/token_verifier.h:

PS2, Line 52: enum 
nit: if it makes sense, consider using strictly types enums


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1713: add a client Partitioner API

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: KUDU-1713: add a client Partitioner API
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5775/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2209:   /// @param [out] partition
> I was thinking of adding a separate API which is 'GetPartitionInfo()' which
Sounds reasonable.


Line 2216:   Status PartitionRow(const KuduPartialRow& row, int* partition);
> That wouldn't give us the ability to fetch the partition mapping up-front a
So it's either:

  shared_ptr table;
  RETURN_NOT_OK(client->OpenTable("my table", ));
  KuduPartitionerBuilder kpb(table);
  KuduPartitioner* kp;
  RETURN_NOT_OK(kpb.Build()); // "slow" call that caches results
  int part_num;
  RETURN_NOT_OK(kp->PartitionRow(, _num)); // all calls are 
"fast"

And:

  shared_ptr table;
  RETURN_NOT_OK(client->OpenTable("my table", ));
  int part_num;
  RETURN_NOT_OK(table->PartitionRow(, _num)); // first call is 
"slow" and caches results, subsequent calls are "fast"

>From the POV of the client application, why is the first approach better?


-- 
To view, visit http://gerrit.cloudera.org:8080/5775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS10, Line 73: tls_handshake-test
> nit: why not to put it under the test sub-dir?
oh, I meant to point this out to you -- the idea of the 'test' module and 
corresponding dir is that those are test infrastructure that are usable by 
other modules, etc, rather than saying that we'll put all unit tests into the 
test/ dir. (I found it hard to find the cert management test from my editor the 
other day because it was in test/)


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: generate certs on the tserver, sign them on the master

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5766

to look at the new patch set (#4).

Change subject: security: generate certs on the tserver, sign them on the master
..

security: generate certs on the tserver, sign them on the master

This adds a bit of plumbing for the self-hosted PKI:

* Servers (both TS and Master) have a new ServerCertManager instance
  which generate a private key on startup. They also generate a CSR and
  adopt a signed cert once provided. This is also a convenient place to
  stash the set of CA certs and plumb them through to the SSL library,
  though that isn't implemented yet.

* Similarly, the master has a MasterCertAuthority instance which generates
  a key and self-signed CA cert on startup. It can then sign certs
  provided by other servers. This may change a bit in the future as the
  CA cert will have to be loaded from the system tablet if it's
  available, rather than generated on startup.

* When the TS heartbeats, it checks if the cert manager has a signed
  cert yet. If not, it sends the CSR to the master in DER format.

* If the master gets a heartbeat with a CSR, it signs it and returns the
  signed cert in the heartbeat response. The tablet server then adopts
  this as its cert.

A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:

* Currently the master doesn't yet sign its own cert. This is going to
  have some interaction with the storage of certs in the catalog table,
  so want to wait until that code is integrated before figuring out
  where to plug this in.

* The built-in PKI stuff should have a flag to disable it. Again I
  wasn't sure the best place to put it for now, and it's nice to get the
  test coverage of this new code all the time. We can add this flag at
  the same point when we add the appropriate flags to configure your own
  PKI.

* Various other questions and vague thoughts that we can address as we
  go.

Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.

Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 512 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5761/11/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

Line 318: const char *GenericCalculatorService::kSecondString =
> warning: variable 'kSecondString' defined in a header file; variable defini
Ignoring...


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: generate certs on the tserver, sign them on the master

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: security: generate certs on the tserver, sign them on the master
..


Patch Set 4:

> Not sure how this patch could be causing the Java test to fail,
 > unless it's the extra load of generating certs on startup and the
 > test is super sensitive. Anyone have any ideas?

To me the test failure looks like a glitch.  Do you see it reproducible on your 
local machine?

-- 
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] security: generate certs on the tserver, sign them on the master

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: generate certs on the tserver, sign them on the master
..


Patch Set 4:

Nope, just curious if I may have missed something. I guess if it becomes flaky 
going forward we'll notice it. Definitely not a functional issue, potentially 
just a timing change due to slower startup.

(we could consider setting the RSA key size to 512 bits for tests or somesuch)

-- 
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] webserver: improve SSL certificate handling

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5015

to look at the new patch set (#7).

Change subject: webserver: improve SSL certificate handling
..

webserver: improve SSL certificate handling

* Allows users to specify a separate location for PEM private-key file
  using --webserver_private_key_file (previously required private-key
  and cert. to be in same file).

* Allows users to specify a shell command to run to get the password
  for the webserver's private-key file using
  --webserver_private_key_password_cmd

The separate configuration of cert and key is apparently more commonly
used, so this simplifies deployment. The use of a script to provide the
password for the private key makes it easier to integrate with external
credential providers.

This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7).

Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
10 files changed, 244 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: initial work on token creation and verification

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5796

to look at the new patch set (#4).

Change subject: security: initial work on token creation and verification
..

security: initial work on token creation and verification

This adds classes for managing Token Signing Keys (TSKs) on the
signer (masters) and verifiers (all servers). The new classes aren't
hooked up with the actual servers as of yet, nor do they actually use
real signature routines (blocked on another in-flight patch for that).

A unit test is included which drives the APIs using a stubbed-out
"signature" algorithm.

Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/token-test.cc
A src/kudu/security/token.proto
A src/kudu/security/token_signer.cc
A src/kudu/security/token_signer.h
A src/kudu/security/token_signing_key.cc
A src/kudu/security/token_signing_key.h
A src/kudu/security/token_verifier.cc
A src/kudu/security/token_verifier.h
9 files changed, 922 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5796/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: initial work on token creation and verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS2, Line 60: // Sanity check the key.
: CHECK(!pb.has_private_key_der());
> In general aren't we sort of assuming that core files are going to have sen
Yep, of course we assume the core files are going to have sensitive data.  My 
point was: splitting that structure/message would beneficial from this side as 
well -- no need to perform such CHECKs, etc.

Thank you for the link -- will take a look.


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: generate certs on the tserver, sign them on the master

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: generate certs on the tserver, sign them on the master
..


Patch Set 4: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] webserver: improve SSL certificate handling

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5015

to look at the new patch set (#6).

Change subject: webserver: improve SSL certificate handling
..

webserver: improve SSL certificate handling

* Allows users to specify a separate location for PEM private-key file
  using --webserver_private_key_file (previously required private-key
  and cert. to be in same file).

* Allows users to specify a shell command to run to get the password
  for the webserver's private-key file using
  --webserver_private_key_password_cmd

The separate configuration of cert and key is apparently more commonly
used, so this simplifies deployment. The use of a script to provide the
password for the private key makes it easier to integrate with external
credential providers.

This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7).

Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/CMakeLists.txt
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
11 files changed, 290 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: initial work on token creation and verification

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto
File src/kudu/security/token.proto:

Line 33: UNKNOWN = 999;
What's the point of the UNKNOWN variant, aren't we just going to check that all 
of the features are valid enum variants?


PS2, Line 47: incompatible_features
Maybe this should be 'required_features'?


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

PS2, Line 70: .
Missing close paren.


PS2, Line 98: virtual
Why virtual?


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signing_key.h
File src/kudu/security/token_signing_key.h:

Line 36:   explicit TokenSigningKey(TokenSigningKeyPB pb);
If PBs can't be moved this may as well take a const ref.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS2, Line 60: // Sanity check the key.
: CHECK(!pb.has_private_key_der());
> Yep, of course we assume the core files are going to have sensitive data.  
I agree in principal that this is a serious enough check that its fine to crash 
a production server over it, but that being said I'm not sure why there is even 
a private_key_der field to begin with.  When would we ever want to send the 
private key?


Line 107: if (tsk->pb().expire_unix_epoch_seconds() < now) {
It's probably worth a DCHECK here to ensure that 
tsk->pb().expire_unix_epoch_seconds() < token.expire_unix_epoch_seconds(), 
since the signer should never break that constraint.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.h
File src/kudu/security/token_verifier.h:

PS2, Line 52: enum 
> nit: if it makes sense, consider using strictly types enums
I agree, would be better to have this be an enum class defined outside of 
TokenVerifier.


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: webserver: improve SSL certificate handling
..


Patch Set 5:

(2 comments)

Done those. Next revision is a rebase

http://gerrit.cloudera.org:8080/#/c/5015/5/src/kudu/server/webserver_options.cc
File src/kudu/server/webserver_options.cc:

PS5, Line 56: .PEM
> I might miss something, but the sentence meaning seems to be '... certifica
Done


PS5, Line 63: .PEM
> nit: I didn't notice in the first pass, but consider dropping.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: initial work on token creation and verification

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto
File src/kudu/security/token.proto:

Line 33: UNKNOWN = 999;
> What's the point of the UNKNOWN variant, aren't we just going to check that
it wouldn't let me define an enum with no values :(


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signer.cc
File src/kudu/security/token_signer.cc:

Line 65:   unique_ptr new_tsk(new 
TokenSigningKey(std::move(tsk_pb)));
> warning: passing result of std::move() as a const reference argument; no mo
Done


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

PS2, Line 70: .
> Missing close paren.
Done


PS2, Line 98: virtual
> Why virtual?
my little yasnippet-mode template does this by default since non-virtual 
constructors are bad news with inheritance... but we don't intend this to be 
inherited so I'll remove.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signing_key.cc
File src/kudu/security/token_signing_key.cc:

Line 22: #include "kudu/util/status.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_signing_key.h
File src/kudu/security/token_signing_key.h:

Line 36:   explicit TokenSigningKey(TokenSigningKeyPB pb);
> If PBs can't be moved this may as well take a const ref.
Done


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS2, Line 60: // Sanity check the key.
: CHECK(!pb.has_private_key_der());
> I agree in principal that this is a serious enough check that its fine to c
yea, Alexey brought that up elsewhere, so I'll work on removing it


Line 107: if (tsk->pb().expire_unix_epoch_seconds() < now) {
> It's probably worth a DCHECK here to ensure that tsk->pb().expire_unix_epoc
hrm, it's useful in tests to break that constraint though, to force the code 
path of an expired key. Even though it's a constraint that is necessary to 
"make sense", the only thing that happens due to it is early expiration, so I'd 
be hesitant to actually crash a server.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.h
File src/kudu/security/token_verifier.h:

PS2, Line 52: enum 
> I agree, would be better to have this be an enum class defined outside of T
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1853. Tablet copy: Don't orphan blocks on failure

[Mike Percy (Code Review)]

Hello Adar Dembo, Todd Lipcon,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/5799

to review the following change.

Change subject: KUDU-1853. Tablet copy: Don't orphan blocks on failure
..

KUDU-1853. Tablet copy: Don't orphan blocks on failure

Previously, if a tablet copy failed we would orphan data blocks. This
patch makes it so that a failed tablet copy operation that does not
involve a process crash does not orphan data blocks.

Change-Id: Ifbc0720d460de8d912a241a5186ef6e8f524b830
---
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
3 files changed, 101 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5799/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5799
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifbc0720d460de8d912a241a5186ef6e8f524b830
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: initial work on token creation and verification

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5796

to look at the new patch set (#2).

Change subject: security: initial work on token creation and verification
..

security: initial work on token creation and verification

This adds classes for managing Token Signing Keys (TSKs) on the
signer (masters) and verifiers (all servers). The new classes aren't
hooked up with the actual servers as of yet, nor do they actually use
real signature routines (blocked on another in-flight patch for that).

A unit test is included which drives the APIs using a stubbed-out
"signature" algorithm.

Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/token-test.cc
A src/kudu/security/token.proto
A src/kudu/security/token_signer.cc
A src/kudu/security/token_signer.h
A src/kudu/security/token_signing_key.cc
A src/kudu/security/token_signing_key.h
A src/kudu/security/token_verifier.cc
A src/kudu/security/token_verifier.h
9 files changed, 902 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5796/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1852. KuduTableAlterer should not crash with nullptr arguments

[Todd Lipcon (Code Review)]

Hello Dan Burkert,

I'd like you to do a code review.  Please visit

http://gerrit.cloudera.org:8080/5797

to review the following change.

Change subject: KUDU-1852. KuduTableAlterer should not crash with nullptr 
arguments
..

KUDU-1852. KuduTableAlterer should not crash with nullptr arguments

Change-Id: Ifa7c34b476111ecd33d4fd3ef5cc363a410ad76a
---
M src/kudu/client/client.cc
M src/kudu/integration-tests/alter_table-test.cc
2 files changed, 13 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/5797/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifa7c34b476111ecd33d4fd3ef5cc363a410ad76a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] KUDU-1852. KuduTableAlterer should not crash with nullptr arguments

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1852. KuduTableAlterer should not crash with nullptr 
arguments
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa7c34b476111ecd33d4fd3ef5cc363a410ad76a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1713: add a client Partitioner API

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-1713: add a client Partitioner API
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5775/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2216:   Status PartitionRow(const KuduPartialRow& row, int* partition);
> So it's either:
The second approach is still coupled to the metacache, which could be 
arbitrarily invalidated by other users of the same client object. That includes 
the possibility that the number/set of partitions could change if a range is 
added/dropped, which makes the 'partition number' sort of API infeasible.

(even outside the possibility of concurrent changes, I'm not sure how we'd do 
the efficient mapping to partition numbers when going through the whole 
metacache shenanigans internally)


-- 
To view, visit http://gerrit.cloudera.org:8080/5775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [security] introduced crypto-test
..

[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 209 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5798/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP [openssl util] method to extract public part of RSA key

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP [openssl_util] method to extract public part of RSA key
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5783/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 102: class RSAPublicKey : public BasicWrapper {
> What do you think about moving these key classes to their own file?  Their 
This sounds good to me.  Will do.


-- 
To view, visit http://gerrit.cloudera.org:8080/5783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5798

to look at the new patch set (#3).

Change subject: [security] introduced crypto-test
..

[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 207 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5798/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP [openssl util] method to extract public part of RSA key

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: WIP [openssl_util] method to extract public part of RSA key
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5783/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 102: class RSAPublicKey : public BasicWrapper {
What do you think about moving these key classes to their own file?  Their 
implementation is OpenSSL specific, but I think the [Public|Private]Key 
abstractions are pretty general.


-- 
To view, visit http://gerrit.cloudera.org:8080/5783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1835 (part 2): enable WAL compression

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1835 (part 2): enable WAL compression
..


Patch Set 8:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS8, Line 340: c
> I forget, we don't capitalize these?
yea we've been moving towards not capitalizing them, so they don't look funny 
when concatenated (eg "Unable to do foo: Unable to do bar: Problem with baz" 
isn't correct capitalization)


http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/consensus/log.proto
File src/kudu/consensus/log.proto:

PS8, Line 55: / Log format major/minor version. These were written by Kudu 1.2 
and
:   // earlier, and marked as required in those versions, but 
unfortunately
:   // they were never verified on read. So, in order to make the 
logs written
:   // by newer versions give a reasonable error if an old version 
tries to
:   // read them, we no longer write these fields.
:   optional uint32 DEPRECATED_major_version = 1;
:   optional uint32 DEPRECATED_minor_version = 2;
> so you're replacing these with feature flags, right? should we do the same 
we already did while you were on vacation, see 
f82cf6918c00dff6aecad5a6b50836b7eb5db508


Line 70:   repeated FeatureFlag incompatible_features = 10;
I discovered today that this doesn't work properly, will edit. (see KUDU-1850)


http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/consensus/log_reader.cc
File src/kudu/consensus/log_reader.cc:

PS8, Line 262: kEntryHeaderSizeV2
> what if this is reading an older log segment?
fixed


http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:

PS8, Line 429: verison
> typo
Done


http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/consensus/log_util.h
File src/kudu/consensus/log_util.h:

PS8, Line 50: Ssee
> typo
Done


PS8, Line 305: Older versions of Kudu used a different log entry header format.
> Older than what?
Done


http://gerrit.cloudera.org:8080/#/c/5736/8/src/kudu/util/compression/compression_codec.h
File src/kudu/util/compression/compression_codec.h:

PS8, Line 59: CompressionType
> docs
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0fc790ddc72a519a82bbb6b71a902418051b35a5
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1835 (part 2): enable WAL compression

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5736

to look at the new patch set (#9).

Change subject: KUDU-1835 (part 2): enable WAL compression
..

KUDU-1835 (part 2): enable WAL compression

This enables compression of the entries in the WAL.

To test the efficiency, I loaded some data into a local tablet server
using kudu-ts and the influxdb benchmark as a data generator. I think
restarted the server with different configurations, and disabled log
segment prealloocation in order to see the _actual_ length of the WALs
instead of the preallocated length.

Compression Size
NONE311M
LZ4  98M - 3.17x
SNAPPY   91M - 3.41x
ZLIB 59M - 5.27x

Based on some results in the JIRA I expect more significant improvements
in some other workloads, since kudu-ts already does its own sort of
"dictionary encoding" by normalizing out the tagset IDs.

In order to enable this feature, an incompatible change was made in the
WAL format. Each entry header now contains an extra field indicating the
uncompressed length.

Given that, it's desirable that old servers be prohibited from opening
new-format WALs. Ideally, we could have bumped the major version field
in the WAL header. However, the old reader code never actually checked
the version fields on read. So, this takes the approach of not setting
the versions anymore, and making them deprecated. Since they were
'required' proto fields in old versions, this will make the reader fail
to parse the header with an error that the version field is missing.
This is preferable to failing later with a Corruption error about an
entry header CRC mismatch.

To solve this problem for future versions, I also added an
'incompatible_features' flag set. See commit f82cf6918c00dff6aec for
more information about this approach.

Note that it might have been possible to do this in a more
backward-compatible way by making the default codec be "NONE" and having
the new version write old-format headers when compression is disabled.
I initially took this approach, but eventually abandoned it as the code
was more complex. Additionally, we already have a different data format
incompatible change coming in Kudu 1.3 (the above-referenced
f82cf6918c00dff6aec). So adding a second one doesn't add additional
burden on users.

I tested the compatibility manually:
- started a Kudu 1.2 server, wrote some data
- upgraded to a version with this patch
- downgraded back to a version without this patch, and saw that the
  tablets failed to start, with a message that the major/minor version
  fields were missing
- upgraded back to this patch and verified the tablets started

Change-Id: I0fc790ddc72a519a82bbb6b71a902418051b35a5
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log.proto
M src/kudu/consensus/log_anchor_registry.h
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/compression/compression_codec.h
15 files changed, 274 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/5736/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5736
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0fc790ddc72a519a82bbb6b71a902418051b35a5
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: generate certs on the tserver, sign them on the master

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: generate certs on the tserver, sign them on the master
..


Patch Set 4:

Not sure how this patch could be causing the Java test to fail, unless it's the 
extra load of generating certs on startup and the test is super sensitive. 
Anyone have any ideas?

-- 
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP [openssl util] method to extract public part of RSA key

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5783

to look at the new patch set (#2).

Change subject: WIP [openssl_util] method to extract public part of RSA key
..

WIP [openssl_util] method to extract public part of RSA key

Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
---
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/cert_management-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
7 files changed, 224 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5783/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] security: initial work on token creation and verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS2, Line 60: // Sanity check the key.
: CHECK(!pb.has_private_key_der());
This check might present some security complications: if the private key is 
present, the process will crash and that private key would be ready to pick up 
from the core file.

Consider replacing this with DCHECK() at least.

>From other other side, if using different proto structures for signing and 
>verification, no such checks would be necessary.


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1713: add a client Partitioner API

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: KUDU-1713: add a client Partitioner API
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5775/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2216:   Status PartitionRow(const KuduPartialRow& row, int* partition);
> The second approach is still coupled to the metacache, which could be arbit
If your concern is concurrent changes, I don't think either approach provides 
true safety. The first approach does narrows the window of vulnerability to the 
loop inside KuduPartitionerBuilder::Build(), but it doesn't eliminate it 
altogether.

But, your other point (about mapping to partition numbers) is valid, and I can 
see how the first approach models the KuduPartitioner as a "view" of the 
metacache. I can also see how, in the future, we could introduce some kind of 
locking so that the loop in KuduPartitionerBuilder::Build() could be completely 
safe.

OK. I'm convinced.


-- 
To view, visit http://gerrit.cloudera.org:8080/5775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1852. KuduTableAlterer should not crash with nullptr arguments

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1852. KuduTableAlterer should not crash with nullptr 
arguments
..


KUDU-1852. KuduTableAlterer should not crash with nullptr arguments

Change-Id: Ifa7c34b476111ecd33d4fd3ef5cc363a410ad76a
Reviewed-on: http://gerrit.cloudera.org:8080/5797
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/client/client.cc
M src/kudu/integration-tests/alter_table-test.cc
2 files changed, 13 insertions(+), 0 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifa7c34b476111ecd33d4fd3ef5cc363a410ad76a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS10, Line 73: tls_handshake-test
nit: why not to put it under the test sub-dir?


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

PS10, Line 41: nullptr, SSL_CTX_free
nit: you could use ssl_make_unique() here; add the trait for SSL_CTX type.


PS10, Line 46: CHECK(!ctx_.get());
nit: CHECK(!ctx) would be enough, I think


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_context.h
File src/kudu/security/tls_context.h:

PS10, Line 58: std::unique_ptr>
nit: openssl_util.h have a handy typedef for this type of wrappers.


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

PS10, Line 89: unique_ptr cert
nit: you could use ssl_make_unique here:

auto cert = ssl_make_unique(SSL_get_peer_certificate(...));

The trait for X509 is needed.


PS10, Line 94:   // Get the peer's hostname
 :   Sockaddr peer_addr;
 :   if (!socket.GetPeerAddress(_addr).ok()) {
 : return Status::NotAuthorized("Handshake failed: Could not 
retrieve peer address");
 :   }
 :   string peer_hostname;
 :   RETURN_NOT_OK(peer_addr.LookupHostname(_hostname));
It might be subject alternative name (SAN) instead.  I think we are about to 
use that for our TLS certs.


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_handshake.h
File src/kudu/security/tls_handshake.h:

PS10, Line 83: ssl_
I could not find where it's set.  Probably I'm missing something.


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1713: add a client Partitioner API

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-1713: add a client Partitioner API
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5775/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 2216:   Status PartitionRow(const KuduPartialRow& row, int* partition);
> If your concern is concurrent changes, I don't think either approach provid
I guess my point isn't that the view you get is necessarily a consistent 
snapshot at any particular point in time of the cluster, but rather that 
whatever view you get isn't going to change.

Sort of like "panorama" mode on your phone - if stuff's changing you might have 
some tearing on the photo, but the photo isn't going to change after you took 
it :)


-- 
To view, visit http://gerrit.cloudera.org:8080/5775
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] security: initial work on token creation and verification

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: initial work on token creation and verification
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token.proto
File src/kudu/security/token.proto:

PS2, Line 71: TokenSigningKeyPB
> Is it just for verification or for signing?  If for both, then may be it ma
Let me ponder this... I think you're right that right now we only need to pass 
and persist around the public variant through RPCs and such.


PS2, Line 75:   optional bytes public_key_der = 2;
:   optional bytes private_key_der = 3;
> If it's necessary to have public key only, why do we need the private part?
yea, like you said above probably mutually exclusive.


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS2, Line 60: // Sanity check the key.
: CHECK(!pb.has_private_key_der());
> This check might present some security complications: if the private key is
In general aren't we sort of assuming that core files are going to have 
sensitive data? eg we have tons of user data in memory, not just these various 
keys, etc.

We could try to get fancy at some point with MADV_DONTDUMP but it's not 
supported on most of our target operating systems and it's also terribly hard 
to get this to work all the way down through OpenSSL, etc.

Given that, I'd prefer to crash if we are accidentally sending private keys 
where we meant to be sending public ones, such that we hear about it quickly 
due to people complaining about crashes (rather than hearing about it from 
someone finding an exploit)

On a side note, I found this an interesting read: 
https://www.agwa.name/blog/post/protecting_the_openssl_private_key_in_a_separate_process


http://gerrit.cloudera.org:8080/#/c/5796/2/src/kudu/security/token_verifier.h
File src/kudu/security/token_verifier.h:

PS2, Line 52: enum 
> nit: if it makes sense, consider using strictly types enums
downside is then you end up with the longer name 
TokenVerifier::VerificationResult::VALID instead of just TokenVerifier::VALID, 
which is a bit of a mouthful.

Suppose I could move it up out of TokenVerifier though, which wouldn't be bad. 
Will give it a shot.


-- 
To view, visit http://gerrit.cloudera.org:8080/5796
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53ae50082d69028315952ac0732af6a83ffdbe
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS10, Line 73: tls_handshake-test
> oh, I meant to point this out to you -- the idea of the 'test' module and c
I'm not sure I understand, but I'll take a look at the ADD_KUDU_TEST() cmake 
macro.

A quick question: does this mean tls_handshake-test is not going to be in the 
list of tests for ctest?


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS10, Line 73: tls_handshake-test
> I'm not sure I understand, but I'll take a look at the ADD_KUDU_TEST() cmak
I just mean that the 'test/' subdir isn't meant to contain unit tests, but 
rather to contain reusable test library code for other parts of kudu tests to 
use (eg minikdc, etc).


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5798

Change subject: [security] introduced crypto-test
..

[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 213 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5798/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..


Patch Set 10:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

PS10, Line 73: tls_handshake-test
> I just mean that the 'test/' subdir isn't meant to contain unit tests, but 
OK going to keep it as is for now.   It definitely shows up in the list of 
ctest tests.


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

PS10, Line 41: nullptr, SSL_CTX_free
> nit: you could use ssl_make_unique() here; add the trait for SSL_CTX type.
Done


PS10, Line 46: CHECK(!ctx_.get());
> nit: CHECK(!ctx) would be enough, I think
Done


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_context.h
File src/kudu/security/tls_context.h:

PS10, Line 58: std::unique_ptr>
> nit: openssl_util.h have a handy typedef for this type of wrappers.
Done


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

PS10, Line 89: unique_ptr cert
> nit: you could use ssl_make_unique here:
Done


PS10, Line 94:   // Get the peer's hostname
 :   Sockaddr peer_addr;
 :   if (!socket.GetPeerAddress(_addr).ok()) {
 : return Status::NotAuthorized("Handshake failed: Could not 
retrieve peer address");
 :   }
 :   string peer_hostname;
 :   RETURN_NOT_OK(peer_addr.LookupHostname(_hostname));
> It might be subject alternative name (SAN) instead.  I think we are about t
I think that's covered by the call to 'X509_check_host' (at least according to 
the comment).


http://gerrit.cloudera.org:8080/#/c/5761/10/src/kudu/security/tls_handshake.h
File src/kudu/security/tls_handshake.h:

PS10, Line 83: ssl_
> I could not find where it's set.  Probably I'm missing something.
It's set directly by TlsContext, but now that you point it out that's pretty 
confusing.  I'll make it a private setter instead.


-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5761

to look at the new patch set (#11).

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..

TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS 
handshakes

The new TLS negotiation protocol calls for tunneling the TLS handshake
through negotiation protobufs. The existing SSL socket helper classes
were not built with this in mind. This commit introduces new helpers,
largely based off the existing versions. A followup commit will switch
KRPC to use the new classes and remove the old.

Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/security-test-util.h
A src/kudu/security/tls_context.cc
A src/kudu/security/tls_context.h
A src/kudu/security/tls_handshake-test.cc
A src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_handshake.h
A src/kudu/security/tls_socket.cc
A src/kudu/security/tls_socket.h
12 files changed, 832 insertions(+), 74 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/5761/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] store CA information in the system table

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5793

to look at the new patch set (#3).

Change subject: WIP [master] store CA information in the system table
..

WIP [master] store CA information in the system table

Use CatalogManager::SetCAInfo() to set CA private key and certificate.

TBD: unit tests

Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 440 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] store CA information in the system table

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP [master] store CA information in the system table
..


Patch Set 2:

(3 comments)

Thank you for the review of this WIP-ish changelist.  I'll continue tailoring 
this draft up to our needs.

http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

Line 272: // The generic wrapper around {id, metadata} tuple from the system 
table's row.
> hrm, does this buy us much? I think we'll need sequence numbers on our keys
That's just set of fields necessary to plug those into the existing model of 
the catalog_manager.

The sequence numbers and other I thought to add into the corresponding PB 
structures/messages and expose via the wrappers as well (the wrappers might 
start diverging).

At this point it a lot of WIP-ish things.


http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:

PS2, Line 181:   // The identifier of the entry in the system table for the 
certificate's
 :   // private key. If empty, then the private key is stored along 
with the
 :   // certificate data itself.
 :   optional string private_key_id = 3;
> what's the purpose of the flexibility here? Aren't we always going to be st
Current code assumes we always store the key separately.  The coment was about 
generic semantics of this field: I didn't want to make it a required field and 
this comment says what it would be meaning if the field were absent.

It turned to be a misleading comment since that functionality is not present 
now (we always store those separately).


Line 190: message SysCAPrivateKeyEntryPB {
> see above, not sure why it has to be stored separately?
The idea was to support the case when we have multiple CA certs based on the 
same private key.  And I liked the idea to store separate entities in different 
records.

Say, for the certificate entry we could add additional sequence/serial numbers, 
expose validity dates via additional fields, etc.  For private keys there might 
be different set of properties/fields we want to store along.

Do you think it's not worth it?


-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] store CA information in the system table

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP [master] store CA information in the system table
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:

PS2, Line 181:   // The identifier of the entry in the system table for the 
certificate's
 :   // private key. If empty, then the private key is stored along 
with the
 :   // certificate data itself.
 :   optional string private_key_id = 3;
> Current code assumes we always store the key separately.  The coment was ab
Yea, I think given these are just internal-facing PBs, we shouldn't document 
"what-ifs" if they are things that don't actually happen.


Line 190: message SysCAPrivateKeyEntryPB {
> The idea was to support the case when we have multiple CA certs based on th
Yea, that makes sense, I forgot about other metadata that you might want to 
store with a key such as the timestamps, etc, and the idea that the cert and 
key may rotate at different frequencies.


-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP [master] store CA information in the system table

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP [master] store CA information in the system table
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

Line 272: // The generic wrapper around {id, metadata} tuple from the system 
table's row.
hrm, does this buy us much? I think we'll need sequence numbers on our keys and 
such anyway (which we'll expose out to clients), so even if it's slightly 
redundant, maybe we can just derive the table row from the key's sequence 
number, and then just pass around the PBs?


http://gerrit.cloudera.org:8080/#/c/5793/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:

PS2, Line 181:   // The identifier of the entry in the system table for the 
certificate's
 :   // private key. If empty, then the private key is stored along 
with the
 :   // certificate data itself.
 :   optional string private_key_id = 3;
what's the purpose of the flexibility here? Aren't we always going to be 
storing the cert along with its key rather than separately?


Line 190: message SysCAPrivateKeyEntryPB {
see above, not sure why it has to be stored separately?


-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS handshakes

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5761

to look at the new patch set (#8).

Change subject: TLS-negotiation [7/n]: Add TLS helper classes for handling 
tunneled TLS handshakes
..

TLS-negotiation [7/n]: Add TLS helper classes for handling tunneled TLS 
handshakes

The new TLS negotiation protocol calls for tunneling the TLS handshake
through negotiation protobufs. The existing SSL socket helper classes
were not built with this in mind. This commit introduces new helpers,
largely based off the existing versions. A followup commit will switch
KRPC to use the new classes and remove the old.

Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/tls_context.cc
A src/kudu/security/tls_context.h
A src/kudu/security/tls_handshake-test.cc
A src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_handshake.h
A src/kudu/security/tls_socket.cc
A src/kudu/security/tls_socket.h
10 files changed, 708 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/5761/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6776dbdc488eee56f7273cdd8bcd2b2b8c1ffa04
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP [master] store CA information in the system table

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5793

to look at the new patch set (#2).

Change subject: WIP [master] store CA information in the system table
..

WIP [master] store CA information in the system table

Use CatalogManager::SetCAInfo() to set CA private key and certificate.

TBD: unit tests

Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
5 files changed, 440 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/5793/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5793
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16fa077e39f5d75f682cca7220371acdef4f0630
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1831. Java client does not check if the primary key 
columns are specified first
..


Patch Set 3:

(1 comment)

I'm good with the change, just one javadoc comment to fix. Thanks!

http://gerrit.cloudera.org:8080/#/c/5723/3/java/kudu-client/src/main/java/org/apache/kudu/Schema.java
File java/kudu-client/src/main/java/org/apache/kudu/Schema.java:

Line 71:* Schema constructor won't check if the primary key columns are 
specified first
I think this would be confusing for the lay user. We could either not say 
anything, or say that primary keys must be specified first if this Schema is 
used to create a table but can be anywhere if used to for a projection.


-- 
To view, visit http://gerrit.cloudera.org:8080/5723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Jun He 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Update Impala docs for upcoming Impala 2.8 release

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Update Impala docs for upcoming Impala 2.8 release
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5733/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

Line 30: Kudu has tight integration with Apache Impala, allowing you to use 
Impala
> (incubating) necessary?
typically it's OK to use "incubating" only in the first reference in a 
document, iirc, and it's referenced as incubating in the title. But I'll add it 
here just in case


PS1, Line 39: Impala 2.8 
question: I just noticed that the build I'm testing (CDH 5.10 pre-release) says:
impalad version 2.7.0-cdh5.10.0 RELEASE (build 
1c57740759e175d8810c6790af3c5a60862a0ce9

even though it seems to correspond more or less to upstream 2.8. Should I 
instead be documenting this in a different way? Or will the final bits say 2.8?


PS1, Line 365: // TODO(todd) Composite partition keys for range partitioning 
are not allowed
 : // anymore in Impala 2.8?
> Except when specifying '=', i.e. no n-dimensional ranges
ok, I'm just going to delete this section for now, and assume that this 
document doesn't need to be fully comprehensive.


PS1, Line 703: regardless of whether the table is an internal or external
 : table. This avoids disruption to other applications that may be 
accessing the
 : underlying Kudu table.
> this isn't true, internal tables will be renamed. I was surprised by this a
It's true in the version I'm testing with: 
https://gist.github.com/a2c5e6a869459a7121f2e71b29c86677
impalad version 2.7.0-cdh5.10.0 RELEASE (build 
1c57740759e175d8810c6790af3c5a60862a0ce9


-- 
To view, visit http://gerrit.cloudera.org:8080/5733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Update Impala docs for upcoming Impala 2.8 release

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Update Impala docs for upcoming Impala 2.8 release
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5733/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 39: Impala 2.8 
> question: I just noticed that the build I'm testing (CDH 5.10 pre-release) 
I think unfortunately the version in the code will still say 2.7, but we're 
talking about 2.8. I guess this will be like a known issue. I'll double check.


PS1, Line 703: regardless of whether the table is an internal or external
 : table. This avoids disruption to other applications that may be 
accessing the
 : underlying Kudu table.
> It's true in the version I'm testing with: https://gist.github.com/a2c5e6a8
My bad


PS1, Line 707: .Rename the underlying Kudu table for an internal table
 : 
 : If a table is an internal table, the underlying Kudu table may 
be renamed by
 : changing the `kudu.table_name` property:
> I think we wanna talk about this for external tables, given what I said abo
ignore; this is too complicated I can't even keep it straight I have to try it 
every time :/


-- 
To view, visit http://gerrit.cloudera.org:8080/5733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Update Impala docs for upcoming Impala 2.8 release

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5733

to look at the new patch set (#2).

Change subject: Update Impala docs for upcoming Impala 2.8 release
..

Update Impala docs for upcoming Impala 2.8 release

* No longer needs to document the special 'IMPALA_KUDU' build.
* Fixed syntax for new style
* Added back CTAS, which has syntax (we'd mistakenly thought it was
  removed)
* Fixed a few typos/issues elsewhere (eg use of 'int32' instead of 'int'
  type)
* Removed the docs for composite range partitioning, which seems to
  no longer be supported in Impala.

Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
---
M docs/kudu_impala_integration.adoc
M docs/quickstart.adoc
2 files changed, 148 insertions(+), 400 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/5733/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] TLS-negotiation [6/n]: Refactor RPC negotiation

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Update Impala docs for upcoming Impala 2.8 release

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Update Impala docs for upcoming Impala 2.8 release
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5733/2/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS2, Line 144: same names and types as the columns in `old_table`
technically its the names and types of the columns in the select stmt; you 
could alias, cast, or join.


-- 
To view, visit http://gerrit.cloudera.org:8080/5733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes