Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12020 )
Change subject: IMPALA-7917 (Part 1): Decouple Sentry from Impala ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/12020/7/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/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@32 PS7, Line 32: protected final AuthorizationConfig config_; > This class is now just a wrapper around an authorization call. I wonder if I will revisit this in my next patch. Trying to keep the first part of the CR minimal. http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@56 PS7, Line 56: > While you're in here, might as well simplify this: Done http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java: http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@23 PS7, Line 23: public interface AuthorizationConfig { > I wonder if this interface is a bit too leaky. Seems we're making an assump Yeah it's very leaky. I updated the CR to be more generic. http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java: http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationProxy.java@55 PS7, Line 55: > Are we making a rather strong assumption here? That Impala will cache somet Yeah, I went a bit too far trying to generalize everything. I agree with you that this caching logic should belong stay in the implementation detail. http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/Authorizer.java File fe/src/main/java/org/apache/impala/authorization/Authorizer.java: http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/Authorizer.java@32 PS7, Line 32: > I wonder if the name could be improved. Seems that this is not so much an a Since I'm still not certain if we need to create an interface for this. I decided to not make it an interface yet, especially since a different authorization provider like Ranger prefers to do the administration via non-SQL. I will revisit this again in the next patch. http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/Privilege.java File fe/src/main/java/org/apache/impala/authorization/Privilege.java: http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/authorization/Privilege.java@23 PS7, Line 23: * List of Impala privileges. > Per comments elsewhere, I wonder about the model. The set of privileges is There are two models that we can take. 1. Let Impala decide what privileges it needs and the authorization provider will need to map what Impala needs to what is available in the authorization provider. 2. Let the authorization provider decide what privileges Impala needs. I believe Sentry plugin for Hive plugin does this. Currently I'm leaning towards 1, the reason is we can expect the authorization behavior to remain the same when switching to a different authorization provider. Let me know what your thought on this, though. For the User, we do have an abstraction for that: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/User.java http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/12020/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@52 PS7, Line 52: > As noted elsewhere, should Impala see this? Isn't this a private detail of Yeah, I totally agree with this. Unfortunately Impala itself exposes certain things via SQL such as "create role", "show roles", etc. Because of that, we have Role and User to be part of Impala catalog. It would be nice if in the future, Impala only cares whether user X can perform action Y on object Z and authorization catalog should be an implementation detail. That's what I'm trying to figure out in the later patches. -- To view, visit http://gerrit.cloudera.org:8080/12020 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1fd1df0b38ddd7cfa41299e95f5827f8a9e9c1f Gerrit-Change-Number: 12020 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 07 Feb 2019 21:54:40 +0000 Gerrit-HasComments: Yes
