Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 )
Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala ...................................................................... Patch Set 9: (26 comments) http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@66 PS9, Line 66: be may *become* stale, right? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@41 PS9, Line 41: boolean isAdmin(User user) throws ImpalaException; why is admin a special thing rather than just a separate privilege verified by the authorization checker interface? Just curious, doesn't need to change now. http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@56 PS9, Line 56: * Gets all roles; typo http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@85 PS9, Line 85: * Gets all privileges. can you clarify, this is all privileges granted to anyone anywhere? Or just on this server/service? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@93 PS9, Line 93: void updateDatabaseOwnerPrivilege(String serverName, String databaseName, can you document any expected canonicalization of the names? should they be lower cased? Are they case sensitive? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@95 PS9, Line 95: PrincipalType newOwnerType, TDdlExecResponse response) throws ImpalaException; I think it's worth expanding the javadoc slightly to explain why the _old_ values are passed in. Can multiple users be an owner of a database? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java@100 PS9, Line 100: */ same http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@77 PS9, Line 77: public void createRole(User requestingUser, TCreateDropRoleParams params, should these functions throw UnsupportedOperationException so that if somehow it gets configured, it won't silently ignore requests? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@52 PS9, Line 52: Sentry for Catalogd parsing this sentence is a bit difficult - I read this as "sentry for catalogd". Maybe "for catalogd that uses Sentry" makes more sense? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@57 PS9, Line 57: Impalads for operations, such as SHOW ROLES and SHOW GRANT I think just saying "broadcasted to all Impalads" is enough, since it's used for anything that requires authorization metadata, right? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@127 PS9, Line 127: String roleName = params.getRole_names().get(0); For this and other calls below, can we verify that getRole_names().size() == 1 if we expect exactly one? (same with getGroup_names()) Can probably use Iterables.getOnlyElement(params.getRole_names()) http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@261 PS9, Line 261: catalog_.getSentryProxy() == null || this would be a bug, right? perhaps we can call verifySentryServiceEnabled() here? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@263 PS9, Line 263: return; if object ownership is not enabled, doesnt it seem like we should throw an error back to the user instead of no-opping this? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@265 PS9, Line 265: Preconditions.checkNotNull(serverName); perhaps checkNotNull(databaseName) too? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@296 PS9, Line 296: public void updateTableOwnerPrivilege(String serverName, String databaseName, I think it's worth having a private method which looks like the old CatalogOpExecutor.updateOwnerPrivileges to capture the commonality of this method and the one above? Perhaps take TPrivilege as an argument http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@335 PS9, Line 335: private TPrivilege createTableOwnerPrivilegeFilter(String databaseName, can be static http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@346 PS9, Line 346: private TPrivilege createDatabaseOwnerPrivilegeFilter(String databaseName, same http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@49 PS9, Line 49: Sentry for Impalad see comment on catalogd equivalent -- hard to parse this http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@94 PS9, Line 94: public TShowRolesResult getRoles(TShowRolesParams params) throws ImpalaException { just a thought, no need to do it in this patch, but it seems that the methods on the impalad are the "read-only" methods. Would it make sense to just move these to the "checker" interface so that this class is only reponsibility with "mutating" methods? (fine to disagree) http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@483 PS9, Line 483: public void addSummary(TDdlExecResponse response, String summary) { can this be static? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3174 PS9, Line 3174: Preconditions.checkNotNull(grantRevokeRoleParams.isIs_grant()); checkArgument? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3188 PS9, Line 3188: Preconditions.checkNotNull(!grantRevokeRoleParams.isIs_grant()); checkArgument http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3202 PS9, Line 3202: Preconditions.checkNotNull(grantRevokePrivParams.isIs_grant()); this should be checkArgument, right? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3216 PS9, Line 3216: Preconditions.checkNotNull(!grantRevokePrivParams.isIs_grant()); checkArgument http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java@304 PS9, Line 304: * TODO: rename this method name. did you mean to do this in this patch? http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/util/ClassUtil.java File fe/src/main/java/org/apache/impala/util/ClassUtil.java: http://gerrit.cloudera.org:8080/#/c/12684/9/fe/src/main/java/org/apache/impala/util/ClassUtil.java@22 PS9, Line 22: * Returns the current method name. hrm... is this really worth it for the use case? My fear is that this is actually pretty expensive and people might not realize it -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 14 Mar 2019 00:12:03 +0000 Gerrit-HasComments: Yes
