From 2898bc3d8b404bdbd28be6867c922e890f779207 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Wed, 23 Aug 2023 14:49:18 -0700 Subject: [PATCH 1/5] HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem --- .../snapshot/SnapshotDescriptionUtils.java | 40 +++++++++++++-- .../TestSnapshotDescriptionUtils.java | 49 +++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java index 1e2bbb68c41d..3302e2f9efe5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java @@ -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) ) { @@ -432,6 +430,40 @@ 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 && !rootURI.getScheme().equalsIgnoreCase("file")) + ) { + 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 diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java index 2093baf36075..14f181a610ba 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java @@ -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; @@ -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"); + assertFalse( + 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)); + } + } From 8ca3b0ba8accdf388414ec15162a40f60f950893 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Wed, 23 Aug 2023 23:01:05 -0700 Subject: [PATCH 2/5] test addendum --- .../security/access/TestSnapshotScannerHDFSAclController.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 d79e3f308104..a5a194f79743 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,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); @@ -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); From 880ffd73ec2e96f3fe3a287dc8a50955fe206247 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 24 Aug 2023 11:43:12 -0700 Subject: [PATCH 3/5] addendum --- .../hadoop/hbase/snapshot/SnapshotDescriptionUtils.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java index 3302e2f9efe5..689cd89259ba 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java @@ -432,10 +432,7 @@ 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 && !rootURI.getScheme().equalsIgnoreCase("file")) - ) { + if (workingURI.getScheme() == null && rootURI.getScheme() != null) { return true; } if (workingURI.getScheme() != null && !workingURI.getScheme().equals(rootURI.getScheme())) { From 0603f20c55e4cab613a30efa560b50d0ed433ca5 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 24 Aug 2023 13:27:03 -0700 Subject: [PATCH 4/5] addendum - test order changes for read/write perm grant --- .../access/TestSnapshotScannerHDFSAclController.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 a5a194f79743..99d5a89ac2c9 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 @@ -196,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); @@ -230,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); From ce1b14eec8d6a602b93bcaa282b68fb1ac0b8d71 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 24 Aug 2023 14:06:28 -0700 Subject: [PATCH 5/5] addendum --- .../hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java index 14f181a610ba..8e62ef16bbfe 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java @@ -204,7 +204,7 @@ public void testShouldSkipRenameSnapshotDirectories() { workingDirURI = URI.create("/User/test1"); rootDirURI = URI.create("file:///User/test2"); - assertFalse( + assertTrue( SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI)); // skip rename when either scheme or authority are the not same