from:"Adam Holley \(Code Review\)"

[Impala-ASF-CR] IMPALA-7764: Improve SentryProxy test coverage

2018-11-05 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11880 )

Change subject: IMPALA-7764: Improve SentryProxy test coverage
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1190638283f051cc37fda7ea5dd0c5e8a70d40db
Gerrit-Change-Number: 11880
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 05 Nov 2018 22:16:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7794: Rewrite flaky ownership authorization tests

2018-10-31 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11837 )

Change subject: IMPALA-7794: Rewrite flaky ownership authorization tests
..


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic98f8dbec41360261fd0339d835f3ce6b504ee29
Gerrit-Change-Number: 11837
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 31 Oct 2018 17:45:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7794: Rewrite flaky ownership authorization tests

2018-10-31 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11837 )

Change subject: IMPALA-7794: Rewrite flaky ownership authorization tests
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11837/3/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/11837/3/tests/authorization/test_grant_revoke.py@116
PS3, Line 116: "all"
Could you "for privilege in ["all", "select"]"?


http://gerrit.cloudera.org:8080/#/c/11837/3/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11837/3/tests/authorization/test_owner_privileges.py@97
PS3, Line 97: count
count always seems to be 0 in usage.  Any reason to keep that, or just rename 
to "validate_no_user_privileges"?


http://gerrit.cloudera.org:8080/#/c/11837/3/tests/authorization/test_owner_privileges.py@97
PS3, Line 97: validate_user_privilege_count
You can remove the "query" parameter, and just use the user to run "show grant 
user %s" % user.


http://gerrit.cloudera.org:8080/#/c/11837/3/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11837/3/tests/common/sentry_cache_test_suite.py@40
PS3, Line 40: # We ignore the create_time column since it can be NULL.
If it's NULL after invalidate, that would be a problem.


http://gerrit.cloudera.org:8080/#/c/11837/3/tests/common/sentry_cache_test_suite.py@56
PS3, Line 56: validate_privileges
You could get rid of the "query" parameter in this method and add an optional 
"for_role" parameter for the cases where you want to show role privileges.


http://gerrit.cloudera.org:8080/#/c/11837/3/tests/common/sentry_cache_test_suite.py@68
PS3, Line 68: root user
"specified client" - incorrect in original comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic98f8dbec41360261fd0339d835f3ce6b504ee29
Gerrit-Change-Number: 11837
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 31 Oct 2018 15:35:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7760: Privilege version inconsistency causes a hang when running invalidate metadata

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11794 )

Change subject: IMPALA-7760: Privilege version inconsistency causes a hang when 
running invalidate metadata
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1e0db2b1f727476f489c732c4f4e5bc1582429f
Gerrit-Change-Number: 11794
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 26 Oct 2018 03:19:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/11786 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..

IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException

The problem was acache consistency issue between impalad and catalogd.
Because a Sentry refresh was occuring during an update to privileges
from the alter table set owner, impalad had the correct privileges,
which allowed the "show grant role" to succeed but the privileges in
catalogd were being overwritten from the sentry refresh. Added a delay
in the drop call to ensure privileges are updated. This is a
workaround to get the tests to pass with the existing behaviour and
should be reassessed if IMPALA-7763 is implemented.  This would add a
lock to possibly prevent this, but will need it's own assessment.

Testing:
- Ran custom cluster tests 50 times

Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
---
M tests/authorization/test_owner_privileges.py
1 file changed, 21 insertions(+), 16 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
Gerrit-Change-Number: 11786
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11786 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..

IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException

The problem was another cache consistency issue between Impalad and
Catalogd. Because a Sentry refresh was occuring during an update to
privileges from the alter table set owner, Impalad had the correct
privileges, which allowed the "show grant role" to succeed but
the privileges in catalogd were being overwritten from the sentry
refresh. Added a delay in the drop call to ensure privileges are
updated.

Testing:
- Ran custom cluster tests 50 times

Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
---
M tests/authorization/test_owner_privileges.py
1 file changed, 21 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/11786/2
--
To view, visit http://gerrit.cloudera.org:8080/11786
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
Gerrit-Change-Number: 11786
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11786 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11786/1/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11786/1/tests/authorization/test_owner_privileges.py@95
PS1, Line 95: return self.count_user_privileges(result) == count:
:
:   def _test_clea
> Can be simplified to return self.count_user_privileges_result)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
Gerrit-Change-Number: 11786
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 25 Oct 2018 17:36:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11786


Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..

IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException

The problem was another cache consistency issue between Impalad and
Catalogd. Because a Sentry refresh was occuring during an update to
privileges from the alter table set owner, Impalad had the correct
privileges, which allowed the "show grant role" to succeed but
the privileges in catalogd were being overwritten from the sentry
refresh. Added a delay in the drop call to ensure privileges are
updated.

Testing:
- Ran custom cluster tests 50 times

Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
---
M tests/authorization/test_owner_privileges.py
1 file changed, 22 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/11786/1
--
To view, visit http://gerrit.cloudera.org:8080/11786
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5a1babd3dcbb94ffaa1f3e6ef2cebf1a1d391219
Gerrit-Change-Number: 11786
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has abandoned this change. ( http://gerrit.cloudera.org:8080/11723 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
Gerrit-Change-Number: 11723
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11723 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..

IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException

The problem was another cache consistency issue between Impalad and
Catalogd. Because a Sentry refresh was occuring during an update to
privileges from the alter table set owner, Impalad had the correct
privileges, which allowed the "show grant role" to succeed but
the privileges in catalogd were being overwritten from the sentry
refresh. Added a delay in the drop call to ensure privileges are
updated.

Testing:
- Ran custom cluster tests

Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
---
M tests/authorization/test_owner_privileges.py
1 file changed, 22 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11723/2
--
To view, visit http://gerrit.cloudera.org:8080/11723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
Gerrit-Change-Number: 11723
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-18 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11723 )

Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11723/1/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11723/1/tests/authorization/test_owner_privileges.py@229
PS1, Line 229: test_obj.obj_name), user="oo_user1", 
delay_s=sentry_refresh_timeout_s)
> Are you able to reproduce the issue? We have bunch of calls to self.user_qu
I was not able to reproduce manually but reviewing the failed builds, this 
seems to be specific to alter set owner.  I don't want to put the delay on each 
user_query call because of the additional overhead. I created 
https://issues.apache.org/jira/browse/IMPALA-7722 to further investigate.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
Gerrit-Change-Number: 11723
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Thu, 18 Oct 2018 15:33:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7710: test owner privileges with grant failed with AuthorizationException

2018-10-18 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11723


Change subject: IMPALA-7710: test_owner_privileges_with_grant failed with 
AuthorizationException
..

IMPALA-7710: test_owner_privileges_with_grant failed with AuthorizationException

The problem was another cache consistency issue between Impalad and
Catalogd. Because a Sentry refresh was occuring during an update to
privileges from the alter table set owner, Impalad had the correct
privileges, which allowed the "show grant role" to succeed but
the privileges in catalogd were being overwritten from the sentry
refresh. Added a delay in the drop call to ensure privileges are
updated.

Testing:
- Ran custom cluster tests

Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
---
M tests/authorization/test_owner_privileges.py
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11723/1
--
To view, visit http://gerrit.cloudera.org:8080/11723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If8bb2e1fba334ba26e6ae25fbb04c1a5785ac677
Gerrit-Change-Number: 11723
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya

[Impala-ASF-CR] IMPALA-7076: [DOCS] Document ALTER TABLE / VIEW SET OWNER statement

