----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64661/#review207538 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Lines 255 (patched) <https://reviews.apache.org/r/64661/#comment290934> 1) I believe that when kafka calls the groups is a list with only one member, which is "null". 1.1) the group name is null in SentryGenericServiceClientDefaultImpl @Override public Set<TSentryRole> listAllRoles(String requestorUserName, String component) throws SentryUserException { return listRolesByGroupName(requestorUserName, null, component); } 1.2) the group name of "null" is in request to generic service public Set<TSentryRole> listRolesByGroupName( String requestorUserName, String groupName, String component) throws SentryUserException { TListSentryRolesRequest request = new TListSentryRolesRequest(); request.setProtocol_version(sentry_common_serviceConstants.TSENTRY_SERVICE_V2); request.setRequestorUserName(requestorUserName); request.setGroupName(groupName); request.setComponent(component); TListSentryRolesResponse response; try { response = client.list_sentry_roles_by_group(request); Status.throwIfNotOk(response.getStatus()); return response.getRoles(); } catch (TException e) { throw new SentryUserException(THRIFT_EXCEPTION_MESSAGE, e); } } 1.3) groups contains "null" when calling DelegateSentryStore.getRolesByGroups(). public TListSentryRolesResponse list_sentry_roles_by_group( final TListSentryRolesRequest request) throws TException { Response<Set<TSentryRole>> respose = requestHandle(new RequestHandler<Set<TSentryRole>>() { @Override public Response<Set<TSentryRole>> handle() throws Exception { validateClientVersion(request.getProtocol_version()); Set<String> groups = getRequestorGroups(conf, request.getRequestorUserName()); if (!AccessConstants.ALL.equalsIgnoreCase(request.getGroupName())) { boolean admin = inAdminGroups(groups); //Only admin users can list all roles in the system ( groupname = null) //Non admin users are only allowed to list only groups which they belong to if(!admin && (request.getGroupName() == null || !groups.contains(request.getGroupName()))) { throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE + request.getRequestorUserName()); } groups.clear(); groups.add(request.getGroupName()); } Set<String> roleNames = store.getRolesByGroups(request.getComponent(), groups); 2) This implementation using SentryStore.getRolesForGroups() does not have groups contains a single element of "null" gracefully. That is why I am really worried about the test you have done for this update. public Set<MSentryRole> getRolesForGroups(PersistenceManager pm, Set<String> groups) { Set<MSentryRole> result = Sets.newHashSet(); if (groups != null) { Query query = pm.newQuery(MSentryGroup.class); query.addExtension(LOAD_RESULTS_AT_COMMIT, "false"); query.setFilter(":p1.contains(this.groupName)"); List<MSentryGroup> sentryGroups = (List) query.execute(groups.toArray()); if (sentryGroups != null) { for (MSentryGroup sentryGroup : sentryGroups) { result.addAll(sentryGroup.getRoles()); } } } return result; } 3) You should check if there is a member of null in groups and handle it properly, and have testing case for that scenario. - Na Li On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64661/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2018, 7:01 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and > Sergio Pena. > > > Bugs: SENTRY-1944 > https://issues.apache.org/jira/browse/SENTRY-1944 > > > Repository: sentry > > > Description > ------- > > When Solr is using Sentry server for authorization, it issues a lot of calls > to getGroupsByRoles() function in DelegateSentryStore. > > This function isn't very efficient - it walks over each role in the set, > obtains role by name, get groups for each role, and collects all group names > into a set. > > It may be possible to optimize it. > > Also, in SentryGenericPolicyProcessor class method > list_sentry_roles_by_group() would make N transactions to build the roles to > set of groups map. Instead, make it to a single transaction. This will > significantly speed up operation > > Attach one or more files to this issue > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java > 1cc4b1b37 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 3026a6225 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > eec2757d3 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java > 4c207e9b4 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java > 69d16238f > > > Diff: https://reviews.apache.org/r/64661/diff/10/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test > > > Thanks, > > Arjun Mishra > >
