Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6f61f55
HDDS-12870. Add maxKey validation for S3 listObjects function
Jimmyweng006 Apr 20, 2025
365c67b
HDDS-12870. Unit test for maxKey validation of S3 listObjects function
Jimmyweng006 Apr 20, 2025
7bd5677
HDDS-12870. Integration test for maxKey validation of S3 listObjects …
Jimmyweng006 Apr 20, 2025
19f7ba5
HDDS-12870. Fix checkstyle and add Apache License header
Jimmyweng006 Apr 20, 2025
f283cc0
HDDS-12870. Add smoke test for maxKey validation in S3 listObjects fu…
Jimmyweng006 Apr 26, 2025
5035f36
HDDS-12870. Simplify new exception syntax in validateMaxKeys
Jimmyweng006 Apr 26, 2025
7611798
HDDS-12870. Add maxkeys_validation robot test into compatbility_check…
Jimmyweng006 Apr 26, 2025
228bacd
Revert "HDDS-12870. Fix checkstyle and add Apache License header"
Jimmyweng006 Apr 26, 2025
a4e074d
Revert "HDDS-12870. Integration test for maxKey validation of S3 list…
Jimmyweng006 Apr 26, 2025
beffb26
HDDS-12870. Fix for check style
Jimmyweng006 Apr 26, 2025
c8d7db3
HDDS-12870. Make maxKeysLimit configurable
Jimmyweng006 Apr 27, 2025
7e2d5d4
HDDS-12870. Add robot tests for testing max-keys is lower or higher t…
Jimmyweng006 Apr 27, 2025
ab8092b
HDDS-12870. Set default maxKeysLimit to avoid uninitialized value in …
Jimmyweng006 Apr 27, 2025
edcb530
HDDS-12870. Check Contents length using jq with temp file to avoid JS…
Jimmyweng006 May 3, 2025
93f6cb8
HDDS-12870. Make robot test name more general
Jimmyweng006 May 10, 2025
ff1408f
Merge remote-tracking branch 'origin/master' into HDDS-12870
Jimmyweng006 May 10, 2025
aaf2783
HDDS-12870. Add unit test to verify max-keys limit is configurable an…
Jimmyweng006 May 18, 2025
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
37 changes: 37 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/s3/maxkeys_validation.robot
Copy link
Member

Choose a reason for hiding this comment

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

I think this file should rename to objectlist.robot, maxkeys_validation.robot is too specific.

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
Copy link
Member

Choose a reason for hiding this comment

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

you should add this smoke test to

run_robot_test awss3
run_robot_test boto3
run_robot_test bucketcreate
run_robot_test bucketdelete
run_robot_test buckethead
run_robot_test bucketlist
run_robot_test objectputget
run_robot_test objectdelete
run_robot_test objectcopy
run_robot_test objectmultidelete
run_robot_test objecthead
run_robot_test MultipartUpload
run_robot_test objecttagging

too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure let me add in, thanks for remind.

# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You under the Apache License, Version 2.0
# (the "License"); you may not use this file except in compliance with
# the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

*** Settings ***
Documentation S3 max-keys validation test for negative and zero values
Library OperatingSystem
Library String
Resource ../commonlib.robot
Resource commonawslib.robot
Test Timeout 3 minutes
Suite Setup Setup s3 tests

*** Variables ***
${ENDPOINT_URL} http://s3g:9878
${BUCKET} generated

*** Test Cases ***

List objects with negative max-keys should fail
${result} = Execute AWSS3APICli and checkrc list-objects-v2 --bucket ${BUCKET} --max-keys -1 255
Should Contain ${result} InvalidArgument
Copy link
Member

Choose a reason for hiding this comment

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

Does it send request to s3g, or just intercept by the awscli client similar to aws sdk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the server log we can see that the request did send to s3g server, and return the fail response(InvalidArgument).

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s3g log path inside container: /var/log/hadoop/s3g-audit-${hostName}.log


List objects with zero max-keys should fail
${result} = Execute AWSS3APICli and checkrc list-objects-v2 --bucket ${BUCKET} --max-keys 0 255
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the server-side max configurable, you can set it low for this test and test server-side enforcement as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, just add two robot tests for testing max-keys is lower or higher than maxKeysLimit scenarios.

Should Contain ${result} InvalidArgument
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,6 @@ run_robot_test objectmultidelete
run_robot_test objecthead
run_robot_test MultipartUpload
run_robot_test objecttagging
run_robot_test maxkeys_validation

