[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 17: Code-Review+2 -- 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: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 14 Jun 2018 22:26:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. The authorized_proxy_user/group_config parser will now strip leading and trailing whitespaces from the user/group names. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Reviewed-on: http://gerrit.cloudera.org:8080/10510 Tested-by: Impala Public Jenkins Reviewed-by: Tim Armstrong --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 13 files changed, 384 insertions(+), 36 deletions(-) Approvals: Impala Public Jenkins: Verified Tim Armstrong: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 18 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 17: Verified+1 -- 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: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 14 Jun 2018 16:12:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 17: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2668/ -- 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: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 14 Jun 2018 12:48:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 17: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2661/ -- 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: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 14 Jun 2018 09:35:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 17: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2661/ -- 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: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 13 Jun 2018 23:35:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger 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: Code-Review+1 This looks fine to me at this point. I'm going to kick off GVO, but I've not +2'd it because Tim's still gating commits. I'll poke him to see if he wants to hold off on this or not. -- 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 Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 13 Jun 2018 23:35:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 07 Jun 2018 18:38:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#16). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. The authorized_proxy_user/group_config parser will now strip leading and trailing whitespaces from the user/group names. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 13 files changed, 384 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/16 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 16 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups
..
Patch Set 14:
(8 comments)
Thanks for all the changes. I've made another pass and have some more questions.
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 "
: "groups during authorization) and whom they are allowed to
delegate. "
For other components that have done this, how have they documented this?
I don't think it's quite right to say "delegate to other groups", because the
delegation actually happens to a user. This is saying that a given user can
delegate to any user in the named groups, which is slightly different than
delegating to the group.
http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/frontend.cc@66
PS14, Line 66: "changed via --authorized_proxy_group_config_delimiter), or
'*' to indicate all "
What does "*" mean here? If you wanted to be able to delegate to anyone, you'd
use the user mechanism. I'm not sure "*" makes sense in context of groups.
Does it?
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 group(s) they are
nit: "set of groups" and ("set of users" on line 1027). There's no reading here
where you want "set of group".
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: Status status =
exec_env_->frontend()->ValidateSettings(request);
I believe the way we typically do this is TBackendGflags. I think you can
update backend-gflag-util.cc to populate that. Then you don't need this.
See JniFrontend.java's constructor for how it's initialized.
http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@417
PS14, Line 417: boost::trim(proxy_user);
The old impl didn't trim. I think that's fine, but wanted to check that's a
conscious choice we're making; I don't think it was mentioned in the commit
message.
http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1382
PS14, Line 1382: boost::unordered_set users = proxy_user->second;
I think we're slowly migrating from boost::unordered_set to the c++11
equivalent:
$git grep unordered_set | grep '#include' | awk -F: '{ print $2 }' | sort |
uniq -c
19 #include
22 #include
You don't have to, but if you wanted to change this away from boost, I think it
would be welcome and trivial.
http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1393
PS14, Line 1393: boost::unordered_set groups = proxy_group->second;
You're referring to proxy_group here without checking if it's groups.end()
(which you do on line 1394). So, something's wrong here, or I'm mis-reading
this.
If this is wrong, please add a test that would have caught it?
http://gerrit.cloudera.org:8080/#/c/10510/14/be/src/service/impala-server.cc@1402
PS14, Line 1402: VLOG_QUERY << "Getting Hadoop groups took " <<
If we're going to log it, let's include the short_user here as well.
--
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: 14
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 07 Jun 2018 04:09:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 14: Code-Review+1 Phil, do you want to +2? -- 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: 14 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 07 Jun 2018 03:12:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 380 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/14 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 14 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc@1394
PS13, Line 1394: if (groups.find("*") != groups.end()) return Status::OK();
:
: i
> nit: single line, no braces (same below)
Done
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc@1406
PS13, Line 1406: );
> nit: PrettyPrinter::Print(MonotinicMillis() - start, TUnit::TIME_MS)
Done
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@224
PS11, Line 224: ent_resp = self.hs2
> I see. That coverage could be achieved by modifying the args for the test t
That works. 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: 14
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 07 Jun 2018 01:16:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 13:
(3 comments)
LGTM once my comments are addressed.
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc@1394
PS13, Line 1394: if (groups.find("*") != groups.end()) {
: return Status::OK();
: }
nit: single line, no braces (same below)
http://gerrit.cloudera.org:8080/#/c/10510/13/be/src/service/impala-server.cc@1406
PS13, Line 1406: end
nit: PrettyPrinter::Print(MonotinicMillis() - start, TUnit::TIME_MS)
remove L1404
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@224
PS11, Line 224: er is 'hue'
> Yes, for this particular test, the code path to getHadoopGroups will not be
I see. That coverage could be achieved by modifying the args for the test
test_group_impersonation (above) by adding
--authorized_proxy_user_config=hue=something_that_is_never_used
Can we do that instead?
--
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: 13
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 07 Jun 2018 00:05:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 13:
(10 comments)
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: /// Returns (in the output parameter) the list of groups for
the given user.
> Comment
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@401
PS8, Line 401: const string& authorized_proxy_config_delimiter,
: AuthorizedProxyMap* authorized_proxy_config_map
> I'm not the expert here but I don't think we worry about the nullability as
Done
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389
PS8, Line 1389: () > 0) {
> Yep, let's attach a timer to this and log it.
Done
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: OG_QUERY << "Getting Hadoop groups took " <<
> Move out of the loop?
Done
http://gerrit.cloudera.org:8080/#/c/10510/11/be/src/service/impala-server.cc@1406
PS11, Line 1406: rettyPrinter::Print(end - start,
> Can we move it before L1394? If this passes, we don't need to do a group lo
Yeah good idea. Done.
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: A flag to indicate wh
> update
Done
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: TGetHadoopGroupsRequest request =
> Do we need to do this everytime?
Oops. That was unintentional. Done.
http://gerrit.cloudera.org:8080/#/c/10510/11/fe/src/main/java/org/apache/impala/service/JniFrontend.java@653
PS11, Line 653:* provider implementation.
> Returns.
Done
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_user_config=hue=%s\
: --authorized_proxy_group_config=hue=%s\
: --abort_on_failed_audit_event=false\
: --audit_event_log_dir=%s" % (AUTH_POLICY_FILE,
: getuser(),
: ','.join(get_groups()),
: AUDIT_LOG_DIR))
: def test_user_and_group_impersonation(self):
: """End-to-end user and grou
> I mean "custom delimiters are being parsed correctly."
Will remove. Done.
http://gerrit.cloudera.org:8080/#/c/10510/11/tests/authorization/test_authorization.py@224
PS11, Line 224: er is 'hue'
> We don't seem to be delegating it to a group user in __test_impersonation()
Yes, for this particular test, the code path to getHadoopGroups will not be
called. The goal is to make sure having the two authorized user and groups
configs still work. I can remove this if you think this test doesn't make sense.
--
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: 13
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 06 Jun 2018 22:56:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 396 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/13 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 401 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/11 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 11: (1 comment) 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@659 PS8, Line 659: // This can cause issues such as zombie processes, running out of file descriptors, > So, for checking, all you need is Sure that works, too. 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: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 06 Jun 2018 17:37:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 8: (1 comment) 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@659 PS8, Line 659: GroupMappingServiceProvider provider = > I need to know the current implementation of GroupMappingServiceProvider. T So, for checking, all you need is Configuration.get(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING). Why not use 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: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 06 Jun 2018 17:25:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 10:
(2 comments)
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_delimiter,
authorized_proxy_user_config_);
: if (!status.ok()) {
: CLEAN_EXIT_WITH_ERROR(Substitute("Invalid proxy user
configuration."
: "No mapping value specified for the proxy user. For
more information review "
:
> I still think you should get rid of the lambda.
Done
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@659
PS8, Line 659:
conf.getClass(CommonConfigurationKeys.HADOOP_SECURITY_GROUP_MAPPING,
> It still doesn't belong here. If you need it for the test, change the signa
I need to know the current implementation of GroupMappingServiceProvider.
There's no way to get that from Groups since Groups isn't an implementation of
GroupMappingServiceProvider. Groups is a wrapper class that contains an
implementation of GroupMappingServiceProvider which unfortunately is private.
--
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: 10
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 06 Jun 2018 17:17:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 8:
(1 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@320
PS8, Line 320: [](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);
: }
> I still think you should get rid of the lambda.
Yea, not totally sure I follow. The caller knows the context in which the
function is being called right?
--
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: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 06 Jun 2018 16:53:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups
..
Patch Set 8:
(2 comments)
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: [](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);
: }
> I had the same discussion in my earlier review. The problem is we need to d
I still think you should get rid of the lambda.
if (!AddAuthorizedProxyConfig(...).ok()) {
CLEAN_EXIT_WITH_ERROR(...)
}
You know that you're dealing with users because you call this function twice,
so you can just change the error message here in the caller.
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@659
PS8, Line 659: GroupMappingServiceProvider provider =
> It's the impl in the GROUPS which unfortunately is private. I copied the co
It still doesn't belong here. If you need it for the test, change the signature
of checkGroupsMappingProvider() to take a GroupMappingServiceProvider instead,
and do the reflection business in the test.
--
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: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 06 Jun 2018 15:59:54 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 8:
(13 comments)
Just came across this CR. I have some comments. Will look at tests in the next
round.
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: /// Adds authorized proxy config into the given map.
Maybe a little more verbose? The context is not clear by just reading this
comment
/// Utility to parse --foo and --bar and populate .
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.h@820
PS8, Line 820: AddAuthorizedProxyConfig
nit: How about we call it PopulateAuthorizedProxyConfig()?
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: [](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);
: }
I think using 'Status' as the return of AddAuthorizedProxyConfig() is an Impala
way of doing this (caller checks for !status.ok()). I don't see why we should
unnecessarily complicate this function signature with a lambda. thoughts?
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@401
PS8, Line 401: const string& authorized_proxy_config,
: const string& authorized_proxy_config_delimiter
Input params precede output vals [1]. Also we generally use output params as
pointers.
[1] https://google.github.io/styleguide/cppguide.html#Output_Parameters
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@425
PS8, Line 425: parsed_allowed_users_or_groups.begin(),
parsed_allowed_users_or_groups.end());
nit: should we check for duplicate values instead of overwriting? ex:
foo=bar;foo=car
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1386
PS8, Line 1386: TGetHadoopGroupsRequest req;
Could you add a comment here that breiefly summarizes what we are doing
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1389
PS8, Line 1389: GetHadoopGroups
I have seen issues around "GroupMapping" (especially with LDAP etc) taking
longer than expected in the HDFS land. I'd assume we can run into similar
issues, especially if we are calling this for every OpenSession(). Any thoughts
on what we can do to help debug such issues without having to take thread dumps
and then figuring it out.
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1391
PS8, Line 1391: There was an
nit: remove
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1397
PS8, Line 1397: AuthorizedProxyMap::const_iterator proxy_group =
: authorized_proxy_group_config_.find(short_user);
: if (proxy_group != authorized_proxy_group_config_.end()) {
Move it outside of the loop? Also can we bypass the GetHadoopGroups() if this
fails in the first place?
http://gerrit.cloudera.org:8080/#/c/10510/8/be/src/service/impala-server.cc@1400
PS8, Line 1400: for (const string& group : proxy_group->second) {
: if (group == "*" || group == do_as_group) return
Status::OK();
: }
Why are we looping here? Isn't this a set? Could we find() in O(1) ?
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: GROUPS
Is this thread safe?
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 not
working as expected.
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 ||
: provider instanceof ShellBasedUnixGroupsNetgroupMapping)
{
Please add comments as to why these are problematic.
--
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:
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups
..
Patch Set 8:
(5 comments)
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@1389
PS8, Line 1389: Status status = exec_env_->frontend()->GetHadoopGroups(req,
);
Does this code run even if the flag isn't set? If so, someone upgrading but
using a "bad" UserGroupMapping provider will start getting the wrong message.
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@657
PS8, Line 657: protected static void checkGroupsMappingProvider(Configuration
conf)
We should be doing this at start-up rather than once per request. The issue
isn't so much that it's expensive, but that you want to fail fast on this sort
of configuration error.
If we do it at boot time, we should also log the value of this configuration.
http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@659
PS8, Line 659: GroupMappingServiceProvider provider =
Isn't this just GROUPS?
http://gerrit.cloudera.org:8080/#/c/10510/8/fe/src/main/java/org/apache/impala/service/JniFrontend.java@665
PS8, Line 665: Map recommendations = new HashMap<>();
: recommendations.put(
: ShellBasedUnixGroupsMapping.class.getName(),
: JniBasedUnixGroupsMappingWithFallback.class.getName());
: recommendations.put(
: ShellBasedUnixGroupsNetgroupMapping.class.getName(),
:
JniBasedUnixGroupsNetgroupMappingWithFallback.class.getName());
This could go into the if below.
But, really, just use a longer string. I think the clever map for
recommendations takes away from the point. You can also just use an if/else for
the two cases with the same number of lines.
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() {
This seems like a good place to check the user group mapping once and for all.
Please look into what it does, of course.
--
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: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 05 Jun 2018 22:32:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 8: Philip, can you review this and give a +2 if you don't have any concerns? -- 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: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 05 Jun 2018 21:11:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 8: Code-Review+1 (1 comment) Carry Vuk's +1 http://gerrit.cloudera.org:8080/#/c/10510/5/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/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@635 PS5, Line 635: sues.apach > thx for the info.. yeah that's ugly. if there is a jira to either fix this 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: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 30 May 2018 21:10:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 385 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/8 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 7: Code-Review+1 (1 comment) thx for the changes. lgtm, but let others settle on their reviews as well. http://gerrit.cloudera.org:8080/#/c/10510/5/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/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@635 PS5, Line 635: startsWith > Unfortunately there's no way to do this at the moment. I agree this is very thx for the info.. yeah that's ugly. if there is a jira to either fix this api or for impala to use something else, please add it. in either case, pls add a comment about why this is done (e.g., // HACK: ... ) -- 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: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 30 May 2018 20:13:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@406
PS5, Line 406: thorized_proxy_user_config or FLAGS_authorized_proxy_group_config
> can be specific to to the authorized_proxy_config for which this method was
I initially did that with lambda:
https://gerrit.cloudera.org/c/10510/2/be/src/service/impala-server.cc#323. Let
me know what you think about it and I can use the old code back.
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@412
PS5, Line 412: st string& config: proxy_confi
> if ws is included around a group or user string, e.g., " * ", will (and s
Make sense I'll update it.
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@416
PS5, Line 416:
> can use a literal pair constructor: { ... }
Done
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@1383
PS5, Line 1383: if (user == "*" |
> its an error, no need to wrap.
Done
http://gerrit.cloudera.org:8080/#/c/10510/5/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/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@635
PS5, Line 635: startsWith
> This makes the error messages in the security package part of the api. Is t
Unfortunately there's no way to do this at the moment. I agree this is very
ugly. This is another example in Impala that relies on the same exact exception
message:
https://github.com/apache/impala/blob/e9bd917a218b5e1717fced983f70f64850c6e02f/fe/src/main/java/org/apache/impala/util/RequestPoolService.java#L302
The source in Hadoop API:
https://github.com/apache/hadoop/blob/47c31ff16b452d47afc6ffc1cf936ac2de9b788d/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Groups.java#L198-L200
--
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: 7
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 30 May 2018 18:55:41 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 382 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/7 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/10510/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10510/5//COMMIT_MSG@11 PS5, Line 11: impalad > question(s) on the design, since I've not seen this feature (or the related This is consistent with the existing user delegation flags. For what it's worth, we have a many options (including HDFS's core-site.xml and hadoop-site.xml) where effectively you need coherent configs across the cluster. We assume people use tools to make it happen. -- 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: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 30 May 2018 18:17:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 367 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/6 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/10510 )
Change subject: IMPALA-5552: Add support for authorized proxy groups
..
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10510/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10510/5//COMMIT_MSG@11
PS5, Line 11: impalad
question(s) on the design, since I've not seen this feature (or the related,
user delegation support). are these flags only relevant for coordinators? why
are these flags added to impalad? is there a case for different impalads having
different auth lists? if there is no case, then this looks like it opens up
opportunities for misconfiguration. perhaps catalogd is a better place for
this? (I could be wrong on this since I didn't see how these proxies are used)
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@406
PS5, Line 406: --authorized_proxy_user_config flag or
--authorized_proxy_group_config
can be specific to to the authorized_proxy_config for which this method was
called.
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@412
PS5, Line 412: parsed_allowed_users_or_groups
if ws is included around a group or user string, e.g., " * ", will (and
should) that ws be trimmed from begin/end?
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@416
PS5, Line 416: make_pair(
can use a literal pair constructor: { ... }
http://gerrit.cloudera.org:8080/#/c/10510/5/be/src/service/impala-server.cc@1383
PS5, Line 1383: RETURN_IF_ERROR(s);
its an error, no need to wrap.
http://gerrit.cloudera.org:8080/#/c/10510/5/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/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@635
PS5, Line 635: startsWith
This makes the error messages in the security package part of the api. Is there
any existing alternative to avoid this? If not, is there a JIRA filed to change
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: 5
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 30 May 2018 17:47:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
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 5:
(2 comments)
> Patch Set 3:
>
> Update the commit message - IMPALA-5552 vs 5522
Fixed the typo.
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: }
:
: if (!FLAGS_authorized_proxy_group_config.empty()) {
: AddAuthorizedProxyConfig(
: aut
> The error can be more generic saying that there's something invalid about t
Done. I made the error more generic and removed the use of lambda.
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:
> For common Impala deployments, do you know what's in use?
That's weird that CM defaults to ShellBasedUnixGroupsMapping and I think that
needs to be updated to use the JNI with fallback implementation instead. FYI,
Sentry also relies calls the same API to get the groups:
https://github.com/apache/sentry/blob/97f666345c1c068fed56a29512f9dcea77e44c8a/sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java#L60
I added a check to fail hard when the groups mapping provider is a shell-based
implementation.
--
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: 5
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Greg Rahn
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 30 May 2018 06:28:23 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5552: Add support for authorized proxy groups
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10510 ) Change subject: IMPALA-5552: Add support for authorized proxy groups .. IMPALA-5552: Add support for authorized proxy groups The patch adds support for mapping of users to a list of proxy groups. The following flags are added in impalad: - authorized_proxy_group_config - authorized_proxy_group_config_delimiter Example: --authorized_proxy_group_config=hue=group1,group2;user1=* This feature is not supported on Shell-based Hadoop groups mapping providers. Testing: - Added FE unit test to check for groups mapping provider - Added BE unit test for the parsing logic - Added a new test in test_authorization.py - Ran all end-to-end test_authorization.py Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb --- M be/src/service/CMakeLists.txt M be/src/service/frontend.cc M be/src/service/frontend.h A be/src/service/impala-server-test.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/util/backend-gflag-util.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/test/java/org/apache/impala/service/JniFrontendTest.java M tests/authorization/test_authorization.py 11 files changed, 364 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/5 -- 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: newpatchset Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb Gerrit-Change-Number: 10510 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Vuk Ercegovac
