Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5522: Add support for authorized proxy groups ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/10510/2/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/10510/2/be/src/service/frontend.cc@69 PS2, Line 69: "Specifies the delimiter used in authorized_proxy_group_config. "); > I see that we're copying an older pattern, but this seems unfortunate. How We can specify a custom delimiter to something other than comma. It just defaults it to comma when the authorized_proxy_user/group_config_delimiter is not specified. I'll add some tests for custom delimiters. http://gerrit.cloudera.org:8080/#/c/10510/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/2/be/src/service/impala-server.cc@319 PS2, Line 319: [](const string& config) { : return Substitute("Invalid proxy user configuration. No mapping value " : "specified for the proxy user. For more information review usage of the " : "--authorized_proxy_user_config flag: $0", config); : }); > Others can comment, but I don't think this pattern (passing a lambda to err I don't think we can call CLEAN_EXIT_WITH_ERROR since it needs the config object for the string substitution, which comes from a loop. http://gerrit.cloudera.org:8080/#/c/10510/2/be/src/service/impala-server.cc@398 PS2, Line 398: void ImpalaServer::AddAuthorizedProxyConfig( > You're doing parsing here. I think it'd be lovely to see a C++ unit test th Done. http://gerrit.cloudera.org:8080/#/c/10510/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/10510/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@623 PS2, Line 623: result.setGroups(GROUPS.getGroups(request.getUser())); It depends on the Hadoop mapping provider implementation and yes it can invoke a subprocess as a fallback. Let me know if there's a better way to get Hadoop groups without using Hadoop API. https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-common/GroupsMapping.html > The default implementation. It will determine if the Java Native Interface > (JNI) is available. If JNI is available, the implementation will use the API > within hadoop to resolve a list of groups for a user. If JNI is not available > then the shell-based implementation, ShellBasedUnixGroupsMapping, is used. http://gerrit.cloudera.org:8080/#/c/10510/2/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/10510/2/tests/authorization/test_authorization.py@193 PS2, Line 193: --audit_event_log_dir=%s" % (AUTH_POLICY_FILE, > Please explain what this test is doing. Yeah that's true. It only needs to be in one group for it to work. I wanted to test to make sure the code works with multiple groups, too. Good point on having authorized user and group settings at the same time. I'll add another test for that. -- 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: 3 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 30 May 2018 01:53:33 +0000 Gerrit-HasComments: Yes
