Repository: sentry Updated Branches: refs/heads/master e0d99fef5 -> f099f0abd
SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles() and update SentryGenericPolicyProcessor to retrieve roles to group mapping in a single transaction (Arjun Mishra reveiwed by Na Li and Sergio Pena) Change-Id: If044f0c9edcca077a4633db8274724dbd0bcb2e8 Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/f099f0ab Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/f099f0ab Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/f099f0ab Branch: refs/heads/master Commit: f099f0abdbeae094fa78d612812cc49b443b48df Parents: e0d99fe Author: amishra <amis...@cloudera.com> Authored: Sat Aug 18 08:20:20 2018 -0400 Committer: amishra <amis...@cloudera.com> Committed: Sat Aug 18 08:20:36 2018 -0400 ---------------------------------------------------------------------- .../thrift/SentryGenericPolicyProcessor.java | 7 +- .../service/persistent/DelegateSentryStore.java | 86 +++++++++++++------ .../service/persistent/SentryStoreLayer.java | 10 +++ .../db/service/persistent/SentryStore.java | 2 +- .../TestSentryGenericPolicyProcessor.java | 7 ++ .../persistent/TestDelegateSentryStore.java | 89 +++++++++++++++++--- 6 files changed, 155 insertions(+), 46 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java index 1cc4b1b..ef6aff3 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java @@ -585,12 +585,7 @@ public class SentryGenericPolicyProcessor implements SentryGenericPolicyService. groups.add(request.getGroupName()); } - Set<String> roleNames = store.getRolesByGroups(request.getComponent(), groups); - Set<TSentryRole> tSentryRoles = Sets.newHashSet(); - for (String roleName : roleNames) { - Set<String> groupsForRoleName = store.getGroupsByRoles(request.getComponent(), Sets.newHashSet(roleName)); - tSentryRoles.add(new TSentryRole(roleName, groupsForRoleName)); - } + Set<TSentryRole> tSentryRoles = store.getTSentryRolesByGroupName(request.getComponent(), groups); return new Response<Set<TSentryRole>>(Status.OK(), tSentryRoles); } }); http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java index 3026a62..850c593 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java @@ -22,6 +22,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; +import javax.jdo.Query; import org.apache.hadoop.conf.Configuration; import org.apache.sentry.core.common.Authorizable; import org.apache.sentry.core.common.exception.SentryAccessDeniedException; @@ -32,10 +33,12 @@ import org.apache.sentry.core.common.exception.SentryUserException; import org.apache.sentry.provider.db.service.model.MSentryGMPrivilege; import org.apache.sentry.provider.db.service.model.MSentryGroup; import org.apache.sentry.provider.db.service.model.MSentryRole; +import org.apache.sentry.provider.db.service.persistent.QueryParamBuilder; import org.apache.sentry.provider.db.service.persistent.SentryStore; import org.apache.sentry.api.service.thrift.SentryPolicyStoreProcessor; import org.apache.sentry.api.service.thrift.TSentryGroup; -import org.apache.sentry.api.service.thrift.TSentryRole; +import org.apache.sentry.api.generic.thrift.TSentryRole; +import org.apache.sentry.provider.db.service.persistent.TransactionBlock; import org.apache.sentry.service.common.ServiceConstants.ServerConfig; import javax.jdo.PersistenceManager; @@ -228,43 +231,74 @@ public class DelegateSentryStore implements SentryStoreLayer { if (groups == null || groups.isEmpty()) { return Collections.emptySet(); } - Set<String> roles = Sets.newHashSet(); - for (TSentryRole tSentryRole : delegate.getTSentryRolesByGroupName(groups, - true)) { - roles.add(tSentryRole.getRoleName()); + if(groups.contains(null)) { + roles = delegate.getAllRoleNames(); + } else { + roles = delegate.getRoleNamesForGroups(groups); } return roles; } @Override - public Set<String> getGroupsByRoles(final String component, final Set<String> roles) + public Set<TSentryRole> getTSentryRolesByGroupName(String component, Set<String> groups) throws Exception { - // In all calls roles contain exactly one group - if (roles.isEmpty()) { + if (groups == null || groups.isEmpty()) { return Collections.emptySet(); } - // Collect resulting group names in a set - Set<String> groupNames = new HashSet<>(); - for (String role : roles) { - MSentryRole sentryRole = null; - try { - sentryRole = delegate.getMSentryRoleByName(role); - } - catch (SentryNoSuchObjectException e) { - // Role disappeared - not a big deal, just ognore it - continue; - } - // Collect all group names for this role. - // Since we use a set, a group can appear multiple times and will only - // show up once in a set - for (MSentryGroup group : sentryRole.getGroups()) { - groupNames.add(group.getGroupName()); - } + return delegate.getTransactionManager().executeTransaction( + new TransactionBlock<Set<TSentryRole>>() { + public Set<TSentryRole> execute(PersistenceManager pm) throws Exception { + Set<TSentryRole> tRoles = Sets.newHashSet(); + pm.setDetachAllOnCommit(false); // No need to detach objects + Set<MSentryRole> mSentryRoles = Sets.newHashSet(); + if(groups.contains(null)) { + mSentryRoles.addAll(delegate.getAllRoles(pm)); + } else { + mSentryRoles = delegate.getRolesForGroups(pm, groups); + } + for(MSentryRole mSentryRole: mSentryRoles) { + String roleName = mSentryRole.getRoleName().intern(); + Set<String> groupNames = Sets.newHashSet(); + Set<MSentryGroup> mSentryGroups = mSentryRole.getGroups(); + for(MSentryGroup mSentryGroup: mSentryGroups) { + groupNames.add(mSentryGroup.getGroupName()); + } + tRoles.add(new TSentryRole(roleName, groupNames)); + } + + return tRoles; + } + }); + } + + @Override + public Set<String> getGroupsByRoles(final String component, final Set<String> roles) + throws Exception { + if (roles.isEmpty()) { + return Collections.emptySet(); } - return groupNames; + return delegate.getTransactionManager().executeTransaction( + new TransactionBlock<Set<String>>() { + public Set<String> execute(PersistenceManager pm) throws Exception { + pm.setDetachAllOnCommit(false); // No need to detach objects + Query query = pm.newQuery(MSentryGroup.class); + // Find privileges matching all roles + QueryParamBuilder paramBuilder = QueryParamBuilder.addRolesFilter(query, null, + roles); + query.setFilter(paramBuilder.toString()); + List<MSentryGroup> mGroups = + (List<MSentryGroup>)query.executeWithMap(paramBuilder.getArguments()); + + Set<String> groupNames = new HashSet<>(); + for(MSentryGroup g: mGroups) { + groupNames.add(g.getGroupName()); + } + return groupNames; + } + }); } @Override http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java index eec2757..ada46b5 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java @@ -20,6 +20,7 @@ package org.apache.sentry.provider.db.generic.service.persistent; import java.util.List; import java.util.Set; +import org.apache.sentry.api.generic.thrift.TSentryRole; import org.apache.sentry.core.common.Authorizable; import org.apache.sentry.provider.db.service.model.MSentryGMPrivilege; @@ -125,6 +126,15 @@ public interface SentryStoreLayer { Set<String> getRolesByGroups(String component, Set<String> groups) throws Exception; /** + * Get roles + * @param component: The request respond to which component + * @param groups: The name of groups + * @returns the set of roles + * @throws Exception + */ + Set<TSentryRole> getTSentryRolesByGroupName(String component, Set<String> groups) throws Exception; + + /** * Get groups * @param component: The request respond to which component * @param roles: The name of roles http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index 8b32e7b..6455597 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -332,7 +332,7 @@ public class SentryStore implements SentryStoreInterface { * @return List of all roles */ @SuppressWarnings("unchecked") - private List<MSentryRole> getAllRoles(PersistenceManager pm) { + public List<MSentryRole> getAllRoles(PersistenceManager pm) { Query query = pm.newQuery(MSentryRole.class); query.addExtension(LOAD_RESULTS_AT_COMMIT, "false"); return (List<MSentryRole>) query.execute(); http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java index 4c207e9..77d9ed4 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java @@ -289,6 +289,13 @@ public class TestSentryGenericPolicyProcessor extends org.junit.Assert { Mockito.when(mockStore.getGroupsByRoles(anyString(), anySetOf(String.class))) .thenReturn(Sets.newHashSet(groupName)); + Set<TSentryRole> mockTRoles = Sets.newHashSet(); + TSentryRole tSentryRole = new TSentryRole(roleName, Sets.newHashSet(groupName)); + mockTRoles.add(tSentryRole); + Mockito.when(mockStore.getTSentryRolesByGroupName(anyString(), anySetOf(String.class))) + .thenReturn(mockTRoles); + + Mockito.when(mockStore.getPrivilegesByAuthorizable(anyString(), anyString(), anySetOf(String.class), anyListOf(Authorizable.class))) .thenReturn(Sets.newHashSet(mSentryGMPrivilege)); http://git-wip-us.apache.org/repos/asf/sentry/blob/f099f0ab/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java index 69d1623..6126a58 100644 --- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java +++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java @@ -22,6 +22,7 @@ import static org.junit.Assert.fail; import java.util.Set; +import org.apache.sentry.api.generic.thrift.TSentryRole; import org.apache.sentry.core.common.exception.SentryAlreadyExistsException; import org.apache.sentry.core.common.exception.SentryNoSuchObjectException; import org.apache.sentry.provider.file.PolicyFile; @@ -104,27 +105,49 @@ public class TestDelegateSentryStore extends SentryStoreIntegrationBase{ @Test public void testAddDeleteRoleToGroups() throws Exception { - String role1 = "r1", role2 = "r2"; + String role1 = "r1", role2 = "r2", role3 = "r3"; Set<String> twoGroups = Sets.newHashSet("g1", "g2"); Set<String> oneGroup = Sets.newHashSet("g3"); String grantor = "grantor"; sentryStore.createRole(SEARCH, role1, grantor); sentryStore.createRole(SEARCH, role2, grantor); + sentryStore.createRole(SEARCH, role3, grantor); sentryStore.alterRoleAddGroups(SEARCH, role1, twoGroups, grantor); - assertEquals(twoGroups, sentryStore.getGroupsByRoles(SEARCH,Sets.newHashSet(role1))); + Set<TSentryRole> tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, twoGroups); + assertEquals(1, tRoles.size()); + for(TSentryRole tRole:tRoles) { + assertEquals(role1, tRole.getRoleName()); + assertEquals(twoGroups, tRole.getGroups()); + } assertEquals(Sets.newHashSet(role1), sentryStore.getRolesByGroups(SEARCH, twoGroups)); sentryStore.alterRoleAddGroups(SEARCH, role2, oneGroup, grantor); - assertEquals(oneGroup, sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role2))); + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, oneGroup); + assertEquals(1, tRoles.size()); + for(TSentryRole tRole:tRoles) { + assertEquals(role2, tRole.getRoleName()); + assertEquals(oneGroup, tRole.getGroups()); + } + //Test with null group added + Set<String>tempGroups = Sets.newHashSet(); + tempGroups.add(null); + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, tempGroups); + assertEquals(3, tRoles.size()); //Should get all roles + assertEquals(Sets.newHashSet(role1, role2, role3), sentryStore.getRolesByGroups(SEARCH, tempGroups)); sentryStore.alterRoleDeleteGroups(SEARCH, role1, Sets.newHashSet("g1"), grantor); - assertEquals(Sets.newHashSet("g2"), sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role1))); - + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, twoGroups); + assertEquals(1, tRoles.size()); + for(TSentryRole tRole:tRoles) { + assertEquals(role1, tRole.getRoleName()); + assertEquals(Sets.newHashSet("g2"), tRole.getGroups()); + } sentryStore.alterRoleDeleteGroups(SEARCH, role2, oneGroup, grantor); - assertEquals(Sets.newHashSet(), sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role2))); + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, oneGroup); + assertEquals(0, tRoles.size()); } @Test @@ -144,19 +167,59 @@ public class TestDelegateSentryStore extends SentryStoreIntegrationBase{ @Test public void testGetGroupsByRoleNames() throws Exception { - String role1 = "r1", role2 = "r2"; - Set<String> twoGroups = Sets.newHashSet("g1", "g2"); + String role1 = "r1", role2 = "r2", role3 = "r3"; + Set<String> groups1 = Sets.newHashSet("g1", "g2", "g3", "g4"); + Set<String> groups2 = Sets.newHashSet("g1", "g3", "g5", "g6", "g7"); + Set<String> allGroups = Sets.newHashSet("g1", "g2", "g3", "g4", "g5", "g6", "g7"); String grantor = "grantor"; sentryStore.createRole(SEARCH, role1, grantor); sentryStore.createRole(SEARCH, role2, grantor); + sentryStore.createRole(SEARCH, role3, grantor); + + sentryStore.alterRoleAddGroups(SEARCH, role1, groups1, grantor); + sentryStore.alterRoleAddGroups(SEARCH, role2, groups2, grantor); + + Set<TSentryRole> tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, Sets.newHashSet(groups1)); + assertEquals(2, tRoles.size()); + for(TSentryRole tRole:tRoles) { + if(tRole.getRoleName().equals(role1)) { + assertEquals(groups1, tRole.getGroups()); + } else if(tRole.getRoleName().equals(role2)) { + assertEquals(groups2, tRole.getGroups()); + } + } - sentryStore.alterRoleAddGroups(SEARCH, role1, twoGroups, grantor); - sentryStore.alterRoleAddGroups(SEARCH, role2, twoGroups, grantor); + //Get the same output as before + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, Sets.newHashSet(groups2)); + assertEquals(2, tRoles.size()); + for(TSentryRole tRole:tRoles) { + if(tRole.getRoleName().equals(role1)) { + assertEquals(groups1, tRole.getGroups()); + } else if(tRole.getRoleName().equals(role2)) { + assertEquals(groups2, tRole.getGroups()); + } + } - assertEquals(twoGroups, sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role1))); - assertEquals(twoGroups, sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role2))); - assertEquals(twoGroups, sentryStore.getGroupsByRoles(SEARCH, Sets.newHashSet(role1,role2))); + //Again get the same output as before + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, allGroups); + assertEquals(2, tRoles.size()); + for(TSentryRole tRole:tRoles) { + if(tRole.getRoleName().equals(role1)) { + assertEquals(groups1, tRole.getGroups()); + } else if(tRole.getRoleName().equals(role2)) { + assertEquals(groups2, tRole.getGroups()); + } + } + + sentryStore.alterRoleAddGroups(SEARCH, role3, groups1, grantor); + tRoles = sentryStore.getTSentryRolesByGroupName(SEARCH, allGroups); + assertEquals(3, tRoles.size()); + for(TSentryRole tRole:tRoles) { + if(tRole.getRoleName().equals(role3)) { + assertEquals(groups1, tRole.getGroups()); + } + } } @Test
