This is an automated email from the ASF dual-hosted git repository.

vjasani pushed a commit to branch branch-3
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-3 by this push:
     new f3717d482af HBASE-28081 Snapshot working dir does not retain ACLs 
after snapshot commit phase (#5437)
f3717d482af is described below

commit f3717d482af612b93d7a37554a92fc28d24d7ead
Author: Viraj Jasani <vjas...@apache.org>
AuthorDate: Sat Sep 30 10:39:26 2023 -0800

    HBASE-28081 Snapshot working dir does not retain ACLs after snapshot commit 
phase (#5437)
    
    Signed-off-by: Duo Zhang <zhang...@apache.org>
    Signed-off-by: Andrew Purtell <apurt...@apache.org>
    Signed-off-by: Aman Poonia <aman.poonia...@gmail.com>
---
 .../hbase/master/snapshot/SnapshotManager.java     | 40 ++++++++++++++++++++++
 .../access/SnapshotScannerHDFSAclHelper.java       | 14 ++++----
 .../TestSnapshotScannerHDFSAclController.java      | 13 +++----
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git 
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
 
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
index 3c421dd8bd0..ed7ef583ec5 100644
--- 
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
+++ 
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
@@ -38,10 +38,13 @@ import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.stream.Collectors;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.CommonPathCapabilities;
 import org.apache.hadoop.fs.FSDataInputStream;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.permission.AclEntry;
+import org.apache.hadoop.fs.permission.AclStatus;
 import org.apache.hadoop.hbase.HBaseInterfaceAudience;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.ServerName;
@@ -564,6 +567,7 @@ public class SnapshotManager extends MasterProcedureManager 
implements Stoppable
           "Couldn't create working directory (" + workingDir + ") for 
snapshot",
           ProtobufUtil.createSnapshotDesc(snapshot));
       }
+      updateWorkingDirAclsIfRequired(workingDir, workingDirFS);
     } catch (HBaseSnapshotException e) {
       throw e;
     } catch (IOException e) {
@@ -573,6 +577,42 @@ public class SnapshotManager extends 
MasterProcedureManager implements Stoppable
     }
   }
 
