Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13309 )
Change subject: IMPALA-8400: Implement Ranger audit event handler ...................................................................... Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/13309/7/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/13309/7/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@411 PS7, Line 411: * This method is used by {@link #analyzeAndAuthorize(StatementBase, StmtTableCache, > Is this _only_ for testing? Or visible for testing? I think it'd be a littl The comment is wrong. It is visible for testing and not only for testing. Will update the comment The analyzeAndAuthorize is called by https://github.com/apache/impala/blob/f9bf62eefab7fb807f4e5d6900064b612b455a5e/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java#L302. So it's easier to just extend method to be more testable without using any mocking framework and it feels a bit more functional :) http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@42 PS7, Line 42: default AuthorizationContext preAuthorize() > It doesn't look like any of the implementations of preAuthorize() actually My original idea was for the case where a different implementation needed access to the AnalysisResult and FeCatalog, but I guess we can just pass what we need right now for this implementation. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@57 PS7, Line 57: is method is > Doesn't look like 'result' or 'catalog' are used here in any of the impls e Removed. Done. http://gerrit.cloudera.org:8080/#/c/13309/7/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/13309/7/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@98 PS7, Line 98: void au > this variable is misnamed -- should be something like 'durationMs' Done http://gerrit.cloudera.org:8080/#/c/13309/7/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/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@82 PS7, Line 82: that goes through {@link au > not sure what this means. Isn't this used for _all_ SQL statements? This ma There are certain things where we don't want to audit the log, basically for filtering out unauthorized columns, for example if we have a table: create table t(i int, j int); grant select(i) on table t to user foobar; describe t; --> the describe will have an audit log checking for whether i and j needs to be filtered out from the column output will not be logged. https://github.com/apache/impala/blob/7af981f7afdd25c6e0ed7e86c21bbc7710098a9e/fe/src/main/java/org/apache/impala/service/Frontend.java#L971-L984 Similarly for "show databases", "show tables", etc. I will update the comment. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@250 PS7, Line 250: s = tmp > typo Done http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java: http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@36 PS7, Line 36: > Worth adding to the Javadoc that this is scoped once-per-statement, rather Done http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@37 PS7, Line 37: > Would it be possible to make this directly implement RangerAccessResultProc Sure, composition is better than inheritance. Done. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@38 PS7, Line 38: > Related to the above comment, I see that this is used in some cases where i I implemented something similar to your idea. LMK what you think. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@56 PS7, Line 56: > typo Done http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@93 PS7, Line 93: : > It seems odd that this logic is implemented in two places -- here and also We can actually just do it close(). This is just a tiny performance improvement to bail out early. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java: http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java@351 PS7, Line 351: /** > Should this code be removed? Forgot to remove. Done. http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@44 PS7, Line 44: "create database test_db", onServer(TPrivilegeLevel.CREATE)); : : authzOk(events -> { : assertEquals(1, events.size()); > this pattern shows up a bunch of places below. What about adding a utility Good idea. Done. -- To view, visit http://gerrit.cloudera.org:8080/13309 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife93c3e708875ef5fc0117153ad8ee225a88518b Gerrit-Change-Number: 13309 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 21 May 2019 18:50:03 +0000 Gerrit-HasComments: Yes