2018-10-15 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11673 )

Change subject: IMPALA-7076: [DOCS] Document ALTER TABLE / VIEW SET OWNER 
statement
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_view.xml
File docs/topics/impala_alter_view.xml:

http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_view.xml@105
PS1, Line 105: An owner of a view has the ALL privilege
> if the ALL privilege is enabled in Sentry? What should be enabled in Sentry
Object ownership in Sentry is controlled with one of three options.  
all_with_grant, all, or none.  But I think this should be documented in Sentry 
and linked to from here because you can't control through Impala config.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I203a800855a413069a40c728dfa157939ea15caf
Gerrit-Change-Number: 11673
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 15 Oct 2018 16:57:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7076: [DOCS] Document ALTER TABLE / VIEW SET OWNER statement

2018-10-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11673 )

Change subject: IMPALA-7076: [DOCS] Document ALTER TABLE / VIEW SET OWNER 
statement
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_table.xml
File docs/topics/impala_alter_table.xml:

http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_table.xml@269
PS1, Line 269:  An
 :   owner of a table has the ALL privilege
Add "if enabled with Sentry", or something to that effect.


http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_view.xml
File docs/topics/impala_alter_view.xml:

http://gerrit.cloudera.org:8080/#/c/11673/1/docs/topics/impala_alter_view.xml@105
PS1, Line 105: An owner of a view has the ALL privilege
if enabled with Sentry.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I203a800855a413069a40c728dfa157939ea15caf
Gerrit-Change-Number: 11673
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Sat, 13 Oct 2018 03:01:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7705: [DOCS] Documented the ALTER DATABASE SET OWNER statement

2018-10-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11674 )

Change subject: IMPALA-7705: [DOCS] Documented the ALTER DATABASE SET OWNER 
statement
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifac0b689d55f525145b37846967a7a22f0e9245b
Gerrit-Change-Number: 11674
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Sat, 13 Oct 2018 02:57:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7706: [DOCS] Added the privileges required for ALTER TABLE SET OWNER

2018-10-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11675 )

Change subject: IMPALA-7706: [DOCS] Added the privileges required for ALTER 
TABLE SET OWNER
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11675/1/docs/shared/impala_common.xml
File docs/shared/impala_common.xml:

http://gerrit.cloudera.org:8080/#/c/11675/1/docs/shared/impala_common.xml@299
PS1, Line 299: ALTER TABLE SET OWNER
need an entry for ALTER DATABASE SET OWNER.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I671dbe3e6fb3118a67c59fb1fcaf1ec53139b587
Gerrit-Change-Number: 11675
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Sat, 13 Oct 2018 02:55:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry

2018-10-11 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/11624 )

Change subject: IMPALA-7554: Update custom cluster tests to have new logs for 
sentry
..

IMPALA-7554: Update custom cluster tests to have new logs for sentry

This patch adds the ability to create a new log for each spawn of the
sentry service. This will enable better trouble shooting for the
custom cluster tests that restart the sentry service.

Testing:
- Ran all custom cluster tests.

Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
---
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_authorization.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/custom_cluster/test_redaction.py
5 files changed, 87 insertions(+), 57 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11624/3
--
To view, visit http://gerrit.cloudera.org:8080/11624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
Gerrit-Change-Number: 11624
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry

2018-10-11 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11624 )

Change subject: IMPALA-7554: Update custom cluster tests to have new logs for 
sentry
..


Patch Set 3:

Fixed formatting


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
Gerrit-Change-Number: 11624
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 11 Oct 2018 20:40:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry

2018-10-10 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11624 )

Change subject: IMPALA-7554: Update custom cluster tests to have new logs for 
sentry
..

IMPALA-7554: Update custom cluster tests to have new logs for sentry

This patch adds the ability to create a new log for each spawn of the
sentry service. This will enable better trouble shooting for the
custom cluster tests that restart the sentry service.

Testing:
- Ran all custom cluster tests.

Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
---
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_authorization.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/custom_cluster/test_redaction.py
5 files changed, 48 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11624/2
--
To view, visit http://gerrit.cloudera.org:8080/11624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
Gerrit-Change-Number: 11624
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges

2018-10-10 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11649 )

Change subject: IMPALA-7688: Fix spurious error messages when updating owner 
privileges
..


Patch Set 6: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0
Gerrit-Change-Number: 11649
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 10 Oct 2018 16:20:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges

2018-10-10 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11649 )

Change subject: IMPALA-7688: Fix spurious error messages when updating owner 
privileges
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11649/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11649/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2869
PS5, Line 2869:   PrincipalPrivilege removedPrivilege = null;
Add precondition for filter not null.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0
Gerrit-Change-Number: 11649
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 10 Oct 2018 16:05:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe

2018-10-09 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11644 )

Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2
Gerrit-Change-Number: 11644
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 10 Oct 2018 04:49:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7676: DESCRIBE on table should require VIEW METADATA privilege

2018-10-09 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11617 )

Change subject: IMPALA-7676: DESCRIBE on table should require VIEW_METADATA 
privilege
..


Patch Set 3: Code-Review+1

(1 comment)

looks good after nit.

http://gerrit.cloudera.org:8080/#/c/11617/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11617/3//COMMIT_MSG@15
PS3, Line 15: o
nit: not



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I283e30ebff6d61e779a4cec8284cae0ccb90cc49
Gerrit-Change-Number: 11617
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 10 Oct 2018 04:48:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry

2018-10-09 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11624


Change subject: IMPALA-7554: Update custom cluster tests to have new logs for 
sentry
..

IMPALA-7554: Update custom cluster tests to have new logs for sentry

This patch adds the ability to create a new log for each spawn of the
sentry service. This will enable better trouble shooting for the
custom cluster tests that restart the sentry service.

Testing:
- Ran all custom cluster tests.

Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
---
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
3 files changed, 38 insertions(+), 10 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11624/1
--
To view, visit http://gerrit.cloudera.org:8080/11624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516
Gerrit-Change-Number: 11624
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7671: Fix broken SHOW GRANT USER ON

2018-10-05 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11598 )

Change subject: IMPALA-7671: Fix broken SHOW GRANT USER ON 
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11598/1/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
File testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test:

http://gerrit.cloudera.org:8080/#/c/11598/1/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test@3
PS1, Line 3: fwijaya
replace with $USER



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7adc403caddd18e5a954cf6affd5d1d555b9f5f0
Gerrit-Change-Number: 11598
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 05 Oct 2018 21:03:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7633: count user privilege isn't 0 at the end of test owner

2018-10-05 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11595 )

Change subject: IMPALA-7633: count_user_privilege isn't 0 at the end of 
test_owner
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11595/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11595/1//COMMIT_MSG@29
PS1, Line 29: - After retry was added, ran tests until log entry appeared and
> I re-ran the cluster tests until the error appeared in the log file.  Appro
Just to add.  increasing the statestore update time will not make the problem 
more frequent as it relies on a sentry refresh between 1322 and 1341 of 
CatalogOpExecutor.  Increasing the statestore time will just allow a longer 
window to see the problem when it occurs.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b
Gerrit-Change-Number: 11595
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 05 Oct 2018 16:02:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7633: count user privilege isn't 0 at the end of test owner

2018-10-05 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11595 )

Change subject: IMPALA-7633: count_user_privilege isn't 0 at the end of 
test_owner
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11595/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11595/1//COMMIT_MSG@29
PS1, Line 29: - After retry was added, ran tests until log entry appeared and
> How did you reproduce the issue? Did you increase the statestore update tim
I re-ran the cluster tests until the error appeared in the log file.  
Approximately 12 times.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b
Gerrit-Change-Number: 11595
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 05 Oct 2018 15:47:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7633: count user privilege isn't 0 at the end of test owner

