Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-28081 Snapshot working dir does not retain ACLs after snapshot commit phase #5437

Merged
merged 11 commits into from
Sep 30, 2023
Merged

HBASE-28081 Snapshot working dir does not retain ACLs after snapshot commit phase #5437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2beaee7
HBASE-28081 De-flake TestSnapshotScannerHDFSAclController#testModifyT…
virajjasani Sep 26, 2023
6b15633
Revert "HBASE-28081 De-flake TestSnapshotScannerHDFSAclController#tes…
virajjasani Sep 27, 2023
205fd62
addendum
virajjasani Sep 27, 2023
82b5023
addendum
virajjasani Sep 27, 2023
a142fc8
addendum - spotless
virajjasani Sep 27, 2023
21e8029
addendum - handle disabled ACL support
virajjasani Sep 27, 2023
82910fd
addendum
virajjasani Sep 27, 2023
f42260a
addendum
virajjasani Sep 27, 2023
f968613
addendum
virajjasani Sep 27, 2023
f6096eb
addendum
virajjasani Sep 27, 2023
5451118
Merge branch 'master' of github.com:apache/hbase into HBASE-28081-master
virajjasani Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@
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;
Expand Down Expand Up @@ -564,6 +567,7 @@ public synchronized void prepareWorkingDirectory(SnapshotDescription snapshot)
"Couldn't create working directory (" + workingDir + ") for snapshot",
ProtobufUtil.createSnapshotDesc(snapshot));
}
updateWorkingDirAclsIfRequired(workingDir, workingDirFS);
} catch (HBaseSnapshotException e) {
throw e;
} catch (IOException e) {
Expand All @@ -573,6 +577,42 @@ public synchronized void prepareWorkingDirectory(SnapshotDescription snapshot)
}
}

/**
* 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is workingDir guaranteed to have a parent of a parent to avoid any possible NullPointerException? Or could workingDir theoretically be / or /tmp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it can be configured to be different, and in that case even original ACLs would not be applicable so better to have null check and return in such cases:

  /**
   * Get the general working directory for snapshots - where they are built, where they are
   * temporarily copied on export, etc.
   * @param rootDir root directory of the HBase installation
   * @param conf    Configuration of the HBase instance
   * @return Path to the snapshot tmp directory, relative to the passed root directory
   */
  public static Path getWorkingSnapshotDir(final Path rootDir, final Configuration conf) {
    return new Path(
      conf.get(SNAPSHOT_WORKING_DIR, getDefaultWorkingSnapshotDir(rootDir).toString()));
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

} 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}.
Expand Down
14 changes: 8 additions & 6 deletions ...r/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,12 @@ public boolean grantAcl(UserPermission userPermission, Set<String> skipNamespace
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;
}
}
Expand All @@ -174,11 +175,12 @@ public boolean revokeAcl(UserPermission userPermission, Set<String> skipNamespac
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;
}
}
Expand Down
13 changes: 7 additions & 6 deletions ...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ public void testGrantGlobal1() throws Exception {

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);
Expand All @@ -175,6 +174,8 @@ public void testGrantGlobal1() throws Exception {
// 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);
Expand All @@ -196,10 +197,10 @@ public void testGrantGlobal2() throws Exception {
// 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);
Expand Down Expand Up @@ -230,11 +231,11 @@ public void testGrantGlobal3() throws Exception {
// 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);
Expand Down