sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ann...@apache.org
Subject sentry git commit: SENTRY-1471: TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly. Also disable testAuthzObjOnPartitionMultipleTables until it gets fixed. ( Vadim Spector, reviewed by Anne Yu)
Date Fri, 16 Sep 2016 04:01:32 GMT
Repository: sentry
Updated Branches:
  refs/heads/master ed965d871 -> 5acaa8ee2


SENTRY-1471: TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification
incorrectly. Also disable testAuthzObjOnPartitionMultipleTables until it gets fixed. ( Vadim
Spector, reviewed by Anne Yu)


Project: http://git-wip-us.apache.org/repos/asf/sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/5acaa8ee
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/5acaa8ee
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/5acaa8ee

Branch: refs/heads/master
Commit: 5acaa8ee265e8c83926013b208b786aa4083f5f5
Parents: ed965d8
Author: Anne Yu <anneyu@cloudera.com>
Authored: Thu Sep 15 21:40:14 2016 -0700
Committer: Anne Yu <anneyu@cloudera.com>
Committed: Thu Sep 15 21:40:14 2016 -0700

----------------------------------------------------------------------
 .../e2e/hdfs/TestHDFSIntegrationAdvanced.java   | 15 +++++---
 .../tests/e2e/hdfs/TestHDFSIntegrationBase.java | 37 +++++++++-----------
 2 files changed, 27 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/5acaa8ee/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 8a425c9..1b5eb53 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
@@ -29,6 +29,7 @@ import org.apache.sentry.hdfs.PathsUpdate;
 import org.apache.sentry.tests.e2e.hive.StaticUserGroup;
 
 import org.junit.Assert;
+import org.junit.Ignore;
 import org.junit.Test;
 
 import org.slf4j.Logger;
@@ -130,8 +131,9 @@ public class TestHDFSIntegrationAdvanced extends TestHDFSIntegrationBase
{
     //HDFS to local file system, also make sure does not specifying scheme still works
     stmt.execute("create external table tab3(a int) location '/tmp/external/tab3_loc'");
     // SENTRY-546
-    // verifyOnAllSubDirs("/tmp/external/tab3_loc", FsAction.ALL, StaticUserGroup.USERGROUP1,
true);
-    verifyOnAllSubDirs("/tmp/external/tab3_loc", null, StaticUserGroup.USERGROUP1, true);
+    // SENTRY-1471 - fixing the validation logic revealed that FsAction.ALL is the right
value.
+    verifyOnAllSubDirs("/tmp/external/tab3_loc", FsAction.ALL, StaticUserGroup.USERGROUP1,
true);
+    // verifyOnAllSubDirs("/tmp/external/tab3_loc", null, StaticUserGroup.USERGROUP1, true);
     stmt.execute("alter table tab3 set location 'file:///tmp/external/tab3_loc'");
     verifyOnAllSubDirs("/tmp/external/tab3_loc", null, StaticUserGroup.USERGROUP1, false);
 
@@ -140,8 +142,9 @@ public class TestHDFSIntegrationAdvanced extends TestHDFSIntegrationBase
{
     stmt.execute("alter table tab4 set location 'hdfs:///tmp/external/tab4_loc'");
     miniDFS.getFileSystem().mkdirs(new Path("/tmp/external/tab4_loc"));
     // SENTRY-546
-    // verifyOnAllSubDirs("/tmp/external/tab4_loc", FsAction.ALL, StaticUserGroup.USERGROUP1,
true);
-    verifyOnAllSubDirs("/tmp/external/tab4_loc", null, StaticUserGroup.USERGROUP1, true);
+    // SENTRY-1471 - fixing the validation logic revealed that FsAction.ALL is the right
value.
+    verifyOnAllSubDirs("/tmp/external/tab4_loc", FsAction.ALL, StaticUserGroup.USERGROUP1,
true);
+    // verifyOnAllSubDirs("/tmp/external/tab4_loc", null, StaticUserGroup.USERGROUP1, true);
     stmt.close();
     conn.close();
   }
@@ -557,6 +560,10 @@ public class TestHDFSIntegrationAdvanced extends TestHDFSIntegrationBase
{
   }
 
   /* SENTRY-953 */
