[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
[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 (#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 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
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: 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
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
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: (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
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. http://gerrit.cloudera.org:8080/#/c/15068/7/fe/src
[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
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
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 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
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 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 (#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
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 expec
[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
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
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
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 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
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
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
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
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
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
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
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
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
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
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 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
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