Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups ...................................................................... Patch Set 16: (8 comments) 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 " : "users belonging to the specified groups during authorization) and who > For other components that have done this, how have they documented this? Borrowing some words from: https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/Superusers.html Done. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/frontend.cc@66 PS14, Line 66: "may be changed via --authorized_proxy_group_config_delimiter), or '*' to indicate " > What does "*" mean here? If you wanted to be able to delegate to anyone, yo That's true, but then they're forced to use the user flag just for *. I think it's nice to also have * in the group flag since users don't have to user the user flag if they don't want to. As a reference, Oozie supports * in groups: http://doc.mapr.com/display/MapR/User+Impersonation+for+Oozie I'm open to removing * if you still think it doesn't make any sense for groups. 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 groups they are > nit: "set of groups" and ("set of users" on line 1027). There's no reading Done 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: LOG(ERROR) << status.GetDetail(); > I believe the way we typically do this is TBackendGflags. I think you can u Done http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@417 PS14, Line 417: boost::trim(config_str); > The old impl didn't trim. I think that's fine, but wanted to check that's a I'll update the comment. I believe user and group names don't allow spaces. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1382 PS14, Line 1382: users.find(do_as_user) != users.end()) { > I think we're slowly migrating from boost::unordered_set to the c++11 equiv I can change all boost::unordered_set into std::unordered_set, but it's probably better to not do it in this CR. http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1393 PS14, Line 1393: if (groups.find("*") != groups.end()) return Status::OK(); > You're referring to proxy_group here without checking if it's groups.end() Done http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1402 PS14, Line 1402: if (!status.ok()) { > If we're going to log it, let's include the short_user here as well. Done -- 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: 16 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 18:38:07 +0000 Gerrit-HasComments: Yes
