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 <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-Comment-Date: Fri, 19 Jan 2018 21:42:48 +0000
Gerrit-HasComments: Yes
