Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14106 )
Change subject: IMPALA-8228: Ownership support for Ranger authz ...................................................................... Patch Set 6: (14 comments) http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java File fe/src/main/java/org/apache/impala/authorization/Authorizable.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java@62 PS6, Line 62: return getName().hashCode(); > If we're comparing owner, we should update the hashCode() as well. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java@62 PS6, Line 62: return getName().hashCode(); > If we're comparing owner, we should update the hashCode() as well. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/Authorizable.java@70 PS6, Line 70: temp.getOwnerUser() == this.getOwnerUser() > Shouldn't this use equals()? Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@31 PS6, Line 31: private final String ownerUser_; > For consistency, annotate with @Nullable. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@34 PS6, Line 34: String ownerUser > I think it's more readable to also annotate the parameter with @Nullable. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java@33 PS6, Line 33: String ownerUser > Same comment about @Nullable parameter. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java@45 PS6, Line 45: /** : * Creates a new instance of column {@link Authorizable} for a given database name and : * gives access to all tables and columns. : */ : Authorizable newColumnAllTbls(String dbName, @Nullable String dbOwnerUser); : : /** : * Creates a new instance of column {@link Authorizable} for given database and table : * names and gives access to all columns. : */ : Authorizable newColumnInTable( : String dbName, String tableName, @Nullable String tblOwnerUser); : : /** : * Creates a new instance of column {@link Authorizable} for given database, table, and : * column names. : */ : Authorizable newColumnInTable( : String dbName, String tableName, String columnName, @Nullable String tblOwnerUser); > nit: update javadoc about the new owner parameter. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java@34 PS6, Line 34: String ownerUser > Same comment about @Nullable parameter. Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@94 PS6, Line 94: resources.add(new RangerImpalaResourceBuilder() : .database("*").table("*").column("*").build()); : resources.add(new RangerImpalaResourceBuilder() : .database("*").function("*").build()); : resources.add(new RangerImpalaResourceBuilder().uri("*").build()); > Should we add .owner() here as well? This patch was meant to add the ownership only for db/tbl[col]. Also curious what does it even mean to be a owner of a server? http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/catalog/Db.java@544 PS6, Line 544: if (db == null) return null; : return db.getOwnerName(); > nit: can be inlined: return db == null ? null : db.getOwnerName() Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/main/java/org/apache/impala/service/Frontend.java@783 PS6, Line 783: TODO > mind filing a JIRA and putting the number here? Done http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/14106/6/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@3060 PS6, Line 3060: authorize("select count(*) from functional.alltypes") > nit: might make this test a little easier to read to do something like crea Done http://gerrit.cloudera.org:8080/#/c/14106/6/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/14106/6/tests/authorization/test_ranger.py@669 PS6, Line 669: We need to fix this by caching the HMS metadata : # more aggressively when the table loads. > add a TODO(IMPALA-xxxxx) with jira Done http://gerrit.cloudera.org:8080/#/c/14106/6/tests/authorization/test_ranger.py@665 PS6, Line 665: # Run show tables on the db. The resulting list should be empty. This happens : # because the created table's ownership information is not aggresively cached : # by the current Catalog implementations. Hence the analysis pass does not : # have access to the ownership information to verify if the current session : # user is actually the owner. We need to fix this by caching the HMS metadata : # more aggressively when the table loads. > this is true only in localcatalog, right? otherwise the 'create table' call This is true for legacy catalog impl too. https://github.com/apache/impala/blob/de77c61f383794601c79d9783d36235482740417/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L2256 Btw, I missed test coverage for localcatalog. While adding it, I found some goofy behavior with getTableIfCached(), which doesn't load the hms descriptor, so show table never works. I added a test with xfail, will fix in a follow up commit. -- To view, visit http://gerrit.cloudera.org:8080/14106 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I737b7164a3e7afb9996b3402e6872effd663f7b4 Gerrit-Change-Number: 14106 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fre...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 11 Sep 2019 08:04:05 +0000 Gerrit-HasComments: Yes
