Adar Dembo has posted comments on this change. Change subject: [master] store CA information in the system table ......................................................................
Patch Set 9: (20 comments) I didn't review the changes to master_cert_authority since I have no context for that stuff. http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: Line 73: ADD_KUDU_TEST(master_cert_authority-itest RESOURCE_LOCK "master-rpc-ports") Nit: sort alphabetically. http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/integration-tests/master_cert_authority-itest.cc File src/kudu/integration-tests/master_cert_authority-itest.cc: PS9, Line 55: afeinberg Heh, this should be you now. PS9, Line 164: *has_signed_certificate = true; : *signed_certificate = ts_cert_str; Maybe combine using a boost::optional<> or unique_ptr<> ? Line 241: ASSERT_OK(WaitForLeader()); We didn't bring down any master, so haven't we already waited for the leader as part of SetUp()? http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS9, Line 337: // Not logging due to security reasons? : //VLOG(1) << "Root CA private key " << id : // << ": " << SecureShortDebugString(data); If we're redacting the data (and we should; is it marked as secure in master.proto?) then this would be fine. Line 785: CHECK_OK(VisitCertAuthorityInfo()); Could you restructure this code such that leader_lock_ is taken only once and held for the entire duration of these operations? Releasing/reacquiring leader_lock_ a couple times probably doesn't matter since the write to leader_ready_term_ has yet to happen, but I think we should treat all of this work as atomic w.r.t. leader elections, and holding leader_lock_ for the entire duration would be safer. Line 1328: shared_ptr<CACertInfo> CatalogManager::CreateCACertInfo( Could be static. Better yet, could be a factory function in CACertInfo. Same for all of these, I think. http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 288: // As in 'metadata' column of the system table Nit: terminate with period. Line 606: Could you document these? I'm especially curious about 'id' vs. 'key_id'. PS9, Line 787: Protected by lock_ and the special : // protocol should be used to update both values Here you talk about a special protocol to update both values, but steps 1 to 5 below only update ca_cert_, not ca_private_key_info_. PS9, Line 794: the certificate Nit: ca_cert_ to be more clear. http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: PS9, Line 37: #include "kudu/security/openssl_util.h" : #include "kudu/security/ca/cert_management.h" Are these used? http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master.cc File src/kudu/master/master.cc: Line 113: Nit: remove this? http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: Line 156: // 7. Send any active CA certs which the TS doesn't have. This is TODO, presumably? http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog-test.cc File src/kudu/master/sys_catalog-test.cc: PS9, Line 110: As for now, Not for ever? Line 456: TEST_F(SysCatalogTest, UpdateCertAuthorityInfo) { Probably don't need this test anymore if we're not yet going to support DELETE. http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: Line 538: Status SysCatalogTable::VisitEntries(V* visitor) const { You introduced these nice generic methods but then aren't using them in VisitTables()/VisitTablets(). Why not? http://gerrit.cloudera.org:8080/#/c/5793/9/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: PS9, Line 64: class CACertVisitor { : public: : virtual Status Visit(const std::string& id, : const SysCACertEntryPB& data) = 0; : }; : : class CAPrivateKeyVisitor { : public: : virtual Status Visit(const std::string& id, : const SysCAPrivateKeyEntryPB& data) = 0; : }; Given that there's either zero or one of these rows, I don't think a visitor pattern makes sense. Perhaps provide a higher level function like this: Status GetCACertAndKey(unique_ptr<pair<SysCACertentryPB, SysCAPrivateKeyEntryPB>>* out) const; 'out' could be null if we've yet to write a CA key/cert. This function could also enforce that there aren't multiple entries in the tablet, returning a bad status (or even a CHECK()) if that's the case. PS9, Line 92: static const char* const kSysCaPrivateKeyId; : static const char* const kSysCaCertId; I think this implies that there's only one root CA private key and certificate at any given time in the tablet, but that (very important) semantic isn't stated anywhere in this change AFAICT. It probably belongs in several places: both master.proto and somewhere here or in catalog_manager. PS9, Line 138: CACertInfo* ca_cert_to_add; : CACertInfo* ca_cert_to_delete; : : CAPrivateKeyInfo* ca_private_key_to_add; : CAPrivateKeyInfo* ca_private_key_to_delete; I don't think these should be here. All table/tablet changes are here so that they can all be encapsulated in a single transaction, but that's not a requirement for CA cert/key operations. Instead, let's have SysCatalogTable provide functions that mirror the desired use cases of the data. We clearly need a ProcessTableActions() that takes a struct Actions and makes some table/tablet changes in a single transaction. Then, for the root CA cert/key data, we should provide just one function that INSERTs a pair, since that's the only functionality we need right now. That doesn't even need to be encapsulated in a struct; they could just be floating arguments to a InsertCACertAndKey() function (or some such). -- 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: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
