You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2020/01/17 19:55:27 UTC
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Csaba Ringhofer has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15068
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
WIP IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table to filter out the
bulk of unrelated privileges. The cost of the check is still
O(num_of_privileges), but the expensive
(Impala) PrincipalPrivilege->String->(Sentry)Privilege is avoided
for the filtered privileges.
To improve performance further, privileges could be stored in a
hierarchical way (like Sentry's TreePrivilegeCache) to allow a much
more efficient lookup, but my feeling is that reimplementing the whole
privilege check in Impala and skipping Sentry (at least for
SHOW DATABASES/TABLES) would be simpler.
Another possible optimization would be to store Sentry privileges
in Impala instead of/in addittion to TPriviliges, but that may need
more memory than the current solution.
TODOs:
- measure whether it actually helps with performance
- 'server' probably doesn't need to be checked for all
privileges
- cleaning up the code
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
3 files changed, 130 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/1
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@54
PS11, Line 54: two
> Nit: to.
Done
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@166
PS11, Line 166: build
> Nit: builds.
Done
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 16:58:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#8).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
4 files changed, 467 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/8
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 21:30:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5183/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 16:18:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 12:
(3 comments)
Patch looks good to me. Had some questions related to URI privileges below. Rest looks good.
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG@20
PS7, Line 20: String.intern()
> This change added a "handmade" interner to Impala:
thanks for the pointer. Will take a look.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@49
PS7, Line 49: public class SentryAuthorizationPolicy implements FilteredPrivilegeCache {
> I see 3 reason why the fallback could be useful:
makes sense. Thanks for considering the suggestion.
http://gerrit.cloudera.org:8080/#/c/15068/12/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/12/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140
PS12, Line 140: private List<String> toPath() {
Curious to understand the motivation to disallow creating filter like server1->uri1 since both server1->uri1 and server1->uri2 both will have the path as [server1] with the boolean flag set to true. Does this mean a getFilteredList() will return all the server level privileges and all the URI privileges when we are only interested in the authorization heirarchy of server1-uri1?
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 19:34:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 14: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Feb 2020 15:02:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Feb 2020 10:10:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG@20
PS7, Line 20: String.intern()
> Curisous to know if we have any references like past JIRAs to show to Strin
This change added a "handmade" interner to Impala:
https://gerrit.cloudera.org/#/c/11158/
It mentions this post with Java 8 benchmarks: https://shipilev.net/jvm-anatomy-park/10-string-intern/
The intention of the change was to reduce memory consumption by interning some common strings, and it avoided String.intern() to be sure that no regression is introduced.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@49
PS7, Line 49: public class SentryAuthorizationPolicy implements FilteredPrivilegeCache {
> Do you think we should refactor this code such that we can revert back to P
I see 3 reason why the fallback could be useful:
1: to be able to work with a version of Sentry that doesn't support FilteredPrivilegeCache
2: to be able to avoid the memory cost of the PrincipalPrivilegeTree
3: to be able to turn this off if it turns out to be buggy
I think that 1 is not a valid scenario, as Impala wouldn't even compile with such a Sentry version. I expect this patch to be backported together with the relevant Sentry changes.
2 makes sense as the tree does consume some extra memory for privileges, but I assume that most users who have enough privileges for this to matter will also have problems with the slow SHOW DATABASES / TABLES. (this is not necessarily true, as Impala caches privileges for every user/role, while the speed is only affected by the number of privileges of the current user and its roles).
I am not enthusiastic about 3, as adding an option could also introduce issues (there were some Sentry bugs that only occurred if it had to fall back to PrivilegeCache, see SENTRY-2545 and SENTRY-2549), so I am not sure that we would really help here. Actually SENTRY-2549 is still not merged to ASF/master Sentry, so falling back to PrivilegeCache is still buggy.
So I think that 2 is the only good reason, but if we want to spend time to reduce the mem cost/privilege, I would prefer other ways, e.g. interning database/table names in privileges (we already intern these string in other CatalogObjects).
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@156
PS7, Line 156: filter
> It unclear why we are not storing the uri here? Can you clarify?
I added some explanation in PrincipalPrivilegeTree.
+1 reason is that URIs are also a bit more problematic than other objects due to their case sensitivity.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@200
PS7, Line 200:
> Oops, I forgot this, I will do this in the next patch.
Done
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 16:37:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15068/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@143
PS6, Line 143:
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 15:50:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@110
PS2, Line 110: public Set<String> listPrivileges(Set<String> groups, Set<String> users, ActiveRoleSet roleSet,
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@200
PS2, Line 200: && (table_ == null || table_.equalsIgnoreCase(privilege.getTable_name()));
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 17:46:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#2).
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
WIP IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table to filter out the
bulk of unrelated privileges. The cost of the check is still
O(num_of_privileges), but the expensive
(Impala) PrincipalPrivilege->String->(Sentry)Privilege is avoided
for the filtered privileges.
To improve performance further, privileges could be stored in a
hierarchical way (like Sentry's TreePrivilegeCache) to allow a much
more efficient lookup, but my feeling is that reimplementing the whole
privilege check in Impala and skipping Sentry (at least for
SHOW DATABASES/TABLES) would be simpler.
Another possible optimization would be to store Sentry privileges
in Impala instead of/in addittion to TPriviliges, but that may need
more memory than the current solution.
TODOs:
- measure whether it actually helps with performance
- 'server' probably doesn't need to be checked for all
privileges
- cleaning up the code
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
3 files changed, 131 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/2
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#13).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 513 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/13
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Feb 2020 06:14:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward (498) has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/Principal.java@85
PS1, Line 85: * Returns all privilege names for this principal, or an empty set of no privileges are
"or an empty set of no privileges" should be "or an empty set if no privileges"
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@196
PS1, Line 196: return (server_ == null || server_.equalsIgnoreCase(privilege.getServer_name()))
the condition check logic is not correct.
It should be
(server_ == null || (server_.equalsIgnoreCase(privilege.getServer_name()) && (db_ == null || db_.equalsIgnoreCase(privilege.getDb_name())));
if server_ == null, always returns true.
if server_match, return true if (db_ == null) or (db_ match)
same for table check
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 21:37:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java@104
PS7, Line 104: * Returns all privilege names for this principal, or an empty set if no privileges are
The doc comment is the same as for getPrivilegeNames() and does not mention the filter.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@34
PS7, Line 34: for
Isn't this 'for' superfluous?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@37
PS7, Line 37: public class PrincipalPrivilegeTree {
The implementation seems correct to me, though I haven't tried it yet, only read it.
I think it is a good idea to add unit tests.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@134
PS7, Line 134: s
Nit: object.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150
PS7, Line 150: void add(T value, List<String> path, int depth) {
I think 'depth' should be documented.
Also it seems to me that depth is always 0 when the method is called from outside and any other depth value only appears during the recursion. I think it would be clearer to make a public wrapper method that calls this with depth=0 and make this method private.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@167
PS7, Line 167: void remove(T value, List<String> path, int depth) {
See the comment about 'depth' for the 'add' method.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@200
PS7, Line 200: void getAllMatchingValues(List<T> results, List<String> path, int depth) {
See the comment about 'depth' for the 'add' method.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 18:36:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150
PS7, Line 150: void add(T value, List<String> path, int depth) {
> I think 'depth' should be documented.
As an alternative, instead of passing 'depth', we could also pass a sublist view of 'path' to the next recursion step and always use path.at(0). Personally for me it looks cleaner though I'm not sure if it has performance implications.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 08:07:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5463/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 21:23:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5484/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 18:31:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#10).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 506 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/10
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5182/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 15:58:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#11).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 509 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/11
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 8:
I wanted to upload the unit tests first and then fix/answer the other. Then I realized that I messed the update logic up, so I need some more work before a proper patch.
The failed jenkins code review check seems unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2020 19:09:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#6).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
3 files changed, 342 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/6
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Reviewed-on: http://gerrit.cloudera.org:8080/15068
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 513 insertions(+), 9 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 9:
(17 comments)
Thanks everyone for your comments!
Some guide to view the new patch sets:
PS8: adds unit tests and fixed a bug about not returning server privileges when listing URI privileges
PS9: fixes update behavior - this also needed Nodes to store name->PrincipalPrivilege maps for values instead of sets of PrincipalPrivileges.
PS10: Fixes most of the review comments.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java@104
PS7, Line 104: * Returns all privilege names for this principal, or an empty set if no privileges are
> The doc comment is the same as for getPrivilegeNames() and does not mention
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java@118
PS7, Line 118: Set<String> results = new HashSet<>();
> May be I am overthinking this. Is there a race here? We are releasing the r
PrincipalPrivilege's fields are accessed at a lot of places in the code, so I am hesitant about adding a comment about this here.
PrincipalPrivilege is practically treated as immutable - most of its fields are included in the name (returned by getName()), which is also used as a key in CatalogObjectCache, so changing any of these members would break our assumptions about CatalogObjectCache regardless of thread safety. The only way it should be changed is by replacing it with a new PrincipalPrivilege with the same name (and higher catalog version). If this happens on another thread, the old PrincipalPrivilege will be held by the current thread for the given privilege check - my understanding is that this is the intended way to use CatalogObjects.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@34
PS7, Line 34: Tree that allows efficient lookup
> Can we have a more detailed documentation of the class? Specifically, why i
I added more details in the comment.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@34
PS7, Line 34: for
> Isn't this 'for' superfluous?
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@37
PS7, Line 37: public class PrincipalPrivilegeTree {
> The implementation seems correct to me, though I haven't tried it yet, only
I added unit tests which (of course :/) found some bugs (wrong update logic + not returning match server privileges when listing privileges for an URI).
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@41
PS7, Line 41: Node<PrincipalPrivilege> tableRoot_ = new Node<>();
:
: // Contains URI privileges grouped by server name.
: Node<PrincipalPrivilege> uriRoot_ = new Node<>();
:
: // Contains server privileges grouped by server name.
: Node<PrincipalPrivilege> serverRoot_ = new Node<>();
> Can you clarify why we need to have 3 separate trees instead of just one?
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@105
PS7, Line 105: return path;
> Unclear what this means. Can you please clarify?
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@124
PS7, Line 124:
> Does server double up for storing uris too?
Yes, URIs are stored per server. Added some explanation in the class comment.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@125
PS7, Line 125: private List<String> toPath() {
> if uri is not null, should you add it into path before return?
The URI string are intentionally not added to the path. Added some explanation about this in the class comment.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@134
PS7, Line 134:
> Nit: object.
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@138
PS7, Line 138: Tree node that holds the privileges for a given objects (server, database,
> probably unnecessary and can be removed.
I would prefer to leave it here - generally if there is a generic class, I would assume that it is expected to be reused with different classes.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140
PS7, Line 140: * with paths like ["server1", "db1", "table1"].
> nit, can we move this class near the top?
I would prefer to keep the members + public functions at the top and leave to more complex tree logic at the end. I am not sure if we have a general rule for this.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150
PS7, Line 150: /**
> As an alternative, instead of passing 'depth', we could also pass a sublist
I reversed that paths and removed the last element on every level at first - but this caused a bug as the same path is used with multiple "roots".
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150
PS7, Line 150:
> +1 to Daniel's first comment above. Since all the public usage of this meth
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@167
PS7, Line 167: * Finds the Node at 'path' (it is treated as error if it doesn't exist),
> See the comment about 'depth' for the 'add' method.
Done
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@172
PS7, Line 172: if (path.size() <= depth) {
> Will this line ever get executed given the Preconditions check above?
Yes, it can be executed. "found" means that the given PrincipalPrivilege was really found and removed (the comment of the function mentions that it is an error of the element to remove doesn't exist), while the next line sets the value_ to null if it became empty to save some memory.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@200
PS7, Line 200:
> See the comment about 'depth' for the 'add' method.
Oops, I forgot this, I will do this in the next patch.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 15:48:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15068/12/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/12/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140
PS12, Line 140: private List<String> toPath() {
> Curious to understand the motivation to disallow creating filter like serve
There is some explanation in the comment from line 38 to 42.
The goal was to speed up SHOW DATABASES/TABLES, and indexing URIs would not help with this, while it could potentially eat a lot of memory if there are a lot of URI privileges because each URI would need a Node object + a hash map for values.
I still wanted to add them to the PrincipalPrivilegeTree to give a simpler interface towards Privilege. This is a compromise, there is a cost though as every URI privilege is added to a hash map.
+1 reason is that the Sentry solution also doesn't create a Node for each URI: https://github.com/apache/sentry/blob/e2dd73d995be51f237cce3ae8240cc8c3407e820/sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java#L222
The reason there is probably the case sensitivity of URIs.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 21:03:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5485/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 18:36:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5142/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2020 20:39:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jan 2020 01:59:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@110
PS1, Line 110: public Set<String> listPrivileges(Set<String> groups, Set<String> users, ActiveRoleSet roleSet,
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@202
PS1, Line 202: && (table_ == null || table_.equalsIgnoreCase(privilege.getTable_name()));
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 19:56:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5324/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 Feb 2020 10:10:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11: Code-Review+1
(2 comments)
Looks good to me, only small typos.
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@54
PS11, Line 54: two
Nit: to.
http://gerrit.cloudera.org:8080/#/c/15068/11/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@166
PS11, Line 166: build
Nit: builds.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 14:51:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward (498) has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@125
PS7, Line 125: if (db_ == null) return path;
if uri is not null, should you add it into path before return?
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 22:17:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#12).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 509 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/12
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5310/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 16:38:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@110
PS2, Line 110: public Set<String> listPrivileges(Set<String> groups, Set<String> users,
> line too long (97 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/Principal.java@85
PS1, Line 85: * Returns all privilege names for this principal, or an empty set if no privileges are
> "or an empty set of no privileges" should be "or an empty set if no privile
Done
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/15068/1/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@196
PS1, Line 196: case TABLE:
> the condition check logic is not correct.
Done
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/15068/2/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@200
PS2, Line 200: && (table_ == null || table_.equalsIgnoreCase(privilege.getTable_name()));
> line too long (91 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Jan 2020 17:52:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5139/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2020 14:34:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5439/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 19:58:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#3).
Change subject: WIP IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
WIP IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table to filter out the
bulk of unrelated privileges. The cost of the check is still
O(num_of_privileges), but the expensive
(Impala) PrincipalPrivilege->String->(Sentry)Privilege is avoided
for the filtered privileges.
To improve performance further, privileges could be stored in a
hierarchical way (like Sentry's TreePrivilegeCache) to allow a much
more efficient lookup, but my feeling is that reimplementing the whole
privilege check in Impala and skipping Sentry (at least for
SHOW DATABASES/TABLES) would be simpler.
Another possible optimization would be to store Sentry privileges
in Impala instead of/in addittion to TPriviliges, but that may need
more memory than the current solution.
TODOs:
- measure whether it actually helps with performance
- 'server' probably doesn't need to be checked for all
privileges
- cleaning up the code
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
3 files changed, 131 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/3
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15068/5/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/5/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@48
PS5, Line 48:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/15068/5/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@143
PS5, Line 143:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/15068/5/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@145
PS5, Line 145:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/15068/5/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@212
PS5, Line 212: }
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 15:48:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Vihang Karajgaonkar, Daniel Becker, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#9).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
A fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
5 files changed, 486 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/9
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5119/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 16:33:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#7).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
3 files changed, 342 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/7
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 7:
(13 comments)
Took a first pass and left some comments and suggestions. Thanks a lot for fixing this.
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15068/7//COMMIT_MSG@20
PS7, Line 20: String.intern()
Curisous to know if we have any references like past JIRAs to show to String interning causes problems? In my experience, since Java 8 String.intern() is pretty stable and useful on large scale systems (eg: HMS uses it for interning the partition descriptions etc after object deserialization)
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@49
PS7, Line 49: public class SentryAuthorizationPolicy implements FilteredPrivilegeCache {
Do you think we should refactor this code such that we can revert back to PrivilegeCache if needed? One way to do that would be to extend this class which implements FilteredPrivilegeCache and override the methods as needed. Then we can use some configuration to instantiate the right class and we can easily revert to older behavior.
That would also mean that the changes in the PrincipalCache will use a NoOpPrivilegeTree instance in case the config is turned off.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java@156
PS7, Line 156: filter
It unclear why we are not storing the uri here? Can you clarify?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java
File fe/src/main/java/org/apache/impala/catalog/Principal.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/Principal.java@118
PS7, Line 118: Set<String> results = new HashSet<>();
May be I am overthinking this. Is there a race here? We are releasing the readlock while still accessing the PrincipalPrivilege's fields. In order to truely guarantee that this will not have races, we would need PrincipalPrivilege to be Immutable object but its not. I don't see a problem currently with this implementation but we should atleast add a comment here explaining why releasing the readlock is safe.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java:
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@34
PS7, Line 34: Tree that allows efficient lookup
Can we have a more detailed documentation of the class? Specifically, why is it more efficient and if there are any caveats?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@41
PS7, Line 41: Node<PrincipalPrivilege> tableRoot_ = new Node<>();
:
: // Contains URI privileges grouped by server name.
: Node<PrincipalPrivilege> uriRoot_ = new Node<>();
:
: // Contains server privileges grouped by server name.
: Node<PrincipalPrivilege> serverRoot_ = new Node<>();
Can you clarify why we need to have 3 separate trees instead of just one?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@105
PS7, Line 105: Lossy representation
Unclear what this means. Can you please clarify?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@124
PS7, Line 124: server_
Does server double up for storing uris too?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@138
PS7, Line 138: Only used to store privileges, but creating a generic class seemed clearer.
probably unnecessary and can be removed.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@140
PS7, Line 140: private static class Node<T> {
nit, can we move this class near the top?
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@150
PS7, Line 150: depth
+1 to Daniel's first comment above. Since all the public usage of this method is always with depth 0 perhaps make this method private and add a public method which takes in just the value and path. Same for the remove method as well.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@152
PS7, Line 152: if (values_ == null) values_ = new HashSet<>();
Do we need a sanity check here? Preconditions.checkState(path.size() == depth-1)? Can skip the Precondititions check if we mark this method as private so that we always control the value of depth.
http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java@172
PS7, Line 172: if (values_.isEmpty()) values_ = null;
Will this line ever get executed given the Preconditions check above?
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2020 20:07:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5120/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 16:35:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5204/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 21:51:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5121/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2020 16:35:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5203/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2020 17:43:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Anonymous Coward (498), Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15068
to look at the new patch set (#5).
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
---
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Principal.java
A fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
3 files changed, 342 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/15068/5
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15068 )
Change subject: IMPALA-9242: Filter privileges before returning them to Sentry
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5181/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15068
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Gerrit-Change-Number: 15068
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2020 14:59:15 +0000
Gerrit-HasComments: No