Alexey Serbin has posted comments on this change. Change subject: [master] store CA information in the system table ......................................................................
Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master.proto File src/kudu/master/master.proto: PS11, Line 179: // Private key body in PEM format. : required bytes private_key = 1 [(kudu.REDACT) = true]; : // Certificate body in PEM format. : > why PEM instead of DER? we use DER elsewhere for our PBs, and most of the d I thought it might be better because that's the format to store the info. Also, I thought it might be better for troubleshooting. http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/master_cert_authority.h File src/kudu/master/master_cert_authority.h: Line 40: class MasterCertAuthorityTest; > don't think this forward decl is necessary But otherwise the compiler spits an error for the 'friend' statement below: kudu/src/kudu/master/master_cert_authority.h:72:32: error: no class named 'MasterCertAuthorityTest' in namespace 'kudu::master' friend class ::kudu::master::MasterCertAuthorityTest; Line 55: // bound to the aggregated server UUID. Does not require Init() to be called. > should note whether this also initializes the instance using the given cert I thought 'const' specified would be enough, but having this explicitly in the doc is better, I agree. PS11, Line 77: // Mutex to protect access to variables below. : Mutex lock_; : > I think you need to rebase -- the class no longer has a cert-signer member, But object of this class is used to sign incoming CSRs in master_service.cc. That means we don't want to have the private key and the cert to be updated while SignServerCSR() is in the middle of operation, right? http://gerrit.cloudera.org:8080/#/c/5793/11/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS11, Line 92: 00000000000000000000000000000001 > hm, I dont think there's any need to make this look like a tablet id. How a Done Line 516: *entry_id = str_id; > can you avoid a copy here by making 'str_id' non-const and std::move? Done Line 583: ProcessEntries<SysCertAuthorityEntryPB, CERT_AUTHORITY_INFO>(processor); > missing a RETURN_NOT_OK Good catch. That was those issues with pre-processor parsing. I put extra braces and now it's fine. -- 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
