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 11: (6 comments) Thanks for making that refactor. Liking the new test setup much better. A few small remaining suggestions, but I think this is pretty close. http://gerrit.cloudera.org:8080/#/c/13309/11/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/11/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@44 PS11, Line 44: return createAuthorizationContext(); Could we get rid of preAuthorize entirely if we add an argument to 'createAuthorizationContext' like: AuthorizationContext createAuthorizationContext(boolean doAudits); where the default implementation ignores 'doAudits', but the Ranger implementation uses that to determine whether to set up the audit event buffer? That way we have a clear 'flow' that is always 'createAuthorizationContext(bool) -> authorize -> postAuthorize'. In the current state of the patch there are some paths where we use that flow and others where we use preAuthorize() -> authorize -> postAuthorize, with a slightly non-obvious side effect that one flow causes audits and the other doesn't. http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java: http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationContext.java@29 PS11, Line 29: to means nit: "to mean" or "meaning" http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java: http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/main/java/org/apache/impala/authorization/ranger/RangerBufferAuditHandler.java@112 PS11, Line 112: // If there's at least one access denied result, we want to only log the first per comment elsewhere, I don't think this optimization is worth the code complexity. Let's just do the filtering / "first failure" handling in flush(), or in getAuthzEvents() or somesuch if it's necessary to get the filtered results for tests. Otherwise we have more or less the same logic in two places which gets a bit confusing as to whether they compose in the expected way. http://gerrit.cloudera.org:8080/#/c/13309/11/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/11/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@89 PS11, Line 89: assertEquals(1, events.get(1).getAccessResult()); this is encompassed by the previous line, no? http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@156 PS11, Line 156: consumer mind renaming this to 'resultChecker' so it's clear what should be done with the consumed list? http://gerrit.cloudera.org:8080/#/c/13309/11/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@164 PS11, Line 164: consumer same -- 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: 11 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: Thu, 23 May 2019 05:09:27 +0000 Gerrit-HasComments: Yes
