You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by ls...@apache.org on 2014/12/06 09:19:31 UTC
incubator-sentry git commit: SENTRY-547: Drop table may deadlock,
getChildPrivileges should be in one transaction with revoke (Xiaomeng
Huang via Lenni Kuff)
Repository: incubator-sentry
Updated Branches:
refs/heads/master eb53de39e -> ea8663e98
SENTRY-547: Drop table may deadlock, getChildPrivileges should be in one transaction with revoke (Xiaomeng Huang via Lenni Kuff)
Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/ea8663e9
Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/ea8663e9
Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/ea8663e9
Branch: refs/heads/master
Commit: ea8663e98569d3919888ae8054ab1df0c7ae0423
Parents: eb53de3
Author: Lenni Kuff <ls...@cloudera.com>
Authored: Sat Dec 6 00:18:04 2014 -0800
Committer: Lenni Kuff <ls...@cloudera.com>
Committed: Sat Dec 6 00:18:33 2014 -0800
----------------------------------------------------------------------
.../db/service/persistent/SentryStore.java | 108 +++++++++----------
.../db/service/persistent/TestSentryStore.java | 40 +++++++
2 files changed, 90 insertions(+), 58 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/ea8663e9/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 7d2cb12..83ac3be 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
@@ -69,6 +69,8 @@ import org.apache.sentry.provider.db.service.thrift.TSentryRole;
import org.apache.sentry.service.thrift.ServiceConstants.PrivilegeScope;
import org.apache.sentry.service.thrift.ServiceConstants.ServerConfig;
import org.datanucleus.store.rdbms.exceptions.MissingTableException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import com.codahale.metrics.Gauge;
import com.google.common.annotations.VisibleForTesting;
@@ -79,8 +81,6 @@ import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* SentryStore is the data access object for Sentry data. Strings
@@ -504,7 +504,7 @@ public class SentryStore {
privilegeGraph.add(mFalse);
}
// Get the privilege graph
- populateChildren(Sets.newHashSet(roleName), mPrivilege, privilegeGraph);
+ populateChildren(pm, Sets.newHashSet(roleName), mPrivilege, privilegeGraph);
for (MSentryPrivilege childPriv : privilegeGraph) {
revokePartial(pm, tPrivilege, mRole, childPriv);
}
@@ -567,19 +567,21 @@ public class SentryStore {
/**
- * Explore Privilege graph and collect child privileges
+ * Explore Privilege graph and collect child privileges.
+ * The responsibility to commit/rollback the transaction should be handled by the caller.
*/
- private void populateChildren(Set<String> roleNames, MSentryPrivilege priv,
+ private void populateChildren(PersistenceManager pm, Set<String> roleNames, MSentryPrivilege priv,
Set<MSentryPrivilege> children) throws SentryInvalidInputException {
+ Preconditions.checkNotNull(pm);
if ((!isNULL(priv.getServerName())) || (!isNULL(priv.getDbName()))
|| (!isNULL(priv.getTableName()))) {
// Get all TableLevel Privs
- Set<MSentryPrivilege> childPrivs = getChildPrivileges(roleNames, priv);
+ Set<MSentryPrivilege> childPrivs = getChildPrivileges(pm, roleNames, priv);
for (MSentryPrivilege childPriv : childPrivs) {
// Only recurse for table level privs..
if ((!isNULL(childPriv.getDbName())) && (!isNULL(childPriv.getTableName()))
&& (!isNULL(childPriv.getColumnName()))) {
- populateChildren(roleNames, childPriv, children);
+ populateChildren(pm, roleNames, childPriv, children);
}
// The method getChildPrivileges() didn't do filter on "action",
// if the action is not "All", it should judge the action of children privilege.
@@ -604,61 +606,51 @@ public class SentryStore {
}
}
- private Set<MSentryPrivilege> getChildPrivileges(Set<String> roleNames,
+ private Set<MSentryPrivilege> getChildPrivileges(PersistenceManager pm, Set<String> roleNames,
MSentryPrivilege parent) throws SentryInvalidInputException {
// Column and URI do not have children
- if ((!isNULL(parent.getColumnName()))||(!isNULL(parent.getURI()))) return new HashSet<MSentryPrivilege>();
- boolean rollbackTransaction = true;
- PersistenceManager pm = null;
- try {
- pm = openTransaction();
- Query query = pm.newQuery(MSentryPrivilege.class);
- query
- .declareVariables("org.apache.sentry.provider.db.service.model.MSentryRole role");
- List<String> rolesFiler = new LinkedList<String>();
- for (String rName : roleNames) {
- rolesFiler.add("role.roleName == \"" + rName.trim().toLowerCase() + "\"");
- }
- StringBuilder filters = new StringBuilder("roles.contains(role) "
- + "&& (" + Joiner.on(" || ").join(rolesFiler) + ")");
- filters.append(" && serverName == \"" + parent.getServerName() + "\"");
- if (!isNULL(parent.getDbName())) {
- filters.append(" && dbName == \"" + parent.getDbName() + "\"");
- if (!isNULL(parent.getTableName())) {
- filters.append(" && tableName == \"" + parent.getTableName() + "\"");
- filters.append(" && columnName != \"__NULL__\"");
- } else {
- filters.append(" && tableName != \"__NULL__\"");
- }
+ if ((!isNULL(parent.getColumnName())) || (!isNULL(parent.getURI()))) {
+ return new HashSet<MSentryPrivilege>();
+ }
+
+ Query query = pm.newQuery(MSentryPrivilege.class);
+ query.declareVariables("org.apache.sentry.provider.db.service.model.MSentryRole role");
+ List<String> rolesFiler = new LinkedList<String>();
+ for (String rName : roleNames) {
+ rolesFiler.add("role.roleName == \"" + rName.trim().toLowerCase() + "\"");
+ }
+ StringBuilder filters = new StringBuilder("roles.contains(role) "
+ + "&& (" + Joiner.on(" || ").join(rolesFiler) + ")");
+ filters.append(" && serverName == \"" + parent.getServerName() + "\"");
+ if (!isNULL(parent.getDbName())) {
+ filters.append(" && dbName == \"" + parent.getDbName() + "\"");
+ if (!isNULL(parent.getTableName())) {
+ filters.append(" && tableName == \"" + parent.getTableName() + "\"");
+ filters.append(" && columnName != \"__NULL__\"");
} else {
- filters.append(" && (dbName != \"__NULL__\" || URI != \"__NULL__\")");
+ filters.append(" && tableName != \"__NULL__\"");
}
+ } else {
+ filters.append(" && (dbName != \"__NULL__\" || URI != \"__NULL__\")");
+ }
- query.setFilter(filters.toString());
- query
- .setResult("privilegeScope, serverName, dbName, tableName, columnName," +
- " URI, action, grantOption");
- Set<MSentryPrivilege> privileges = new HashSet<MSentryPrivilege>();
- for (Object[] privObj : (List<Object[]>) query.execute()) {
- MSentryPrivilege priv = new MSentryPrivilege();
- priv.setPrivilegeScope((String) privObj[0]);
- priv.setServerName((String) privObj[1]);
- priv.setDbName((String) privObj[2]);
- priv.setTableName((String) privObj[3]);
- priv.setColumnName((String) privObj[4]);
- priv.setURI((String) privObj[5]);
- priv.setAction((String) privObj[6]);
- priv.setGrantOption((Boolean) privObj[7]);
- privileges.add(priv);
- }
- rollbackTransaction = false;
- commitTransaction(pm);
- return privileges;
- } finally {
- if (rollbackTransaction) {
- rollbackTransaction(pm);
- }
+ query.setFilter(filters.toString());
+ query.setResult("privilegeScope, serverName, dbName, tableName, columnName," +
+ " URI, action, grantOption");
+ Set<MSentryPrivilege> privileges = new HashSet<MSentryPrivilege>();
+ for (Object[] privObj : (List<Object[]>) query.execute()) {
+ MSentryPrivilege priv = new MSentryPrivilege();
+ priv.setPrivilegeScope((String) privObj[0]);
+ priv.setServerName((String) privObj[1]);
+ priv.setDbName((String) privObj[2]);
+ priv.setTableName((String) privObj[3]);
+ priv.setColumnName((String) privObj[4]);
+ priv.setURI((String) privObj[5]);
+ priv.setAction((String) privObj[6]);
+ priv.setGrantOption((Boolean) privObj[7]);
+ privileges.add(priv);
}
+ return privileges;
}
private List<MSentryPrivilege> getMSentryPrivileges(TSentryPrivilege tPriv, PersistenceManager pm) {
@@ -1543,9 +1535,9 @@ public class SentryStore {
Set<MSentryPrivilege> privilegeGraph = Sets.newHashSet();
if (parent != null) {
privilegeGraph.add(parent);
- populateChildren(Sets.newHashSet(role.getRoleName()), parent, privilegeGraph);
+ populateChildren(pm, Sets.newHashSet(role.getRoleName()), parent, privilegeGraph);
} else {
- populateChildren(Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege),
+ populateChildren(pm, Sets.newHashSet(role.getRoleName()), convertToMSentryPrivilege(tPrivilege),
privilegeGraph);
}
// 2. revoke privilege and child privileges
http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/ea8663e9/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 9dca5fb..8ca1f83 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
@@ -1260,6 +1260,46 @@ public class TestSentryStore {
.size());
}
+ /**
+ * Regression test for SENTRY-547
+ * Use case:
+ * GRANT INSERT on TABLE tbl1 to ROLE role1
+ * GRANT SELECT on TABLE tbl1 to ROLE role1
+ * DROP TABLE tbl1
+ *
+ * After drop tbl1, role1 should have 0 privileges
+ */
+ @Test
+ public void testDropTableWithMultiAction() throws Exception {
+ String roleName1 = "role1";
+ String grantor = "g1";
+ sentryStore.createSentryRole(roleName1);
+
+ TSentryPrivilege privilege_tbl1 = new TSentryPrivilege();
+ privilege_tbl1.setPrivilegeScope("TABLE");
+ privilege_tbl1.setServerName("server1");
+ privilege_tbl1.setDbName("db1");
+ privilege_tbl1.setTableName("tbl1");
+ privilege_tbl1.setCreateTime(System.currentTimeMillis());
+
+ TSentryPrivilege privilege_tbl1_insert = new TSentryPrivilege(
+ privilege_tbl1);
+ privilege_tbl1_insert.setAction(AccessConstants.INSERT);
+
+ TSentryPrivilege privilege_tbl1_select = new TSentryPrivilege(
+ privilege_tbl1);
+ privilege_tbl1_select.setAction(AccessConstants.SELECT);
+
+ sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName1, privilege_tbl1_insert);
+ sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName1, privilege_tbl1_select);
+
+ assertEquals(2, sentryStore.getAllTSentryPrivilegesByRoleName(roleName1).size());
+
+ // after drop privilege_tbl1, role1 should have 0 privileges
+ sentryStore.dropPrivilege(toTSentryAuthorizable(privilege_tbl1));
+ assertEquals(0, sentryStore.getAllTSentryPrivilegesByRoleName(roleName1).size());
+ }
+
@Test
public void testDropTableWithColumn() throws Exception {
String roleName1 = "role1", roleName2 = "role2";