[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-22 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 10: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 23 Nov 2017 03:25:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach 
Amsden, Alex Behm,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#10).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have an invalid Role " ",
and SHOW ROLES statement was executed from impala shell to see
how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/10
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS9, Line 146: } catch (AuthorizationException e) {
> The original issue that prompted this JIRA saw listRolePrivileges() throwin
Ok so changing AuthorizationException to ImpalaException



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 22 Nov 2017 00:24:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 9: Code-Review-1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS9, Line 146: } catch (AuthorizationException e) {
The original issue that prompted this JIRA saw listRolePrivileges() throwing a 
org.apache.sentry.provider.db.SentryNoSuchObjectException. I don't think this 
case is correctly handled because that will be returned as an ImpalaException.

I was following the arguments about handling generic exceptions or specific 
exceptions. We ended up with a solution that does not address the original 
issue.

I vote for catching ImpalaException here or for considerably revamping the 
exception handling in listRolePrivileges() to make this clearer.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 23:50:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: Exception e) {
> I feel we should catch the specific set of exceptions documented for the AP
I agree with Zach's thought that we won't be changing from the current behavior 
of this code except for the case when an AuthorizationException is encountered.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 21:25:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Zach Amsden (Code Review) 
Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> While I agree that catching a generic Exception is a bad practice, my point
I feel we should catch the specific set of exceptions documented for the API 
(exposed in their throws specification) that we actually want to do something 
with (in this case AuthorizationException), and bail completely if unknown 
exceptions arise (IOException, Timeout, etc...) because if that happens we 
don't fully understand how to continue the operation.

In that case, the safest thing to do is abort, which is done easily enough by 
just not catching the generic Exception.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:40:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Bharath Vissapragada (Code Review) 
Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> I'm not sure I agree with Bharath. We don't catch generic exceptions (like
While I agree that catching a generic Exception is a bad practice, my point was 
that the projects we rely on have a habit of breaking APIs.  Reason being, 
there is no way to enforce the API contracts, except to realize after a third 
party upgrade that things start failing to compile.

That said, I'm fine if we catch very specific Exception for this particular 
case, if everyone else thinks that's a better idea. Pranay, sorry for the back 
and forth.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:23:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Philip Zeyliger (Code Review) 
Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 8:

(1 comment)

I'm not too familiar with the details here, but I noticed that 
TSentryGrantOption can be false as well as true (and unset). Could a role deny 
privileges? If so, does removing that role (because we couldn't load it for 
some reason) potentially increase the privileges of the person operating the 
query?

In other words, when we fail, do we need to leave a marker to indicate that 
role X is poisonous and we should fail anything that needs it?

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: Exception e) {
> Changed the code to handle all exceptions
I'm not sure I agree with Bharath. We don't catch generic exceptions (like 
ConnectionException, timeouts, NPE, ClassNotFound, and so on) for things like 
HDFS operations. Sentry is supposed to have an API such that we know what 
exceptions it can and can't throw, and we should catch based on that.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:02:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#8).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have an invalid Role " ",
and SHOW ROLES statement was executed from impala shell to see
how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/8
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#7).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/7
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 7
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@148
PS5, Line 148:   LOG.error("Error listing the Role name:" + 
roleName, e);
> Would be nice to have a space after the ':'
Done



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:47:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#6).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/6
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@144
PS5, Line 144: sentryPrivlist =
 :   
sentryPolicyService_.listRolePrivileges(processUser_, role.getName());
> indentation is off
Corrected the identation.


http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> An AuthorizationException is thrown when the sentry client throws a SentryA
Changed the code to handle all exceptions



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:40:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Bharath Vissapragada (Code Review) 
Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> An AuthorizationException is thrown when the sentry client throws a SentryA
IMO, we should catch a generic Exception given we have no control over what 
listRolePrivileges can throw and it can change over time. We should probably 
not let a single bad role cause the entire thread to throw (we do it at 
multiple other places too in the code, fwiw).



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:36:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Zach Amsden (Code Review) 
Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> An AuthorizationException is thrown when the sentry client throws a SentryA
With the current version of Sentry being pulled in, catching 
SentryAccessDeniedException should be sufficient.  Upstream things are a bit 
different (I am working on fixing this).


http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@148
PS5, Line 148: LOG.error("Error listing the Role name:" + 
roleName, e);
Would be nice to have a space after the ':'



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:35:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Dimitris Tsirogiannis (Code Review) 
Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@144
PS5, Line 144: sentryPrivlist =
 :   
sentryPolicyService_.listRolePrivileges(processUser_, role.getName());
indentation is off


http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
An AuthorizationException is thrown when the sentry client throws a 
SentryAccessDeniedException. In all other cases, listRolePrivileges throws an 
InternalException. Are you certain that this try/catch will correctly handle 
all the error cases?



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:08:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-21 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#5).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/5
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-20 Thread Zach Amsden (Code Review) 
Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 4:

(3 comments)

What version of Sentry was causing this issue?  Also, what was the exact 
exception being throw?

http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@141
PS4, Line 141:   // need to be removed.
It's fine to leave this as is, the 90 column limit is not exceeded.


http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS4, Line 146:   
sentryPolicyService_.listRolePrivileges(processUser_, role.getName())) {
I think it would be better to call listRolePrivileges directly and catch 
exceptions from that instead of at the end of the function.  If the role 
doesn't exist anymore, we definitely want to remove all its privileges from the 
catalog instead of going on to exception handling.


http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@168
PS4, Line 168: } catch (Exception e) {
Can you catch a more specific exception?  For example, we probably wouldn't 
want to catch a NullPointerException here.



--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:57:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

2017-11-20 Thread Pranay Singh (Code Review) 
Hello Bharath Vissapragada, Philip Zeyliger, Zach Amsden,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8588

to look at the new patch set (#4).

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 31 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/8588/4
--
To view, visit http://gerrit.cloudera.org:8080/8588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden