HDDS-3224. Enforce volume and bucket name rule at create time.

[swagle]

What changes were proposed in this pull request?

Validations performed by RpcClient are not done when using OzoneManager client directly. Added resource name checks for volume and bucket.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3224

How was this patch tested?

Added an integration test.

[swagle]

cc: @ChenSammi / @bharatviswa504 for review.

[bharatviswa504]

I have a few comments.

[swagle]

I have a few comments.

Thanks, Bharat for your comments. I will make those changes requested but one point, like Sammi pointed out in the Jira, this code path is still possible.

[bharatviswa504]

I have a few comments.

Thanks, Bharat for your comments. I will make those changes requested but one point, like Sammi pointed out in the Jira, this code path is still possible.

When using clientsideTranslatorPB, it will reach to OzoneManagerProtocolServerSideTranslator and then it will use new request code. I see only way, old code can be used is by using direct OzoneManager instance, but I see no real use case in doing that.

[swagle]

I have a few comments.

Thanks, Bharat for your comments. I will make those changes requested but one point, like Sammi pointed out in the Jira, this code path is still possible.

When using clientsideTranslatorPB, it will reach to OzoneManagerProtocolServerSideTranslator and then it will use new request code. I see only way, old code can be used is by using direct OzoneManager instance, but I see no real use case in doing that.

I reverted the changes.

[swagle]

S3BucketCreateRequest checked for length but the verifyResourceName is more comprehensive.
We are still doing length check in S3BucketDeleteRequest, technically we should not need validation in the delete path but kept those changes asis.

[bharatviswa504]

+1 LGTM.

Retriggered CI to get a clean run.

[ChenSammi]

@swagle, thanks for working on the fix. I tried the patch locally with non-HA OM. A volume with name "11" still can be created using Nanda's tool. The volume and bucket name check is in RpcClient.java while Nanda's tool use OzoneManagerProtocolClientSideTranslatorPB directly which bypass the check. Can we move these checks to server side as HDFS does? so that we just need one place to verify volume and bucket name integrity.

[swagle]

@swagle, thanks for working on the fix. I tried the patch locally with non-HA OM. A volume with name "11" still can be created using Nanda's tool. The volume and bucket name check is in RpcClient.java while Nanda's tool use OzoneManagerProtocolClientSideTranslatorPB directly which bypass the check. Can we move these checks to server-side as HDFS does? so that we just need one place to verify volume and bucket name integrity.

Hi Sammy, that is correct, it would be possible since I modified the patch based on @bharatviswa504 comments. Do you know where is the source for the tool?

[swagle]

@swagle, thanks for working on the fix. I tried the patch locally with non-HA OM. A volume with name "11" still can be created using Nanda's tool. The volume and bucket name check is in RpcClient.java while Nanda's tool use OzoneManagerProtocolClientSideTranslatorPB directly which bypass the check. Can we move these checks to server-side as HDFS does? so that we just need one place to verify volume and bucket name integrity.

Hi Sammy, that is correct, it would be possible since I modified the patch based on @bharatviswa504 comments. Do you know where is the source for the tool?

Actually, I am not able to verify by writing simple test, it fails as expected:

@Test public void testInvalidVolumeBucketNamesThrowException() throws Exception { OmVolumeArgs volumeArgs = OmVolumeArgs.newBuilder() .setVolume("v1") .setOwnerName("owner1") .setAdminName("admin1") .build(); RPC.setProtocolEngine(conf, OzoneManagerProtocolPB.class, ProtobufRpcEngine.class); OzoneManagerProtocol client = new OzoneManagerProtocolClientSideTranslatorPB( RPC.getProxy(OzoneManagerProtocolPB.class, RPC.getProtocolVersion(OzoneManagerProtocolPB.class), OmUtils.getOmAddressForClients(conf), UserGroupInformation.getCurrentUser(), conf, NetUtils.getDefaultSocketFactory(conf), Client.getRpcTimeout(conf)), "Ozone Manager Perf Test"); client.createVolume(volumeArgs); }

Above failed with exception:
INVALID_VOLUME_NAME org.apache.hadoop.ozone.om.exceptions.OMException: Invalid volume name: v1

Could it be something not updated in your test setup?

[swagle]

@ChenSammi wanted to get your input on above, thanks.

[dineshchitlangia]

Changes look good @swagle , however, there are multiple acceptance test failures for OM HA and Freon. Could you please review that?

[swagle]

Changes look good @swagle, however, there are multiple acceptance test failures for OM HA and Freon. Could you please review that?

@dineshchitlangia The same test seems to fail every time: ozone-om-ha-s3, I see HDDS-3313 is opened to address this.

[adoroszlai]

The same test seems to fail every time: ozone-om-ha-s3, I see HDDS-3313 is opened to address this.

HDDS-3313 is only about ozone-om-ha (enabled in #265 and now disabled in ce6ad30), not ozone-om-ha-s3. The latter has an intermittent, but very rare failure, reported in HDDS-2934.

[adoroszlai]

OM bucket generator fails even in single OM environment:

Freon OM Bucket Generator | FAIL |
255 != 0

due to:

OMException: Invalid bucket name: ELAtCj6bFe7

Can fix it by converting random Freon prefix to lowercase:

https://github.com/apache/hadoop-ozone/blob/61b6be001b78d1f3902db4a3a40734c19a90459c/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java#L240

[swagle]

HDDS-2934

I am actually getting this failure on

[swagle]

OM bucket generator fails even in single OM environment:

Freon OM Bucket Generator | FAIL |
255 != 0

due to:

OMException: Invalid bucket name: ELAtCj6bFe7

Can fix it by converting random Freon prefix to lowercase:

https://github.com/apache/hadoop-ozone/blob/61b6be001b78d1f3902db4a3a40734c19a90459c/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java#L240

Thanks @adoroszlai got the acceptance to pass finally!

[dineshchitlangia]

+1 @swagle , we now have a clean green run :)
Thank you @adoroszlai , @elek , @bharatviswa504, @ChenSammi for chiming in.