Repository: sentry Updated Branches: refs/heads/sentry-ha-redesign 774800343 -> 7d28a41a0
SENTRY-1556: Simplify privilege cleaning (Kalyan Kumar Kalvagadda, Reviewed by: Alex Kolbasov) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/7d28a41a Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/7d28a41a Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/7d28a41a Branch: refs/heads/sentry-ha-redesign Commit: 7d28a41a000b4f2b5006047f4c04bc86dbcc6d3d Parents: 7748003 Author: Alexander Kolbasov <[email protected]> Authored: Fri Apr 14 15:21:07 2017 -0700 Committer: Alexander Kolbasov <[email protected]> Committed: Fri Apr 14 15:21:07 2017 -0700 ---------------------------------------------------------------------- .../service/persistent/DelegateSentryStore.java | 2 - .../provider/db/service/model/MSentryRole.java | 12 +- .../db/service/persistent/SentryStore.java | 391 ++++++++----------- .../sentry/service/thrift/ServiceConstants.java | 2 - .../service/persistent/TestSentryRole.java | 172 ++++++++ .../db/service/persistent/TestSentryStore.java | 204 +++++++++- 6 files changed, 521 insertions(+), 262 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java index ed47441..ebb1916 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java @@ -63,8 +63,6 @@ public class DelegateSentryStore implements SentryStoreLayer { DelegateSentryStore(Configuration conf) throws Exception { this.privilegeOperator = new PrivilegeOperatePersistence(conf); - // The generic model doesn't turn on the thread that cleans hive privileges - conf.set(ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL,"false"); this.conf = conf; //delegated old sentryStore this.delegate = new SentryStore(conf); http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java index f1d7a86..f9da589 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java @@ -173,14 +173,16 @@ public class MSentryRole { } } - public Set<MSentryPrivilege> removePrivileges() { - // copy is required since privilege.removeRole will call remotePrivilege - Set<MSentryPrivilege> copy = ImmutableSet.copyOf(privileges); - for (MSentryPrivilege privilege : copy) { + public void removePrivileges() { + // As we iterate through the loop below Method removeRole will modify the privileges set + // will be updated. + // Copy of the <code>privileges<code> is taken at the beginning of the loop to avoid using + // the actual privilege set in MSentryRole instance. + + for (MSentryPrivilege privilege : ImmutableSet.copyOf(privileges)) { privilege.removeRole(this); } Preconditions.checkState(privileges.isEmpty(), "Privileges should be empty: " + privileges); - return copy; } @Override http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index aaea979..08520f3 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -32,9 +32,6 @@ import java.util.List; import java.util.Map; import java.util.Properties; import java.util.Set; -import java.util.concurrent.locks.Condition; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; import javax.jdo.FetchGroup; import javax.jdo.JDODataStoreException; @@ -144,8 +141,6 @@ public class SentryStore { private final PersistenceManagerFactory pmf; private Configuration conf; - private PrivCleaner privCleaner = null; - private Thread privCleanerThread = null; private final TransactionManager tm; /** @@ -238,15 +233,6 @@ public class SentryStore { pmf = JDOHelper.getPersistenceManagerFactory(prop); tm = new TransactionManager(pmf, conf); verifySentryStoreSchema(checkSchemaVersion); - - // Kick off the thread that cleans orphaned privileges (unless told not to) - privCleaner = this.new PrivCleaner(); - if (conf.get(ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL, - ServerConfig.SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL_DEFAULT) - .equalsIgnoreCase("true")) { - privCleanerThread = new Thread(privCleaner); - privCleanerThread.start(); - } } public TransactionManager getTransactionManager() { @@ -274,14 +260,6 @@ public class SentryStore { } public synchronized void stop() { - if (privCleanerThread != null) { - privCleaner.exit(); - try { - privCleanerThread.join(); - } catch (InterruptedException e) { - // Ignore... - } - } if (pmf != null) { pmf.close(); } @@ -681,12 +659,10 @@ public class SentryStore { MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm); if (mSelect != null && mRole.getPrivileges().contains(mSelect)) { mSelect.removeRole(mRole); - privCleaner.incPrivRemoval(); pm.makePersistent(mSelect); } if (mInsert != null && mRole.getPrivileges().contains(mInsert)) { mInsert.removeRole(mRole); - privCleaner.incPrivRemoval(); pm.makePersistent(mInsert); } } else { @@ -854,41 +830,59 @@ public class SentryStore { * privilege and add SELECT (INSERT was revoked) or INSERT (SELECT was revoked). */ private void revokePartial(PersistenceManager pm, - TSentryPrivilege requestedPrivToRevoke, MSentryRole mRole, - MSentryPrivilege currentPrivilege) throws SentryInvalidInputException { - MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); + TSentryPrivilege requestedPrivToRevoke, MSentryRole mRole, + MSentryPrivilege currentPrivilege) throws SentryInvalidInputException { + MSentryPrivilege persistedPriv = + getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); if (persistedPriv == null) { + // The privilege corresponding to the currentPrivilege doesn't exist in the persistent + // store, so we create a fake one for the code below. The fake one is not associated with + // any role and shouldn't be stored in the persistent storage. persistedPriv = convertToMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege)); } - if (requestedPrivToRevoke.getAction().equalsIgnoreCase("ALL") || requestedPrivToRevoke.getAction().equalsIgnoreCase("*")) { - persistedPriv.removeRole(mRole); - privCleaner.incPrivRemoval(); - pm.makePersistent(persistedPriv); + if (requestedPrivToRevoke.getAction().equalsIgnoreCase("ALL") || + requestedPrivToRevoke.getAction().equalsIgnoreCase("*")) { + if (!persistedPriv.getRoles().isEmpty()) { + persistedPriv.removeRole(mRole); + if (persistedPriv.getRoles().isEmpty()) { + pm.deletePersistent(persistedPriv); + } else { + pm.makePersistent(persistedPriv); + } + } } else if (requestedPrivToRevoke.getAction().equalsIgnoreCase(AccessConstants.SELECT) - && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.INSERT)) { + && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.INSERT)) { revokeRolePartial(pm, mRole, currentPrivilege, persistedPriv, AccessConstants.INSERT); } else if (requestedPrivToRevoke.getAction().equalsIgnoreCase(AccessConstants.INSERT) - && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.SELECT)) { + && !currentPrivilege.getAction().equalsIgnoreCase(AccessConstants.SELECT)) { revokeRolePartial(pm, mRole, currentPrivilege, persistedPriv, AccessConstants.SELECT); } } private void revokeRolePartial(PersistenceManager pm, MSentryRole mRole, - MSentryPrivilege currentPrivilege, MSentryPrivilege persistedPriv, String addAction) - throws SentryInvalidInputException { + MSentryPrivilege currentPrivilege, + MSentryPrivilege persistedPriv, + String addAction) throws SentryInvalidInputException { // If table / URI, remove ALL - persistedPriv.removeRole(mRole); - privCleaner.incPrivRemoval(); - pm.makePersistent(persistedPriv); - + persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(persistedPriv), pm); + if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) { + persistedPriv.removeRole(mRole); + if (persistedPriv.getRoles().isEmpty()) { + pm.deletePersistent(persistedPriv); + } else { + pm.makePersistent(persistedPriv); + } + } currentPrivilege.setAction(AccessConstants.ALL); persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); if (persistedPriv != null && mRole.getPrivileges().contains(persistedPriv)) { persistedPriv.removeRole(mRole); - privCleaner.incPrivRemoval(); - pm.makePersistent(persistedPriv); - + if (persistedPriv.getRoles().isEmpty()) { + pm.deletePersistent(persistedPriv); + } else { + pm.makePersistent(persistedPriv); + } currentPrivilege.setAction(addAction); persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(currentPrivilege), pm); if (persistedPriv == null) { @@ -904,7 +898,8 @@ public class SentryStore { * Revoke privilege from role */ private void revokePrivilegeFromRole(PersistenceManager pm, TSentryPrivilege tPrivilege, - MSentryRole mRole, MSentryPrivilege mPrivilege) throws SentryInvalidInputException { + MSentryRole mRole, MSentryPrivilege mPrivilege) + throws SentryInvalidInputException { if (PARTIAL_REVOKE_ACTIONS.contains(mPrivilege.getAction())) { // if this privilege is in {ALL,SELECT,INSERT} // we will do partial revoke @@ -913,10 +908,13 @@ public class SentryStore { // if this privilege is not ALL, SELECT nor INSERT, // we will revoke it from role directly MSentryPrivilege persistedPriv = getMSentryPrivilege(convertToTSentryPrivilege(mPrivilege), pm); - if (persistedPriv != null) { - mPrivilege.removeRole(mRole); - privCleaner.incPrivRemoval(); - pm.makePersistent(mPrivilege); + if (persistedPriv != null && !persistedPriv.getRoles().isEmpty()) { + persistedPriv.removeRole(mRole); + if (persistedPriv.getRoles().isEmpty()) { + pm.deletePersistent(persistedPriv); + } else { + pm.makePersistent(persistedPriv); + } } } } @@ -1100,14 +1098,34 @@ public class SentryStore { if (sentryRole == null) { throw noSuchRole(lRoleName); } - int numPrivs = sentryRole.getPrivileges().size(); + removePrivileges(pm, sentryRole); + pm.deletePersistent(sentryRole); + } + + /** + * Removes all the privileges associated with + * a particular role. After this dis-association if the + * privilege doesn't have any roles associated it will be + * removed from the underlying persistence layer. + * @param pm Instance of PersistenceManager + * @param sentryRole Role for which all the privileges are to be removed. + */ + private void removePrivileges(PersistenceManager pm, MSentryRole sentryRole) { + List<MSentryPrivilege> privilegesCopy = new ArrayList<>(sentryRole.getPrivileges()); + List<MSentryPrivilege> stalePrivileges = new ArrayList<>(0); + sentryRole.removePrivileges(); // with SENTRY-398 generic model sentryRole.removeGMPrivileges(); - privCleaner.incPrivRemoval(numPrivs); - pm.deletePersistent(sentryRole); + for (MSentryPrivilege privilege : privilegesCopy) { + if(privilege.getRoles().isEmpty()) { + stalePrivileges.add(privilege); + } + } + if(!stalePrivileges.isEmpty()) { + pm.deletePersistentAll(stalePrivileges); + } } - /** * Assign a given role to a set of groups. * @@ -1328,6 +1346,53 @@ public class SentryStore { }); } + /** + * Gets the MSentryPrivilege from sentry persistent storage based on TSentryPrivilege + * provided + * + * Method is currently used only in test framework + * @param tPrivilege + * @return MSentryPrivilege if the privilege is found in the storage + * null, if the privilege is not found in the storage. + * @throws Exception + */ + @VisibleForTesting + MSentryPrivilege findMSentryPrivilegeFromTSentryPrivilege(final TSentryPrivilege tPrivilege) throws Exception { + return tm.executeTransaction( + new TransactionBlock<MSentryPrivilege>() { + public MSentryPrivilege execute(PersistenceManager pm) throws Exception { + return getMSentryPrivilege(tPrivilege, pm); + } + }); + } + + /** + * Returns a list with all the privileges in the sentry persistent storage + * + * Method is currently used only in test framework + * @return List of all sentry privileges in the store + * @throws Exception + */ + @VisibleForTesting + List<MSentryPrivilege> getAllMSentryPrivileges () throws Exception { + return tm.executeTransaction( + new TransactionBlock<List<MSentryPrivilege>>() { + public List<MSentryPrivilege> execute(PersistenceManager pm) throws Exception { + return getAllMSentryPrivilegesCore(pm); + } + }); + } + + /** + * Method Returns all the privileges present in the persistent store as a list. + * @param pm PersistenceManager + * @returns list of all the privileges in the persistent store + */ + private List<MSentryPrivilege> getAllMSentryPrivilegesCore (PersistenceManager pm) { + Query query = pm.newQuery(MSentryPrivilege.class); + return (List<MSentryPrivilege>) query.execute(); + } + private boolean hasAnyServerPrivileges(final Set<String> roleNames, final String serverName) throws Exception { if (roleNames == null || roleNames.isEmpty()) { return false; @@ -2121,7 +2186,12 @@ public class SentryStore { // 1. get privilege and child privileges Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet(); if (parent != null) { - privilegeGraph.add(parent); + + // Parent privilege object is used after revoking. + // If the privilege was associated to only role which is revoked, + // privilege object should not be used. It is safe to insert + // a copy of the parent object in the privilegeGraph + privilegeGraph.add(convertToMSentryPrivilege(convertToTSentryPrivilege(parent))); populateChildren(pm, Sets.newHashSet(role.getRoleName()), parent, privilegeGraph); } else { populateChildren(pm, Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege), @@ -2486,197 +2556,46 @@ public class SentryStore { } /** - * This thread exists to clean up "orphaned" privilege rows in the database. - * These rows aren't removed automatically due to the fact that there is - * a many-to-many mapping between the roles and privileges, and the - * detection and removal of orphaned privileges is a wee bit involved. - * This thread hangs out until notified by the parent (the outer class) - * and then runs a custom SQL statement that detects and removes orphans. - */ - private class PrivCleaner implements Runnable { - // Kick off priv orphan removal after this many notifies - private static final int NOTIFY_THRESHOLD = 50; - - // How many times we've been notified; reset to zero after orphan removal - // This begins at the NOTIFY_THRESHOLD, so that we clear any potential - // orphans on startup. - private int currentNotifies = NOTIFY_THRESHOLD; - - // Internal state for threads - private boolean exitRequired = false; - - // This lock and condition are needed to implement a way to drop the - // lock inside a while loop, and not hold the lock across the orphan - // removal. - private final Lock lock = new ReentrantLock(); - private final Condition cond = lock.newCondition(); - - /** - * Waits in a loop, running the orphan removal function when notified. - * Will exit after exitRequired is set to true by exit(). We are careful - * to not hold our lock while removing orphans; that operation might - * take a long time. There's also the matter of lock ordering. Other - * threads start a transaction first, and then grab our lock; this thread - * grabs the lock and then starts a transaction. Handling this correctly - * requires explicit locking/unlocking through the loop. - */ - public void run() { - while (true) { - lock.lock(); - try { - // Check here in case this was set during removeOrphanedPrivileges() - if (exitRequired) { - return; - } - while (currentNotifies <= NOTIFY_THRESHOLD) { - try { - cond.await(); - } catch (InterruptedException e) { - // Interrupted - } - // Check here in case this was set while waiting - if (exitRequired) { - return; - } - } - currentNotifies = 0; - } finally { - lock.unlock(); - } - try { - removeOrphanedPrivileges(); - } catch (Exception e) { - LOGGER.warn("Privilege cleaning thread encountered an error: " + - e.getMessage()); - } - } - } + * Method detects orphaned privileges + * + * @return True, If there are orphan privileges + * False, If orphan privileges are not found. + * non-zero value if an orphan is found. + * <p> + * Method currently used only by tests. + * <p> + */ - /** - * This is called when a privilege is removed from a role. This may - * or may not mean that the privilege needs to be removed from the - * database; there may be more references to it from other roles. - * As a result, we'll lazily run the orphan cleaner every - * NOTIFY_THRESHOLD times this routine is called. - * @param numDeletions The number of potentially orphaned privileges - */ - void incPrivRemoval(int numDeletions) { - if (privCleanerThread != null) { - try { - lock.lock(); - currentNotifies += numDeletions; - if (currentNotifies > NOTIFY_THRESHOLD) { - cond.signal(); - } - } finally { - lock.unlock(); + @VisibleForTesting + Boolean findOrphanedPrivileges() throws Exception { + return tm.executeTransaction( + new TransactionBlock<Boolean>() { + public Boolean execute(PersistenceManager pm) throws Exception { + return findOrphanedPrivilegesCore(pm); } - } - } + }); + } - /** - * Simple form of incPrivRemoval when only one privilege is deleted. - */ - void incPrivRemoval() { - incPrivRemoval(1); + Boolean findOrphanedPrivilegesCore(PersistenceManager pm) { + ArrayList<Object> idList = new ArrayList<Object>(); + //Perform a SQL query to get things that look like orphans + List<MSentryPrivilege> results = getAllMSentryPrivilegesCore(pm); + for (MSentryPrivilege orphan : results) { + idList.add(pm.getObjectId(orphan)); } - - /** - * Tell this thread to exit. Safe to call multiple times, as it just - * notifies the run() loop to finish up. - */ - void exit() { - if (privCleanerThread != null) { - lock.lock(); - try { - exitRequired = true; - cond.signal(); - } finally { - lock.unlock(); - } - } + if (idList.isEmpty()) { + return false; } - - /** - * Run a SQL query to detect orphaned privileges, and then delete - * each one. This is complicated by the fact that datanucleus does - * not seem to play well with the mix between a direct SQL query - * and operations on the database. The solution that seems to work - * is to split the operation into two transactions: the first is - * just a read for privileges that look like they're orphans, the - * second transaction will go and get each of those privilege objects, - * verify that there are no roles attached, and then delete them. - */ - @SuppressWarnings("unchecked") - private void removeOrphanedPrivileges() { - final String privDB = "SENTRY_DB_PRIVILEGE"; - final String privId = "DB_PRIVILEGE_ID"; - final String mapDB = "SENTRY_ROLE_DB_PRIVILEGE_MAP"; - final String privFilter = - "select " + privId + - " from " + privDB + " p" + - " where not exists (" + - " select 1 from " + mapDB + " d" + - " where p." + privId + " != d." + privId + - " )"; - boolean rollback = true; - int orphansRemoved = 0; - ArrayList<Object> idList = new ArrayList<>(); - PersistenceManager pm = pmf.getPersistenceManager(); - - // Transaction 1: Perform a SQL query to get things that look like orphans - try { - Transaction transaction = pm.currentTransaction(); - transaction.begin(); - transaction.setRollbackOnly(); // Makes the tx read-only - Query query = pm.newQuery("javax.jdo.query.SQL", privFilter); - query.setClass(MSentryPrivilege.class); - List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute(); - for (MSentryPrivilege orphan : results) { - idList.add(pm.getObjectId(orphan)); - } - transaction.rollback(); - rollback = false; - } finally { - if (rollback && pm.currentTransaction().isActive()) { - pm.currentTransaction().rollback(); - } else { - LOGGER.debug("Found {} potential orphans", idList.size()); - } - } - - if (idList.isEmpty()) { - pm.close(); - return; - } - - Preconditions.checkState(!rollback); - - // Transaction 2: For each potential orphan, verify it's really an - // orphan and delete it if so - rollback = true; - try { - Transaction transaction = pm.currentTransaction(); - transaction.begin(); - pm.refreshAll(); // Try to ensure we really have correct objects - for (Object id : idList) { - MSentryPrivilege priv = (MSentryPrivilege) pm.getObjectById(id); - if (priv.getRoles().isEmpty()) { - pm.deletePersistent(priv); - orphansRemoved++; - } - } - transaction.commit(); - pm.close(); - rollback = false; - } finally { - if (rollback) { - rollbackTransaction(pm); - } else { - LOGGER.debug("Cleaned up {} orphaned privileges", orphansRemoved); - } + //For each potential orphan, verify it's really a orphan. + // Moment an orphan is identified return 1 indicating an orphan is found. + pm.refreshAll(); // Try to ensure we really have correct objects + for (Object id : idList) { + MSentryPrivilege priv = (MSentryPrivilege) pm.getObjectById(id); + if (priv.getRoles().isEmpty()) { + return true; } } + return false; } /** get mapping datas for [group,role], [user,role] with the specific roles */ http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java index d3a68c9..8bda2f8 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java @@ -119,8 +119,6 @@ public class ServiceConstants { public static final String SENTRY_STORE_LOCAL_GROUP_MAPPING = "org.apache.sentry.provider.file.LocalGroupMappingService"; public static final String SENTRY_STORE_GROUP_MAPPING_DEFAULT = SENTRY_STORE_HADOOP_GROUP_MAPPING; - public static final String SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL = "sentry.store.orphaned.privilege.removal"; - public static final String SENTRY_STORE_ORPHANED_PRIVILEGE_REMOVAL_DEFAULT = "false"; public static final String SENTRY_STORE_CLEAN_PERIOD_SECONDS = "sentry.store.clean.period.seconds"; public static final long SENTRY_STORE_CLEAN_PERIOD_SECONDS_DEFAULT = 43200; // 12 hours. http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java index 29134fe..9be4a8b 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java @@ -24,6 +24,7 @@ import static org.junit.Assert.fail; import java.io.File; import java.util.Arrays; import java.util.Properties; +import java.util.List; import javax.jdo.JDOHelper; import javax.jdo.PersistenceManager; @@ -47,6 +48,8 @@ import com.google.common.io.Files; /** * The class tests that the new feature SENTRY-398 generic model adds the new field in the MSentryRole * will not affect the functionality of the origin hive/impala authorization model + * Some Tests below make sure that privileges are removed from sentry storage the moment they are not associated to any role. + * This avoid the need for PrivCleaner to perform periodic cleanup. */ public class TestSentryRole { private static PersistenceManagerFactory pmf; @@ -342,6 +345,175 @@ public class TestSentryRole { commitTransaction(pm); } + /** + * Removes a role and makes sure that privileges are removed from sentry storage + * moment they are not associated to any role. + * @throws Exception + */ + @Test + public void testDeleteRole() throws Exception { + String roleName = "r1"; + //hive/impala privilege + MSentryPrivilege hivePrivilege = new MSentryPrivilege(); + hivePrivilege.setServerName("hive.server1"); + hivePrivilege.setDbName("db1"); + hivePrivilege.setTableName("tb1"); + hivePrivilege.setPrivilegeScope("table"); + hivePrivilege.setAction("select"); + hivePrivilege.setURI(SentryStore.NULL_COL); + hivePrivilege.setColumnName(SentryStore.NULL_COL); + hivePrivilege.setGrantOption(true); + + //solr privilege + MSentryGMPrivilege solrPrivilege = new MSentryGMPrivilege(); + solrPrivilege.setComponentName("solr"); + solrPrivilege.setServiceName("solr.server1"); + solrPrivilege.setAuthorizables(Arrays.asList(new Collection("c1"))); + solrPrivilege.setAction("query"); + solrPrivilege.setGrantOption(true); + + PersistenceManager pm = null; + //create role + pm = openTransaction(); + pm.makePersistent(new MSentryRole(roleName, System.currentTimeMillis())); + commitTransaction(pm); + + //grant hivePrivilege and solrPrivilege to role + pm = openTransaction(); + MSentryRole role = getMSentryRole(pm, roleName); + hivePrivilege.appendRole(role); + solrPrivilege.appendRole(role); + pm.makePersistent(hivePrivilege); + pm.makePersistent(solrPrivilege); + pm.makePersistent(role); + commitTransaction(pm); + + //check + pm = openTransaction(); + role = getMSentryRole(pm, roleName); + pm.retrieve(role); + assertEquals(1, role.getPrivileges().size()); + assertEquals(1, role.getGmPrivileges().size()); + commitTransaction(pm); + + //delete role + pm = openTransaction(); + role = getMSentryRole(pm, roleName); + + // pm.deletePersistent(role); + role.removePrivileges(); + role.removeGMPrivileges(); + pm.deletePersistent(role); + commitTransaction(pm); + + //check for privileges + //There shouldn't be any privilages + pm = openTransaction(); + Query query = pm.newQuery(MSentryPrivilege.class); + List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute(); + assertEquals(1, results.size()); + Query query1 = pm.newQuery(MSentryGMPrivilege.class); + List<MSentryGMPrivilege> results1 = (List<MSentryGMPrivilege>) query1.execute(); + assertEquals(1, results1.size()); + commitTransaction(pm); + + //check + pm = openTransaction(); + role = getMSentryRole(pm, roleName); + assertTrue(role == null); + commitTransaction(pm); + } + + /** + * Removes a role and makes sure that privileges are not removed from sentry storage if + * they are associated to any other role as well. + * @throws Exception + */ + @Test + public void testDeleteRole1() throws Exception { + String roleName1 = "r1"; + String roleName2 = "r2"; + //hive/impala privilege + MSentryPrivilege hivePrivilege = new MSentryPrivilege(); + hivePrivilege.setServerName("hive.server1"); + hivePrivilege.setDbName("db1"); + hivePrivilege.setTableName("tb1"); + hivePrivilege.setPrivilegeScope("table"); + hivePrivilege.setAction("select"); + hivePrivilege.setURI(SentryStore.NULL_COL); + hivePrivilege.setColumnName(SentryStore.NULL_COL); + hivePrivilege.setGrantOption(true); + + //solr privilege + MSentryGMPrivilege solrPrivilege = new MSentryGMPrivilege(); + solrPrivilege.setComponentName("solr"); + solrPrivilege.setServiceName("solr.server1"); + solrPrivilege.setAuthorizables(Arrays.asList(new Collection("c1"))); + solrPrivilege.setAction("query"); + solrPrivilege.setGrantOption(true); + + PersistenceManager pm = null; + //create role1 + pm = openTransaction(); + pm.makePersistent(new MSentryRole(roleName1, System.currentTimeMillis())); + commitTransaction(pm); + + //create role2 + pm = openTransaction(); + pm.makePersistent(new MSentryRole(roleName2, System.currentTimeMillis())); + commitTransaction(pm); + + //grant hivePrivilege and solrPrivilege to role1 and role2 + pm = openTransaction(); + MSentryRole role1 = getMSentryRole(pm, roleName1); + MSentryRole role2 = getMSentryRole(pm, roleName2); + hivePrivilege.appendRole(role1); + solrPrivilege.appendRole(role1); + hivePrivilege.appendRole(role2); + solrPrivilege.appendRole(role2); + pm.makePersistent(hivePrivilege); + pm.makePersistent(solrPrivilege); + pm.makePersistent(role1); + pm.makePersistent(role2); + commitTransaction(pm); + + //check + pm = openTransaction(); + role1 = getMSentryRole(pm, roleName1); + pm.retrieve(role1); + assertEquals(1, role1.getPrivileges().size()); + assertEquals(1, role1.getGmPrivileges().size()); + role2 = getMSentryRole(pm, roleName2); + pm.retrieve(role2); + assertEquals(1, role2.getPrivileges().size()); + assertEquals(1, role2.getGmPrivileges().size()); + commitTransaction(pm); + + //delete role + pm = openTransaction(); + role1 = getMSentryRole(pm, roleName1); + role1.removePrivileges(); + role1.removeGMPrivileges(); + pm.deletePersistent(role1); + commitTransaction(pm); + + //check for privileges + //Privileges should be present + pm = openTransaction(); + Query query = pm.newQuery(MSentryPrivilege.class); + List<MSentryPrivilege> results = (List<MSentryPrivilege>) query.execute(); + assertEquals(1, results.size()); + Query query1 = pm.newQuery(MSentryGMPrivilege.class); + List<MSentryGMPrivilege> results1 = (List<MSentryGMPrivilege>) query1.execute(); + assertEquals(1, results1.size()); + commitTransaction(pm); + + //check + pm = openTransaction(); + role1 = getMSentryRole(pm, roleName1); + assertTrue(role1 == null); + commitTransaction(pm); + } private PersistenceManager openTransaction() { PersistenceManager pm = pmf.getPersistenceManager(); Transaction currentTransaction = pm.currentTransaction(); http://git-wip-us.apache.org/repos/asf/sentry/blob/7d28a41a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java index 31a309b..87e3295 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java @@ -25,6 +25,12 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.List; import org.apache.commons.io.FileUtils; import org.apache.hadoop.conf.Configuration; @@ -55,7 +61,6 @@ import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; -import org.junit.Ignore; import org.junit.Test; import com.google.common.collect.Iterables; @@ -76,7 +81,7 @@ public class TestSentryStore extends org.junit.Assert { private static String[] adminGroups = { "adminGroup1" }; private static PolicyFile policyFile; private static File policyFilePath; - final long NUM_PRIVS = 60; // > SentryStore.PrivCleaner.NOTIFY_THRESHOLD + final long NUM_PRIVS = 5; // > SentryStore.PrivCleaner.NOTIFY_THRESHOLD private static Configuration conf = null; private static char[] passwd = new char[] { '1', '2', '3'}; @@ -519,21 +524,12 @@ public class TestSentryStore extends org.junit.Assert { assertEquals(table, mPrivilege.getTableName()); assertEquals(AccessConstants.INSERT, mPrivilege.getAction()); assertFalse(mPrivilege.getGrantOption()); + long numDBPrivs = sentryStore.countMSentryPrivileges(); + assertEquals("Privilege count", numDBPrivs,1); } private void verifyOrphanCleanup() throws Exception { - boolean success = false; - int iterations = 30; - while (!success && iterations > 0) { - Thread.sleep(1000); - long numDBPrivs = sentryStore.countMSentryPrivileges(); - if (numDBPrivs < NUM_PRIVS) { - assertEquals(0, numDBPrivs); - success = true; - } - iterations--; - } - assertTrue("Failed to cleanup orphaned privileges", success); + assertFalse("Failed to cleanup orphaned privileges", sentryStore.findOrphanedPrivileges()); } /** @@ -542,7 +538,7 @@ public class TestSentryStore extends org.junit.Assert { * cleanup thread runs, and expect them all to be gone from the database. * @throws Exception */ - @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems") + // @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems") @Test public void testPrivilegeCleanup() throws Exception { final String roleName = "test-priv-cleanup"; @@ -566,7 +562,7 @@ public class TestSentryStore extends org.junit.Assert { } // Make sure we really have the expected number of privs in the database - assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS); + assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS); // Now to make a bunch of orphans, we just remove the role that // created them. @@ -574,6 +570,9 @@ public class TestSentryStore extends org.junit.Assert { // Now wait and see if the orphans get cleaned up verifyOrphanCleanup(); + + List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges(); + assertEquals(list.size(), 0); } /** @@ -584,7 +583,7 @@ public class TestSentryStore extends org.junit.Assert { * cleanup thread. * @throws Exception */ - @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems") + // @Ignore("Disabled with SENTRY-545 following SENTRY-140 problems") @Test public void testPrivilegeCleanup2() throws Exception { final String roleName = "test-priv-cleanup"; @@ -622,8 +621,179 @@ public class TestSentryStore extends org.junit.Assert { // Drop the role and clean up as before sentryStore.dropSentryRole(roleName); verifyOrphanCleanup(); + //There should not be any Privileges left + List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges(); + assertEquals(list.size(), 0); + } + + /** + * This method tries to add ALL privileges on + * databases to a role and immediately revokes + * SELECT and INSERT privileges. At the end of + * each iteration we should not find any privileges + * for that role. Finally we should not find any + * privileges, as we are cleaning up orphan privileges + * immediately. + * @throws Exception + */ + @Test + public void testPrivilegeCleanup3() throws Exception { + final String roleName = "test-priv-cleanup"; + final String grantor = "g1"; + final String server = "server"; + final String dBase = "db"; + final String table = "table-"; + + createRole(roleName); + + // Create NUM_PRIVS unique privilege objects in the database once more, + // this time granting ALL and revoking SELECT to make INSERT. + for (int i=0 ; i < NUM_PRIVS; i++) { + TSentryPrivilege priv = new TSentryPrivilege(); + priv.setPrivilegeScope("DATABASE"); + priv.setServerName(server); + priv.setAction(AccessConstants.ALL); + priv.setCreateTime(System.currentTimeMillis()); + priv.setTableName(table + i); + priv.setDbName(dBase); + priv.setGrantOption(TSentryGrantOption.TRUE); + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv); + + priv.setAction(AccessConstants.SELECT); + priv.setGrantOption(TSentryGrantOption.UNSET); + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv); + + sentryStore.findOrphanedPrivileges(); + //After having ALL and revoking SELECT, we should have INSERT + //Remove the INSERT privilege as well. + //There should not be any more privileges in the sentry store + priv.setAction(AccessConstants.INSERT); + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv); + + MSentryRole role = sentryStore.getMSentryRoleByName(roleName); + assertEquals("Privilege Count", 0, role.getPrivileges().size()); + + } + + // Drop the role and clean up as before + verifyOrphanCleanup(); + + //There should not be any Privileges left + List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges(); + assertEquals(list.size(), 0); + } + /** + * This method "n" privileges with action "ALL" on + * tables to a role. Later we are revoking insert + * and select privileges to of the tables making the + * the privilege orphan. Finally we should find only + * n -1 privileges, as we are cleaning up orphan + * privileges immediately. + * @throws Exception + */ + @Test + public void testPrivilegeCleanup4 () throws Exception { + final String roleName = "test-priv-cleanup"; + final String grantor = "g1"; + final String server = "server"; + final String dBase = "db"; + final String table = "table-"; + + createRole(roleName); + + // Create NUM_PRIVS unique privilege objects in the database + for (int i = 0; i < NUM_PRIVS; i++) { + TSentryPrivilege priv = new TSentryPrivilege(); + priv.setPrivilegeScope("TABLE"); + priv.setServerName(server); + priv.setAction(AccessConstants.ALL); + priv.setCreateTime(System.currentTimeMillis()); + priv.setTableName(table + i); + priv.setDbName(dBase); + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv); + } + + // Make sure we really have the expected number of privs in the database + assertEquals(sentryStore.countMSentryPrivileges(), NUM_PRIVS); + + //Revoking INSERT privilege. This is change the privilege to SELECT + TSentryPrivilege priv = new TSentryPrivilege(); + priv.setPrivilegeScope("TABLE"); + priv.setServerName(server); + priv.setAction(AccessConstants.INSERT); + priv.setCreateTime(System.currentTimeMillis()); + priv.setTableName(table + '0'); + priv.setDbName(dBase); + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv); + + //There should be SELECT privilege in the sentry store + priv = new TSentryPrivilege(); + priv.setPrivilegeScope("TABLE"); + priv.setServerName(server); + priv.setAction(AccessConstants.SELECT); + priv.setCreateTime(System.currentTimeMillis()); + priv.setTableName(table + '0'); + priv.setDbName(dBase); + MSentryPrivilege mPriv = sentryStore.findMSentryPrivilegeFromTSentryPrivilege(priv); + assertNotNull(mPriv); + + MSentryRole role = sentryStore.getMSentryRoleByName(roleName); + assertEquals("Privilege Count", NUM_PRIVS, role.getPrivileges().size()); + + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv); + role = sentryStore.getMSentryRoleByName(roleName); + assertEquals("Privilege Count", NUM_PRIVS-1, role.getPrivileges().size()); + + } + + /** + * This method tries to add alter privileges on + * databases to a role and immediately revokes + * them. At the end of each iteration we should + * not find any privileges for that role + * Finally we should not find any privileges, as + * we are cleaning up orphan privileges immediately. + * @throws Exception + */ + @Test + public void testPrivilegeCleanup5() throws Exception { + final String roleName = "test-priv-cleanup"; + final String grantor = "g1"; + final String server = "server"; + final String dBase = "db"; + final String table = "table-"; + + createRole(roleName); + + // Create NUM_PRIVS unique privilege objects in the database once more, + // this time granting ALL and revoking SELECT to make INSERT. + for (int i=0 ; i < NUM_PRIVS; i++) { + TSentryPrivilege priv = new TSentryPrivilege(); + priv.setPrivilegeScope("DATABASE"); + priv.setServerName(server); + priv.setAction(AccessConstants.ALTER); + priv.setCreateTime(System.currentTimeMillis()); + priv.setTableName(table + i); + priv.setDbName(dBase); + priv.setGrantOption(TSentryGrantOption.TRUE); + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, priv); + + priv.setAction(AccessConstants.ALTER); + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, priv); + + MSentryRole role = sentryStore.getMSentryRoleByName(roleName); + assertEquals("Privilege Count", 0, role.getPrivileges().size()); + } + //There should not be any Privileges left + List<MSentryPrivilege> list = sentryStore.getAllMSentryPrivileges(); + assertEquals(list.size(), 0); + + } + + //TODO Use new transaction Manager logic, Instead of + @Test public void testGrantRevokeMultiPrivileges() throws Exception { String roleName = "test-privilege";
