Alexey Serbin has posted comments on this change. Change subject: [master] store CA information in the system table ......................................................................
Patch Set 11: (16 comments) Thank you for review. I spot some extra things myself and decided to update the patch, thinking you haven't started reviewing it yet. http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 680: Status CatalogManager::CheckInitCertAuthority() { > BTW, shouldn't there be a short circuit path here for users who aren't usin Yep, calls to this method should not be done from the upper level if some security-related flag is not set (or, vice versa, is set). There is corresponding TODO at the call site. I think we can add in a separate changelist -- that flag should disable security features for all the components. PS10, Line 684: shared_ptr<security::PrivateKey> ca_private_key( : std::make_shared<security::PrivateKey>()); : shared_ptr<security::Cert> ca_cert(std::make_shared<security::Cert>()); > Why do these need shared ownership? Because the MasterCertAuthority has such interface. Or it's no longer true? Line 688: auto info(make_shared<SysCertAuthorityEntryPB>()); > Why does this need shared ownership? good point: this can be just an object on stack. Line 697: if (PREDICT_TRUE(found_ca_info)) { > Maybe just if s.ok() here? I don't think we're doing anything with found_ca Done Line 712: RETURN_NOT_OK(sys_catalog_->AddCertAuthorityEntry(*info)); > I don't think we should do this with lock_ held, since it requires disk and Good point -- I already removed the lock. Thanks! Line 715: } > What would be the motivation for moving it? I was not happy with it in here because MasterCertAuthority might have some logic which would be brittle to re-initting the object in the middle. But it's OK in its current implementation -- I'll remove this TODO. Line 770: LOG_SLOW_EXECUTION(WARNING, 1000, LogPrefix() + "Loading CA info into memory") { > This still needs to be refactored with VisitTablesAndTablets() so that lead I thought the idea was not to do that twice only in this part of the code -- there were a couple methods acquiring the lock. Will fix. http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 689: const Status s = sys_catalog_->GetCertAuthorityEntry(info.get()); > So we're still going to need to cache this in memory so that we're not read I'm not sure I understand. This method (i.e. CatalogManager::CheckInitCertAuthority) is called every time a master server becomes a leader. I thought it's done only when leadership role transfers from one server to another. As for the usage of the certificate authority information, it's needed for the MasterCertAuthority -- that component signs tablet server's certificate requests every heartbeat. Once MasterCertAuthority::Init() has been called, the master will have the necessary data to sign those certificate requests. http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 34: #include "kudu/master/master_cert_authority.h" > Don't need? Done PS10, Line 57: namespace master { : : class CatalogManagerBgTasks; : class Master; > Don't need these anymore? Done PS10, Line 529: // becomes the leader of a consensus configuration. Executes VisitTablesAndTabletsTask : // via 'worker_pool_'. > These no longer exist, right? Done http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 182: required bytes certificate = 2; > Talk to Dan about this; he suggested we may want to redact this too, not fo Dan is not currently available in Slack channel, but I'll update this as soon as we make a decision. http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/sys_catalog-test.cc File src/kudu/master/sys_catalog-test.cc: Line 45: using std::unique_ptr; > warning: using decl 'unique_ptr' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/5793/10/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: Line 24: #include <vector> > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: PS11, Line 40: template<typename T> : class TupleInfo; > Unused. Done Line 134: Status GetCertAuthorityEntry(SysCertAuthorityEntryPB* entry); > Should be a unique_ptr<>, since the caller takes ownership, right? The caller provides a placeholder for the result. I don't think we want unique_ptr here. But if you feel strong about having unique_ptr<> here, I will change 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: 11 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
