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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[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 19:44:25 +0000 Gerrit-HasComments: Yes
