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 9: (18 comments) http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h@819 PS8, Line 819: /// Populates authorized proxy config into the given map. > Maybe a little more verbose? The context is not clear by just reading this Done http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h@820 PS8, Line 820: ple: > nit: How about we call it PopulateAuthorizedProxyConfig()? Done 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@320 PS8, Line 320: FLAGS_authorized_proxy_user_config, : FLAGS_authorized_proxy_user_config_delimiter, : [](const string& config) { : return Substitute("Invalid proxy user configuration. No mapping value " : > I think using 'Status' as the return of AddAuthorizedProxyConfig() is an Im I had the same discussion in my earlier review. The problem is we need to differentiate the error message between invalid proxy user vs invalid proxy group. I think it also make debugging a lot easier. If we remove the lambda and use a Status, we need a flag in AuthorizedProxyConfig to indicate whether the error is for user or group, which I don't know if it's a cleaner design since the idea for AddAuthorizedProxConfig is to be more generic regardless whether it's used for user or group. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@401 PS8, Line 401: : > Input params precede output vals [1]. Also we generally use output params a Done. I think we should use pointers for output params for nullable params. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@425 PS8, Line 425: vector<string> parsed_allowed_users_or_groups; > nit: should we check for duplicate values instead of overwriting? ex: foo=b This is the way the existing behavior works for --authorized_proxy_user_config. I don't think it's a good idea to break this in case users rely on this behavior (maybe unknowingly). http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1386 PS8, Line 1386: boost::unordered_set<string> users = proxy_user->second; > Could you add a comment here that breiefly summarizes what we are doing Done http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: return Status::OK(); > Does this code run even if the flag isn't set? If so, someone upgrading but Yeah, that's a good point. The idea is we shouldn't enable this feature if the flag isn't enabled. I'll update the code. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389 PS8, Line 1389: > I have seen issues around "GroupMapping" (especially with LDAP etc) taking Maybe we can print the execution time? If there was an exception thrown when getting the groups, the Java side will throw an InternalException I believe should print the stacktrace. http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1391 PS8, Line 1391: > nit: remove Done http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1397 PS8, Line 1397: if (proxy_group != authorized_proxy_group_config_.end()) { : TGetHadoopGroupsRequest req; : req.__set_user(do_as_user); > Move it outside of the loop? Also can we bypass the GetHadoopGroups() if th Done http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1400 PS8, Line 1400: TGetHadoopGroupsResponse res; : Status status = exec_env_->frontend()->GetHadoopGroups(req, &res); : i > Why are we looping here? Isn't this a set? Could we find() in O(1) ? Yeah, you're totally right. I was just following the existing code for authorized proxy user, but I'll update the authorized proxy user code as well. http://gerrit.cloudera.org:8080/#/c/10510/8/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/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@585 PS8, Line 585: > Is this thread safe? Looking at the implementation, it looks like it's thread safe: https://github.com/apache/hadoop/blob/774c1f199e11d886d0c0a1069325f0284da35deb/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@641 PS8, Line 641: throw new InternalException(e.getMessage()); > Can you LOG the full exception here. Helps debug issues with group mapping Done http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@657 PS8, Line 657: GroupMappingServiceProvider provider = > We should be doing this at start-up rather than once per request. The issue Done http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@659 PS8, Line 659: conf.getClass(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING, > Isn't this just GROUPS? It's the impl in the GROUPS which unfortunately is private. I copied the code below from https://github.com/apache/hadoop/blob/0afc036deb35df7e86ede3dcebc430c8f05ed368/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java#L105-L109 http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@665 PS8, Line 665: // etc. : if (provider instanceof ShellBasedUnixGroupsNetgroupMapping) { : return String.format("Hadoop groups mapping provider: %s is " + : "known to be problematic. Consider using: %s instead.", : provider.getClass().getName(), : JniBasedUnixGroupsNetgroupMappingWithFallback.class.getName()); : } > This could go into the if below. Done http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@672 PS8, Line 672: if (provider instanceof ShellBasedUnixGroupsMapping) { : return String.format("Hadoop groups mapping provider: %s is > Please add comments as to why these are problematic. Done http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@685 PS8, Line 685: public String checkConfiguration(byte[] serializedRequest) throws ImpalaException { > This seems like a good place to check the user group mapping once and for a 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: 9 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 06:55:08 +0000 Gerrit-HasComments: Yes
