Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8247 )
Change subject: Allow configuration of values passed into kebreros env vars ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG@7 PS1, Line 7: Allow configuration of values passed into kebreros env vars typo http://gerrit.cloudera.org:8080/#/c/8247/1//COMMIT_MSG@15 PS1, Line 15: arguments to InitKerberosForServer(). mind adding a sentence here saying _why_ you want to make them configurable? ie what's your use case in Impala? (iirc it's because you depend on the kinits done within the C++ side to get credentials into a cache that's then read by the Java side, right?) http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.h File src/kudu/security/init.h: http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.h@41 PS1, Line 41: FLAGS_keytab_file is used if this is empty I think using boost::optional<> is nicer here http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@470 PS1, Line 470: setenv("KRB5_KTNAME", krb5_ktname.c_str(), 1); hrm, given that this is already configurable via a flag, why do we need it as a parameter? Couldn't impala just set FLAGS_keytab_file as necessary? Also, it seems a little confusing that, if you override it here, you'll ignore the keytab, but then we have a bunch of other references to FLAGS_keytab_file in the rpc/ library which wouldn't match up with the keytab that got logged-in from, right? http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@467 PS1, Line 467: if (krb5_ktname.empty()) { : setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1); : } else { : setenv("KRB5_KTNAME", krb5_ktname.c_str(), 1); : } > This is a little ugly, but making the FLAGS_keytab_file available in the in maybe just scratch the defaults and instead change the call site in server_base.cc to use the flags? http://gerrit.cloudera.org:8080/#/c/8247/1/src/kudu/security/init.cc@473 PS1, Line 473: if (disable_krb5_replay_cache) { why do you need to configure this? We found that the replay cache was a huge scalability and performance issue, so I would definitely recommend that Impala disable it as well. Is the issue that you are also using Kerberos to authenticate Thrift clients which don't have the KRPC nonce? I wonder if there is any way you can selectively disable it for KRPC without disabling for Thrift. -- To view, visit http://gerrit.cloudera.org:8080/8247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404 Gerrit-Change-Number: 8247 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 13 Oct 2017 00:17:39 +0000 Gerrit-HasComments: Yes
