Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20508 )
Change subject: IMPALA-12398: Ranger role not exists when altering db/table/view owner to a role ...................................................................... Patch Set 3: (4 comments) Thanks for the detailed explanation Ji! I took a closer look at the patch set 3 and understand now why it's necessary to have the class CatalogServiceTestCatalogWithRanger. It's because after the patch set 3, Impala's Analyzer starts checking whether or not the role exists in the given ALTER statement, i.e., AnalysisUtils#checkRole() will be executed at runtime and in the JUnit tests, e.g., AuthorizationStmtTest#testAlterDatabase(). I only have some minor comments. We could also wait for Quanlong to see he has some additional suggestion. http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG@7 PS3, Line 7: Ranger role not exists when altering db/table/view owner to a role nit: Could we change the title to something like "Check the existence of the role in the correct place in SET OWNER ROLE"? http://gerrit.cloudera.org:8080/#/c/20508/3//COMMIT_MSG@9 PS3, Line 9: When Role '<ROLE_NAME>' is created with Ranger authorization enabled, if : 'ALTER TABLE <TABLE_NAME> SET OWNER ROLE <ROLE_NAME>' statement is : executed to assign role as the owner of the table, it will throw : AnalysisException:Role '<ROLE_NAME>' does not exist. : : It is due to in the original design, AuthorizationPolicy class is used to : cache the roles/principal, So it is used to check the existence of the role. : However, this class is obsolete in certain degree in the current design, : in the catalogd side,create/drop role/show role will bypass the : AuthorizationPolicy object, and directly use the ranger plugin to perform : operation on the roles, it also utilize cache within ranger plugin. That is : why no roles are available in AuthorizationPolicy object. : : This patch will directly use ranger impala plugin to check the existence of the role, : instead of using AuthorizationPolicy object. nit: I was wondering if it would be clearer to change the commit message to the following. It should be okay that we do not say too much about AuthorizationPolicy. Before this patch, given the ALTER DATABASE/TABLE/VIEW SET OWNER ROLE statement, Impala always checked the existence of the given role in its AuthorizationPolicy. However, when the support for role-related statements with Ranger was added in IMPALA-10211, we only added the roles in RangerImpalaPlugin instead of AuthorizationPolicy. Therefore, the statement above would fail even though an authorized user tries to set the owner to an existing role in RangerImpalaPlugin. This patch fixes the problem by making Impala check the existence of a given role in RangerImpalaPlugin when the authorization provider is Ranger. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java File fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java: http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@68 PS3, Line 68: cs.setRangerImpalaPlugin(rangerImpalaPlugin); : cs.setAuthzManager(factory.newAuthorizationManager(cs)); : cs.setMetastoreEventProcessor(NoOpEventProcessor.getInstance()); : cs.setCatalogMetastoreServer(NoOpCatalogMetastoreServer.INSTANCE); : cs.setCatalogOpExecutor(new CatalogOpExecutor(cs, : new NoopAuthorizationFactory().getAuthorizationConfig(), : new NoopAuthorizationFactory.NoopAuthorizationManager(), : new TestHiveJavaFunctionFactory())); : cs.setEventFactoryForSyncToLatestEvent( : new MetastoreEvents.EventFactoryForSyncToLatestEvent( : cs.getCatalogOpExecutor())); : cs.reset(); CatalogServiceTestCatalogWithRanger#createWithAuth() shares these statements with CatalogServiceTestCatalog#createWithAuth(). It would be good if we put them in a method in the class CatalogServiceTestCatalog and call this method here and in CatalogServiceTestCatalog#createWithAuth() to eliminate duplicate code. http://gerrit.cloudera.org:8080/#/c/20508/3/fe/src/test/java/org/apache/impala/authorization/CatalogServiceTestCatalogWithRanger.java@96 PS3, Line 96: List<RangerRole.RoleMember> roleMemberList = : grantGroups.stream() : .map(new Function<String, RangerRole.RoleMember>() { : @Override : public RangerRole.RoleMember apply(String s) { : RangerRole.RoleMember roleMember = : new RangerRole.RoleMember(s, s.equals(RANGER_ADMIN_USER)); : return roleMember; : } : }) : .collect(Collectors.toList()); : role.setGroups(roleMemberList); It's not clear to me whether we need to call Role#setGroups() on 'roleMemberList' here. We don't seem to call Role#setGroups() at runtime. In addition, I tried commenting out this piece of code and found out that tests like AuthorizationStmtTest#testAlterDatabase() could still succeed without the code. Did I miss something? -- To view, visit http://gerrit.cloudera.org:8080/20508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b029bdb90111dbd0eab5189360cc81090225cda Gerrit-Change-Number: 20508 Gerrit-PatchSet: 3 Gerrit-Owner: ji chen <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: ji chen <[email protected]> Gerrit-Comment-Date: Wed, 11 Oct 2023 19:00:58 +0000 Gerrit-HasComments: Yes
