Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15074 )
Change subject: [java] KUDU-2972: add Kudu Ranger plugin ...................................................................... Patch Set 4: (17 comments) How do you feel about predicating this change on MiniRanger support? The mock-based testing is good, but as it stands we lack any coverage that the Ranger plugin internals themselves are working given our input. http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/build.gradle File java/kudu-ranger/build.gradle: http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/build.gradle@21 PS4, Line 21: // Note: We don't use the shaded version to avoid protobuf message type : // got shaded and causes incompatible bounds error for type casting. I see this is a copy of a comment from kudu-subprocess-echo but I'm not really understanding it; could you try explaining it again, and then I can help you come up with the exact wording to convey what you're trying to say? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/build.gradle@39 PS4, Line 39: Do we need the javadoc/publishing changes that Grant added to kudu-subprocess*? http://gerrit.cloudera.org:8080/#/c/15074/4/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/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java@42 PS4, Line 42: static RangerKuduAuthorizer authz = new RangerKuduAuthorizer(); Why static? Also should be final, right? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java@51 PS4, Line 51: for (RangerRequestPB request : requests.getRequestsList()) { Looks like Ranger supports authorizing in batch: https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java#L327 So we should probably refactor our code to do the same; I imagine it's more performant? http://gerrit.cloudera.org:8080/#/c/15074/4/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/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@45 PS4, Line 45: RangerBasePlugin plugin; final? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@48 PS4, Line 48: plugin = new RangerBasePlugin(SERVICE_TYPE, APP_ID); How do we configure the plugin? For example, how does it know where Ranger is located and how to connect to it? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@52 PS4, Line 52: public void init() { : LOG.info("Initializing Ranger Kudu plugin"); : plugin.init(); : } Curious why do this in a separate init() and not the constructor? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@84 PS4, Line 84: result != null Under what circumstances is result null? Can you document? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@126 PS4, Line 126: UserGroupInformation ugi; : ugi = UserGroupInformation.createRemoteUser(user); : return new HashSet<>(ugi.getGroups()); What does this do under the hood? How does it retrieve map a user to groups, and what systems does it leverage to do that? http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/resources/log4j2.properties File java/kudu-ranger/src/main/resources/log4j2.properties: PS4: Why doesn't kudu-subprocess-echo have a log4j2.properties? http://gerrit.cloudera.org:8080/#/c/15074/4/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/4/java/kudu-ranger/src/test/java/org/apache/kudu/ranger/TestRangerSubprocess.java@28 PS4, Line 28: import org.junit.Assert; You can statically import this (with a wildcard too) to avoid a bunch of the Assert. boilerplate. http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: PS4: Should clarify in a comment somewhere here that this is the interface between Kudu and the Ranger plugin operating in a Java subprocess managed by the Kudu server. http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@34 PS4, Line 34: ranger Nit: capital-r Ranger here and below. http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@48 PS4, Line 48: performs Nit: performing. http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@54 PS4, Line 54: // Whether or not a request is allowed. Do we want to add a field for error info, either here or in RangerResponseListPB? http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@60 PS4, Line 60: repeated RangerRequestPB requests = 1; If you expect all batching to be done on behalf of a single user, you could pull the 'user' field out of the request and into the request list. http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/subprocess/subprocess.proto File src/kudu/subprocess/subprocess.proto: http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/subprocess/subprocess.proto@36 PS4, Line 36: // Describes an authorization request sent to the Ranger service. This duplicates ranger.proto? -- 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: 4 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: Wed, 12 Feb 2020 21:15:41 +0000 Gerrit-HasComments: Yes
