Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/13744 )
Change subject: IMPALA-8716: Log a group of privileges into a single audit event. ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/13744/2/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/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@318 PS2, Line 318: result nit: authorized ? (same below) http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@319 PS2, Line 319: RangerBufferAuditHandler originalAuditHandler = authzCtx.getAuditHandler(); : RangerBufferAuditHandler tmpAuditHandler = : originalAuditHandler == null ? null : new RangerBufferAuditHandler( : originalAuditHandler.getSqlStmt(), originalAuditHandler.getClusterName(), : originalAuditHandler.getClientIp()); I think this could use a comment. Also, create a copy c'tor? (same below) http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@385 PS2, Line 385: dest.getAuthzEvents().add(newAuditEvent); just checking, is the accessResult automatically inferred from newAuditEvent? http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@408 PS2, Line 408: RangerAccessResult result = plugin_.isAccessAllowed(request, auditHandler); > Tthe plugin._isAccessAllowed() does some logic that passes the correct Rang I see, ok! http://gerrit.cloudera.org:8080/#/c/13744/2/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/13744/2/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@141 PS2, Line 141: 1 Mind adding a couple more tests? - accessResult = 0 isAny = true - accessResult = 1 isAny = false -- To view, visit http://gerrit.cloudera.org:8080/13744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib53102bc1ceaf9d62544090dc00f3231fae0efca Gerrit-Change-Number: 13744 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 01 Jul 2019 18:24:04 +0000 Gerrit-HasComments: Yes
