Todd Lipcon has posted comments on this change. Change subject: [TLS cert management] TLS cert generation unit tests ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5672/1/src/kudu/security/test/cert_management-test.cc File src/kudu/security/test/cert_management-test.cc: PS1, Line 115: .ca_cert_path = ca_cert_file_, : .ca_private_key_path = ca_private_key_file_, in real usage, we'll be using a CA cert generated programmatically instead of loaded from a file, right? PS1, Line 140: size_t I think int is more appropriate here Line 143: CHECK_OK(GenerateKey(key_bits, &key)); curious: when we generate a new key, does that drain system entropy in such a way that this test is likely to hang? PS1, Line 297: aExp can you rename this to avoid abbreviating 'Expired'? PS1, Line 352: : // A test which verifies that non-initialized CertRequestGenerator and : // CertSigner behave as expected. per comments on the code, I dont think it's worth coding for or testing these negative cases. These are just internal Kudu-facing APIs, so need to be this defensive, instead of just crashing in the case that a Kudu developer forgets to initialize. PS1, Line 382: static const struct TestKeyInfo { : const string fpath; : const char* data; : } kKeyInfo[] = { : { : ca_private_key_file_, : kCaPrivateKey_, : }, : { : ca_exp_private_key_file_, : kCaExpPrivateKey_, : }, : }; hrm, I think this would be simpler to read in a more C++-style: const vector<pair<string, string>> key_info = { {ca_private_key_file_, kCaPrivateKey_}, {ca_exp_private_key_file_, kCaExpPrivateKey_} }; and then a simple foreach loop PS1, Line 614: transformtion typo -- To view, visit http://gerrit.cloudera.org:8080/5672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
