Repository: sentry Updated Branches: refs/heads/master c4b057310 -> f4a9ad51f
SENTRY-1184: Clean up HMSPaths.renameAuthzObject ( Sravya Tirukkovalur, Reviewed by: Hao Hao) Change-Id: I551a7310d077cfd7b338f7676bffeb89cf331aab Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/f4a9ad51 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/f4a9ad51 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/f4a9ad51 Branch: refs/heads/master Commit: f4a9ad51f5056696f7b14d821b30b3613a66af25 Parents: c4b0573 Author: Sravya Tirukkovalur <[email protected]> Authored: Wed Apr 6 12:06:46 2016 -0700 Committer: Sravya Tirukkovalur <[email protected]> Committed: Tue Apr 12 19:41:52 2016 -0700 ---------------------------------------------------------------------- .../java/org/apache/sentry/hdfs/HMSPaths.java | 90 +++++++++++++------- .../sentry/hdfs/UpdateableAuthzPaths.java | 13 +-- .../org/apache/sentry/hdfs/TestHMSPaths.java | 45 +++++++++- .../sentry/hdfs/TestUpdateableAuthzPaths.java | 13 ++- .../tests/e2e/hdfs/TestHDFSIntegration.java | 25 +++++- 5 files changed, 140 insertions(+), 46 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/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 ceb1da8..80a25aa 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 @@ -260,6 +260,9 @@ public class HMSPaths implements AuthzPaths { if (prefix != null) { // we only create the entry if is under a prefix, else we ignore it entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj); + } else { + LOG.info("Skipping to create authzObjPath as it is outside of prefix. authObj = " + authzObj + + " pathElements=" + pathElements); } return entry; } @@ -290,6 +293,7 @@ public class HMSPaths implements AuthzPaths { } } + public void delete() { if (getParent() != null) { if (getChildren().isEmpty()) { @@ -469,15 +473,17 @@ public class HMSPaths implements AuthzPaths { if (e != null) { newEntries.add(e); } else { - LOG.warn("Ignoring path, no prefix"); + LOG.info("Path outside prefix"); } } entries.addAll(newEntries); } else { if (createNew) { addAuthzObject(authzObj, authzObjPathElements); + } else { + LOG.warn("Path was not added to AuthzObject, could not find key in authzObjToPath. authzObj = " + authzObj + + " authzObjPathElements=" + authzObjPathElements); } - LOG.warn("Object does not exist"); } } @@ -489,6 +495,11 @@ public class HMSPaths implements AuthzPaths { addPathsToAuthzObject(authzObj, authzObjPaths, false); } + /* + 1. Removes authzObj from all entries corresponding to the authzObjPathElements + ( which also deletes the entry if no more authObjs to that path and does it recursively upwards) + 2. Removes it from value of authzObjToPath Map for this authzObj key, does not reset entries to null even if entries is empty + */ void deletePathsFromAuthzObject(String authzObj, List<List<String>> authzObjPathElements) { Set<Entry> entries = authzObjToPath.get(authzObj); @@ -501,12 +512,13 @@ public class HMSPaths implements AuthzPaths { entry.deleteAuthzObject(authzObj); toDelEntries.add(entry); } else { - LOG.warn("Ignoring path, it was not registered"); + LOG.info("Path was not deleted from AuthzObject, path not registered. This is possible for implicit partition locations. authzObj = " + authzObj + " authzObjPathElements=" + authzObjPathElements); } } entries.removeAll(toDelEntries); } else { - LOG.warn("Object does not exist"); + LOG.info("Path was not deleted from AuthzObject, could not find key in authzObjToPath. authzObj = " + authzObj + + " authzObjPathElements=" + authzObjPathElements); } } @@ -548,41 +560,59 @@ public class HMSPaths implements AuthzPaths { return (entry != null) ? entry.getAuthzObjs() : null; } - boolean renameAuthzObject(String oldName, List<String> oldPathElems, - String newName, List<String> newPathElems) { - // Handle '/' - if (oldPathElems == null || oldPathElems.size() == 0) { - return false; - } - Entry entry = - root.find(oldPathElems.toArray(new String[oldPathElems.size()]), false); - if (entry != null && entry.getAuthzObjs().contains(oldName)) { - // Update pathElements - String[] newPath = newPathElems.toArray(new String[newPathElems.size()]); - // Can't use Lists.newArrayList() because of whacky generics - List<List<String>> pathElemsAsList = new LinkedList<List<String>>(); - pathElemsAsList.add(oldPathElems); - deletePathsFromAuthzObject(oldName, pathElemsAsList); - if (isUnderPrefix(newPath)) { - // Can't use Lists.newArrayList() because of whacky generics - pathElemsAsList = new LinkedList<List<String>>(); - pathElemsAsList.add(newPathElems); - addPathsToAuthzObject(oldName, pathElemsAsList); + /* + Following condition should be true: oldName != newName + If oldPath == newPath, Example: rename external table (only HMS meta data is updated) + => new_table.add(new_path), new_table.add(old_table_partition_paths), old_table.dropAllPaths. + If oldPath != newPath, Example: rename managed table (HMS metadata is updated as well as physical files are moved to new location) + => new_table.add(new_path), old_table.dropAllPaths. + */ + 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); + 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) + 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; + } } - // This would be true only for table rename - if (!oldName.equals(newName)) { - Set<Entry> eSet = authzObjToPath.get(oldName); + } + 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.removeAuthzObj(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); } } - authzObjToPath.remove(oldName); } } - return true; + + //old_table.dropAllPaths + deleteAuthzObject(oldName); } @Override http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java index 8fc5470..5ff7294 100644 --- a/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java +++ b/sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java @@ -61,7 +61,7 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate> @Override public UpdateableAuthzPaths updateFull(PathsUpdate update) { UpdateableAuthzPaths other = getPathsDump().initializeFromDump( - update.toThrift().getPathsDump()); + update.toThrift().getPathsDump()); other.seqNum.set(update.getSeqNum()); return other; } @@ -87,8 +87,8 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate> } private void applyPartialUpdate(PathsUpdate update) { - // Handle alter table rename : will have exactly 2 patch changes - // 1 an add path and the other a del path + // Handle alter table rename : will have exactly 2 path changes + // 1 is add path and the other is del path and oldName != newName if (update.getPathChanges().size() == 2) { List<TPathChanges> pathChanges = update.getPathChanges(); TPathChanges newPathInfo = null; @@ -102,10 +102,11 @@ public class UpdateableAuthzPaths implements AuthzPaths, Updateable<PathsUpdate> newPathInfo = pathChanges.get(1); oldPathInfo = pathChanges.get(0); } - if (newPathInfo != null && oldPathInfo != null) { + if (newPathInfo != null && oldPathInfo != null && + !newPathInfo.getAuthzObj().equalsIgnoreCase(oldPathInfo.getAuthzObj())) { paths.renameAuthzObject( - oldPathInfo.getAuthzObj(), oldPathInfo.getDelPaths().get(0), - newPathInfo.getAuthzObj(), newPathInfo.getAddPaths().get(0)); + oldPathInfo.getAuthzObj(), oldPathInfo.getDelPaths(), + newPathInfo.getAuthzObj(), newPathInfo.getAddPaths()); return; } } http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/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 bb74779..0dee102 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 @@ -17,7 +17,7 @@ */ package org.apache.sentry.hdfs; -import java.util.List; +import java.util.*; import org.apache.hadoop.fs.Path; import org.junit.Assert; @@ -353,6 +353,49 @@ public class TestHMSPaths { Assert.assertEquals(prefix, root.findPrefixEntry( Lists.newArrayList("a", "b", "t", "p3"))); } + @Test + public void testRenameDiffPaths() { + String[] prefixes = {"/user/hive/warehouse"}; + HMSPaths paths = new HMSPaths(prefixes); + //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))); + + //Create new table location + String table2Path = "/user/hive/warehouse/db2.db/table2"; + paths.renameAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(table1Path)), + "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 that new path is associated with new table + Set<String> expectedSet = new HashSet<String>(); + expectedSet.add("db2.table2"); + Assert.assertEquals(expectedSet, paths.findAuthzObject(HMSPaths.getPathElements(table2Path).toArray(new String[0]))); + + } + + @Test + public void testRenameSamePaths() { + String[] prefixes = {"/user/hive/warehouse"}; + HMSPaths paths = new HMSPaths(prefixes); + //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))); + + //Create new table + paths.renameAuthzObject("db1.table1", HMSPaths.getPathsElements(Arrays.asList(tablePath)), + "db2.table2", HMSPaths.getPathsElements(Arrays.asList(tablePath))); + + //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]))); + } @Test public void testAuthzObjCaseInsensitive() { http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java index a5bc313..909ff3a 100644 --- a/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java +++ b/sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java @@ -17,17 +17,15 @@ */ package org.apache.sentry.hdfs; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; - import java.util.concurrent.locks.ReentrantReadWriteLock; import org.apache.sentry.hdfs.service.thrift.TPathChanges; -import static org.junit.Assert.assertTrue; import org.junit.Test; import com.google.common.collect.Lists; +import static org.junit.Assert.*; + public class TestUpdateableAuthzPaths { @Test @@ -105,10 +103,9 @@ public class TestUpdateableAuthzPaths { // Verify name change assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1"}).contains("db1")); assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl11"}).contains("db1.xtbl11")); - // Explicit set location has to be done on the partition else it will be associated to - // the old location - assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part111"}).contains("db1.xtbl11")); - assertTrue(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part112"}).contains("db1.xtbl11")); + // When both name and location are changed, old paths should not contain either new or old table name + assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part111"})); + assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "tbl11", "part112"})); // Verify other tables are not touched assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl12"})); assertNull(authzPaths.findAuthzObjectExactMatches(new String[]{"db1", "xtbl12", "part121"})); http://git-wip-us.apache.org/repos/asf/sentry/blob/f4a9ad51/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java index 4799d36..d726176 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java @@ -621,10 +621,14 @@ public class TestHDFSIntegration { stmt.execute("revoke select on table p1 from role p1_admin"); verifyOnAllSubDirs("/user/hive/warehouse/p1", FsAction.WRITE_EXECUTE, "hbase", true); - // Verify table rename works + + // Verify table rename works when locations are also changed stmt.execute("alter table p1 rename to p3"); verifyOnAllSubDirs("/user/hive/warehouse/p3", FsAction.WRITE_EXECUTE, "hbase", true); + //This is true as parent hive object's (p3) ACLS are used. + verifyOnAllSubDirs("/user/hive/warehouse/p3/month=1/day=1", FsAction.WRITE_EXECUTE, "hbase", true); + // Verify when oldName == newName and oldPath != newPath stmt.execute("alter table p3 partition (month=1, day=1) rename to partition (month=1, day=3)"); verifyOnAllSubDirs("/user/hive/warehouse/p3", FsAction.WRITE_EXECUTE, "hbase", true); verifyOnAllSubDirs("/user/hive/warehouse/p3/month=1/day=3", FsAction.WRITE_EXECUTE, "hbase", true); @@ -816,6 +820,24 @@ public class TestHDFSIntegration { verifyOnPath("/tmp/external/tables/ext2_after/i=2/stuff.txt", FsAction.ALL, "hbase", true); // END : Verify external table set location.. + //Create a new table partition on the existing partition + stmt.execute("create table tmp (s string) partitioned by (i int)"); + stmt.execute("alter table tmp add partition (i=1)"); + stmt.execute("alter table tmp partition (i=1) set location \'hdfs:///tmp/external/tables/ext2_after/i=1\'"); + stmt.execute("grant all on table tmp to role tab_role"); + verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "flume", true); + + //Alter table rename of external table => oldName != newName, oldPath == newPath + stmt.execute("alter table ext2 rename to ext3"); + //Verify all original paths still have the privileges + verifyOnPath("/tmp/external/tables/ext2_after", FsAction.ALL, "hbase", true); + verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "hbase", true); + verifyOnPath("/tmp/external/tables/ext2_after/i=1", FsAction.ALL, "flume", true); + verifyOnPath("/tmp/external/tables/ext2_after/i=2", FsAction.ALL, "hbase", true); + verifyOnPath("/tmp/external/tables/ext2_after/i=1/stuff.txt", FsAction.ALL, "hbase", true); + verifyOnPath("/tmp/external/tables/ext2_after/i=2/stuff.txt", FsAction.ALL, "hbase", true); + + // Restart HDFS to verify if things are fine after re-start.. // TODO : this is currently commented out since miniDFS.restartNameNode() does @@ -889,6 +911,7 @@ public class TestHDFSIntegration { //Partition on local file system stmt.execute("alter table tab2 add partition (i=1)"); stmt.execute("alter table tab2 partition (i=1) set location 'file:///tmp/external/tab2_loc/i=1'"); + verifyOnAllSubDirs("/tmp/external/tab2_loc/i=1", null, StaticUserGroup.USERGROUP1, false); //HDFS to local file system, also make sure does not specifying scheme still works