+  /* SENTRY-1471 - fixing the validation logic revealed that this test is broken.
+   * Disabling this test for now; to be fixed in a separate JIRA.
+   */
+  @Ignore
   @Test
   public void testAuthzObjOnPartitionMultipleTables() throws Throwable {
     String dbName = "db1";

http://git-wip-us.apache.org/repos/asf/sentry/blob/5acaa8ee/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
----------------------------------------------------------------------
diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
index 0cf018a..f52f9f9 100644
--- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
+++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
@@ -205,14 +205,17 @@ public abstract class TestHDFSIntegrationBase {
   }
 
   protected void verifyOnAllSubDirs(Path p, FsAction fsAction, String group, boolean groupShouldExist,
boolean recurse, int retry) throws Throwable {
-    Assert.assertTrue("Failed to verify ACLs on path and its children: " + p.getName(),
-        verifyOnAllSubDirsHelper(p, fsAction, group, groupShouldExist, recurse, retry));
+    verifyOnAllSubDirsHelper(p, fsAction, group, groupShouldExist, recurse, retry);
   }
 
-  private boolean verifyOnAllSubDirsHelper(Path p, FsAction fsAction, String group,
+  /* SENTRY-1471 - fixing the validation logic.
+   * a) When the number of retries exceeds the limit, propagate the Assert exception to the
caller.
+   * b) Throw an exception instead of returning false, to pass valuable debugging info up
the stack
+   *    - expected vs. found permissions.
+   */
+  private void verifyOnAllSubDirsHelper(Path p, FsAction fsAction, String group,
                                            boolean groupShouldExist, boolean recurse, int
retry) throws Throwable {
     FileStatus fStatus = null;
-    boolean hasSucceeded = false;
     // validate parent dir's acls
     try {
       fStatus = miniDFS.getFileSystem().getFileStatus(p);
@@ -223,32 +226,22 @@ public abstract class TestHDFSIntegrationBase {
             " group : " + group + " ;", getAcls(p).containsKey(group));
       }
       LOGGER.info("Successfully found acls for path = " + p.getName());
-      hasSucceeded = true;
     } catch (Throwable th) {
       if (retry > 0) {
         LOGGER.info("Retry: " + retry);
         Thread.sleep(RETRY_WAIT);
-        hasSucceeded = verifyOnAllSubDirsHelper(p, fsAction, group, groupShouldExist, recurse,
retry - 1);
+        verifyOnAllSubDirsHelper(p, fsAction, group, groupShouldExist, recurse, retry - 1);
       } else {
-        LOGGER.info("Successfully found ACLs for path = " + p.getName());
-        hasSucceeded = true;
+        throw th;
       }
     }
-    if (!hasSucceeded) {
-      LOGGER.error("Failed to validate ACLs for path = " + p.getName());
-      return false;
-    }
     // validate children dirs
     if (recurse && fStatus.isDirectory()) {
       FileStatus[] children = miniDFS.getFileSystem().listStatus(p);
       for (FileStatus fs : children) {
-        if (!verifyOnAllSubDirsHelper(fs.getPath(), fsAction, group, groupShouldExist, recurse,
NUM_RETRIES)) {
-          LOGGER.error("Failed to validate ACLs for child path = " + fs.getPath().getName());
-          return false;
-        }
+        verifyOnAllSubDirsHelper(fs.getPath(), fsAction, group, groupShouldExist, recurse,
NUM_RETRIES);
       }
     }
-    return true;
   }
 
   protected Map<String, FsAction> getAcls(Path path) throws Exception {
@@ -296,25 +289,27 @@ public abstract class TestHDFSIntegrationBase {
     verifyQuery(stmt, table, n, NUM_RETRIES);
   }
 
+  /* SENTRY-1471 - fixing the validation logic.
+   * a) When the number of retries exceeds the limit, propagate the Assert exception to the
caller.
+   * b) Throw an exception immediately, instead of using boolean variable, to pass valuable
debugging
+   *    info up the stack - expected vs. found number of rows.
+   */
   protected void verifyQuery(Statement stmt, String table, int n, int retry) throws Throwable
{
     ResultSet rs = null;
-    boolean isSucceeded = false;
     try {
       rs = stmt.executeQuery("select * from " + table);
       int numRows = 0;
       while (rs.next()) { numRows++; }
       Assert.assertEquals(n, numRows);
-      isSucceeded = true;
     } catch (Throwable th) {
       if (retry > 0) {
         LOGGER.info("Retry: " + retry);
         Thread.sleep(RETRY_WAIT);
         verifyQuery(stmt, table, n, retry - 1);
       } else {
-        isSucceeded = true;
+        throw th;
       }
     }
-    Assert.assertTrue(isSucceeded);
   }
 
   protected void verifyAccessToPath(String user, String group, String path, boolean hasPermission)
throws Exception{


Mime
View raw message