+  /**
+   * If the parent dir of the snapshot working dir (e.g. 
/hbase/.hbase-snapshot) has non-empty ACLs,
+   * use them for the current working dir (e.g. 
/hbase/.hbase-snapshot/.tmp/{snapshot-name}) so that
+   * regardless of whether the snapshot commit phase performs atomic rename or 
non-atomic copy of
+   * the working dir to new snapshot dir, the ACLs are retained.
+   * @param workingDir   working dir to build the snapshot.
+   * @param workingDirFS working dir file system.
+   * @throws IOException If ACL read/modify operation fails.
+   */
+  private static void updateWorkingDirAclsIfRequired(Path workingDir, 
FileSystem workingDirFS)
+    throws IOException {
+    if (
+      !workingDirFS.hasPathCapability(workingDir, 
CommonPathCapabilities.FS_ACLS)
+        || workingDir.getParent() == null || 
workingDir.getParent().getParent() == null
+    ) {
+      return;
+    }
+    AclStatus snapshotWorkingParentDirStatus;
+    try {
+      snapshotWorkingParentDirStatus =
+        workingDirFS.getAclStatus(workingDir.getParent().getParent());
+    } catch (IOException e) {
+      LOG.warn("Unable to retrieve ACL status for path: {}, current working 
dir path: {}",
+        workingDir.getParent().getParent(), workingDir, e);
+      return;
+    }
+    List<AclEntry> snapshotWorkingParentDirAclStatusEntries =
+      snapshotWorkingParentDirStatus.getEntries();
+    if (
+      snapshotWorkingParentDirAclStatusEntries != null
+        && snapshotWorkingParentDirAclStatusEntries.size() > 0
+    ) {
+      workingDirFS.modifyAclEntries(workingDir, 
snapshotWorkingParentDirAclStatusEntries);
+    }
+  }
+
   /**
    * Take a snapshot of a disabled table.
    * @param snapshot description of the snapshot to take. Modified to be 
{@link Type#DISABLED}.
diff --git 
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
 
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
index 41f61a6efa3..0fd41e4748d 100644
--- 
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
+++ 
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
@@ -152,11 +152,12 @@ public class SnapshotScannerHDFSAclHelper implements 
Closeable {
       long start = EnvironmentEdgeManager.currentTime();
       handleGrantOrRevokeAcl(userPermission, 
HDFSAclOperation.OperationType.MODIFY, skipNamespaces,
         skipTables);
-      LOG.info("Set HDFS acl when grant {}, cost {} ms", userPermission,
-        EnvironmentEdgeManager.currentTime() - start);
+      LOG.info("Set HDFS acl when grant {}, skipNamespaces: {}, skipTables: 
{}, cost {} ms",
+        userPermission, skipNamespaces, skipTables, 
EnvironmentEdgeManager.currentTime() - start);
       return true;
     } catch (Exception e) {
-      LOG.error("Set HDFS acl error when grant: {}", userPermission, e);
+      LOG.error("Set HDFS acl error when grant: {}, skipNamespaces: {}, 
skipTables: {}",
+        userPermission, skipNamespaces, skipTables, e);
       return false;
     }
   }
@@ -174,11 +175,12 @@ public class SnapshotScannerHDFSAclHelper implements 
Closeable {
       long start = EnvironmentEdgeManager.currentTime();
       handleGrantOrRevokeAcl(userPermission, 
HDFSAclOperation.OperationType.REMOVE, skipNamespaces,
         skipTables);
-      LOG.info("Set HDFS acl when revoke {}, cost {} ms", userPermission,
-        EnvironmentEdgeManager.currentTime() - start);
+      LOG.info("Set HDFS acl when revoke {}, skipNamespaces: {}, skipTables: 
{}, cost {} ms",
+        userPermission, skipNamespaces, skipTables, 
EnvironmentEdgeManager.currentTime() - start);
       return true;
     } catch (Exception e) {
-      LOG.error("Set HDFS acl error when revoke: {}", userPermission, e);
+      LOG.error("Set HDFS acl error when revoke: {}, skipNamespaces: {}, 
skipTables: {}",
+        userPermission, skipNamespaces, skipTables, e);
       return false;
     }
   }
diff --git 
a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
 
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
index 99d5a89ac2c..d79e3f30810 100644
--- 
a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
+++ 
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
@@ -158,7 +158,6 @@ public class TestSnapshotScannerHDFSAclController {
 
     TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table);
     snapshotAndWait(snapshot1, table);
-    snapshotAndWait(snapshot2, table);
     // grant G(R)
     SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, READ);
     TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
@@ -175,6 +174,8 @@ public class TestSnapshotScannerHDFSAclController {
     // grant G(R)
     SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, READ);
     TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
+    // take a snapshot and ACLs are inherited automatically
+    snapshotAndWait(snapshot2, table);
     TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, 6);
     assertTrue(hasUserGlobalHdfsAcl(aclTable, grantUserName));
     deleteTable(table);
@@ -196,10 +197,10 @@ public class TestSnapshotScannerHDFSAclController {
     // create table in namespace1 and snapshot
     TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1);
     snapshotAndWait(snapshot1, table1);
-    // grant G(W)
-    SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
     admin.grant(new UserPermission(grantUserName,
       Permission.newBuilder(namespace1).withActions(READ).build()), false);
+    // grant G(W)
+    SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
     // create table in namespace2 and snapshot
     TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2);
     snapshotAndWait(snapshot2, table2);
@@ -230,11 +231,11 @@ public class TestSnapshotScannerHDFSAclController {
     // grant table1(R)
     TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1);
     snapshotAndWait(snapshot1, table1);
-    TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2);
-    snapshotAndWait(snapshot2, table2);
+    TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ);
     // grant G(W)
     SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
-    TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ);
+    TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2);
+    snapshotAndWait(snapshot2, table2);
     // check scan snapshot
     TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
     TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, -1);

Reply via email to