You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by ak...@apache.org on 2017/03/24 00:52:35 UTC
sentry git commit: SENTRY-1556: Simplify privilege cleaning (Kalyan
Kumar Kalvagadda, Reviewed by: Alex Kolbasov)
Repository: sentry
Updated Branches:
refs/heads/master 4ccc45320 -> a913b9d0e
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/a913b9d0
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/a913b9d0
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/a913b9d0
Branch: refs/heads/master
Commit: a913b9d0ed66f2ebbc81712e88605a3b7fb4fa8c
Parents: 4ccc453
Author: Alexander Kolbasov <ak...@cloudera.com>
Authored: Thu Mar 23 17:52:12 2017 -0700
Committer: Alexander Kolbasov <ak...@cloudera.com>
Committed: Thu Mar 23 17:52:12 2017 -0700
----------------------------------------------------------------------
.../sentry/service/thrift/ServiceConstants.java | 2 -
.../service/persistent/DelegateSentryStore.java | 2 -
.../provider/db/service/model/MSentryRole.java | 12 +-
.../db/service/persistent/SentryStore.java | 388 ++++++++-----------
.../service/persistent/TestSentryRole.java | 172 ++++++++
.../db/service/persistent/TestSentryStore.java | 199 +++++++++-
6 files changed, 515 insertions(+), 260 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java b/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
index f9fb0f3..de97a31 100644
--- a/sentry-service/sentry-service-common/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
+++ b/sentry-service/sentry-service-common/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_HA_ENABLED = "sentry.ha.enabled";
public static final boolean SENTRY_HA_ENABLED_DEFAULT = false;
public static final String SENTRY_HA_ZK_PROPERTY_PREFIX = "sentry.ha.zookeeper.";
http://git-wip-us.apache.org/repos/asf/sentry/blob/a913b9d0/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 fc84e23..3be6a0b 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
@@ -64,8 +64,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/a913b9d0/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
index f1d7a86..f9da589 100644
--- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
+++ b/sentry-service/sentry-service-server/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/a913b9d0/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 32637d2..f0d2a6b 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
@@ -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;
@@ -120,8 +117,6 @@ public class SentryStore {
private final PersistenceManagerFactory pmf;
private Configuration conf;
- private PrivCleaner privCleaner = null;
- private Thread privCleanerThread = null;
private final TransactionManager tm;
public SentryStore(Configuration conf) throws Exception {
@@ -200,15 +195,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() {
@@ -232,14 +218,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();
}
@@ -487,12 +465,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 {
@@ -584,41 +560,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) {
@@ -634,7 +628,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
@@ -643,10 +638,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);
+ }
}
}
}
@@ -805,14 +803,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);
+ }
}
-
public void alterSentryRoleAddGroups(final String grantorPrincipal,
final String roleName, final Set<TSentryGroup> groupNames) throws Exception {
tm.executeTransactionWithRetry(
@@ -953,6 +971,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;
@@ -1661,7 +1726,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),
@@ -1897,195 +1967,45 @@ 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.
+ * 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>
*/
- 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
- private int currentNotifies = 0;
-
- // 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());
- }
- }
- }
-
- /**
- * 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/a913b9d0/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
index 29134fe..9be4a8b 100644
--- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java
+++ b/sentry-service/sentry-service-server/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/a913b9d0/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
index 20ce392..3327e68 100644
--- a/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
+++ b/sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
@@ -24,6 +24,7 @@ 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;
@@ -49,7 +50,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;
@@ -66,7 +66,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'};
@@ -509,21 +509,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());
}
/**
@@ -532,7 +523,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";
@@ -556,7 +547,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.
@@ -564,6 +555,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);
}
/**
@@ -574,7 +568,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";
@@ -612,8 +606,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";