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 4: (3 comments) > Patch Set 2: > > (1 comment) > > Thanks for the efforts Xuebin! > > I left a comment on KillQueryStmt.java regarding my understanding of the > usage of setMaskPrivChecks(). > > I will keep reading the rest of the patch and provide suggestions if there is > any. Thanks Xuebin! I took another closer look at the Java code related to authorization and left some minor comments. Let me know what you think about them. http://gerrit.cloudera.org:8080/#/c/21930/2/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java File fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java: http://gerrit.cloudera.org:8080/#/c/21930/2/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java@60 PS2, Line 60: setMaskPrivChecks(null) > Thanks! The masked privilege check is used here because Thanks for the explanation! I think it makes sense to mask the privilege request for the KILL QUERY statement. I also left a minor comment on https://gerrit.cloudera.org/c/21930/4/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java. Let me know if that make sense to you. http://gerrit.cloudera.org:8080/#/c/21930/4/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java File fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java: http://gerrit.cloudera.org:8080/#/c/21930/4/fe/src/main/java/org/apache/impala/analysis/KillQueryStmt.java@61 PS4, Line 61: analyzer.registerPrivReq(builder -> builder.onServer(authzServer).all().build()); I left some thoughts at https://issues.apache.org/jira/browse/IMPALA-12648?focusedCommentId=17891679&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17891679 on what privilege we'd like to check for the KILL QUERY statement. In RANGER-1851, a new type of resource and action were introduced for the KILL QUERY statement in Apache Hive (HIVE-17483). For now I think it's okay we register the ALL privilege on SERVER for the KILL QUERY statement. Adding a new type of resource and action could be done in a follow-up JIRA so that this patch won't be too large. On a related note, IMPALA-10436 was a patch that added a new type of resource in Impala. It can be seen that such a change could be tedious since we may also want to support the respective GRANT/REVOKE/SHOW GRANT statements after adding a new type of resource. http://gerrit.cloudera.org:8080/#/c/21930/4/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/4/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@188 PS4, Line 188: analysisResult.setUserHasProfileAccess(false) It looks like before this patch, setUserHasProfileAccess(false) was mostly/only used to prevent a requesting user from accessing the information about a view's underlying tables on which the requesting user did not have the necessary privilege. After this patch, it's also possible to mask a privilege request registered during the analysis of the KILL QUERY statement. Would it make sense to not call setUserHasProfileAccess(false) unconditionally when there is an AuthorizationException thrown after the check of a masked privilege request? For instance, we call setUserHasProfileAccess(false) only when the current statement is not a KILL QUERY statement? This way we may not have to call setUserHasProfileAccess(true) in KillQueryStmt#handleAuthorizationException(). What do you think? -- 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: 4 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: Mon, 21 Oct 2024 23:58:22 +0000 Gerrit-HasComments: Yes
