This is an automated email from the ASF dual-hosted git repository.
vjasani pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2 by this push:
new a7f12e1dd14 HBASE-28081 Snapshot working dir does not retain ACLs
after snapshot commit phase (#5437)
a7f12e1dd14 is described below
commit a7f12e1dd14dafcc22d82546a201f33048665f33
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 49ac4ca118e..249ef966186 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 f5af1254a71..2579229d609 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
@@ -156,11 +156,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;
}
}
@@ -178,11 +179,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 20784e70e43..8369f840ea5 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);