rebot --outputdir results/ results/*.xml
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ public Response get(
return listMultipartUploads(bucketName, prefix, keyMarker, uploadIdMarker, maxUploads);
}

maxKeys = validateMaxKeys(maxKeys);

if (prefix == null) {
prefix = "";
}
Expand Down Expand Up @@ -292,6 +294,14 @@ public Response get(
return Response.ok(response).build();
}

private int validateMaxKeys(int maxKeys) throws OS3Exception {
if (maxKeys <= 0) {
throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0");
}

return Math.min(maxKeys, 1000);
Copy link
Member

Choose a reason for hiding this comment

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

Same behaviour as AWS S3 does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In AWS S3 ListObjects & ListObjectsV2 API reference, they both mention below.

Returns some or all (up to 1,000) of the objects in a bucket.

Therefore I think add the maxKeys up to 1000 is appropriate.

reference:
ListObjects
ListObjectsV2

Copy link
Contributor

Choose a reason for hiding this comment

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

This sever side max needs to be configurable. Depending on deployment scale and resources, a larger number maybe desired for performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implement this function but not yet commit.

If below statement is not wrong, should config maxKeys still need? Or for the purpose to reduce maxKeys like limit to 500(some number is smaller than 1000)?

In AWS S3 ListObjects & ListObjectsV2 API reference, they both mention below.

Returns some or all (up to 1,000) of the objects in a bucket.

Therefore I think add the maxKeys up to 1000 is appropriate.

reference: ListObjects ListObjectsV2

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that AWS has a limit it enforces. For Ozone, we should set the default to 1000, but in some high-performance use cases, it might make sense to have it higher as well, where the listing needs to be completed in one round trip. If we make it configurable, the customer will have a choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get it now. I kind of stuck at AWS S3 protocol, but in Ozone we can make it more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configurable maxKey test result with some local log
image

}

@PUT
public Response put(@PathParam("bucket") String bucketName,
@QueryParam("acl") String aclMarker,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,28 @@ public void testEncodingTypeException() throws IOException {
assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e.getCode());
}

@Test
Copy link
Member

@peterxcli peterxcli May 8, 2025

Choose a reason for hiding this comment

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

Should add another test case to check if the max-keys would be set to the max-key num defined in config. This can be verified by creating 1000+ key and list bucket with 1000+ max-key param, then check if the response key num is 1000.
(If we allow the config to be set to lower value then it would be better to set a low value for it, but it depends on #8307 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The max-keys config is already covered by the Robot test "List objects with max-keys exceeding config limit should not return more than limit". Since the unit test does not initialize the maxKeysLimit config, the robot test is better suited to verify this behavior.

  2. For the low value it can still be controlled by ozone.s3g.list.max.keys.limit

Copy link
Member

@peterxcli peterxcli May 17, 2025

Choose a reason for hiding this comment

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

Since the unit test does not initialize the maxKeysLimit config, the robot test is better suited to verify this behavior.

@Jimmyweng006 Use setConfig of the builder to set the config with custom lower max key limit, then call bucketEndpoint#init

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice @peterxcli , I update the PR but local CI will take some time.

Should add another test case to check if the max-keys would be set to the max-key num defined in config. This can be verified by creating 1000+ key and list bucket with 1000+ max-key param, then check if the response key num is 1000.

The new commit achieves above testing scenario with a lower max key limit, please help have a look later. Thank you.

public void testListObjectsWithInvalidMaxKeys() throws Exception {
OzoneClient client = createClientWithKeys("file1");
BucketEndpoint bucketEndpoint = EndpointBuilder.newBucketEndpointBuilder()
.setClient(client)
.build();

// maxKeys < 0
OS3Exception e1 = assertThrows(OS3Exception.class, () ->
bucketEndpoint.get("bucket", null, null, null, -1, null,
null, null, null, null, null, null, 1000, null)
);
assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e1.getCode());

// maxKeys == 0
OS3Exception e2 = assertThrows(OS3Exception.class, () ->
bucketEndpoint.get("bucket", null, null, null, 0, null,
null, null, null, null, null, null, 1000, null)
);
assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e2.getCode());
}

private void assertEncodingTypeObject(
String exceptName, String exceptEncodingType, EncodingTypeObject object) {
assertEquals(exceptName, object.getName());
Expand Down