[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13285 Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker as an interface and introduces BaseAuthorizationChecker that contains authorization helper methods. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 7 files changed, 372 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/13285/1 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker as an interface and introduces BaseAuthorizationChecker that contains authorization helper methods. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 7 files changed, 372 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/13285/2 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3134/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 May 2019 23:31:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 2: I terminated clang-tidy-ub1604 because it was stuck. You probably want to rebase. -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 May 2019 23:31:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3136/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 May 2019 00:06:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 5: Rebased to fix the Maven hang issue. -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 May 2019 00:42:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker into an interface and it also introduces preAuthorize() and postAuthorize() methods to add custom logic before and after the authorization check. The existing implementation code of AuthorizationChecker is now moved to BaseAuthorizationChecker. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 8 files changed, 419 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/13285/5 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3142/ : 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/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 May 2019 01:15:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Austin Nobis has posted comments on this change. (
http://gerrit.cloudera.org:8080/13285 )
Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@421
PS5, Line 421: AuthorizationContext authzCtx = null;
: try {
: authzCtx = authzChecker.preAuthorize(analysisResult_,
catalog_);
: authzChecker.authorize(authzCtx, analysisResult_,
catalog_);
: } catch (AuthorizationException e) {
: authException = e;
: } finally {
: if (authzCtx != null) {
: authzChecker.postAuthorize(authzCtx, analysisResult_,
catalog_);
: }
: }
:
: // AuthorizationExceptions take precedence over
AnalysisExceptions so as not
: // to reveal the existence/absence of objects the user is
not authorized to see.
: if (authException != null) throw authException;
Can't this just be a try { } finally { } and you can remove the authException
local variable?
Also it seems like preAuthorize, authorize, postAuthorize are all called
sequentially. I'm not sure of the benefit of having them be separate methods in
the interface.
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@29
PS5, Line 29: public interface AuthorizationChecker {
Is there a reason that we need preAuth, auth, postAuth as opposed to just
having an auth method and leaving those details up the class that implements
this interface?
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@38
PS5, Line 38: Executes some code before the authorization check.
nit: this documentation seems a bit casual.
maybe: "Function to be executed before an authorization check occurs."
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@53
PS5, Line 53:* Executes some code after the authorization check.
nit: same as above
--
To view, visit http://gerrit.cloudera.org:8080/13285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c
Gerrit-Change-Number: 13285
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 10 May 2019 19:47:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker into an interface and it also introduces preAuthorize() and postAuthorize() methods to add custom logic before and after the authorization check. The existing implementation code of AuthorizationChecker is now moved to BaseAuthorizationChecker. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/AuthorizationContext.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 8 files changed, 415 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/13285/6 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/13285 )
Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@421
PS5, Line 421: AuthorizationContext authzCtx = null;
: try {
: authzCtx = authzChecker.preAuthorize(analysisResult_,
catalog_);
: authzChecker.authorize(authzCtx, analysisResult_,
catalog_);
: } catch (AuthorizationException e) {
: authException = e;
: } finally {
: if (authzCtx != null) {
: authzChecker.postAuthorize(authzCtx, analysisResult_,
catalog_);
: }
: }
:
: // AuthorizationExceptions take precedence over
AnalysisExceptions so as not
: // to reveal the existence/absence of objects the user is
not authorized to see.
: if (authException != null) throw authException;
> Can't this just be a try { } finally { } and you can remove the authException
> local variable?
I think it just makes it more readable since it's easy to tell the precedence
with regards to AnalysisException vs AuthorizationException.
> Also it seems like preAuthorize, authorize, postAuthorize are all called
> sequentially. I'm not sure of the benefit of having them be separate methods
> in the interface.
The idea is to provide pre and post hooks for every authorization provider
without authorization provider worries where and when the hooks are going to be
applied. For example if we change the logic to
try {
pre authorize
do A
do B
authorize
do C
} finally {
do D
post authorize
}
none of the authorization provider code needs to be modified.
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@29
PS5, Line 29: public interface AuthorizationChecker {
> Is there a reason that we need preAuth, auth, postAuth as opposed to just h
See previous comment.
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@38
PS5, Line 38: Executes some code before the authorization check.
> nit: this documentation seems a bit casual.
Done
http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@53
PS5, Line 53:* Executes some code after the authorization check.
> nit: same as above
Done
--
To view, visit http://gerrit.cloudera.org:8080/13285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c
Gerrit-Change-Number: 13285
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 10 May 2019 21:14:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3180/ : 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/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 10 May 2019 21:44:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 May 2019 03:30:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 6: (2 comments) Not quite following the pre/post/context stuff. I'm guessing this will be used for audit? Probably cleaner to just keep this to a refactor (moving code around) and not also introduce this new "context" concept until it's needed for audits? http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@423 PS6, Line 423: authzCtx = authzChecker.preAuthorize(analysisResult_, catalog_); I'm not entirely following the purpose of the pre/post stuff here. Couldn't this all be done within the 'authorize()' call itself? http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@38 PS6, Line 38:* Executes some code before the authorization check. this seems to be a bit more in that it's also supposed to return an authorization context -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 May 2019 21:58:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 7: (2 comments) > Patch Set 6: > > (2 comments) > > Not quite following the pre/post/context stuff. I'm guessing this will be > used for audit? Probably cleaner to just keep this to a refactor (moving code > around) and not also introduce this new "context" concept until it's needed > for audits? Yeah I will introduce the new "context" concept until it's needed for audits in the later CR. http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@423 PS6, Line 423: authException = e; > I'm not entirely following the purpose of the pre/post stuff here. Couldn't Yeah this will be needed for the Ranger audit handler, but yeah I will introduce this class in the ranger audit handler CR instead. Done. http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13285/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@38 PS6, Line 38:* Authorize an analyzed statement. > this seems to be a bit more in that it's also supposed to return an authori Removed the pre/post methods in this CR. Done. -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 May 2019 22:16:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker into an interface The existing implementation code of AuthorizationChecker is now moved to BaseAuthorizationChecker. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 7 files changed, 346 insertions(+), 283 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/13285/7 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3205/ : 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/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 May 2019 22:42:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 May 2019 23:01:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 14 May 2019 00:51:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4248/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 14 May 2019 00:51:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. IMPALA-8528: Refactor authorization check in AnalysisContext This patch moves the authorization check logic from AnalysisContext into BaseAuthorizationChecker to consolidate the logic into a single place. This patch also converts AuthorizationChecker into an interface The existing implementation code of AuthorizationChecker is now moved to BaseAuthorizationChecker. This patch has no functionality change. Testing: - Ran FE tests - Ran E2E authorization tests Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Reviewed-on: http://gerrit.cloudera.org:8080/13285 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 7 files changed, 346 insertions(+), 283 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8528: Refactor authorization check in AnalysisContext
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 ) Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 14 May 2019 07:28:20 + Gerrit-HasComments: No
