[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 12 Feb 2020 15:02:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Tested-by: Impala Public Jenkins --- 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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 12 Feb 2020 10:10:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 12 Feb 2020 10:10:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 12 Feb 2020 06:14:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 11 Feb 2020 21:51:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Tue, 11 Feb 2020 21:03:35 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Tue, 11 Feb 2020 19:34:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 11 Feb 2020 17:43:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 11 Feb 2020 16:58:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 11 Feb 2020 14:51:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 10 Feb 2020 21:30:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 10 Feb 2020 16:38:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Mon, 10 Feb 2020 16:37:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 10 Feb 2020 16:18:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 10 Feb 2020 15:58:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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 tableRoot_ = new Node<>();
:
:// Contains URI privileges grouped by server name.
:Node uriRoot_ = new Node<>();
:
:// Contains server privileges grouped by server name.
:Node 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 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
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 10 Feb 2020 14:59:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 06 Feb 2020 20:39:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 06 Feb 2020 19:09:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 06 Feb 2020 14:34:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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 tableRoot_ = new Node<>();
:
:// Contains URI privileges grouped by server name.
:Node uriRoot_ = new Node<>();
:
:// Contains server privileges grouped by server name.
:Node 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 {
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.
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 05 Feb 2020 08:07:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 22:17:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 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 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 results, List
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
Gerrit-Reviewer: Anonymous Coward (498)
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 04 Feb 2020 18:36:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 16:35:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 16:35:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 16:33:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 15:50:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 04 Feb 2020 15:48:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9242: Filter privileges before returning them to Sentry
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 Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins
