You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ozone.apache.org by si...@apache.org on 2021/11/16 09:16:07 UTC
[ozone] branch master updated: HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check (#2814)
This is an automated email from the ASF dual-hosted git repository.
siyao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new b4a785c HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check (#2814)
b4a785c is described below
commit b4a785c6a933a9072111eee4fd2c4fe68e5ab907
Author: Siyao Meng <50...@users.noreply.github.com>
AuthorDate: Tue Nov 16 01:14:28 2021 -0800
HDDS-5891. OFS mkdir -p does not work as expected for bucket creation when volume exists due to volume create ACL check (#2814)
---
.../hadoop/fs/ozone/TestRootedOzoneFileSystem.java | 81 +++++++++++++++++++++-
.../apache/hadoop/ozone/om/BucketManagerImpl.java | 21 +++++-
.../hadoop/ozone/om/TestBucketManagerImpl.java | 55 ++++++++++++---
.../ozone/BasicRootedOzoneClientAdapterImpl.java | 8 ++-
4 files changed, 147 insertions(+), 18 deletions(-)
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
index 7a9b51c..6cfafe6 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
@@ -38,12 +38,12 @@ import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.TestDataUtil;
+import org.apache.hadoop.ozone.client.BucketArgs;
import org.apache.hadoop.ozone.client.ObjectStore;
import org.apache.hadoop.ozone.client.OzoneBucket;
import org.apache.hadoop.ozone.client.OzoneKeyDetails;
import org.apache.hadoop.ozone.client.OzoneVolume;
import org.apache.hadoop.ozone.client.VolumeArgs;
-import org.apache.hadoop.ozone.client.BucketArgs;
import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.OMMetrics;
@@ -58,6 +58,7 @@ import org.apache.ozone.test.GenericTestUtils;
import org.apache.ozone.test.LambdaTestUtils;
import org.junit.AfterClass;
import org.junit.Assert;
+import org.junit.Assume;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Rule;
@@ -70,6 +71,7 @@ import org.slf4j.LoggerFactory;
import java.io.FileNotFoundException;
import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -160,6 +162,12 @@ public class TestRootedOzoneFileSystem {
private static String rootPath;
private static BucketLayout bucketLayout;
+ private static final String USER1 = "regularuser1";
+ private static final UserGroupInformation UGI_USER1 = UserGroupInformation
+ .createUserForTesting(USER1, new String[] {"usergroup"});
+ // Non-privileged OFS instance
+ private static RootedOzoneFileSystem userOfs;
+
@BeforeClass
public static void init() throws Exception {
conf = new OzoneConfiguration();
@@ -182,12 +190,17 @@ public class TestRootedOzoneFileSystem {
enabledFileSystemPaths);
}
conf.setBoolean(OzoneConfigKeys.OZONE_ACL_ENABLED, enableAcl);
+ // Set ACL authorizer class to OzoneNativeAuthorizer. The default
+ // OzoneAccessAuthorizer always returns true for all ACL checks which
+ // doesn't work for the test.
+ conf.set(OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS,
+ OzoneConfigKeys.OZONE_ACL_AUTHORIZER_CLASS_NATIVE);
cluster = MiniOzoneCluster.newBuilder(conf)
.setNumDatanodes(3)
.build();
cluster.waitForClusterToBeReady();
objectStore = cluster.getClient().getObjectStore();
-
+
// create a volume and a bucket to be used by RootedOzoneFileSystem (OFS)
OzoneBucket bucket =
TestDataUtil.createVolumeAndBucket(cluster, bucketLayout);
@@ -210,6 +223,10 @@ public class TestRootedOzoneFileSystem {
trash = new Trash(conf);
ofs = (RootedOzoneFileSystem) fs;
adapter = (BasicRootedOzoneClientAdapterImpl) ofs.getAdapter();
+
+ userOfs = UGI_USER1.doAs(
+ (PrivilegedExceptionAction<RootedOzoneFileSystem>)()
+ -> (RootedOzoneFileSystem) FileSystem.get(conf));
}
@AfterClass
@@ -874,7 +891,7 @@ public class TestRootedOzoneFileSystem {
* OFS: Test /tmp mount behavior.
*/
@Test
- public void testTempMount() throws Exception {
+ public void testTempMount() throws IOException {
// Prep
// Use ClientProtocol to pass in volume ACL, ObjectStore won't do it
ClientProtocol proxy = objectStore.getClientProxy();
@@ -1517,4 +1534,62 @@ public class TestRootedOzoneFileSystem {
}
}
+ @Test
+ public void testNonPrivilegedUserMkdirCreateBucket() throws IOException {
+ // This test is only meaningful when ACL is enabled
+ Assume.assumeTrue("ACL is not enabled. Skipping this test as it requires " +
+ "ACL to be enabled to be meaningful.", enableAcl);
+
+ // This unit test does the correct check. However, the parameterized
+ // test is not initialized correctly since HDDS-4998 (or HDDS-4040).
+
+ // The cluster is started with enableAcl = false, but across different set
+ // of test parameters, the OM is not restarted. So OM is actually stuck with
+ // enableAcl = false. That's why ACL checks are skipped in the tests.
+ // OM should be restarted when enableAcl is changed.
+
+ // For now, in order to properly run this specific test, on line 147,
+ // explicitly assign:
+ // private static boolean enableAcl = true;
+ // When unset, it defaults to false due to Java primitive defaults.
+
+ // TODO: Fix the parameterized test to properly initialize the cluster.
+ // When the parameterized test is initialized correctly, the line
+ // below can be uncommented.
+// Assert.assertTrue(cluster.getOzoneManager().getAclsEnabled());
+
+ final String volume = "volume-for-test-get-bucket";
+ // Create a volume as admin
+ // Create volume "tmp" with world access. allow non-admin to create buckets
+ ClientProtocol proxy = objectStore.getClientProxy();
+
+ // Get default acl rights for user
+ OzoneAclConfig aclConfig = conf.getObject(OzoneAclConfig.class);
+ ACLType userRights = aclConfig.getUserDefaultRights();
+ // Construct ACL for world access
+ OzoneAcl aclWorldAccess = new OzoneAcl(ACLIdentityType.WORLD, "",
+ userRights, ACCESS);
+ // Construct VolumeArgs, set ACL to world access
+ VolumeArgs volumeArgs = new VolumeArgs.Builder()
+ .setAcls(Collections.singletonList(aclWorldAccess))
+ .build();
+ proxy.createVolume(volume, volumeArgs);
+
+ // Create a bucket as non-admin, should succeed
+ final String bucket = "test-bucket-1";
+ try {
+ final Path myBucketPath = new Path(volume, bucket);
+ // Have to prepend the root to bucket path here.
+ // Otherwise, FS will automatically prepend user home directory path
+ // which is not we want here.
+ Assert.assertTrue(userOfs.mkdirs(new Path("/", myBucketPath)));
+ } catch (IOException e) {
+ Assert.fail("Should not have thrown exception when creating bucket as" +
+ " a regular user here");
+ }
+
+ // Clean up
+ proxy.deleteBucket(volume, bucket);
+ proxy.deleteVolume(volume);
+ }
}
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/BucketManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/BucketManagerImpl.java
index 9bffde9..068c629 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/BucketManagerImpl.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/BucketManagerImpl.java
@@ -45,6 +45,7 @@ import org.slf4j.LoggerFactory;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.INTERNAL_ERROR;
+import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND;
import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;
import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.VOLUME_LOCK;
@@ -127,7 +128,7 @@ public class BucketManagerImpl implements BucketManager {
if (volumeArgs == null) {
LOG.debug("volume: {} not found ", volumeName);
throw new OMException("Volume doesn't exist",
- OMException.ResultCodes.VOLUME_NOT_FOUND);
+ VOLUME_NOT_FOUND);
}
//Check if bucket already exists
if (metadataManager.getBucketTable().get(bucketKey) != null) {
@@ -236,6 +237,12 @@ public class BucketManagerImpl implements BucketManager {
*
* @param volumeName - Name of the Volume.
* @param bucketName - Name of the Bucket.
+ * @return OmBucketInfo
+ * @throws IOException The exception thrown could be:
+ * 1. OMException VOLUME_NOT_FOUND when the parent volume doesn't exist.
+ * 2. OMException BUCKET_NOT_FOUND when the parent volume exists, but bucket
+ * doesn't.
+ * 3. Other exceptions, e.g. IOException thrown from getBucketTable().get().
*/
@Override
public OmBucketInfo getBucketInfo(String volumeName, String bucketName)
@@ -250,8 +257,16 @@ public class BucketManagerImpl implements BucketManager {
if (value == null) {
LOG.debug("bucket: {} not found in volume: {}.", bucketName,
volumeName);
- throw new OMException("Bucket not found",
- BUCKET_NOT_FOUND);
+ // Check parent volume existence
+ final String dbVolumeKey = metadataManager.getVolumeKey(volumeName);
+ if (metadataManager.getVolumeTable().get(dbVolumeKey) == null) {
+ // Parent volume doesn't exist, throw VOLUME_NOT_FOUND
+ throw new OMException("Volume not found when getting bucket info",
+ VOLUME_NOT_FOUND);
+ } else {
+ // Parent volume exists, throw BUCKET_NOT_FOUND
+ throw new OMException("Bucket not found", BUCKET_NOT_FOUND);
+ }
}
return value;
} catch (IOException ex) {
diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java
index e2cc4ce..ae263b6 100644
--- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java
+++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestBucketManagerImpl.java
@@ -45,7 +45,6 @@ import org.mockito.runners.MockitoJUnitRunner;
* Tests BucketManagerImpl, mocks OMMetadataManager for testing.
*/
@RunWith(MockitoJUnitRunner.class)
-@Ignore("Bucket Manager does not use cache, Disable it for now.")
public class TestBucketManagerImpl {
@Rule
public ExpectedException thrown = ExpectedException.none();
@@ -102,6 +101,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testCreateBucket() throws Exception {
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -136,6 +136,7 @@ public class TestBucketManagerImpl {
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testCreateEncryptedBucket() throws Exception {
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -151,6 +152,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testCreateAlreadyExistingBucket() throws Exception {
thrown.expectMessage("Bucket already exist");
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -173,6 +175,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testGetBucketInfoForInvalidBucket() throws Exception {
thrown.expectMessage("Bucket not found");
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -192,23 +195,51 @@ public class TestBucketManagerImpl {
@Test
public void testGetBucketInfo() throws Exception {
+ final String volumeName = "sampleVol";
+ final String bucketName = "bucketOne";
OmMetadataManagerImpl metaMgr = createSampleVol();
BucketManager bucketManager = new BucketManagerImpl(metaMgr);
+ // Check exception thrown when volume does not exist
+ try {
+ bucketManager.getBucketInfo(volumeName, bucketName);
+ Assert.fail("Should have thrown OMException");
+ } catch (OMException omEx) {
+ Assert.assertEquals("getBucketInfo() should have thrown " +
+ "VOLUME_NOT_FOUND as the parent volume is not created!",
+ ResultCodes.VOLUME_NOT_FOUND, omEx.getResult());
+ }
OmBucketInfo bucketInfo = OmBucketInfo.newBuilder()
- .setVolumeName("sampleVol")
- .setBucketName("bucketOne")
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
.setStorageType(StorageType.DISK)
.setIsVersionEnabled(false)
.build();
+ // Note: the helper method createBucket() in this scope won't create the
+ // parent volume DB entry. In order to verify getBucketInfo's behavior, we
+ // need to create the volume entry in DB manually.
+ OmVolumeArgs args = OmVolumeArgs.newBuilder()
+ .setVolume(volumeName)
+ .setAdminName("bilbo")
+ .setOwnerName("bilbo")
+ .build();
+ TestOMRequestUtils.addVolumeToOM(metaMgr, args);
+ // Create bucket
createBucket(metaMgr, bucketInfo);
- OmBucketInfo result = bucketManager.getBucketInfo(
- "sampleVol", "bucketOne");
- Assert.assertEquals("sampleVol", result.getVolumeName());
- Assert.assertEquals("bucketOne", result.getBucketName());
- Assert.assertEquals(StorageType.DISK,
- result.getStorageType());
- Assert.assertEquals(false, result.getIsVersionEnabled());
+ // Check exception thrown when bucket does not exist
+ try {
+ bucketManager.getBucketInfo(volumeName, "bucketNotExist");
+ Assert.fail("Should have thrown OMException");
+ } catch (OMException omEx) {
+ Assert.assertEquals("getBucketInfo() should have thrown " +
+ "BUCKET_NOT_FOUND as the parent volume exists but bucket " +
+ "doesn't!", ResultCodes.BUCKET_NOT_FOUND, omEx.getResult());
+ }
+ OmBucketInfo result = bucketManager.getBucketInfo(volumeName, bucketName);
+ Assert.assertEquals(volumeName, result.getVolumeName());
+ Assert.assertEquals(bucketName, result.getBucketName());
+ Assert.assertEquals(StorageType.DISK, result.getStorageType());
+ Assert.assertFalse(result.getIsVersionEnabled());
metaMgr.getStore().close();
}
@@ -218,6 +249,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testSetBucketPropertyChangeStorageType() throws Exception {
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -246,6 +278,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testSetBucketPropertyChangeVersioning() throws Exception {
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -272,6 +305,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testDeleteBucket() throws Exception {
thrown.expectMessage("Bucket not found");
OmMetadataManagerImpl metaMgr = createSampleVol();
@@ -306,6 +340,7 @@ public class TestBucketManagerImpl {
}
@Test
+ @Ignore("Bucket Manager does not use cache, Disable it for now.")
public void testDeleteNonEmptyBucket() throws Exception {
thrown.expectMessage("Bucket is not empty");
OmMetadataManagerImpl metaMgr = createSampleVol();
diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
index 43406b7..a9a0e96 100644
--- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
+++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
@@ -233,7 +233,7 @@ public class BasicRootedOzoneClientAdapterImpl
// when ACL is disabled. Both exceptions need to be handled.
switch (ex.getResult()) {
case VOLUME_NOT_FOUND:
- case BUCKET_NOT_FOUND:
+ // Create the volume first when the volume doesn't exist
try {
objectStore.createVolume(volumeStr);
} catch (OMException newVolEx) {
@@ -242,7 +242,11 @@ public class BasicRootedOzoneClientAdapterImpl
throw newVolEx;
}
}
-
+ // No break here. Proceed to create the bucket
+ case BUCKET_NOT_FOUND:
+ // When BUCKET_NOT_FOUND is thrown, we expect the parent volume
+ // exists, so that we don't call create volume and incur
+ // unnecessary ACL checks which could lead to unwanted behavior.
OzoneVolume volume = proxy.getVolumeDetails(volumeStr);
// Create the bucket
try {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@ozone.apache.org
For additional commands, e-mail: commits-help@ozone.apache.org