sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sra...@apache.org
Subject sentry git commit: SENTRY-1184: Clean up HMSPaths.renameAuthzObject ( Sravya Tirukkovalur, Reviewed by: Hao Hao)
Date Wed, 13 Apr 2016 02:42:00 GMT
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 <sravya@apache.org>
Authored: Wed Apr 6 12:06:46 2016 -0700
Committer: Sravya Tirukkovalur <sravya@apache.org>
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


Mime
View raw message