2018-10-05 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11595


Change subject: IMPALA-7633: count_user_privilege isn't 0 at the end of 
test_owner
..

IMPALA-7633: count_user_privilege isn't 0 at the end of test_owner

This patch adds a retry loop to validate the count of user privileges
in a SHOW GRANT USER statement after a DDL operation. The core of the
problem is cache consistency. When a DDL operation is executing, like
drop database, HMS is updated with the correct metadata, and Sentry is
updated to remove privileges from HMS. However, if a Sentry Refresh
happens between when HMS is updated CatalogOpExecutor:1322, and when
the local catalog privileges are updated CatalogOpExecutor:1341, then
the remove privilege call will fail and a log entry with "User does
not exist: foo_user" will be written to the log. The result is that
the response back to impalad with catalog updates will not contain
the user and privilege updates. Ultimately, when the "SHOW GRANT USER"
statement is run, it uses the local Impalad catalog which still
contains the privlege because it has not yet been updated from
statestore. This is not a security problem because the privilege
exists for a maximum of 2s by default, for an object that does not
exist. This is the same result as if the database was dropped from
Hive, except in that case it can be up to 62s by default that the
privilege exists for no object.

Testing:
- After retry was added, ran tests until log entry appeared and
  validate test did not fail.

Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b
---
M tests/authorization/test_owner_privileges.py
1 file changed, 23 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/95/11595/1
--
To view, visit http://gerrit.cloudera.org:8080/11595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b
Gerrit-Change-Number: 11595
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7633: count user privilege isn't 0 at the end of test owner

2018-10-05 Thread Adam Holley (Code Review)

Adam Holley has removed Vuk Ercegovac from this change.  ( 
http://gerrit.cloudera.org:8080/11595 )

Change subject: IMPALA-7633: count_user_privilege isn't 0 at the end of 
test_owner
..


Removed reviewer Vuk Ercegovac.
--
To view, visit http://gerrit.cloudera.org:8080/11595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ifbba0fbd0e24a24b3f2af82ad5209f3fb7fb387b
Gerrit-Change-Number: 11595
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster

2018-10-01 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11553 )

Change subject: IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster
..


Patch Set 1:

The effective user there, get converted to a shortName in 
SentryPolicyService.listAllRoles() method.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba4c627b72c8cbc323be25917698a75d153afd31
Gerrit-Change-Number: 11553
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 01 Oct 2018 19:02:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7520: Fix NullPointerException in SentryProxy

2018-10-01 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11552 )

Change subject: IMPALA-7520: Fix NullPointerException in SentryProxy
..


Patch Set 2: Code-Review+1

(1 comment)

one nit

http://gerrit.cloudera.org:8080/#/c/11552/2/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/11552/2/fe/src/main/java/org/apache/impala/util/SentryProxy.java@249
PS2, Line 249:
nit: space



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36af840056a4d037fb5c7b1d9a167c0eb8526a11
Gerrit-Change-Number: 11552
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 01 Oct 2018 18:43:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster

2018-10-01 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11553 )

Change subject: IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11553/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/11553/1/fe/src/main/java/org/apache/impala/service/Frontend.java@511
PS1, Line 511: analysis.getAnalyzer().getUser().getShortName());
> Can we check any code that uses getUser().getName()?
Validated other usages.  Exceptions, trace, and tests which should retain name.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba4c627b72c8cbc323be25917698a75d153afd31
Gerrit-Change-Number: 11553
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 01 Oct 2018 18:40:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster

2018-10-01 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11553


Change subject: IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster
..

IMPALA-7646: SHOW GRANT USER does not work for kerberos cluster

This patch fixes the SHOW GRANT USER statement to properly check
that the requesting user short name matches the name in the
SHOW GRANT USER statement to determine whether or not an admin
check is required for showing the privileges. Previous to this
patch, the full kerberos user name, e.g. foo_user@REALM was
compared against "SHOW GRANT USER foo_user" and did not match
do admin privileges were required.

Testing:
- Ran all fe and custom cluster tests.
- Validated against kerberized cluster.

Change-Id: Iba4c627b72c8cbc323be25917698a75d153afd31
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/11553/1
--
To view, visit http://gerrit.cloudera.org:8080/11553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba4c627b72c8cbc323be25917698a75d153afd31
Gerrit-Change-Number: 11553
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-28 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#9). ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Truncated sample showing two columns that are different from role:
++++--+-...
| principal_type | principal_name | scope  | database | ...
++++--+-...
| USER   | foo| table  | foo_db   | ...
| ROLE   | foo_role   | server |  | ...
++++--+-...

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
M fe/src/test/resources/sentry-site_no_oo.xml.template
M fe/src/test/resources/sentry-site_oo.xml.template
M fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
15 files changed, 1,400 insertions(+), 177 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/9
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-28 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 8:

The reason the build failed is because of "test_user".  This user is not 
considered valid in the cluster tests because it does not belong to a group.  I 
updated the CustomClusterGroupMapping to add new users and groups for the 
owner_privilege tests. This removes previous concerns about running with user 
"root". No core code was changed for patch set 8.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 28 Sep 2018 07:00:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-28 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Truncated sample showing two columns that are different from role:
++++--+-...
| principal_type | principal_name | scope  | database | ...
++++--+-...
| USER   | foo| table  | foo_db   | ...
| ROLE   | foo_role   | server |  | ...
++++--+-...

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
M fe/src/test/resources/sentry-site_no_oo.xml.template
M fe/src/test/resources/sentry-site_oo.xml.template
M fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
15 files changed, 1,400 insertions(+), 177 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@582
PS6, Line 582:* This method adds the rows to the output for the SHOW GRANT 
USER statement for user
> nit: adds
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:38:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#7). (
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
11 files changed, 1,240 insertions(+), 67 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 6:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@478
PS5, Line 478: Role role
> nit: this should be Role
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@559
PS5, Line 559: User user
> nit: this should be User
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@563
PS5, Line 563: }
 :
 : // Ge
