Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15074 )
Change subject: [java] KUDU-2972: add Kudu Ranger plugin ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java File java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java: http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java@53 PS9, Line 53: // The result can be null when Ranger plugin fails to load the policies : // from the Ranger admin server. Add a test for this? http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@82 PS9, Line 82: // According the implementation of RangerBasePlugin.isAccessAllowed() in Ranger, nit: probably don't need this since it's already in the method comment. http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@87 PS9, Line 87: /** : * Gets the authorization decision from Ranger with the specified ranger : * access request. : * : * @param request the ranger access request : * @return the authorization decision of Ranger : */ : @VisibleForTesting : boolean authorize(RangerAccessRequestImpl request) { : // Retrieve the authorization result from Ranger. The result can be : // null when Ranger plugin fails to load the policies from the Ranger : // admin server. : final RangerAccessResult result = plugin.isAccessAllowed(request); : boolean isAllowed = (result != null && result.getIsAllowed()); : if (!isAllowed && LOG.isDebugEnabled()) { : LOG.debug(String.format("Access to RangerAccessRequestImpl [%s] is denied " + : "with the result being [%s]", request.toString(), result.toString())); : } : return isAllowed; : } Will this be used outside of tests? http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@126 PS9, Line 126: resource.setValue(RANGER_DB_RESOURCE_NAME, db); : resource.setValue(RANGER_TABLE_RESOURCE_NAME, table); : resource.setValue(RANGER_COLUMN_RESOURCE_NAME, col); What happens if these are empty? Is it the same as if they were null? http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@150 PS9, Line 150: request.getAction().name(), user, groups, : request.getDatabase(), request.getTable(), : request.getColumn())); Will our protobuf library return 'null' if these aren't set? Or some default value? http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java File java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java: http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@64 PS9, Line 64: switch (action.toUpperCase()) { : case "SELECT": : builder.setAction(ActionPB.SELECT); : break; : case "INSERT": : builder.setAction(ActionPB.INSERT); : break; : case "UPDATE": : builder.setAction(ActionPB.UPDATE); : break; : case "DELETE": : builder.setAction(ActionPB.DELETE); : break; : case "ALTER": : builder.setAction(ActionPB.ALTER); : break; : case "CREATE": : builder.setAction(ActionPB.CREATE); : break; : case "DROP": : builder.setAction(ActionPB.DROP); : break; : case "ALL": : builder.setAction(ActionPB.ALL); : break; : case "METADATA": : builder.setAction(ActionPB.METADATA); : break; : default: : throw new IllegalArgumentException("invalid action type"); : } nit: why not just use an ActionPB? http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/authorization/TestRangerKuduAuthorizer.java File java/kudu-ranger/src/test/java/org/apache/kudu/ranger/authorization/TestRangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/15074/9/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/authorization/TestRangerKuduAuthorizer.java@56 PS9, Line 56: result.setIsAllowed(true); : Mockito.when(authz.plugin.isAccessAllowed(mockRequest)) : .thenReturn(result); : assertTrue(authz.authorize(mockRequest)); : : // Mock with negative authz result. : result.setIsAllowed(false); : Mockito.when(authz.plugin.isAccessAllowed(mockRequest)) : .thenReturn(result); : assertFalse(authz.authorize(mockRequest)); Add a test for the new multi-request authorization method? http://gerrit.cloudera.org:8080/#/c/15074/9/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/15074/9/src/kudu/ranger/ranger.proto@36 PS9, Line 36: that managed by the Kudu master. nit: that is managed http://gerrit.cloudera.org:8080/#/c/15074/9/src/kudu/ranger/ranger.proto@60 PS9, Line 60: per user nit: "for a single user", same elsewhere -- To view, visit http://gerrit.cloudera.org:8080/15074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c995ac1a48ebf57667231cd3a82d3794f6ddf8d Gerrit-Change-Number: 15074 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 20 Feb 2020 00:56:08 +0000 Gerrit-HasComments: Yes
