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 11: (10 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? I can add a TODO to add such a test when the mini Ranger is ready? Now it doesn't make sense to mock it. 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@126 PS4, Line 126: } : : /** > I guess I still don't understand enough about how this works end-to-end to We discussed further offline about this topic. Summarized our discussion here: it turns out to be it is unavoidable to introduce a dependency of Hadoop configuration core-site.xml as Ranger client library uses the UGI to determine if Kerberos is enabled (https://github.com/apache/ranger/blob/master/agents-common/src/main/java/org/apache/ranger/admin/client/RangerAdminRESTClient.java#L129) (basically Ranger assumes its integrated component(s) are using UGI). We also considered to make user group mapping resolution pluggable, such as 1) not include group information or 2) directly integrates with UNIX group mapping. However it may not add too much value as a proper core-site.xm is required in the first place for the Ranger client library to function correctly. 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: public Collection<RangerAccessResult> authorize(RangerRequestListPB requests) { > nit: probably don't need this since it's already in the method comment. Done 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 a list of authorization decision from Ranger with the specified list : * of ranger access request. : * : * @param requests a list of RangerAccessRequest : * @return a list of RangerAccessResult : */ : @VisibleForTesting : Collection<RangerAccessResult> authorize(Collection<RangerAccessRequest> requests) { : return plugin.isAccessAllowed(requests); : } : : /** : * Creates a ranger access request for the specified user who performs : * the given action on the resource. : * : * @param action action to be authorized on the resource. Note that when it : * is null, Ranger will match to any valid actions : * @param user user performs the action : * @param groups the set of groups the user belongs to : > Will this be used outside of tests? No, it is only for 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: } : : /** > What happens if these are empty? Is it the same as if they were null? It is not the same thing, null can be used to determine if table/column level checking is required. 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: : return rangerRequests; : } > Will our protobuf library return 'null' if these aren't set? Or some defaul Good catch! Refer to the official guide(https://developers.google.com/protocol-buffers/docs/proto#optional): "For strings, the default value is the empty string. For enums, the default value is the first value listed in the enum's type definition". 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: builder.setAction(action); : builder.setDatabase(db); : builder.setTable(table); : builder.setColumn(col); : return builder.build(); : } : : private static RangerRequestListPB createRangerRequestList( : List<RangerRequestPB> requests, String user) { : RangerRequestListPB.Builder builder = RangerRequestListPB.newBuilder(); : builder.addAllRequests(requests); : builder.setUser(user); : return builder.build(); : } : : private static SubprocessRequestPB createRangerSubprocessRequest( : RangerRequestListPB request) { : SubprocessRequestPB.Builder builder = SubprocessRequestPB.newBuilder(); : builder.setRequest(Any.pack(request)); : return builder.build(); : } : : @Before : public void mockAuthorizer() { : RangerProtocolHandler.authz = Mockito.mock(RangerKuduAuthorizer.class); : } : : /** : * Sends a list of Ranger request and verifies the response by mocking the authorization : * decisions. : */ > nit: why not just use an ActionPB? Done 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: RangerAccessRequestImpl mockUpdateRequest = Mockito.mock(RangerAccessRequestImpl.class); : final RangerAccessResult updateResult = new RangerAccessResult( : /* policyType= */1,"kudu", : new RangerServiceDef(), mockUpdateRequest); : updateResult.setIsAllowed(true); : : // Mock with a negative authz result. : RangerAccessRequestImpl mockCreateRequest = Mockito.mock(RangerAccessRequestImpl.class); : final RangerAccessResult createResult = new RangerAccessResult( : /* policyType= */1,"kudu", > Add a test for the new multi-request authorization method? Done 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 is managed by the Kudu mast > nit: that is managed Done http://gerrit.cloudera.org:8080/#/c/15074/9/src/kudu/ranger/ranger.proto@60 PS9, Line 60: for a si > nit: "for a single user", same elsewhere Done -- 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: 11 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: Mon, 02 Mar 2020 06:51:19 +0000 Gerrit-HasComments: Yes
