Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/21930 )
Change subject: IMPALA-12648: Add KILL QUERY statement ...................................................................... Patch Set 7: (2 comments) I left some thoughts on how to generate a Ranger audit event that accurately reflects whether the requesting user is authorized to execute the KILL QUERY statement. I think for this patch it's okay if we produce an accurate Ranger audit event only when the requesting user has the ALL privilege on SERVER. We could resolve the general case, i.e., requesting user is a non-administrative user, in a follow-up JIRA. Thanks! http://gerrit.cloudera.org:8080/#/c/21930/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/21930/7/be/src/service/client-request-state.cc@2490 PS7, Line 2490: coodinator nit: coordinator http://gerrit.cloudera.org:8080/#/c/21930/6/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/21930/6/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@185 PS6, Line 185: setRetainAudits(false) > I missed this in my previous review. I have the following 2 new comments. Regarding how to generate a Ranger audit event that accurately reflects whether the requesting user is authorized to execute the KILL QUERY statement, I think we could add "AuthorizationException" to the returned Status when the requesting user is not authorized to execute the KILL QUERY statement. It looks like we could call the following to determine whether there is an AuthorizationException in Status. Frontend::IsAuthorizationError(status) Once CancelInternal() returns, the caller could make a JNI call to a Java method that is used to call the method flush() on a RangerBufferAuditHandler as seen in postAuthorize() of https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java. It seems we already have a pattern that accomplishes a similar task in the context of generating lineage information. Specifically, in https://github.com/apache/impala/blob/master/be/src/service/client-request-state.cc, we make a JNI call to Frontend#callQueryCompleteHooks() (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/Frontend.java) via the following in ClientRequestState::LogLineageRecord(). const Status& status = ExecEnv::GetInstance()->frontend()->CallQueryCompleteHooks( query_complete_context); What we have to figure out in our case is how to instantiate a RangerBufferAuditHandler and the corresponding AuthzAuditEvent so that we could call flush() on the instantiated RangerBufferAuditHandler to send the Ranger audit event to the underlying service(s). I think for this patch it's okay if we produce an accurate Ranger audit event only when the requesting user has the ALL privilege on SERVER. We could resolve the above in a follow-up JIRA. -- To view, visit http://gerrit.cloudera.org:8080/21930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If12d6e47b256b034ec444f17c7890aa3b40481c0 Gerrit-Change-Number: 21930 Gerrit-PatchSet: 7 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Sat, 09 Nov 2024 01:03:27 +0000 Gerrit-HasComments: Yes
