Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5522: Add support for authorized proxy groups ...................................................................... Patch Set 2: (5 comments) My main concern is whether this forks when evaluating the groups. 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 do other systems deal with this? Do they just disallow groups with commas in them? Note that I didn't see a test for custom delimeters. 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 error out with) adds anything as opposed to: if (!AddAuthorizedProxyConfig(...).ok()) { CLEAN_EXIT_WITH_ERROR("....") } seems clearer and is consistent with other stuff nearby. 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 that checks the parsing explicitly. 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())); Does this cause a fork()/clone() or equivalent? We've seen problems with Impala calling out to kinit, causing a fork(), causing failures because individual impalad's can be quite big and run the system out of memory. Historically, implementations of Hadoop's user-group-mapping stuff called out to "groups". 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: ','.join(get_groups()), Please explain what this test is doing. I think this test requires that a user be in at least one group. For line 193, I don't think you need all the groups; you just need to be in one group. Do we need a test that lets you in via user but not via groups, or vice-versa? i.e., do the two settings work together reasonably? -- 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: 2 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: Tue, 29 May 2018 21:17:45 +0000 Gerrit-HasComments: Yes