> This seems to be the same logic as line 584, so it would better to make the
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@583
PS5, Line 583:* and associated roles.
 :*/
 :   private void createShowUserPrivilegesResultRows(TResultSet 
result,
 :   List privileges, TPrivilege filter, 
String name,
 :   TPrincipalType type) {
 : for (PrincipalPrivilege p : privileges) {
> optional: The block in this loop and the one at line 561 seems very similar
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@32
PS5, Line 32: CustomClusterGroupMapper
> I can't find where this class is used.
It's used in test_show_grant_user.py via the 
CustomClusterResourceAuthorizationProvider


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@25
PS5, Line 25: CustomClusterResourceAuthorizationProvider
> I can't find where this class is used.
It's used in test_show_grant_user.py.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py@163
PS5, Line 163:
> optional: ignore_role seems redundant to me - it is always true for "show g
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@47
PS5, Line 47: If the query is
: for "s
> This part of the sentence seem strange.
Reworded.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@62
PS5, Line 62: [2:]
> optional: offsets could be replace with 
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@75
PS5, Line 75: ached. If delay_s is > 0 th
> I do not understand this part of the sentence - can you rephrase it?
Removed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:23:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#6). (
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 5:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@69
PS4, Line 69:   switch (principalType_) {
: case ROLE:
:   throw new AnalysisException(String.format("%s '%s' does 
not exist.",
:   Principal.toString(principalType_), name_));
: case USER:
:   // Create a user object here because it's possible the 
user does not exist in
:   // Sentry, but still exists according to the OS, or 
Hadoop, or other custom
:   // group mapping provider.
:   principal_ = Principal.newInstance(name_, 
principalType_, Sets.newHashSet());
:   break;
: default:
:
> Usually we use switch statement for enums: https://stackoverflow.com/questi
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@404
PS4, Line 404:   }
> nit: extra empty line
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@468
PS4, Line 468:* Allows for filtering based on a specific privilege spec or 
showing all privileges
 :* granted to the role. Used by the SHOW GRANT ROLE statement.
 :*/
> replace the word "principal" to "role"
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@473
PS4, Line 473: ew TResultSet();
> Is the TPrincipalType still necessary since we know it should always be TPr
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS4, Line 480: ege p : role.getPrivileges()) {
> Can we do getUser(principalName) instead?
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@485
PS4, Line 485:
> use && with L484 to avoid nested ifs.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@521
PS4, Line 521: ullToEmpty(privilege.g
> Maybe addShowPrincipalOutputResults is a better name? addCommonOutputResult
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@542
PS4, Line 542: t = new T
> replace principal with user.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@545
PS4, Line 545: dToColumns(new TCol
> Is the TPrincipalType necessary since we know it should always be TPrincipa
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@557
PS4, Line 557: cipalName
> user
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@585
PS4, Line 585: TResultRowBuilder rowBuilder = new TResultRowBuilder();
> Can we do getRole(role.getName()) instead?
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@590
PS4, Line 590:
> use && with L589 to avoid nested ifs.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/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/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@564
PS4, Line 564: switch (params.getPrincipal_type()) {
 :   case USER:
 : result = 
frontend_.getCatalog().getAuthPolicy().getUserPrivileges(
 : params.getName(), params.getPrivilege(), frontend_);
 : break;
 :   case ROLE:
 : result = 
frontend_.getCatalog().getAuthPolicy().getRolePrivileges(
 : params.getName(), params.getPrivilege());
 : break;
 :
> Usually we use switch statement for enums: https://stackoverflow.com/questi
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#5). (
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11531/3/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/11531/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java@571
PS3, Line 571:   throw new AnalysisException("Unexpected TPrincipalType: " +
> line too long (94 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py@293
PS3, Line 293:
> flake8: E122 continuation line missing indentation or outdented
Done


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py@87
PS3, Line 87: .
> flake8: E131 continuation line unaligned for hanging indent
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:26:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#4). (
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 3:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG@11
PS2, Line 11: The output for SHOW
: GRANT USER will have two additional columns for privilege name and
: privilege type so the user can know where the privilege comes 
from.
:
> Can we show a sample output here?
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@471
PS2, Line 471:*/
> I feel like it's better to have two separate methods, getRolePrivileges() a
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@472
PS2, Line 472: Name,
> showPrincipal is probably a better name for this.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@474
PS2, Line 474: AnalysisException
> nit: move this to L473
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@544
PS2, Line 544: ic synchronized TResultSet getUserPrivileges(String 
principalName,
> This call can be quite expensive. We should only call it once L499 and L544
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@553
PS2, Line 553: Type.STRING.toThrift()));
 : addColumnOutputColumns(result.getSchema());
 : resul
> We can avoid this copying if we create a method, e.g. getRolePrivileges tha
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@39
PS2, Line 39:
> remove empty line
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@28
PS2, Line 28:   Model model) {
> fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@33
PS2, Line 33:   PolicyEngine policy, Model model) {
> fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
File testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test:

http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test@1
PS2, Line 1: 
> This test cases are a bit hard to read with the names that contain numbers,
The purpose of the tests is to verify different users to different groups to 
different role mappings.  If we use one group, there we'd only be testing one 
user.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@1
PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one
> nit: need consistency in this file whether to use ' or " for strings.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@18
PS2, Line 18: # Client tests for SQL statement authorization
: # These tests verify the functionality of SHOW GRANT USER. We
: # create several users and groups to verify clear separation.
> this comment doesn't look correct.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75
PS2, Line 75:   # Ignore any errors here because these might fail for 
partial test runs.
:   pass
> the finally isn't useful here.
I need an finally or except to ignore the statements above that might fail 
because of partial test runs.  Empty excepts fail flake8.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@87
PS2, Line 87: 
'org.apache.impala.service.CustomClusterResourceAuthorizationProvi
> nit: indentation
Done

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-27 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#3). (
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/3
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-26 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
10 files changed, 1,187 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/2
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-26 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@479
PS1, Line 479:   result.getSchema().addToColumns(new 
TColumn("principal_type",
> line too long (93 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS1, Line 480:   Type.STRING.toThrift()));
> line too long (93 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@26
PS1, Line 26:
> flake8: F401 'time.sleep' imported but unused
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@30
PS1, Line 30: SENTRY_CONFIG_FILE = getenv('IMPALA_HOME') + \
> flake8: F401 'tests.util.calculation_util.get_random_id' imported but unuse
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@31
PS1, Line 31: '/fe/src/test/resources/sentry-site_oo.xml'
> flake8: F401 'tests.verifiers.metric_verifier.MetricVerifier' imported but
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@62
PS1, Line 62:
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@64
PS1, Line 64:
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@65
PS1, Line 65:
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@66
PS1, Line 66:
> flake8: E226 missing whitespace around arithmetic operator
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 27 Sep 2018 00:01:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

2018-09-26 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11531


Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
..

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A 
fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
10 files changed, 1,188 insertions(+), 55 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/1
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7631: Add sentry.db.explicit.grants.permitted in sentry-site*.xml

2018-09-26 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11527 )

Change subject: IMPALA-7631: Add sentry.db.explicit.grants.permitted in 
sentry-site*.xml
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4adac50fe194cb341d49a40915763f70cd81c348
Gerrit-Change-Number: 11527
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 26 Sep 2018 19:35:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11509 )

Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
..


Patch Set 7: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:

http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@562
PS7, Line 562: //
nit: This comment should be with the case PRIVILEGE, and should be name+id.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 26 Sep 2018 04:57:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11509 )

Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@491
PS6, Line 491: .equalsIgnoreCase
If the name is always lowered, this isn't necessary.


http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:

http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@a565
PS6, Line 565:
Why was this removed? Isn't it necessary for the key?  There can be multiple 
privileges with the same principal id.


http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@525
PS6, Line 525: .equalsIgnoreCase
Case again.


http://gerrit.cloudera.org:8080/#/c/11509/6/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/11509/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@250
PS6, Line 250: .toLowerCase()
The use of toLowerCase seems inconsistent.  Should getName just lowerCase it?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 26 Sep 2018 01:57:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/11502 )

Change subject: IMPALA-7456: Deprecate file-based authorization
..

IMPALA-7456: Deprecate file-based authorization

This patch simply adds a warning message to the log when the
authorization_policy_file run-time flag is used.  Sentry has
deprecated the use of policy files and they do not support
user level privileges which are required for object ownership.
Here is the Jira where it will be removed. SENTRY-1922

Test:
- Added custom cluster test to validate logs
- Ran all custom cluster tests

Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
---
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M tests/authorization/test_authorization.py
M tests/common/custom_cluster_test_suite.py
M tests/common/file_utils.py
M tests/custom_cluster/test_redaction.py
6 files changed, 103 insertions(+), 55 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/11502 )

Change subject: IMPALA-7456: Deprecate file-based authorization
..

IMPALA-7456: Deprecate file-based authorization

This patch simply adds a warning message to the log when the
authorization_policy_file run-time flag is used.  Sentry has
depreated the use of policy files and they do not support
user level privileges which are required for object ownership.
Here is the Jira where it will be removed. SENTRY-1922

Test:
- Added custom cluster test to validate logs
- Ran all custom cluster tests

Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
---
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M tests/authorization/test_authorization.py
M tests/common/custom_cluster_test_suite.py
M tests/common/file_utils.py
M tests/custom_cluster/test_redaction.py
6 files changed, 103 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/11502/3
--
To view, visit http://gerrit.cloudera.org:8080/11502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11502 )

Change subject: IMPALA-7456: Deprecate file-based authorization
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11502/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11502/2//COMMIT_MSG@11
PS2, Line 11: deprecate
> typo: deprecated. Just put the SENTRY-1922 here.
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/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/11502/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@700
PS1, Line 700: authorization_policy_file flag is deprecated
> Wouldn't it better to throw an exception when object ownership and authoriz
Object ownership can only be enabled via the sentry server.  They should be 
mutually exclusive.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 18:28:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 10:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11483/6/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/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@389
PS6, Line 389: st find
> typo: multiple
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@390
PS6, Line 390: case there will b
> Can you add Preconditions.checkArgument(!privileges.isEmpty())?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 18:26:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#10). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 480 insertions(+), 195 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 9: Code-Review+1

(2 comments)

carry +1

http://gerrit.cloudera.org:8080/#/c/11483/7/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/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@476
PS7, Line 476:
> I am still not 100% ok with the naming - privWithGrant has the negated vers
Done


http://gerrit.cloudera.org:8080/#/c/11483/8/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/11483/8/fe/src/main/java/org/apache/impala/util/SentryProxy.java@482
PS8, Line 482: if (rolePriv != null) {
 :   rolePrivileges.add(rolePriv);
 : }
> I have noticed that in CatalogOpEcexutor the last element's version is used
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 17:58:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#9). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 479 insertions(+), 195 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11483/9
--
To view, visit http://gerrit.cloudera.org:8080/11483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11502 )

Change subject: IMPALA-7456: Deprecate file-based authorization
..


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/11502/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11502/1//COMMIT_MSG@9
PS1, Line 9: add
> nit: adds
Done


http://gerrit.cloudera.org:8080/#/c/11502/1//COMMIT_MSG@10
PS1, Line 10: Sentry has
: depreated the use of policy files
> Please put a SENTRY JIRA for this.
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/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/11502/1/fe/src/main/java/org/apache/impala/service/JniFrontend.java@700
PS1, Line 700: authorization_policy_file flag is deprecated
> Instead of mentioning the flag name, maybe it's better to say "Authorizatio
Since this is in the logs and meant for admins, I think it'd be better to let 
them know the exact flag they are using that they shouldn't.


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:

http://gerrit.cloudera.org:8080/#/c/11502/1/tests/authorization/test_authorization.py@378
PS1, Line 378:
> flake8: E502 the backslash is redundant between brackets
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/authorization/test_authorization.py@385
PS1, Line 385:
> flake8: W391 blank line at end of file
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py
File tests/common/file_utils.py:

http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@62
PS1, Line 62:
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@78
PS1, Line 78:
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@88
PS1, Line 88:   return matching_lines
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@90
PS1, Line 90:
> flake8: E231 missing whitespace after ','
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@94
PS1, Line 94:   assert results, "%s should have a file containing '%s' but no 
file was found" \
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/common/file_utils.py@96
PS1, Line 96:
> flake8: E231 missing whitespace after ','
Done


http://gerrit.cloudera.org:8080/#/c/11502/1/tests/custom_cluster/test_redaction.py
File tests/custom_cluster/test_redaction.py:

http://gerrit.cloudera.org:8080/#/c/11502/1/tests/custom_cluster/test_redaction.py@30
PS1, Line 30: from tests.common.file_utils import grep_file, 
assert_file_in_dir_contains,\
> flake8: F401 'tests.common.file_utils.grep_dir' imported but unused
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 14:56:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11502 )

Change subject: IMPALA-7456: Deprecate file-based authorization
..

IMPALA-7456: Deprecate file-based authorization

This patch simply adds a warning message to the log when the
authorization_policy_file run-time flag is used.  Sentry has
depreated the use of policy files and they do not support
user level privileges which are required for object ownership.
Here is the Jira where it will be removed.
https://issues.apache.org/jira/browse/SENTRY-1922

Test:
- Added custom cluster test to validate logs
- Ran all custom cluster tests

Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
---
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M tests/authorization/test_authorization.py
M tests/common/custom_cluster_test_suite.py
M tests/common/file_utils.py
M tests/custom_cluster/test_redaction.py
6 files changed, 103 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/11502/2
--
To view, visit http://gerrit.cloudera.org:8080/11502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11483/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/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@424
PS5, Line 424:   rolePrivileges.add(catalog_.addRolePrivilege(roleName, 
privilege));
> Thanks for the explanation!
It's not ideal but there shouldn't be any security issue with having both 
privileges. Once Sentry has been fixed, this problem will go away.  Unless we 
want to add transactions to this, I'm not sure there's much else we can do 
about it.  Sentry and catalogd should be synced every 60 seconds (default) so 
they will eventually be consistent.  Then the impalad will be updated after the 
sync.


http://gerrit.cloudera.org:8080/#/c/11483/6/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/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@391
PS6, Line 391: // of mutliple column privileges.
 : TPrivilege tWithGrant = privileges.g
> The x.setPrivilege_name(...buildPrivilegeName(x)) pattern seems a bit weird
I agree. I created https://issues.apache.org/jira/browse/IMPALA-7616 to 
address. Please add comments there as necessary.


http://gerrit.cloudera.org:8080/#/c/11483/7/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/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@472
PS7, Line 472: privWithGrant.setHas_grant_opt(!privi
> Is it possible that privilege's grant_opt is already true? In that case the
Done


http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@476
PS7, Line 476:
> Is the naming correct here? We try to remove privWithGrant, while the resul
Done


http://gerrit.cloudera.org:8080/#/c/11483/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@479
PS7, Line 479: if (rolePriv != null) {
> Is it possible in some extreme cases that both are non-null? For example if
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 14:38:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-25 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 479 insertions(+), 195 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-24 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 7:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/11483/6/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/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@369
PS6, Line 369: This code is odd because we need to avoid duplicate privileges 
in Sentry because
 :* the same privilege with and without grant option are two 
different privileges.
> Might be a good idea to reference the issue in Sentry: https://issues.apach
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@381
PS6, Line 381: t, we add
> Please explain what the "downgrade" means.
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@390
PS6, Line 390: case there will b
> Is it possible privileges to have more than one element? if not, can you ad
Yes, it's possible.  Added recent comment.


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@391
PS6, Line 391: // of mutliple column privileges.
 : TPrivilege tWithGrant = privileges.g
> Can be chained with L390 to make it more concise.
No.  the buildPrivilegeName method requires the tWithGrant object which won't 
have been initialized in the chain.


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@396
PS6, Line 396: tWithGrant);
 : TPrivilege tNoGrant = privileges.ge
> Can be chained with L395 to make it more concise.
No.  same as previous comment.


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@402
PS6, Line 402: houl
> Initializing the list with an empty list is probably better. That way in L4
Based on previous review comments, I'd prefer not to create an unused empty 
list.


http://gerrit.cloudera.org:8080/#/c/11483/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@405
PS6, Line 405:
 : if (catNoGrant != null && hasGrantOption) {
 :   toRemove = privileges.stream().map(p -> {
> This is bad because it defeats the purpose of lazy evaluation. I think this
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py@40
PS6, Line 40: return val.lower() == 'true'
:
:   @staticmethod
> Can be simplified to return val.lower() == 'true'
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py@66
PS6, Line 66: "
> nit: delay_sec to indicate the unit of time
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py@74
PS6, Line 74: while
> nit: fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py@88
PS6, Line 88: if delay_s > 0:
:   sleep(delay_s)
: if error_msg is not None:
> Add a comment that it returns None when there is no error_msg.
Done


http://gerrit.cloudera.org:8080/#/c/11483/6/tests/common/sentry_cache_test_suite.py@114
PS6, Line 114:  = "view"
> nit: TestObject(object)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 25 Sep 2018 04:18:39 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-24 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#7). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 473 insertions(+), 194 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7456: Deprecate file-based authorization

2018-09-24 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11502


Change subject: IMPALA-7456: Deprecate file-based authorization
..

IMPALA-7456: Deprecate file-based authorization

This patch simply add a warning message to the log when the
authorization_policy_file run-time flag is used.  Sentry has
depreated the use of policy files and they do not support
user level privileges which are required for object ownership.

Test:
- Added custom cluster test to validate logs
- Ran all custom cluster tests

Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
---
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M tests/authorization/test_authorization.py
M tests/common/custom_cluster_test_suite.py
M tests/common/file_utils.py
M tests/custom_cluster/test_redaction.py
6 files changed, 101 insertions(+), 55 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/11502/1
--
To view, visit http://gerrit.cloudera.org:8080/11502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibbb13f3ef1c3a00812c180ecef022ea638c2ebc7
Gerrit-Change-Number: 11502
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-24 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11483/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/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@388
PS5, Line 388: will b
> Can the different privileges have different grant status? If this case does
Done


http://gerrit.cloudera.org:8080/#/c/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@399
PS5, Line 399:
> Can you move this closer to the place where it is actually used?  Creating
Done


http://gerrit.cloudera.org:8080/#/c/11483/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@424
PS5, Line 424: if (toRemove != null && !toRemove.isEmpty()) {
> Can this line throw an exception? For example if another user was changing
This line may throw an exception because of either failure to get a client, 
authorization denied, or other RPC related call.  If the user changed 
privileges in Hive, the Sentry call is effectively a noop. It will not throw an 
exception if the privilege no longer exists.  Anytime privileges are changed in 
Hive, Impala catalog will not be in sync with Sentry until the next refresh.  
However, because the Sentry calls are done first before adding or removing from 
the catalog, these should be in sync after each call, or will sync on the next 
Sentry refresh.  If there is a network failure between the grant and revoke 
calls, there might be either extra privileges (e.g. same privilege with grant 
option so not a security issue), or missing because they are not part of a 
transaction, however this problem is not new as in the previous code, if it 
failed between revoke and grant, then the expected privilege would be missing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 24 Sep 2018 18:46:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-24 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#6). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 473 insertions(+), 194 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-22 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 472 insertions(+), 194 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-22 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_test_suite.py
8 files changed, 472 insertions(+), 194 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-22 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 4:

Oops.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Sat, 22 Sep 2018 15:39:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-21 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11469 )

Change subject: IMPALA-7591: Use short name for setting owner
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11469/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11469/3//COMMIT_MSG@18
PS3, Line 18: - Ran all frontend, and e2e tests.
> were any manual tests run on a Kerberized cluster?
Yes.


http://gerrit.cloudera.org:8080/#/c/11469/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/11469/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2347
PS3, Line 2347: the shortened name for
> this is a user-visible error, so I doubt they care about method calls. the
Done


http://gerrit.cloudera.org:8080/#/c/11469/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2348
PS3, Line 2348: e
> the exception that's thrown by User has the same message as the line on L23
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 21 Sep 2018 17:38:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-21 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/11469 )

Change subject: IMPALA-7591: Use short name for setting owner
..

IMPALA-7591: Use short name for setting owner

This patch changes the owner to be the shortname in a Kerberized
cluster.  Authorization with Sentry is done using the shortname
and Hive uses the shortname.  Because object ownership privileges
are based on owner, authorization will not work with the full name.
The IOException for org.apache.impala.authorization.User.getShortName
should only ever be thrown if the format for the name is wrong.
i.e. It's actually a BadFormatString exception.

Testing:
- Ran all frontend, and e2e tests.
- Manually verified on kerberized cluster.

Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
6 files changed, 14 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-21 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
7 files changed, 334 insertions(+), 194 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11483/3
--
To view, visit http://gerrit.cloudera.org:8080/11483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-21 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/11483/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11483/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3207
PS2, Line 3207:   Math.max(getLastItemVersion(updatedPrivs),
  :   getLastItemVersion(removedPrivs)));
> Maybe Math.max(getLastItemVersion(updatedPrivs), getLastItemVersion(removed
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/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/11483/2/fe/src/main/java/org/apache/impala/util/SentryProxy.java@418
PS2, Line 418: :
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11483/2/tests/authorization/test_owner_privileges.py@a269
PS2, Line 269:
> What was the reason for removing this and __root_query()? I  think that it
Just to combine the user and failure/success scenarios.  It was a comment in a 
previous review that I recall suggested combining to failure/success so I just 
went one further for user.


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/authorization/test_owner_privileges.py@52
PS2, Line 52:
> This is diamond inheritance, as both base classes inherit from ImpalaTestSu
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py
File tests/common/sentry_cache_base.py:

http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@34
PS2, Line 34:
> This seems weird - I would expect ImpalaTestSuite to be imported from tests
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@37
PS2, Line 37:
> Can you add TestSuite to the name to be consistent with CustomClusterTestSu
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@38
PS2, Line 38:
> As self is not used, this could be a @staticmethod.
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@45
PS2, Line 45:
> The name of the first parameters in class methods typically cls, as a class
Since str_to_bool is static, changed this as well.


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@65
PS2, Line 65:
> see line 45
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@85
PS2, Line 85:
> if verify_exception would be @staticmethod, then this function would only c
Done


http://gerrit.cloudera.org:8080/#/c/11483/2/tests/common/sentry_cache_base.py@99
PS2, Line 99:
> as self is not used, so this could be a @staticmethod
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 21 Sep 2018 17:12:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-20 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11483 )

Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_base.py
8 files changed, 460 insertions(+), 181 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11483/2
--
To view, visit http://gerrit.cloudera.org:8080/11483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-20 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11469 )

Change subject: IMPALA-7591: Use short name for setting owner
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11469/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11469/2//COMMIT_MSG@13
PS2, Line 13: org.apache.i
> Please add the class name as there are more than one getShortName() functio
Done


http://gerrit.cloudera.org:8080/#/c/11469/2//COMMIT_MSG@15
PS2, Line 15: i.e. It's actua
> I would prefer if there was such a test if it is not too hard to create one
There's an existing test in AuthorizationTest.java 
TestShortUsernameWithAuthToLocal.  uses the variable "malformedRule".  The only 
other option is to enable kerberos which I don't think we want to require for 
unit tests yet.


http://gerrit.cloudera.org:8080/#/c/11469/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java:

http://gerrit.cloudera.org:8080/#/c/11469/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@53
PS2, Line 53: //
> optional: I would add a function like getShortName() to Analyzer/StatementB
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 20 Sep 2018 15:04:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-20 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11469 )

Change subject: IMPALA-7591: Use short name for setting owner
..


Patch Set 3: Code-Review+1

carry +1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Thu, 20 Sep 2018 15:04:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-20 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/11469 )

Change subject: IMPALA-7591: Use short name for setting owner
..

IMPALA-7591: Use short name for setting owner

This patch changes the owner to be the shortname in a Kerberized
cluster.  Authorization with Sentry is done using the shortname
and Hive uses the shortname.  Because object ownership privileges
are based on owner, authorization will not work with the full name.
The IOException for org.apache.impala.authorization.User.getShortName
should only ever be thrown if the format for the name is wrong.
i.e. It's actually a BadFormatString exception.

Testing:
- Ran all frontend, and e2e tests.

Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
6 files changed, 14 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/11469/3
--
To view, visit http://gerrit.cloudera.org:8080/11469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-7537: REVOKE GRANT OPTION regression

2018-09-19 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11483


Change subject: IMPALA-7537: REVOKE GRANT OPTION regression
..

IMPALA-7537: REVOKE GRANT OPTION regression

This patch fixes several issues around granting and revoking of
privileges.  This includes:
- REVOKE ALL ON SERVER where the privilege has the grant option was
  removing from the cache but not Sentry.
- With the addition of the grantoption to the name in the catalog
  object, refactoring was required to make grants and revokes work
  correctly.

Assertions with regard to granting and revoking:
- If there is a privilege that has the grant option, that privilege
  can be revoked simply with "REVOKE privilege..." or the grant option
  can be removed with "REVOKE GRANT OPTION ON..."
- We should not limit the privilege being revoked simply because it
  has the grant option.
- If a privilege already exists without the grant option, granting the
  privilege with the grant option should add the grant option to it.
- If a privilege already exists with the grant option, granting the
  privilege without the grant option will not change anything as the
  expectation is if you want to remove the grant option, you should
  explicitly use the "REVOKE GRANT OPTION ON...".

Testing:
- Added new grant/revoke tests that validate cache and Sentry refresh
- Ran all FE, E2E, and custom-cluster tests.

Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
A tests/common/sentry_cache_base.py
8 files changed, 462 insertions(+), 181 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11483/1
--
To view, visit http://gerrit.cloudera.org:8080/11483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3be5c8f15e9bc53e9661347578832bf446abaedc
Gerrit-Change-Number: 11483
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7591: Use short name for setting owner

2018-09-19 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11469


Change subject: IMPALA-7591: Use short name for setting owner
..

IMPALA-7591: Use short name for setting owner

This patch changes the owner to be the shortname in a Kerberized
cluster.  Authorization with Sentry is done using the shortname
and Hive uses the shortname.  Because object ownership privileges
are based on owner, authorization will not work with the full name.
The IOException for getShortName should only ever be thrown if
the format for the name is wrong. i.e. It's actually a
BadFormatString exception.

Testing:
- Ran all frontend, and e2e tests.

Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
5 files changed, 35 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/11469/2
--
To view, visit http://gerrit.cloudera.org:8080/11469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab392a0cd5c0bb98442e1f7ac39f470310a21f11
Gerrit-Change-Number: 11469
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Csaba Ringhofer

[Impala-ASF-CR] IMPALA-7525: [DOCS] SHOW GRANT USER

2018-09-19 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11454 )

Change subject: IMPALA-7525: [DOCS] SHOW GRANT USER
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa39275c595d73b2fba293f77bb21aa3985dfa38
Gerrit-Change-Number: 11454
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Wed, 19 Sep 2018 18:06:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER""

2018-09-18 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11466 )

Change subject: Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, 
DROP, and SET OWNER""
..

Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER""

The problem was caused by update in Hive with changed notifications.
HIVE-15180 was added but was incomplete and resulted in the break.
HIVE-17747 fixed the issue by properly creating the messages.

Change-Id: I4b9276c36bf96afccd7b8ff48803a30b47062c3d
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
34 files changed, 1,365 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/11466/2
--
To view, visit http://gerrit.cloudera.org:8080/11466
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4b9276c36bf96afccd7b8ff48803a30b47062c3d
Gerrit-Change-Number: 11466
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER""

2018-09-18 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11466 )

