Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/24785 )
Change subject: IMPALA-15323: Produce Ranger audit events for CREATE/DROP ROLE ...................................................................... Patch Set 3: (4 comments) I have addressed most of Quanlong's comments. Please let me know if there are additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/24785/2/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/24785/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@89 PS2, Line 89: public static final String DROP_ROLE_ACTION = "DROPROLE"; > Do we need underscores here, i.e. "CREATE_ROLE", "DROP_ROLE"? Are these str We probably don't need underscores here. At https://github.com/apache/ranger/blob/2ad565f/hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java#L344 in RangerHiveAuthorizer#createRole(), we instantiate a RangerAccessResult that will be used to construct the corresponding AuthzAuditEvent (https://github.com/apache/ranger/blob/2ad565f/agents-audit/core/src/main/java/org/apache/ranger/audit/model/AuthzAuditEvent.java). RangerAccessResult accessResult = createAuditEvent(hivePlugin, currentUserName, userNames, HiveOperationType.CREATEROLE, HiveAccessType.CREATE, roleNames, result); HiveOperationType.CREATEROLE is defined at https://github.com/apache/hive/blob/9019223/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveOperationType.java#L112 and it can be seen that there is no underscore. Since this question is raised, I'd like to point out that according to the current implementation, eventually the field of 'accessType' will be "CREATE" and the field of 'action' will be "create". According to my current understanding, this is because - at https://github.com/apache/impala/blob/fe5217c/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java#L132, when constructing the AuthzAuditEvent, we call RangerDefaultAuditHandler#getAuthzEvents() (https://github.com/apache/ranger/blob/2ad565f/agents-common/src/main/java/org/apache/ranger/plugin/audit/RangerDefaultAuditHandler.java#L102) that switched the values of these 2 fields (https://github.com/apache/ranger/blob/2ad565f/agents-common/src/main/java/org/apache/ranger/plugin/audit/RangerDefaultAuditHandler.java#L124 and https://github.com/apache/ranger/blob/2ad565f/agents-common/src/main/java/org/apache/ranger/plugin/audit/RangerDefaultAuditHandler.java#L127) (https://issues.apache.org/jira/browse/RANGER-5594 was created for this), and then - at https://github.com/apache/impala/blob/fe5217c/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java#L140, we uppercase the field of 'accessType' in the AuthzAuditEvent using the value of 'accessType' in the given RangerAccessResult of the method createAuditEvent(RangerAccessResult result). We observed a similar thing (or issue?) as described above in Apache Hive, but it looks like this was worked around by calling auditEvent.setAccessType(action) at https://github.com/apache/ranger/blob/2ad565f/hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuditHandler.java#L210 in RangerHiveAuditHandler#createAuditEvent(). I did not adopt the workaround as done for Apache Hive because I felt that it would put a cognitive burden on Impala developers, making the code ugly and difficult to understand. Recall that this JIRA is already a workaround due to the issue reported in https://issues.apache.org/jira/browse/RANGER-5777. Ideally, RangerBasePlugin#createRole() should have already taken care of all of this for us. http://gerrit.cloudera.org:8080/#/c/24785/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@106 PS2, Line 106: User requestingUser = new User(header.getRequesting_user()); > 'requestingUser' won't be null here. We should check header.isSetRequesting Thanks for pointing this out! I will change this and the Preconditions check at https://gerrit.cloudera.org/c/24785/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java#131 as well in the next patch. http://gerrit.cloudera.org:8080/#/c/24785/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@131 PS2, Line 131: User requestingUser = new User(header.getRe Check header.isSetRequesting_user() above as Quanlong suggested. http://gerrit.cloudera.org:8080/#/c/24785/2/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/24785/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@469 PS2, Line 469: } : Optional<TTableName> tTableName = Optional.empty(); : TDd > This can be removed now. Thanks for pointing this out! I will remove this in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/24785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3fcfe0dcf54041aca57ed66ddef95b0ddd15b6fb Gerrit-Change-Number: 24785 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 08 Sep 2026 22:00:32 +0000 Gerrit-HasComments: Yes
