Skip to content

Commit

Permalink
HBASE-28042 Snapshot corruptions due to non-atomic rename within same…
Browse files Browse the repository at this point in the history 
… filesystem (#5369)

Co-authored-by: Ujjawal <ujjawal4046@gmail.com>

Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
Signed-off-by: Abhey Rana <a.rana@salesforce.com>
Signed-off-by: Ujjawal <ujjawal4046@gmail.com>
Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com>
  • Loading branch information
@virajjasani
virajjasani committed Aug 28, 2023
1 parent dcc5495 commit 5527dd9
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 11 deletions.
37 changes: 33 additions & 4 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,9 @@ public static void completeSnapshot(Path snapshotDir, Path workingDir, FileSyste
// if this fails
URI workingURI = workingDirFs.getUri();
URI rootURI = fs.getUri();

if (
(!workingURI.getScheme().equals(rootURI.getScheme()) || workingURI.getAuthority() == null
|| !workingURI.getAuthority().equals(rootURI.getAuthority())
|| workingURI.getUserInfo() == null
|| !workingURI.getUserInfo().equals(rootURI.getUserInfo())
(shouldSkipRenameSnapshotDirectories(workingURI, rootURI)
|| !fs.rename(workingDir, snapshotDir))
&& !FileUtil.copy(workingDirFs, workingDir, fs, snapshotDir, true, true, conf)
) {
Expand All @@ -432,6 +430,37 @@ public static void completeSnapshot(Path snapshotDir, Path workingDir, FileSyste
}
}

static boolean shouldSkipRenameSnapshotDirectories(URI workingURI, URI rootURI) {
// check scheme, e.g. file, hdfs
if (workingURI.getScheme() == null && rootURI.getScheme() != null) {
return true;
}
if (workingURI.getScheme() != null && !workingURI.getScheme().equals(rootURI.getScheme())) {
return true;
}

// check Authority, e.g. localhost:port
if (workingURI.getAuthority() == null && rootURI.getAuthority() != null) {
return true;
}
if (
workingURI.getAuthority() != null && !workingURI.getAuthority().equals(rootURI.getAuthority())
) {
return true;
}

// check UGI/userInfo
if (workingURI.getUserInfo() == null && rootURI.getUserInfo() != null) {
return true;
}
if (
workingURI.getUserInfo() != null && !workingURI.getUserInfo().equals(rootURI.getUserInfo())
) {
return true;
}
return false;
}

/**
* Check if the user is this table snapshot's owner
* @param snapshot the table snapshot description
Expand Down
13 changes: 6 additions & 7 deletions ...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ 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 @@ -174,8 +175,6 @@ 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 @@ -197,10 +196,10 @@ public void testGrantGlobal2() throws Exception {
// create table in namespace1 and snapshot
TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1);
snapshotAndWait(snapshot1, table1);
admin.grant(new UserPermission(grantUserName,
Permission.newBuilder(namespace1).withActions(READ).build()), false);
// grant G(W)
SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
admin.grant(new UserPermission(grantUserName,
Permission.newBuilder(namespace1).withActions(READ).build()), false);
// create table in namespace2 and snapshot
TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2);
snapshotAndWait(snapshot2, table2);
Expand Down Expand Up @@ -231,11 +230,11 @@ public void testGrantGlobal3() throws Exception {
// grant table1(R)
TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1);
snapshotAndWait(snapshot1, table1);
TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ);
// grant G(W)
SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2);
snapshotAndWait(snapshot2, table2);
// grant G(W)
SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE);
TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ);
// check scan snapshot
TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, -1);
Expand Down
49 changes: 49 additions & 0 deletions ...e-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.junit.Assert.fail;

import java.io.IOException;
import java.net.URI;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
Expand Down Expand Up @@ -191,4 +192,52 @@ public void testIsWithinWorkingDir() throws IOException {
assertTrue(SnapshotDescriptionUtils.isWithinDefaultWorkingDir(
new Path("file:" + hbsaeDir + "/.hbase-snapshot/.tmp/snapshot"), conf));
}

@Test
public void testShouldSkipRenameSnapshotDirectories() {
URI workingDirURI = URI.create("/User/test1");
URI rootDirURI = URI.create("hdfs:///User/test2");

// should skip rename if it's not the same scheme;
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("/User/test1");
rootDirURI = URI.create("file:///User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

// skip rename when either scheme or authority are the not same
workingDirURI = URI.create("hdfs://localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://otherhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("file:///User/test1");
rootDirURI = URI.create("hdfs://localhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs:///User/test1");
rootDirURI = URI.create("hdfs:///User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs://localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://localhost:8020/User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs://user:password@localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://user:password@localhost:8020/User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

// skip rename when user information is not the same
workingDirURI = URI.create("hdfs://user:password@localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://user2:password2@localhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));
}

}

0 comments on commit 5527dd9

Please sign in to comment.