Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/15412 )
Change subject: IMPALA-9350: Produce Ranger audits for column masking ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@419 PS3, Line 419: TClientRequest clientRequest; : AuthorizationContext authzCtx = null; : : try { : clientRequest = queryCtx_.getClient_request(); : authzCtx = authzChecker.createAuthorizationContext(true, : clientRequest.isSetRedacted_stmt() ? : clientRequest.getRedacted_stmt() : clientRequest.getStmt(), : queryCtx_.getSession(), Optional.of(timeline_)); : analyze(stmtTableCache, authzCtx); Generating column masking audit events in analyze phase has the drawback that we loss the opportunity for controlling the final results. Since the analyze phase is done before authorize(), will we generate masking events if the user doesn't have privilege to access a column which has masking policy? Also will we generate duplicate masking events if the table occurs several times in the query? However, it looks unevitable since we do apply masking policies when expanding the masked table into a table-masking view. If so, we can add some TODOs and fix them in IMPALA-9223. http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@342 PS3, Line 342: public AuthorizationContext authzCtx = null; Could you make this "final" as well? http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@714 PS3, Line 714: AuthorizationContext authzCtx I think we don't need this new argument since we can get it by Analyzer#getAuthzCtx() http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@290 PS3, Line 290: InternalException { nit: move to the previous line http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@313 PS3, Line 313: List<AuthzAuditEvent> auditEvents = auditHandler.getAuthzEvents(); Could you add a Precondition check that "auditEvents" is not empty? http://gerrit.cloudera.org:8080/#/c/15412/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@314 PS3, Line 314: auditEvents.get(auditEvents.size() - 1).setAccessType(maskType); Could you add some comments for this? Why do we only deal with the last event? Should we add lower case maskType just like line 467? -- To view, visit http://gerrit.cloudera.org:8080/15412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d8a1181234dcef580f68f56c24ad7e962cfe58e Gerrit-Change-Number: 15412 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: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 31 Mar 2020 07:00:51 +0000 Gerrit-HasComments: Yes
