Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15074 )
Change subject: [java] KUDU-2972: add Kudu Ranger plugin ...................................................................... Patch Set 5: (24 comments) 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 rea Sorry for the confusion, if we are used shaded version, we can get compile error as the following: SubprocessResponsePB response = MessageTestUtil.deserializeMessage( ^ required: byte[],com.google.protobuf.Parser<T> found: byte[],org.apache.kudu.shaded.com.google.protobuf.Parser<SubprocessResponsePB> reason: cannot infer type-variable(s) T (argument mismatch; org.apache.kudu.shaded.com.google.protobuf.Parser<SubprocessResponsePB> cannot be converted to com.google.protobuf.Parser<T>) where T is a type-variable: T extends Message declared in method <T>deserializeMessage(byte[],com.google.protobuf.Parser<T>) as com.google.protobuf.Parser has been shaded to org.apache.kudu.shaded.com.google.protobuf.Parser. Not sure how I can make the comment more understandable? 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-subproce Right, sorry I missed that. 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@33 PS4, Line 33: request to Ranger and get authorization decision > nit: "sends requests to Ranger and gets authorization decisions" Done 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? This cannot be final as it is be mocked in the test. Set it to static to be class wide variable to only initiate once. 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/ Yeah, sorry I forgot about it. Updated. http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/RangerProtocolHandler.java@56 PS4, Line 56: getUser > Do we expect this to change from request to request in a single ListPB? No, so updated it to add user in ListPB instead. 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@37 PS4, Line 37: // The following properties need to match the Kudu service def in Ranger. > nit: maybe link to https://github.com/apache/ranger/blob/master/agents-comm Done 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? Cannot make it final as it is used in the mock test. 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); > +1 To configure the plugin, at least ranger-kudu-security.xml needs to be provided. I did not include a default one it in this project as the info as URL of Ranger admin server is not known until one is started. I am thinking to have the C++ side subprocess construct the config file as it will have the info for the Ranger admin server. 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? Make it a separate init() as we are mocking RangerKuduAuthorizer in the test and init() has to be invoked explicitly. http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@61 PS4, Line 61: * @param action action to be authorized on the resource : * @param user user performs the action : * @param db the database name the action is to be performed on : * @param table the table name the action is to be performed on : * @param col the column name the action is to be performed on > nit: could you also document if/how these can be empty or null? For instanc Done 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? Done 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? Done http://gerrit.cloudera.org:8080/#/c/15074/4/java/kudu-ranger/src/main/java/org/apache/kudu/ranger/authorization/RangerKuduAuthorizer.java@103 PS4, Line 103: createRequest > Could this and getUserGroups be static? Done 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 It is using hadoop user group mapping information (https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-common/GroupsMapping.html) similar to what Impala is doing. Adding more detail in the comment. 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? Actually I don't think we need another log4j2.properties as kudu-subprocess has one. Removed it. 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 th Done 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 betwe Done 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. Done http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@48 PS4, Line 48: performs > Nit: performing. Done 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 RangerResponseL I cannot think of any useful error information we maybe interested in, as I don't see the reasons of why getting a denied response would be needed. Do you think there is any kinds of error message you think would be benefit here? 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 Done http://gerrit.cloudera.org:8080/#/c/15074/4/src/kudu/ranger/ranger.proto@62 PS4, Line 62: : // Describes a list of authorization descison responded from the Ranger : // service for a request. > nit: note that the order is determined by the order of RangerRequestListPB? Done 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: required string data = 1; > This duplicates ranger.proto? Ah, right, removed. -- 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: 5 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: Tue, 18 Feb 2020 17:06:28 +0000 Gerrit-HasComments: Yes
