Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17640 )
Change subject: IMPALA-10436: Support storage handler privileges for external Kudu table creation ...................................................................... Patch Set 8: (9 comments) Hi all, I have revised my previous patch according to Csaba's comments. Let me know if there is any other suggestion. Thanks very much for the help! http://gerrit.cloudera.org:8080/#/c/17640/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17640/5//COMMIT_MSG@29 PS5, Line 29: > Were these tests added somewhere? Sorry if I have just missed them. Thanks Csaba! I think I may have forgotten to add a test case for this GRANT statement. In my next patch I will add some tests to _test_show_grant_basic() which will be called in test_show_grant() of TestRanger. http://gerrit.cloudera.org:8080/#/c/17640/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17640/6//COMMIT_MSG@7 PS6, Line 7: IMPALA-10436: Support storage handler privileges for external Kudu tab > To me the title suggests that creating external Kudu tables will simply nee Done http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@406 PS5, Line 406: // Server is used by column, function, URI, and storage h > Can you update the comment? Done http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@445 PS5, Line 445: / > is it valid if we don't go to the if? Thanks Csaba! According to my observation, when 'resource' corresponds to a storage handler URI and we do not add Privilege.RWSTORAGE to 'accessTypes' of 'request', in the end there won't be a Ranger policy added. We need to consider the cases of TPrivilegeLevel.ALL and TPrivilegeLevel.OWNER because for a statement that grants or revokes the ALL or OWNER privileges on SERVER, 'resource' could also correspond to a storage handler URI. But at the same time, we don't want to ask Ranger to add a policy associated with the RWSTORAGE privilege on a storage handler URI for a query that grants or revokes a privilege other than ALL or OWNER. For instance, we don't want to ask Ranger to add a policy to grant the RWSTORAGE privilege on the wildcard storage handler URI for a query like "GRANT SELECT ON SERVER TO USER non_owner". I will also revise the code comment accordingly in the next patch to make the logic clear. http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@449 PS5, Line 449: non_o This seems problematic in that this allows an administrator to grant the privilege of RWSTORAGE on a resource that is not a storage handler URI. I will change this part to the following in the next patch as well. } else if (level != TPrivilegeLevel.RWSTORAGE) { request.getAccessTypes().add(level.name().toLowerCase()); } http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/17640/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@325 PS5, Line 325: // Server is used by column, function, URI, and storage h > stale comment Done http://gerrit.cloudera.org:8080/#/c/17640/5/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/17640/5/tests/authorization/test_ranger.py@288 PS5, Line 288: ["USER", user, "", "", "", "*", "", "", "", "select", "false"], The privilege of rwstorage on a uri is not well-defined. I will revise the patch so that Impala won't ask Ranger to add such a policy. http://gerrit.cloudera.org:8080/#/c/17640/5/tests/authorization/test_ranger.py@295 PS5, Line 295: ["USER", user, "*", "*", "*", "", "", "", "", "alter", "false"], The privilege of rwstorage on a database is not well-defined. I will revise the patch so that Impala won't ask Ranger to add such a policy. http://gerrit.cloudera.org:8080/#/c/17640/5/tests/authorization/test_ranger.py@302 PS5, Line 302: min_client.execute("grant all on server to user {0}".format(user)) The privilege of rwstorage on all databases/tables/columns is not well-defined. I will revise the patch so that Impala won't ask Ranger to add such a policy. -- To view, visit http://gerrit.cloudera.org:8080/17640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7936e1d8c48696169f7ad7ad92abe44a26eea3c4 Gerrit-Change-Number: 17640 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 06 Oct 2022 01:10:54 +0000 Gerrit-HasComments: Yes
