Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13309 )
Change subject: IMPALA-8400: Implement Ranger audit event handler ...................................................................... Patch Set 7: (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 meant for testing. Is this _only_ for testing? Or visible for testing? I think it'd be a little clearer to say that this is the implementation of the above function, but providing the additional 'authzCtxConsumer'. That said, it seems like the 'postAuthorize' hook already gets access to the context, so couldn't you just spy on that call, eg by passing in a Mockito.spy()-wrapped AuthorizationChecker in the tests? 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: AuthorizationContext preAuthorize(AnalysisResult result, FeCatalog catalog) It doesn't look like any of the implementations of preAuthorize() actually rely on the parameters here. In fact, they basically just delegate to 'createAuthorizationContext()', with the only difference in behavior being whether or not to enable auditing. What if you just moved the 'createAuthorizationContext()' function up into this interface, and added a boolean parameter like 'enableAudits'? http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@57 PS7, Line 57: postAuthorize Doesn't look like 'result' or 'catalog' are used here in any of the impls either 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: endTime this variable is misnamed -- should be something like 'durationMs' 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: authorization SQL statement not sure what this means. Isn't this used for _all_ SQL statements? This makes it sound like it's used only for GRANT/REVOKE http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@250 PS7, Line 250: curcuit typo 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 than being a long-lived multi-threaded thing http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@37 PS7, Line 37: public class RangerImpalaAuditHandler extends RangerDefaultAuditHandler Would it be possible to make this directly implement RangerAccessResultProcessor and _wrap_ another instance (typically RangerDefaultAuditHandler) instead of extending it? That would make the idea of testing via spy/mock a bit easier elsewhere -- you just need to pass in a mock RangerAccessResultProcessor instead of DefaultAuditHandler http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@38 PS7, Line 38: implements AutoCloseable { Related to the above comment, I see that this is used in some cases where it actually is just a temporary holding area for a list of events, and not meant to be flushed. If you make it AutoCloseable, I'm guessing that findbugs and other static checkers are going to complain about usages that don't close it. How about the following idea: - make this implement RangerAccessResultProcessor - make the ctor take no "wrapped class" - add an explicit "flushTo(RangerAccessResultProcessor consumer)" method - add a little wrapper like: AutoCloseable autoFlushTo(RangerAccessResultProcessor consumer) { return new AutoCloseable() { @Override void close() { flushTo(consumer); } } ); so then you can still use it with the "auto flush" like you have, but it's less awkward to use it in the "non-flushing" cases. FWIW I'd also then suggest renaming this class to something like RangerAuditEventBuffer since ti's just a buffer with some extra logic to consoildate the events before passing them along http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@56 PS7, Line 56: first a failure typo http://gerrit.cloudera.org:8080/#/c/13309/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaAuditHandler.java@93 PS7, Line 93: // If there's at least one access denied result, we want to only log the first : // failure. It seems odd that this logic is implemented in two places -- here and also in 'close()'. Is there a reason to do this vs just doing it in close? 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: // private List<WithPrincipal> buildWithPrincipals() { Should this code be removed? 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: assertEquals("@database", events.get(0).getResourceType()); : assertEquals("create", events.get(0).getAccessType()); : assertEquals("test_db", events.get(0).getResourcePath()); : assertEquals(1, events.get(0).getAccessResult()); this pattern shows up a bunch of places below. What about adding a utility method: static void assertEventEquals(String resourceType, String accessType, String resourcePath, int accessResult, AuthzAuditEvent event) { ... } to encapsulate it? Then we'd just have the nice easy to read: assertEventEquals("@database", "create", "test_db", 1, events.get(0)); -- 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: 7 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 21 May 2019 05:24:35 +0000 Gerrit-HasComments: Yes
