Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 9 additions & 0 deletions hadoop-hdds/common/src/main/resources/ozone-default.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1962,6 +1962,15 @@
</description>
</property>

<property>
<name>ozone.s3g.list.max.keys.limit</name>
Copy link
Member

Choose a reason for hiding this comment

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

@kerneltime Do you want this config to apply to all "list" related endpoints? If yes, we should adjust the description and apply the max key logic to other endpoints in new jira.

Copy link
Member

Choose a reason for hiding this comment

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

Also, should we check that this config must >= 1000?

<value>1000</value>
<description>
Maximum number of keys returned by S3 ListObjects/ListObjectsV2 API.
AWS default is 1000. Can be overridden per deployment in ozone-site.xml.
</description>
</property>

<property>
<name>ozone.s3g.secret.http.enabled</name>
<value>false</value>
Expand Down
66 changes: 66 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/s3/objectlist.robot
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# 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

*** Keywords ***
Prepare Many Objects In Bucket
[Arguments] ${count}=1100
Execute mkdir -p /tmp/manyfiles
FOR ${i} IN RANGE ${count}
Execute echo "test-${i}" > /tmp/manyfiles/obj-${i}
END
Execute aws s3 cp /tmp/manyfiles s3://${BUCKET}/ --recursive --endpoint-url=${ENDPOINT_URL}

*** 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

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

List objects with max-keys exceeding config limit should not return more than limit
Prepare Many Objects In Bucket 1100
${result}= Execute AWSS3APICli and checkrc list-objects-v2 --bucket ${BUCKET} --max-keys 9999 --endpoint-url=${ENDPOINT_URL} --output json 0
${tmpfile}= Generate Random String 8
${tmpfile}= Set Variable /tmp/result_${tmpfile}.json
Create File ${tmpfile} ${result}
${count}= Execute and checkrc jq -r '.Contents | length' ${tmpfile} 0
Should Be True ${count} <= 1000
Remove File ${tmpfile}

List objects with max-keys less than config limit should return correct count
Prepare Many Objects In Bucket 1100
${result}= Execute AWSS3APICli and checkrc list-objects-v2 --bucket ${BUCKET} --max-keys 500 --endpoint-url=${ENDPOINT_URL} --output json 0
${tmpfile}= Generate Random String 8
${tmpfile}= Set Variable /tmp/result_${tmpfile}.json
Create File ${tmpfile} ${result}
${count}= Execute and checkrc jq -r '.Contents | length' ${tmpfile} 0
Should Be True ${count} == 500
Remove File ${tmpfile}
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 objectlist

rebot --outputdir results/ results/*.xml
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ public final class S3GatewayConfigKeys {
public static final String OZONE_S3G_METRICS_PERCENTILES_INTERVALS_SECONDS_KEY
= "ozone.s3g.metrics.percentiles.intervals.seconds";

// S3 ListObjects max-keys limit (default: 1000, AWS compatible)
public static final String OZONE_S3G_LIST_MAX_KEYS_LIMIT = "ozone.s3g.list.max.keys.limit";
public static final int OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT = 1000;

/**
* Never constructed.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@
import static org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_KEYS_SHALLOW_ENABLED;
import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_KEYS_SHALLOW_ENABLED_DEFAULT;
import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_MAX_KEYS_LIMIT;
import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.NOT_IMPLEMENTED;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.newError;
import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;

import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
Expand All @@ -37,6 +40,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import javax.annotation.PostConstruct;
import javax.inject.Inject;
import javax.ws.rs.DELETE;
import javax.ws.rs.DefaultValue;
Expand Down Expand Up @@ -91,6 +95,7 @@ public class BucketEndpoint extends EndpointBase {
LoggerFactory.getLogger(BucketEndpoint.class);

private boolean listKeysShallowEnabled;
private int maxKeysLimit = 1000;

@Inject
private OzoneConfiguration ozoneConfiguration;
Expand Down Expand Up @@ -142,6 +147,8 @@ public Response get(
return listMultipartUploads(bucketName, prefix, keyMarker, uploadIdMarker, maxUploads);
}

maxKeys = validateMaxKeys(maxKeys);

if (prefix == null) {
prefix = "";
}
Expand Down Expand Up @@ -292,6 +299,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, maxKeysLimit);
}

@PUT
public Response put(@PathParam("bucket") String bucketName,
@QueryParam("acl") String aclMarker,
Expand Down Expand Up @@ -752,10 +767,24 @@ private void addKey(ListObjectResponse response, OzoneKey next) {
response.addKey(keyMetadata);
}

@VisibleForTesting
public void setOzoneConfiguration(OzoneConfiguration config) {
this.ozoneConfiguration = config;
}

@VisibleForTesting
public OzoneConfiguration getOzoneConfiguration() {
return this.ozoneConfiguration;
}

@Override
@PostConstruct
public void init() {
listKeysShallowEnabled = ozoneConfiguration.getBoolean(
OZONE_S3G_LIST_KEYS_SHALLOW_ENABLED,
OZONE_S3G_LIST_KEYS_SHALLOW_ENABLED_DEFAULT);
maxKeysLimit = ozoneConfiguration.getInt(
OZONE_S3G_LIST_MAX_KEYS_LIMIT,
OZONE_S3G_LIST_MAX_KEYS_LIMIT_DEFAULT);
}
}
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
* 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.
*/

