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/04/04 22:49:13 UTC
sentry git commit: SENTRY-1644: Partition ACLs disappear after
renaming Hive table with partitions (Lei (Eddy) Xu, reviewed by Alex Kolbasov)
Repository: sentry
Updated Branches:
refs/heads/master 1992e5bde -> 0773053d6
SENTRY-1644: Partition ACLs disappear after renaming Hive table with partitions (Lei (Eddy) Xu, 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/0773053d
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/0773053d
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/0773053d
Branch: refs/heads/master
Commit: 0773053d6ff3549c450fcbee8871cc7cb145cca9
Parents: 1992e5b
Author: Alexander Kolbasov <ak...@cloudera.com>
Authored: Tue Apr 4 15:46:58 2017 -0700
Committer: Alexander Kolbasov <ak...@cloudera.com>
Committed: Tue Apr 4 15:49:07 2017 -0700
----------------------------------------------------------------------
.../java/org/apache/sentry/hdfs/HMSPaths.java | 217 +++++++++++++------
.../org/apache/sentry/hdfs/TestHMSPaths.java | 24 +-
.../e2e/hdfs/TestHDFSIntegrationAdvanced.java | 55 ++++-
3 files changed, 216 insertions(+), 80 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/sentry/blob/0773053d/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
index 6d2ab23..18c921d 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
@@ -104,6 +104,28 @@ public class HMSPaths implements AuthzPaths {
}
}
+ /**
+ * Entry represents a node in the tree that {@see HMSPaths} uses to organize the auth objects.
+ * This tree maps the entries in the filesystem namespace in HDFS, and the auth objects are
+ * associated to each entry.
+ *
+ * Each individual entry in the tree contains a children map that maps the path element
+ * (filename) to the child entry.
+ *
+ * For example, for a HDFS file or directory, "hdfs://foo/bar", it is presented in HMSPaths as the
+ * following tree, of which the root node is {@link HMSPaths#root}.
+ *
+ * Entry("/", children: {
+ * "foo": Entry("foo", children: {
+ * "bar": Entry("bar"),
+ * "zoo": Entry("zoo"),
+ * }),
+ * "tmp": Entry("tmp"),
+ * ...
+ * });
+ *
+ * Note that the URI scheme is not presented in the tree.
+ */
@VisibleForTesting
static class Entry {
private Entry parent;
@@ -112,40 +134,49 @@ public class HMSPaths implements AuthzPaths {
// A set of authorizable objects associated with this entry. Authorizable
// object should be case insensitive.
- private Set<String> authzObjs;
+ private Set<String> authzObjs = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
// Path of child element to the path entry mapping.
// e.g. 'b' -> '/a/b'
- private final Map<String, Entry> children;
+ private final Map<String, Entry> children = new HashMap<>();
- Entry(Entry parent, String pathElement, EntryType type,
- String authzObj) {
+ /**
+ * Construct an Entry with one authzObj.
+ *
+ * @param parent the parent node. If not specified, this entry is a root entry.
+ * @param pathElement the path element of this entry on the tree.
+ * @param type Entry type.
+ * @param authzObj the authzObj.
+ */
+ Entry(Entry parent, String pathElement, EntryType type, String authzObj) {
this.parent = parent;
this.type = type;
this.pathElement = pathElement;
- this.authzObjs = new TreeSet<String>(String.CASE_INSENSITIVE_ORDER);
addAuthzObj(authzObj);
- children = new HashMap<String, Entry>();
}
- Entry(Entry parent, String pathElement, EntryType type,
- Set<String> authzObjs) {
+ /**
+ * Construct an Entry with a set of authz objects.
+ * @param parent the parent node. If not specified, this entry is a root entry.
+ * @param pathElement the path element of this entry on the tree.
+ * @param type entry type.
+ * @param authzObjs a set of authz objects.
+ */
+ Entry(Entry parent, String pathElement, EntryType type, Set<String> authzObjs) {
this.parent = parent;
this.type = type;
this.pathElement = pathElement;
- this.authzObjs = new TreeSet<String>(String.CASE_INSENSITIVE_ORDER);
addAuthzObjs(authzObjs);
- children = new HashMap<String, Entry>();
}
// Get all the mapping of the children element to
// the path entries.
- public Map<String, Entry> getChildren() {
+ Map<String, Entry> getChildren() {
return children;
}
void clearAuthzObjs() {
- authzObjs = new HashSet<String>();
+ authzObjs = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
}
void removeAuthzObj(String authzObj) {
@@ -160,9 +191,7 @@ public class HMSPaths implements AuthzPaths {
void addAuthzObjs(Set<String> authzObjs) {
if (authzObjs != null) {
- for (String authObj : authzObjs) {
- this.authzObjs.add(authObj);
- }
+ this.authzObjs.addAll(authzObjs);
}
}
@@ -180,6 +209,33 @@ public class HMSPaths implements AuthzPaths {
}
/**
+ * Create all missing parent entries for an given entry, and return the parent entry for
+ * the entry.
+ *
+ * For example, if {@code pathElements} is ["a", "b", "c"], it creates entries "/a" and "/a/b"
+ * if they do not exist, and then returns "/a/b" as the parent entry.
+ *
+ * @param pathElements path elements of the entry.
+ * @return the direct parent entry of the given entry.
+ */
+ private Entry createParent(List<String> pathElements) {
+ Entry parent = this;
+ // The loop is resilient to 0 or 1 element list.
+ for (int i = 0; i < pathElements.size() - 1; i++) {
+ String elem = pathElements.get(i);
+ Entry child = parent.getChildren().get(elem);
+
+ if (child == null) {
+ child = new Entry(parent, elem, EntryType.DIR, (String) null);
+ parent.getChildren().put(elem, child);
+ }
+
+ parent = child;
+ }
+ return parent;
+ }
+
+ /**
* Create a child entry based on the path, type and authzObj that
* associates with it.
*
@@ -191,22 +247,8 @@ public class HMSPaths implements AuthzPaths {
private Entry createChild(List<String> pathElements, EntryType type,
String authzObj) {
- // Parent entry is the current referring one.
- Entry entryParent = this;
-
- // Creates the entry based on the path elements (if not found) until reaches its
- // direct parent.
- for (int i = 0; i < pathElements.size() - 1; i++) {
- String pathElement = pathElements.get(i);
- Entry child = entryParent.getChildren().get(pathElement);
-
- if (child == null) {
- child = new Entry(entryParent, pathElement, EntryType.DIR, (String) null);
- entryParent.getChildren().put(pathElement, child);
- }
-
- entryParent = child;
- }
+ // Create all the parent entries on the path if they do not exist.
+ Entry entryParent = createParent(pathElements);
String lastPathElement = pathElements.get(pathElements.size() - 1);
Entry child = entryParent.getChildren().get(lastPathElement);
@@ -266,6 +308,17 @@ public class HMSPaths implements AuthzPaths {
return entry;
}
+ /**
+ * Delete this entry from its parent.
+ */
+ private void deleteFromParent() {
+ if (getParent() != null) {
+ getParent().getChildren().remove(getPathElement());
+ getParent().deleteIfDangling();
+ parent = null;
+ }
+ }
+
public void deleteAuthzObject(String authzObj) {
if (getParent() != null) {
if (getChildren().isEmpty()) {
@@ -274,10 +327,8 @@ public class HMSPaths implements AuthzPaths {
// entry no longer maps to any authzObj, removes the
// entry recursively.
authzObjs.remove(authzObj);
- if (authzObjs.size() == 0) {
- getParent().getChildren().remove(getPathElement());
- getParent().deleteIfDangling();
- parent = null;
+ if (authzObjs.isEmpty()) {
+ deleteFromParent();
}
} else {
@@ -292,13 +343,32 @@ public class HMSPaths implements AuthzPaths {
}
}
+ /**
+ * Move this Entry under the new parent.
+ * @param newParent the new parent.
+ * @param pathElem the path element on the new path.
+ *
+ * @return true if success. Returns false if the target with the same name already exists.
+ */
+ private void moveTo(Entry newParent, String pathElem) {
+ Preconditions.checkNotNull(newParent);
+ Preconditions.checkArgument(!pathElem.isEmpty());
+ if (newParent.children.containsKey(pathElem)) {
+ LOG.warn(String.format(
+ "Attempt to move %s to %s: entry with the same name %s already exists",
+ this.getFullPath(), newParent.getFullPath(), pathElem));
+ return;
+ }
+ deleteFromParent();
+ parent = newParent;
+ parent.children.put(pathElem, this);
+ pathElement = pathElem;
+ }
public void delete() {
if (getParent() != null) {
if (getChildren().isEmpty()) {
- getParent().getChildren().remove(getPathElement());
- getParent().deleteIfDangling();
- parent = null;
+ deleteFromParent();
} else {
// if the entry was for an authz object and has children, we
// change it to be a dir entry.
@@ -530,6 +600,10 @@ public class HMSPaths implements AuthzPaths {
}
}
+ Set<String> findAuthzObject(List<String> pathElements) {
+ return findAuthzObject(pathElements.toArray(new String[0]));
+ }
+
@Override
public Set<String> findAuthzObject(String[] pathElements) {
return findAuthzObject(pathElements, true);
@@ -569,48 +643,49 @@ public class HMSPaths implements AuthzPaths {
void renameAuthzObject(String oldName, List<List<String>> oldPathElems,
String newName, List<List<String>> newPathElems) {
- if ( oldPathElems == null || oldPathElems.size() == 0 || newPathElems == null || newPathElems.size() == 0
- || newName == null || oldName == null || oldName == newName) {
- LOG.warn("Unexpected state in renameAuthzObject, inputs invalid: oldName=" + oldName + " newName=" + newName +
- " oldPath=" + oldPathElems + " newPath=" + newPathElems);
+ if (oldPathElems == null || oldPathElems.isEmpty() ||
+ newPathElems == null || newPathElems.isEmpty() ||
+ newName == null || newName.equals(oldName)) {
+ LOG.warn(String.format(
+ "Unexpected state in renameAuthzObject, inputs invalid: " +
+ "oldName=%s newName=%s oldPath=%s newPath=%s",
+ oldName, newName, oldPathElems, newPathElems));
return;
}
- //new_table.add(new_path)
- addPathsToAuthzObject(newName, newPathElems, true);
-
- //if oldPath == newPath, that is path has not changed as part of rename and hence new table needs to have old paths
- // => new_table.add(old_table_partition_paths)
+ // if oldPath == newPath, that is path has not changed as part of rename and hence new table
+ // needs to have old paths => new_table.add(old_table_partition_paths)
List<String> oldPathElements = oldPathElems.get(0);
List<String> newPathElements = newPathElems.get(0);
- boolean samePaths = false;
- if(oldPathElements.size() == newPathElements.size()) {
- samePaths = true;
- for(int i=0; i<oldPathElements.size() ; i++) {
- if (!newPathElements.get(i).equalsIgnoreCase(oldPathElements.get(i))) {
- samePaths = false;
- }
- }
- }
- if(samePaths) {
- Set<Entry> eSet = authzObjToPath.get(oldName);
- if (eSet == null) {
- LOG.warn("Unexpected state in renameAuthzObject, cannot find oldName in authzObjToPath: oldName=" + oldName + " newName=" + newName +
- " oldPath=" + oldPathElems + " newPath=" + newPathElems);
- } else {
- authzObjToPath.put(newName, eSet);
- for (Entry e : eSet) {
- if (e.getAuthzObjs().contains(oldName)) {
- e.addAuthzObj(newName);
- } else {
- LOG.warn("Unexpected state in renameAuthzObject, authzObjToPath has an entry <oldName,entries> where one of the entry does not have oldName : oldName=" + oldName + " newName=" + newName +
- " oldPath=" + oldPathElems + " newPath=" + newPathElems);
- }
+ if (!oldPathElements.equals(newPathElements)) {
+ Entry oldEntry = root.find(oldPathElements.toArray(new String[0]), false);
+ Entry newParent = root.createParent(newPathElements);
+ oldEntry.moveTo(newParent, newPathElements.get(newPathElements.size() - 1));
+ }
+
+ // Re-write authObj from oldName to newName.
+ Set<Entry> entries = authzObjToPath.get(oldName);
+ if (entries == null) {
+ LOG.warn("Unexpected state in renameAuthzObject, cannot find oldName in authzObjToPath: " +
+ "oldName=" + oldName + " newName=" + newName +
+ " oldPath=" + oldPathElems + " newPath=" + newPathElems);
+ } else {
+ authzObjToPath.put(newName, entries);
+ for (Entry e : entries) {
+ e.addAuthzObj(newName);
+
+ if (e.getAuthzObjs().contains(oldName)) {
+ e.removeAuthzObj(oldName);
+ } else {
+ LOG.warn("Unexpected state in renameAuthzObject, authzObjToPath has an " +
+ "entry <oldName,entries> where one of the entry does not have oldName : " +
+ "oldName=" + oldName + " newName=" + newName +
+ " oldPath=" + oldPathElems + " newPath=" + newPathElems);
}
}
}
- //old_table.dropAllPaths
+ // old_table.dropAllPaths
deleteAuthzObject(oldName);
}
http://git-wip-us.apache.org/repos/asf/sentry/blob/0773053d/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
----------------------------------------------------------------------
diff --git a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
index 0dee102..f31ec21 100644
--- a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
+++ b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
@@ -353,6 +353,7 @@ public class TestHMSPaths {
Assert.assertEquals(prefix, root.findPrefixEntry(
Lists.newArrayList("a", "b", "t", "p3")));
}
+
@Test
public void testRenameDiffPaths() {
String[] prefixes = {"/user/hive/warehouse"};
@@ -360,7 +361,8 @@ public class TestHMSPaths {
//Create old table and partition locations
String table1Path = "/user/hive/warehouse/db1.db/table1";
String partition1Path = "/user/hive/warehouse/db1.db/table1/part1";
- paths.addAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(table1Path, partition1Path)));
+ paths.addAuthzObject("db1.table1",
+ HMSPaths.getPathsElements(Arrays.asList(table1Path, partition1Path)));
//Create new table location
String table2Path = "/user/hive/warehouse/db2.db/table2";
@@ -368,13 +370,16 @@ public class TestHMSPaths {
"db2.table2", HMSPaths.getPathsElements(Arrays.asList(table2Path)));
//Assert that old path is not associated with a table
- Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(table1Path).toArray(new String[0])));
- Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(partition1Path).toArray(new String[0])));
+ Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(table1Path)));
+ Assert.assertEquals(null, paths.findAuthzObject(HMSPaths.getPathElements(partition1Path)));
//Assert that new path is associated with new table
- Set<String> expectedSet = new HashSet<String>();
+ Set<String> expectedSet = new HashSet<>();
expectedSet.add("db2.table2");
- Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(table2Path).toArray(new String[0])));
+ Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(table2Path)));
+ String partition2Path = "/user/hive/warehouse/db2.db/table2/part1";
+ Assert.assertEquals(expectedSet,
+ paths.findAuthzObject(HMSPaths.getPathElements(partition2Path)));
}
@Test
@@ -384,7 +389,8 @@ public class TestHMSPaths {
//Create table and partition locations
String tablePath = "/user/hive/warehouse/db1.db/table1";
String partitionPath = "/user/hive/warehouse/db1.db/table1/part1";
- paths.addAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(tablePath, partitionPath)));
+ paths.addAuthzObject("db1.table1",
+ HMSPaths.getPathsElements(Arrays.asList(tablePath, partitionPath)));
//Create new table
paths.renameAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(tablePath)),
@@ -393,8 +399,10 @@ public class TestHMSPaths {
//Assert that old path is associated with new table
Set<String> expectedSet = new HashSet<String>();
expectedSet.add("db2.table2");
- Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(tablePath).toArray(new String[0])));
- Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(partitionPath).toArray(new String[0])));
+ Assert.assertEquals(expectedSet,
+ paths.findAuthzObject(HMSPaths.getPathElements(tablePath)));
+ Assert.assertEquals(expectedSet,
+ paths.findAuthzObject(HMSPaths.getPathElements(partitionPath)));
}
@Test
http://git-wip-us.apache.org/repos/asf/sentry/blob/0773053d/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
----------------------------------------------------------------------
diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
index d079628..35cc7a6 100644
--- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
+++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java
@@ -16,7 +16,9 @@
*/
package org.apache.sentry.tests.e2e.hdfs;
+import java.io.File;
import java.net.URI;
+import java.nio.file.Paths;
import java.sql.Connection;
import java.sql.Statement;
@@ -37,6 +39,8 @@ import org.slf4j.LoggerFactory;
import org.apache.hadoop.hive.metastore.api.Table;
+import static org.junit.Assert.assertTrue;
+
/**
* Advanced tests for HDFS Sync integration
*/
@@ -601,7 +605,7 @@ public class TestHDFSIntegrationAdvanced extends TestHDFSIntegrationBase {
verifyOnPath(tmpHDFSDirStr, FsAction.WRITE_EXECUTE, StaticUserGroup.USERGROUP3, true);
// When alter the table name (tab2 to be tabx), ACLs should remain the same.
- stmt.execute("alter table tab2 rename to tabx");
+ assertTrue(stmt.execute("alter table tab2 rename to tabx"));
syncHdfs();//Wait till sentry cache is updated in Namenode
verifyOnPath(tmpHDFSDirStr, FsAction.READ_EXECUTE, StaticUserGroup.USERGROUP2, true);
verifyOnPath(tmpHDFSDirStr, FsAction.WRITE_EXECUTE, StaticUserGroup.USERGROUP3, true);
@@ -766,4 +770,53 @@ public class TestHDFSIntegrationAdvanced extends TestHDFSIntegrationBase {
syncHdfs();//Wait till sentry cache is updated in Namenode
verifyOnPath("/tmp/external", FsAction.ALL, StaticUserGroup.HIVE, true);
}
+
+ @Test
+ public void testRenameHivePartitions() throws Throwable {
+ final String dbName = "db1";
+ final String tblName = "tab1";
+ final String newTblName = "tab2";
+ final String patName = "pat1";
+ dbNames = new String[]{dbName};
+ roles = new String[]{"admin_role"};
+ admin = StaticUserGroup.ADMIN1;
+
+ try (Connection conn = hiveServer2.createConnection("hive", "hive");
+ Statement stmt = conn.createStatement()) {
+
+ stmt.execute("create role admin_role");
+ stmt.execute("grant all on server server1 to role admin_role");
+ stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP);
+ }
+
+ try (Connection conn = hiveServer2.createConnection(
+ StaticUserGroup.ADMIN1, StaticUserGroup.ADMINGROUP);
+ Statement stmt = conn.createStatement()) {
+ stmt.execute("create database " + dbName);
+ stmt.execute("use " + dbName);
+ stmt.execute("create table " + tblName + " (s string) partitioned by (month int) ");
+ String tblPath = Paths.get("/user/hive/warehouse", dbName + ".db", tblName).toString();
+ String patPath = Paths.get(tblPath, patName).toString();
+ stmt.execute("alter table " + tblName + " add partition (month = 1) location '" +
+ patPath + "'");
+
+ stmt.execute("grant all on TABLE " + tblName + " to role admin_role");
+ stmt.execute("create role user_role");
+ stmt.execute("grant insert on table " + tblName + " to role user_role");
+ stmt.execute("grant role user_role to group " + StaticUserGroup.USERGROUP1);
+ syncHdfs();
+
+ // Rename the hive table
+ stmt.execute("alter table " + tblName + " rename to " + newTblName);
+ syncHdfs();
+
+ // Verify that the permissions are preserved.
+ String newTblPath = Paths.get("/user/hive/warehouse", dbName + ".db", newTblName).toString();
+ verifyOnAllSubDirs(newTblPath, FsAction.ALL, StaticUserGroup.HIVE, true);
+ verifyOnAllSubDirs(newTblPath, FsAction.WRITE_EXECUTE, StaticUserGroup.USERGROUP1, true);
+ String newPatPath = new File(newTblPath, patName).toString();
+ verifyOnPath(newPatPath, FsAction.ALL, StaticUserGroup.ADMINGROUP, true);
+ verifyOnPath(newPatPath, FsAction.WRITE_EXECUTE, StaticUserGroup.USERGROUP1, true);
+ }
+ }
}