Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14433 )
Change subject: IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use krb5_parse_name() instead ...................................................................... Patch Set 5: (7 comments) This is all looking close. I have some picky comments http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@10 PS5, Line 10: creating our own. instead of using custom code. http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@12 PS5, Line 12: As src/kudu/security/init.cc already have g_krb5_ctx initialized, The reader of commit messages is developers like you and me. Will the reader know what 'g_krb5_ctx' is? I think you want to say something like: "When kerberos is initialized in Impala's copy of Kudu code, it stores a global context which is used when accessing the Krb5 library. To use this global context the code that parses the principal name is moved into the Impala Kudu code. This new code is then called from the existing ParseKerberosPrincipal method." http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@18 PS5, Line 18: Add an authentication-test to verify principal with special character. to verify parsing a principal nae containing a special character. http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@19 PS5, Line 19: Manually tested with bad format principal, throw out error code 2 Is it possible to have an automated test for this? http://gerrit.cloudera.org:8080/#/c/14433/5//COMMIT_MSG@21 PS5, Line 21: Do you want to mention running end-to-end tests? http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/kudu/security/init.h File be/src/kudu/security/init.h: http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/kudu/security/init.h@39 PS5, Line 39: // Convert a string representation of a principal name to a krb5_principal structure. Maybe: "Parse a kerberos principal name and extract the ervice_name, hostname, and realm from it." http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/util/auth-util.cc File be/src/util/auth-util.cc: http://gerrit.cloudera.org:8080/#/c/14433/5/be/src/util/auth-util.cc@92 PS5, Line 92: hostname, realm),"bad principal format " + principal); It is more idiomatic and marginally more efficient to use: Substitute("bad principal name $0", principal) -- To view, visit http://gerrit.cloudera.org:8080/14433 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24 Gerrit-Change-Number: 14433 Gerrit-PatchSet: 5 Gerrit-Owner: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Comment-Date: Tue, 22 Oct 2019 00:08:11 +0000 Gerrit-HasComments: Yes