Change subject: Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, 
DROP, and SET OWNER""
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11466/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11466/1//COMMIT_MSG@9
PS1, Line 9: The problem was caused by update in Hive with changed 
notifications.
> nit: I'm just passing by, but you might want to include a sentence on what
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b9276c36bf96afccd7b8ff48803a30b47062c3d
Gerrit-Change-Number: 11466
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 19 Sep 2018 05:22:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER""

2018-09-18 Thread Adam Holley (Code Review)

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11466


Change subject: Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, 
DROP, and SET OWNER""
..

Revert "Revert "IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER""

The problem was caused by update in Hive with changed notifications.

Change-Id: I4b9276c36bf96afccd7b8ff48803a30b47062c3d
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
34 files changed, 1,365 insertions(+), 154 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/11466/1
--
To view, visit http://gerrit.cloudera.org:8080/11466
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4b9276c36bf96afccd7b8ff48803a30b47062c3d
Gerrit-Change-Number: 11466
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley

[Impala-ASF-CR] IMPALA-7525: [DOCS] SHOW GRANT USER

2018-09-18 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11454 )

Change subject: IMPALA-7525: [DOCS] SHOW GRANT USER
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11454/1/docs/topics/impala_show.xml
File docs/topics/impala_show.xml:

http://gerrit.cloudera.org:8080/#/c/11454/1/docs/topics/impala_show.xml@516
PS1, Line 516: and other users that have been granted the
 : specified role.
I'm not sure this is valid.  What is the specified role?  Additionally, the 
current user is allowed to run SHOW GRANT USER for themselves.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa39275c595d73b2fba293f77bb21aa3985dfa38
Gerrit-Change-Number: 11454
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 18 Sep 2018 14:10:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-13 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#42). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
34 files changed, 1,365 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/42
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 42
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-13 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#41). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
34 files changed, 1,370 insertions(+), 156 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/41
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 41
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-13 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..


Patch Set 40:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11314/37/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/37/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2918
PS37, Line 2918:   
response.result.addToUpdated_catalog_objects(cPrivilege.toTCatalogObject());
> I'm somewhat concerned that you modify owner (which is a shared object) wit
Done


http://gerrit.cloudera.org:8080/#/c/11314/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2903
PS38, Line 2903:
> adding
Done


http://gerrit.cloudera.org:8080/#/c/11314/38/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2904
PS38, Line 2904: filter.setPrincipal_id(owner.getId());
> it also add a user if specifying a user and that user does not exist.
Done


http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1341
PS40, Line 1341: updateOwnerPrivileges
> if this is *not* done for drop, what is the harm in waiting for the next up
If we don't drop and a different user creates the object with the same name, 
the old user will have privileges.


http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1493
PS40, Line 1493:   if (table.getMetaStoreTable() != null) {
> same question here (why do this for drop).
Same reason.  Don't want to leave ghost privileges around in case a new object 
with the same name is created.


http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/11314/40/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2218
PS40, Line 2218:   authzCatalog_.addRole("foo_owner");
> confused by this one... after this line, "foo_owner" role exists so the sub
This role was added so that the tests will pass.  The test for when roles do 
not exist result in AnalysisException and didn't add.  Next patch will include.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 40
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 13 Sep 2018 21:13:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-13 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#40). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
34 files changed, 1,349 insertions(+), 156 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/40
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 40
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-13 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#39). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,256 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/39
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 39
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#38). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,256 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/38
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 38
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..


Patch Set 37:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2869
PS33, Line 2869:*/
> Whar if the new type is added later? This code will silently break then.
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2876
PS33, Line 2876:   if (owner != null) {
> The question is why are incrementing catalog version twice here?
Looking around the code, this seems to be the pattern.  I don't think I should 
get the version assigned to removedPrivilege and assign it to owner as well.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 37
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 13 Sep 2018 00:17:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..


Patch Set 37:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1837
PS33, Line 1837:   }
> I was thinking of the case where an impalad has a table of the same name th
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2868
PS33, Line 2868:* and updating the privileges for that owner.
> true, but there are two operations on that cache: 1) get L2868 and 2) mutat
Done