package org.apache.hadoop.ozone.s3.endpoint;

/**
* Builder for BucketEndpoint in tests.
*/
public class BucketEndpointBuilder extends
EndpointBuilder<BucketEndpoint> {

public BucketEndpointBuilder() {
super(BucketEndpoint::new);
}

@Override
public BucketEndpoint build() {
BucketEndpoint endpoint = super.build();
endpoint.setOzoneConfiguration(getConfig());

return endpoint;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static EndpointBuilder<RootEndpoint> newRootEndpointBuilder() {
}

public static EndpointBuilder<BucketEndpoint> newBucketEndpointBuilder() {
return new EndpointBuilder<>(BucketEndpoint::new);
return new BucketEndpointBuilder();
}

public static EndpointBuilder<ObjectEndpoint> newObjectEndpointBuilder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.hadoop.ozone.s3.endpoint;

import static org.apache.hadoop.ozone.s3.S3GatewayConfigKeys.OZONE_S3G_LIST_MAX_KEYS_LIMIT;
import static org.apache.hadoop.ozone.s3.util.S3Consts.ENCODING_TYPE;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand All @@ -26,6 +27,8 @@
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.IOException;
import java.util.stream.IntStream;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.ozone.client.OzoneBucket;
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneClientStub;
Expand Down Expand Up @@ -519,6 +522,61 @@ 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());
}

@Test
public void testListObjectsRespectsConfiguredMaxKeysLimit() throws Exception {
// Arrange: Create a bucket with 1001 keys
String[] keys = IntStream.range(0, 1001).mapToObj(i -> "file" + i).toArray(String[]::new);
OzoneClient client = createClientWithKeys(keys);

// Arrange: Set the max-keys limit in the configuration
OzoneConfiguration config = new OzoneConfiguration();
final String configuredMaxKeysLimit = "900";
Copy link
Member

Choose a reason for hiding this comment

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

This is ok, but my original thought is just want this to be a very low value, like 10, so we don't need to create too much key in test.

config.set(OZONE_S3G_LIST_MAX_KEYS_LIMIT, configuredMaxKeysLimit);

// Arrange: Build and initialize the BucketEndpoint with the config
BucketEndpoint bucketEndpoint = EndpointBuilder.newBucketEndpointBuilder()
.setClient(client)
.setConfig(config)
.build();
bucketEndpoint.init();

// Assert: Ensure the config value is correctly set in the endpoint
assertEquals(configuredMaxKeysLimit,
bucketEndpoint.getOzoneConfiguration().get(OZONE_S3G_LIST_MAX_KEYS_LIMIT));

// Act: Request more keys than the configured max-keys limit
final int requestedMaxKeys = Integer.parseInt(configuredMaxKeysLimit) + 1;
ListObjectResponse response = (ListObjectResponse)
bucketEndpoint.get("b1", null, null, null, requestedMaxKeys,
null, null, null, null, null, null, null,
1000, null).getEntity();

// Assert: The number of returned keys should be capped at the configured limit
assertEquals(Integer.parseInt(configuredMaxKeysLimit), response.getContents().size());
}

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