Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/20916 )
Change subject: IMPALA-12578: Pass owner user of database and table to Ranger in GRANT/REVOKE ...................................................................... Patch Set 2: (6 comments) Hi all, I have addressed Csaba's and Quanlong's comments on patch set 1. Please let me know if there is any additional suggestion. Thank you very much for your help! http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20916/1//COMMIT_MSG@33 PS1, Line 33: the principal type could be : a group or a role. > Will this be supported in future tasks? I.e. a user belongs to the owner gr Thanks Quanlong! Taking a look at the definition of the class GrantRevokeRequest at https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/util/GrantRevokeRequest.java, I found that there is no field that denotes the type of the owner. The only field related to the owner of a resource is 'ownerUser' and thus it seems Ranger implicitly assumes that the provided owner corresponds to a user. With the above being said, it looks like Impala would not be able to support allowing a user belonging to an owner group to execute GRANT and REVOKE unless Ranger adds an additional field in GrantRevokeRequest to distinguish an owner user from an owner group. For easy reference, we provide the class definition in the following. public class GrantRevokeRequest implements Serializable { private static final long serialVersionUID = 1L; private String grantor; private Set<String> grantorGroups; private Map<String, String> resource; private Set<String> users; private Set<String> groups; private Set<String> roles; private Set<String> accessTypes; private List<String> forwardedAddresses; private String remoteIPAddress; private Boolean delegateAdmin = Boolean.FALSE; private Boolean enableAudit = Boolean.TRUE; private Boolean replaceExistingPermissions = Boolean.FALSE; private Boolean isRecursive = Boolean.FALSE; private String clientIPAddress; private String clientType; private String requestData; private String sessionId; private String clusterName; private String zoneName; private String ownerUser; ... } http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/20916/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@236 PS1, Line 236: analyzeTargetDatabase(analyzer); : break; : case URI: : Preconditions.checkNotNull(uri_); : if (privilegeLevel_ != TPrivilegeLevel.ALL) { : throw new AnalysisException("Only 'ALL' privilege may be applied at " + : "URI scope in privilege spec."); : } > nit: We can move codes of the precondition check and the try-catch clause i Done http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1191 PS1, Line 1191: revoke_table_stmt, set_table_owner_group_stmt, resource_owner_group, > +1. This test has ~300 lines. It'd be better to refactor it. Thanks Csaba and Quanlong! Yeah. Indeed. I will move the code for each case to its respective sub-function in the next patch. http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1198 PS1, Line 1198: admin_client, revoke_column_stmt, set_table_owner_group_stmt, > nit: do we really need this refresh? I thought we only need it when we add/ Thanks Quanlong! I am actually not sure whether or not after a GRANT statement we really need a REFRESH AUTHORIZATION before a subsequent SHOW GRANT statement. I added the REFRESH statements because I found that there are several places (but not every place) in this test file that have the same pattern (GRANT/REVOKE, REFRESH, SHOW GRANT). For instance, in _test_grant_revoke(), we have the following. self.execute_query_expect_success(admin_client, "grant select on database {0} to {1} {2}" .format(unique_database, kw, ident), user=ADMIN) self._refresh_authorization(admin_client, refresh_stmt) result = self.execute_query("show grant {0} {1} on database {2}" .format(kw, ident, unique_database)) In the next patch, I will remove those REFRESH AUTHORIZATION statements originally added in the patch set 1, because it seems that such REFRESH statements are not necessary. http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1208 PS1, Line 1208: # from interfering with other tests. > Are these commented logs intentionally in the code? Thanks Csaba! I will remove those commented logging statements in the next patch. I forgot to remove them in this patch. http://gerrit.cloudera.org:8080/#/c/20916/1/tests/authorization/test_ranger.py@1288 PS1, Line 1288: > In this case only the table metadata is reloaded. Thanks for catching this Csaba! I will correct this in the next patch. The change could be found in the function _test_grant_revoke_by_owner_on_table() in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/20916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibac5335c65a860963ef0ccd890a926af80585ef3 Gerrit-Change-Number: 20916 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Tue, 30 Jan 2024 00:09:19 +0000 Gerrit-HasComments: Yes