http://gerrit.cloudera.org:8080/#/c/11314/36/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/36/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@220
PS36, Line 220: 6
> nit: update
Done


http://gerrit.cloudera.org:8080/#/c/11314/36/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1025
PS36, Line 1025:* can be used to try adding the owner privilege again.
> state locking assumptions.
Done


http://gerrit.cloudera.org:8080/#/c/11314/36/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2868
PS36, Line 2868:  and updating the privileges for that
> pls explain why the lock is held in the comment.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 37
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 12 Sep 2018 20:20:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#37). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,251 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/37
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 37
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#36). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,246 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/36
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 36
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-12 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..


Patch Set 33:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1337
PS33, Line 1337: updateOwnerPrivileges(db.getName(), /* tableName */ null, 
params.server_name,
> It does so via that metastoreDdlLock_ afaict. However, updating privs here
My main concern was the locks that were part of the privilege update and having 
a deadlock because someone gets these in a different order.   Since Sentry has 
it's own locks, I wanted to keep them separate from the HMS locks.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1837
PS33, Line 1837: updateOwnerPrivileges(newTable.getDbName(), 
newTable.getTableName(),
> there are possibly multiple impalads, each with multiple, uncoordinated req
But even with multiple impalad that have their own catalog.  If this call 
doesn't return, because of the sleep, the other impalad would not have the 
object in it's catalog to be able to drop.  The catalog update would happen 
after the create call is done afaik.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2896
PS33, Line 2896:   Reference existingUser = new Reference<>();
> this was my suggestion.
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3145
PS33, Line 3145:   private void grantRevokeRolePrivilege(User requestingUser,
> fwict, that lock is acquired when you drill through grantRolePrivs... while
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3153
PS33, Line 3153: List removedGrantOptPrivileges = 
Lists.newArrayList();
> we've been sticking to newArrayList to keep consistency. we could move to a
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3171
PS33, Line 3171:   addedRolePrivileges = Lists.newArrayList();
> here for consistency
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3178
PS33, Line 3178: List updatedPrivs = Lists.newArrayList();
> here for consistency
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3183
PS33, Line 3183: List removedPrivs = Lists.newArrayList();
> here for consistency
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 33
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 12 Sep 2018 17:58:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-11 Thread Adam Holley (Code Review)

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..


Patch Set 33:

(36 comments)

http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java:

http://gerrit.cloudera.org:8080/#/c/11314/30/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@56
PS30, Line 56: serverName_ = analyzer.getServerName();
> Here and in all other similar places please consider interning the string (
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@56
PS33, Line 56: serverName_ = analyzer.getServerName();
> Here and in all other similar places you should consider using string inter
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1639
PS33, Line 1639:* can be added for a user. example: owner privileges.
> Please document locking used.
versionLock_ is documented at the top of the file.  Is there something 
additional you are looking for?


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1641
PS33, Line 1641:   public User addUserIfNotExists(String owner, 
Reference existingUser) {
> Why do you need Reference here?
Because one of the calls needs to know if it's an existing user or not.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1644
PS33, Line 1644:   User user = getAuthPolicy().getUser(owner);
> I guess in most cases you get an existing user - would it make sense to get
versionLock_ cannot be upgraded to write lock.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1009
PS33, Line 1009:   /**
> Style: The first sentence of Javadoc is used in special ways - it should be
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1025
PS33, Line 1025:   private void updateOwnerPrivileges(String databaseName, 
String tableName,
> Style: you may consider having multiple user-facing functions which are sim
This was originally multiple functions and a previous review suggested to 
consolidate.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1037
PS33, Line 1037: if(oldOwner != null && oldOwner.length() > 0) {
> Style: it is better to use isEmpty() rather then compare size with 0.
Done


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1038
PS33, Line 1038:   removePrivilegeFromCatalog(oldOwner, oldOwnerType, 
filter, resp);
> Can this fail?
Yes.  However, there isn't currently an easy way to notify the user of any 
error for these privilege updates.  Throwing an exception would probably cause 
the other metadata catalog updates to fail even though the HMS updates were 
successful.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1337
PS33, Line 1337: updateOwnerPrivileges(db.getName(), /* tableName */ null, 
params.server_name,
> Can someone else create the database with the same name at the same time so
The catalog should prevent creating the database with the same name at the same 
time.  Any exceptions thrown occur before the privilege update calls.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1837
PS33, Line 1837: updateOwnerPrivileges(newTable.getDbName(), 
newTable.getTableName(),
> Is it possible that another threadd drops the table and creates a new one w
I don't think this will happen because these calls are in catalogd.  creates 
and drops are initiated by impalad.  The create on the new thread won't be 
allowed to happen until the drop table completes in the catalog, returns to 
impalad, and all catalogs are synced.


http://gerrit.cloudera.org:8080/#/c/11314/33/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1849
PS33, Line 1849:

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-11 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#35). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,244 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/35
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 35
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

2018-09-11 Thread Adam Holley (Code Review)

Adam Holley has uploaded a new patch set (#34). ( 
http://gerrit.cloudera.org:8080/11314 )

Change subject: IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET 
OWNER
..

IMPALA-7074: Update OWNER privilege on CREATE, DROP, and SET OWNER

This patch adds calls to automatically create or remove owner
privileges in the catalog based on the statement.  This is similar to
the existing pattern where after privileges are granted in Sentry,
they are created in the catalog directly instead of pulled from
Sentry.

When object ownership is enabled:
CREATE DATABASE will grant the user OWNER privileges to that database.
ALTER DATABASE SET OWNER will transfer the OWNER privileges to the
new owner.
DROP DATABASE will revoke the OWNER privileges from the owner.
This will apply to DATABASE, TABLE, and VIEW.

Example:
If ownership is enabled, when a table is created, the creator is the
owner, and Sentry will create owner privileges for the created table so
the user can continue working with it without waiting for Sentry
refresh.  Inserts will be available immediately.

Testing:
- Created new custom cluster tests for object ownership

Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
---
M bin/create-test-configuration.sh
M bin/impala-config.sh
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/resources/mysql-hive-site.xml.template
M fe/src/test/resources/postgresql-hive-site.xml.template
M fe/src/test/resources/sentry-site.xml.template
A fe/src/test/resources/sentry-site_no_oo.xml.template
A fe/src/test/resources/sentry-site_oo.xml.template
A fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
M tests/authorization/test_grant_revoke.py
A tests/authorization/test_owner_privileges.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_test_suite.py
33 files changed, 1,233 insertions(+), 87 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/11314/34
--
To view, visit http://gerrit.cloudera.org:8080/11314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e09332e007ed5aa6a0840683c879a8295c3d2b0
Gerrit-Change-Number: 11314
Gerrit-PatchSet: 34
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Anonymous Coward #424
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac

1 2 3 >

1 - 100 of 283 matches

Mail list logo