You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/01/19 08:22:15 UTC

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters 

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9076


Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/security/tls_context.cc
7 files changed, 187 insertions(+), 23 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/security/tls_context.cc
7 files changed, 180 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/CMakeLists.txt@98
PS4, Line 98: ADD_KUDU_TEST(security-master-certificates-itest RESOURCE_LOCK "master-rpc-ports")
> Nit: retain alphabetical sorting.
Done


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc@49
PS4, Line 49: class SecurityMasterCertsTest : public KuduTest {
> Is there a fixture you could inherit from that would set up the cluster for
I didn't find anything lightweight enough -- there are test other fixtures using InternalMiniCluster, but they include much more custom stuff.


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc@51
PS4, Line 51:   SecurityMasterCertsTest() {
            :   }
> Can omit?
Done


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

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.h@668
PS4, Line 668:   bool NeedToPrepareFollower();
> Doc?
Done


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/mini-cluster/internal_mini_cluster.h
File src/kudu/mini-cluster/internal_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/mini-cluster/internal_mini_cluster.h@80
PS4, Line 80:   // Whether to wait while catalog manager is started and properly initialized
> Nit: specify the default value here.
It turned out this newly introduced field is not needed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 00:21:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 7: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 17:06:21 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/security/tls_context.cc
5 files changed, 155 insertions(+), 15 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 4:

> I'm not a huge fan

Sorry, I thought I moved this partial comment to catalog_manager.cc but evidently I did not. Please ignore.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:53:56 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/security/tls_context.cc
7 files changed, 179 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 6: Code-Review+1

Will defer to Dan/Todd since this is an authn-related change.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 01:01:21 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master had a self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/util/rw_mutex.cc
M src/kudu/util/rw_mutex.h
7 files changed, 177 insertions(+), 18 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 1:

(5 comments)

> (4 comments)
 > 
 > One thing I'm not seeing in this patch is how this works with a
 > newly elected leader in term 0 who is a little bit slow to write
 > the new CA cert to the sys tablet.  Wouldn't the followers fail to
 > find the CA in the tablet upon being transitioned to follower, and
 > then continue not having it for the rest of that term?  Am I
 > missing some retry logic which is hiding somewhere?

The followers retry until they read that CA record and set TLS server certificates.
                                                                                
And yes, originally I started the patch as trying to fetch the CA record upon becoming a follower.  However, since that didn't cover the case of the very first startup of the multi-master cluster (basically, the case you mentioned), I decided to resort to doing that in the periodic background job.

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/integration-tests/security-master-certificates-itest.cc@47
PS1, Line 47: using std::thread;
> warning: using decl 'thread' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/integration-tests/security-master-certificates-itest.cc@92
PS1, Line 92:     if (m->tls_context().has_signed_cert()) {
> Seems easier to fold the asserts into the inner loop like you did with the 
Done


http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc@984
PS1, Line 984:   static const char* kDesc = "set CA information for follower catalog manager";
> should this be 'acquire' instead of 'set'?  (if so, it's also going to be g
Done


http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc@994
PS1, Line 994:   const Status s = ca_preparer();
> Ah ha!  This is the perfect use for Status::AndThen:
Done


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

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/security/tls_context.cc@405
PS1, Line 405:   RETURN_NOT_OK(VerifyCertChainUnlocked(cert));
> I'm curious why this change?  I don't see how it would make a difference in
This particular change is not related to KUDU-2265.  I just noticed that it's possible to optmize here and not call VerifyCertChainUnlocked() if the context has already adopted a signed certificate.

I can move this into a separate changelist, if you think it's better that way.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 21:42:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 6:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.h@415
PS6, Line 415: acquiring the catalog manager's leader_lock_
             :     // for reading in the process. T
> This seems to not quite be correct. In fact it just tries to acquire the le
Good catch, done.


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.h@444
PS6, Line 444:     bool owns_lock() const {
> (see note above about the docs on the constructor -- I was quite confused i
Done


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc@539
PS4, Line 539:       } else if (catalog_manager_->NeedToPrepareFollower() && l.owns_lock()) {
> FWIW Adar and I discussed this offline last week, and my opinion is that lo
OK, thanks for the clarification -- I totally agree that this poll-based approach is not the best what we can do here.  Let it be just simple polling for a while, and later we can replace it with nice and shiny subscription/notification approach.


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@a791
PS6, Line 791: 
> Can we AssertAcquiredForReading in the new impl?
In the new impl, this method can be called holding either with reading or writing lock.

I remember some time ago there was an attempt to have AssertAcquiredForReadingOrWriting(), but then consensus was that it's not worth it.

Let's see how it goes this time :)


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@a814
PS6, Line 814: 
> same
yup


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@997
PS6, Line 997: LOG(INFO) << LogPrefix
> can we use LOG_WITH_PREFIX() here?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 08:02:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 1:

(4 comments)

One thing I'm not seeing in this patch is how this works with a newly elected leader in term 0 who is a little bit slow to write the new CA cert to the sys tablet.  Wouldn't the followers fail to find the CA in the tablet upon being transitioned to follower, and then continue not having it for the rest of that term?  Am I missing some retry logic which is hiding somewhere?

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/integration-tests/security-master-certificates-itest.cc@92
PS1, Line 92:     if (m->tls_context().has_signed_cert()) {
Seems easier to fold the asserts into the inner loop like you did with the ASSERT_EVENTUALLY loop below.  So I think it'd be

    const auto& tls_context = cluster_->mini_master(i)->master()->tls_context();
    ASSERT(tls_context.has_cert());
    ASSERT_FALSE(tls_context.has_signed_cert());


http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc@984
PS1, Line 984:   static const char* kDesc = "set CA information for follower catalog manager";
should this be 'acquire' instead of 'set'?  (if so, it's also going to be grammatically wrong for one of the two cases below)


http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/master/catalog_manager.cc@994
PS1, Line 994:   const Status s = ca_preparer();
Ah ha!  This is the perfect use for Status::AndThen:

Status s = LoadCertAuthorityInfo(&key, &cert)
    .AndThen([&] { return InitCertAuthorityWith(std::move(key), std::move(cert)); });


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

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/security/tls_context.cc@405
PS1, Line 405:   RETURN_NOT_OK(VerifyCertChainUnlocked(cert));
I'm curious why this change?  I don't see how it would make a difference in the context of this patch.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 19:44:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc@539
PS4, Line 539:       } else if (catalog_manager_->NeedToPrepareFollower() && l.owns_lock()) {
> Frankly, I don't like this piece, neither.
FWIW Adar and I discussed this offline last week, and my opinion is that long-term I'd prefer what Adar's suggesting, but I don't think this limited-scope change warrants such an overhaul/refactor.


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

http://gerrit.cloudera.org:8080/#/c/9076/1/src/kudu/security/tls_context.cc@405
PS1, Line 405:   RETURN_NOT_OK(VerifyCertChainUnlocked(cert));
> This particular change is not related to KUDU-2265.  I just noticed that it
Ah ok, I think that's fine.  I was just curious if this had any effect besides the optimization.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jan 2018 19:49:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9076/5/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/5/src/kudu/integration-tests/security-master-certificates-itest.cc@57
PS5, Line 57:     opts.master_rpc_ports = { 55010, 55011, 55012, 55013, 55014, };
These are ephemeral ports which means they may be in use while the test runs. Don't we use 11010-11012 for our other multi-master tests?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 00:26:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 6:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.h@415
PS6, Line 415: acquiring the catalog manager's leader_lock_
             :     // for reading in the process. T
This seems to not quite be correct. In fact it just tries to acquire the leader_lock but doesn't necessarily succeed.

We should update this documentation to be clear that after constructing this, you need to call one of the two Check* methods


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.h@444
PS6, Line 444:     bool owns_lock() const {
(see note above about the docs on the constructor -- I was quite confused initially seeing this when the ctor docs say it unconditionally acquires a lock)


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@a791
PS6, Line 791: 
Can we AssertAcquiredForReading in the new impl?


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@a814
PS6, Line 814: 
same


http://gerrit.cloudera.org:8080/#/c/9076/6/src/kudu/master/catalog_manager.cc@997
PS6, Line 997: LOG(INFO) << LogPrefix
can we use LOG_WITH_PREFIX() here?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jan 2018 22:48:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc@539
PS4, Line 539:       } else if (catalog_manager_->NeedToPrepareFollower() && l.owns_lock()) {
> I'm not a huge fan of this poll-based approach to figuring out whether we n
Frankly, I don't like this piece, neither.

But there is another alternative here that works fine if the CA record is already in the system table, but does not cover one corner case.  The corner case is the very first start of the multi-master cluster when the CA record hasn't yet generated.  And the alternative is to perform this CA-related hustle upon switching into the follower role.  Actually, that's the way I started this patch, but then I realized that corner case would not be covered.

Maybe, we can use the former approach but some get the edge-case covered as well?  Any ideas?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 23:04:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/security/tls_context.cc
7 files changed, 179 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9076/5/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/5/src/kudu/integration-tests/security-master-certificates-itest.cc@57
PS5, Line 57:     opts.master_rpc_ports = { 55010, 55011, 55012, 55013, 55014, };
> These are ephemeral ports which means they may be in use while the test run
Done -- I returned back the 11010-11014 range.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 00:46:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:17:16 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master had a self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Reviewed-on: http://gerrit.cloudera.org:8080/9076
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Dan Burkert <da...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/util/rw_mutex.cc
M src/kudu/util/rw_mutex.h
7 files changed, 177 insertions(+), 18 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Todd Lipcon: Looks good to me, but someone else must approve
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9076 )

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................


Patch Set 4:

(6 comments)

I'm not a huge fan

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/CMakeLists.txt@98
PS4, Line 98: ADD_KUDU_TEST(security-master-certificates-itest RESOURCE_LOCK "master-rpc-ports")
Nit: retain alphabetical sorting.


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc
File src/kudu/integration-tests/security-master-certificates-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc@49
PS4, Line 49: class SecurityMasterCertsTest : public KuduTest {
Is there a fixture you could inherit from that would set up the cluster for you?


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/integration-tests/security-master-certificates-itest.cc@51
PS4, Line 51:   SecurityMasterCertsTest() {
            :   }
Can omit?


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

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.h@668
PS4, Line 668:   bool NeedToPrepareFollower();
Doc?


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/master/catalog_manager.cc@539
PS4, Line 539:       } else if (catalog_manager_->NeedToPrepareFollower() && l.owns_lock()) {
I'm not a huge fan of this poll-based approach to figuring out whether we need to read the CA cert from the master tablet; I would prefer something where the leader "publishes" the CA cert to the master tablet and the followers "subscribe" to it and receive a notification.

That said, what I prefer is far more work and would be a large-scale rearchitecture of the master, and since the poll is cheap, it seems fine for this use case.


http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/mini-cluster/internal_mini_cluster.h
File src/kudu/mini-cluster/internal_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/9076/4/src/kudu/mini-cluster/internal_mini_cluster.h@80
PS4, Line 80:   // Whether to wait while catalog manager is started and properly initialized
Nit: specify the default value here.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:53:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2265 CA-signed server certs for non-leader masters

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2265 CA-signed server certs for non-leader masters
......................................................................

KUDU-2265 CA-signed server certs for non-leader masters

This changelist addresses KUDU-2265.  Prior to this fix, a non-leader
master used self-signed server TLS certificates if it hasn't ever
become a leader.

An integration test to verify the new behavior is added as well.

Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/security/tls_context.cc
5 files changed, 155 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/9076/5
-- 
To view, visit http://gerrit.cloudera.org:8080/9076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3539d58d10ed319ad1d8686c1259c92822fb710
Gerrit-Change-Number: 9076
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>