[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-29 Thread Greg Rahn (Code Review) 
Greg Rahn 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:

Update the commit message - IMPALA-5552 vs 5522


--
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 
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 05:11:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-29 Thread Philip Zeyliger (Code Review) 
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:

(2 comments)

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);
 : });
> I don't think we can call CLEAN_EXIT_WITH_ERROR since it needs the config o
The error can be more generic saying that there's something invalid about the 
configuration. This isn't a huge deal, but I've not seen use use lambdas for 
error handling.


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 inv
For common Impala deployments, do you know what's in use?

For the deployment that I checked, on a nightly cluster at my employer, I found 
that Cloudera Manager seems to default to 
org.apache.hadoop.security.ShellBasedUnixGroupsMapping. If that's the case, 
we'll be forking per-query, and my sense of the comments at 
https://issues.apache.org/jira/browse/IMPALA-5624 is that we've been trying 
pretty hard to avoid it.

I'd go so far as saying that we should check to see what class the user group 
mapping provider is, and, if it's in a list of well-known forkers, we should 
fail.



--
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 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 30 May 2018 03:00:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-29 Thread Fredy Wijaya (Code Review) 
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 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 30 May 2018 01:53:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-29 Thread Fredy Wijaya (Code Review) 
Fredy Wijaya has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/10510 )

Change subject: IMPALA-5522: Add support for authorized proxy groups
..

IMPALA-5522: 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=*

Testing:
- 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
M tests/authorization/test_authorization.py
10 files changed, 274 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/3
--
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: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-29 Thread Philip Zeyliger (Code Review) 
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 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 29 May 2018 21:17:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5522: Add support for authorized proxy groups

2018-05-24 Thread Fredy Wijaya (Code Review) 
Fredy Wijaya has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10510


Change subject: IMPALA-5522: Add support for authorized proxy groups
..

IMPALA-5522: 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=*

Testing:
- Added a new test in test_authorization.py
- Ran all end-to-end test_authorization.py

Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb
---
M be/src/service/frontend.cc
M be/src/service/frontend.h
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
M tests/authorization/test_authorization.py
8 files changed, 166 insertions(+), 31 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10510/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: newchange
Gerrit-Change-Id: I6953f89c293b06b72f523b11802232133d9d6cbb
Gerrit-Change-Number: 10510
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya