Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups ...................................................................... Patch Set 14: (8 comments) Thanks for all the changes. I've made another pass and have some more questions. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/frontend.cc@61 PS14, Line 61: "Specifies the set of authorized proxy groups (users who can delegate to other " : "groups during authorization) and whom they are allowed to delegate. " For other components that have done this, how have they documented this? I don't think it's quite right to say "delegate to other groups", because the delegation actually happens to a user. This is saying that a given user can delegate to any user in the named groups, which is slightly different than delegating to the group. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/frontend.cc@66 PS14, Line 66: "changed via --authorized_proxy_group_config_delimiter), or '*' to indicate all " What does "*" mean here? If you wanted to be able to delegate to anyone, you'd use the user mechanism. I'm not sure "*" makes sense in context of groups. Does it? http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.h@1031 PS14, Line 1031: /// Map of short usernames of authorized proxy users to the set of group(s) they are nit: "set of groups" and ("set of users" on line 1027). There's no reading here where you want "set of group". http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@285 PS14, Line 285: Status status = exec_env_->frontend()->ValidateSettings(request); I believe the way we typically do this is TBackendGflags. I think you can update backend-gflag-util.cc to populate that. Then you don't need this. See JniFrontend.java's constructor for how it's initialized. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@417 PS14, Line 417: boost::trim(proxy_user); The old impl didn't trim. I think that's fine, but wanted to check that's a conscious choice we're making; I don't think it was mentioned in the commit message. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1382 PS14, Line 1382: boost::unordered_set<string> users = proxy_user->second; I think we're slowly migrating from boost::unordered_set to the c++11 equivalent: $git grep unordered_set | grep '#include' | awk -F: '{ print $2 }' | sort | uniq -c 19 #include <boost/unordered_set.hpp> 22 #include <unordered_set> You don't have to, but if you wanted to change this away from boost, I think it would be welcome and trivial. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1393 PS14, Line 1393: boost::unordered_set<string> groups = proxy_group->second; You're referring to proxy_group here without checking if it's groups.end() (which you do on line 1394). So, something's wrong here, or I'm mis-reading this. If this is wrong, please add a test that would have caught it? http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1402 PS14, Line 1402: VLOG_QUERY << "Getting Hadoop groups took " << If we're going to log it, let's include the short_user here as well. -- To view, visit http://gerrit.cloudera.org:8080/10510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 14 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 07 Jun 2018 04:09:18 +0000 Gerrit-HasComments: Yes
