Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups ...................................................................... Patch Set 11: (10 comments) Few more comments, looks pretty close. http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/frontend.h@156 PS11, Line 156: Status GetHadoopGroups(const TGetHadoopGroupsRequest& request, Comment http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@401 PS8, Line 401: const string& authorized_proxy_config_delimiter, : AuthorizedProxyMap& authorized_proxy_config_map > Done. I think we should use pointers for output params for nullable params. I'm not the expert here but I don't think we worry about the nullability aspect. You could find multiple places in this class that just pass a pointer. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: () > 0) { > Maybe we can print the execution time? If there was an exception thrown whe Yep, let's attach a timer to this and log it. http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc@1405 PS11, Line 1405: boost::unordered_set<string> groups = proxy_group->second; Move out of the loop? http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc@1406 PS11, Line 1406: groups.find("*") != groups.end() Can we move it before L1394? If this passes, we don't need to do a group lookup. http://gerrit.cloudera.org:8080/#/c/10510/11/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10510/11/common/thrift/Frontend.thrift@834 PS11, Line 834: Check wither to check update http://gerrit.cloudera.org:8080/#/c/10510/11/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/11/fe/src/main/java/org/apache/impala/service/JniFrontend.java@627 PS11, Line 627: checkGroupsMappingProvider(CONF); Do we need to do this everytime? http://gerrit.cloudera.org:8080/#/c/10510/11/fe/src/main/java/org/apache/impala/service/JniFrontend.java@653 PS11, Line 653: * Check if the groups mapping provider implementation is known to be problematic. Returns..... http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py@200 PS11, Line 200: @pytest.mark.execute_serially : @CustomClusterTestSuite.with_args("--server_name=server1\ : --authorization_policy_file=%s\ : --authorized_proxy_group_config=hue=%s\ : --authorized_proxy_group_config_delimiter=@\ : --abort_on_failed_audit_event=false\ : --audit_event_log_dir=%s" % (AUTH_POLICY_FILE, : '@'.join(get_groups()), : AUDIT_LOG_DIR)) : def test_group_impersonation_with_custom_delimiter(self): : """End-to-end group impersonation with custom delimiter + authorization test""" : self.__test_impersonation() I don't think this is required. Custom cluster tests are usually expensive (since it requires Impala restarts). We already seem to be having a unit test to check if custom delimiters are being used. http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py@224 PS11, Line 224: group impersonation We don't seem to be delegating it to a group user in __test_impersonation() IIUC. Just curious if that I something the test actually should do? Otherwise, we just pass this config, the codepath to do "getHadoopGroups()" is not triggered since there is a proxy_user already. Correct? -- 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: 11 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Jun 2018 21:52:00 +0000 Gerrit-HasComments: Yes
