-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-12870. Fix listObjects corner cases #8307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
peterxcli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jimmyweng006 for this patch.
| String queryString = "max-keys=-1"; | ||
| String url = s3Endpoint + "/" + bucketName + "/?" + queryString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you build this with url builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert the whole integration test as smoke test might be more suitable for this case.
| @Test | ||
| public void testListObjectsWithNegativeMaxKeys() throws Exception { | ||
| final String bucketName = getTestBucketName(); | ||
| String s3Endpoint = getS3EndpointURL(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can move to @BeforeAll
| * @param bucket S3 bucket name | ||
| * @param queryString The query string (e.g., "max-keys=-1") | ||
| */ | ||
| public static void signRequest(HttpURLConnection conn, String accessKey, String secretKey, String region, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any reference for this signing process?
|
|
||
| private int validateMaxKeys(int maxKeys) throws OS3Exception { | ||
| if (maxKeys <= 0) { | ||
| throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); | |
| throw newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); |
| throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); | ||
| } | ||
|
|
||
| return Math.min(maxKeys, 1000); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
cc @chungen0126 |
|
Thanks @Jimmyweng006 for the patch. Instead of adding a REST endpoint test, it would be better to add a smoke test for this. The current smoke test doesn't cover listing objects. |
| throw S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT, "maxKeys must be > 0"); | ||
| } | ||
|
|
||
| return Math.min(maxKeys, 1000); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/rest/S3V4Signer.java
Outdated
Show resolved
Hide resolved
|
Hi @chungen0126 , Sure let me try to use smoke test, and thanks for remind about the test. I just add the previously run link into the PR content and will add on future PR as well. |
| ${result} = Execute AWSS3APICli and checkrc list-objects-v2 --bucket ${BUCKET} --max-keys -1 255 | ||
| Should Contain ${result} InvalidArgument |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
| @@ -0,0 +1,37 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one or more | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…ONDecodeError and shell argument too long error in acceptance test
|
Hi @peterxcli , @chungen0126 , @kerneltime I just rewrite the robot test for passing Acceptance Test, please help for review. Thank you. |
peterxcli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jimmyweng006 for updating the patch.
There was a problem hiding this comment.
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.
| </property> | ||
|
|
||
| <property> | ||
| <name>ozone.s3g.list.max.keys.limit</name> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| assertEquals(S3ErrorTable.INVALID_ARGUMENT.getCode(), e.getCode()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
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.
-
For the low value it can still be controlled by ozone.s3g.list.max.keys.limit
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…d extend BucketEndpointBuilder to support config
peterxcli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jimmyweng006 for this change, LGTM! One comment left, but change it or not is up to you~
|
|
||
| // Arrange: Set the max-keys limit in the configuration | ||
| OzoneConfiguration config = new OzoneConfiguration(); | ||
| final String configuredMaxKeysLimit = "900"; |
There was a problem hiding this comment.
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.
|
Thanks @Jimmyweng006 for the patch, @peterxcli @jojochuang @kerneltime for the review. |
|
Thanks @peterxcli , @chungen0126 , @jojochuang , @kerneltime for the review! |
…239-container-reconciliation Commits: 80 commits 5e273a4 HDDS-12977. Fail build on dependency problems (apache#8574) 5081ba2 HDDS-13034. Refactor DirectoryDeletingService to use ReclaimableDirFilter and ReclaimableKeyFilter (apache#8546) e936e4d HDDS-12134. Implement Snapshot Cache lock for OM Bootstrap (apache#8474) 31d13de HDDS-13165. [Docs] Python client developer guide. (apache#8556) 9e6955e HDDS-13205. Bump common-custom-user-data-maven-extension to 2.0.3 (apache#8581) 750b629 HDDS-13203. Bump Bouncy Castle to 1.81 (apache#8580) ba5177e HDDS-13202. Bump build-helper-maven-plugin to 3.6.1 (apache#8579) 07ee5dd HDDS-13204. Bump awssdk to 2.31.59 (apache#8582) e1964f2 HDDS-13201. Bump jersey2 to 2.47 (apache#8578) 81295a5 HDDS-13013. [Snapshot] Add metrics and tests for snapshot operations. (apache#8436) b3d75ab HDDS-12976. Clean up unused dependencies (apache#8521) e0f08b2 HDDS-13179. rename-generated-config fails on re-compile without clean (apache#8569) f388317 HDDS-12554. Support callback on completed reconfiguration (apache#8391) c13a3fe HDDS-13154 Link more Grafana dashboard json files to the Observability user doc (apache#8533) 2a761f7 HDDS-11967. [Docs]DistCP Integration in Kerberized environment. (apache#8531) 81fc4c4 HDDS-12550. Use DatanodeID instead of UUID in NodeManager CommandQueue. (apache#8560) 2360af4 HDDS-13169. Intermittent failure in testSnapshotOperationsNotBlockedDuringCompaction (apache#8553) f19789d HDDS-13170. Reclaimable filter should always reclaim entries when buckets and volumes have already been deleted (apache#8551) 315ef20 HDDS-13175. Leftover reference to OM-specific trash implementation (apache#8563) 902e715 HDDS-13159. Refactor KeyManagerImpl for getting deleted subdirectories and deleted subFiles (apache#8538) 46a93d0 HDDS-12817. Addendum rename ecIndex to replicaIndex in chunkinfo output (apache#8552) 19b9b9c HDDS-13166. Set pipeline ID in BlockExistenceVerifier to avoid cached pipeline with different node (apache#8549) b3ff67c HDDS-13068. Validate Container Balancer move timeout and replication timeout configs (apache#8490) 7a7b9a8 HDDS-13139. Introduce bucket layout flag in freon rk command (apache#8539) 3c25e7d HDDS-12595. Add verifier for container replica states (apache#8422) 6d59220 HDDS-13104. Move auditparser acceptance test under debug (apache#8527) 8e8c432 HDDS-13071. Documentation for Container Replica Debugger Tool (apache#8485) 0e8c8d4 HDDS-13158. Bump junit to 5.13.0 (apache#8537) 8e552b4 HDDS-13157. Bump exec-maven-plugin to 3.5.1 (apache#8534) 168f690 HDDS-13155. Bump jline to 3.30.4 (apache#8535) cc1e4d1 HDDS-13156. Bump awssdk to 2.31.54 (apache#8536) 3bfb7af HDDS-13136. KeyDeleting Service should not run for already deep cleaned snapshots (apache#8525) 006e691 HDDS-12503. Compact snapshot DB before evicting a snapshot out of cache (apache#8141) 568b228 HDDS-13067. Container Balancer delete commands should not be sent with an expiration time in the past (apache#8491) 53673c5 HDDS-11244. OmPurgeDirectoriesRequest should clean up File and Directory tables of AOS for deleted snapshot directories (apache#8509) 07f4868 HDDS-13099. ozone admin datanode list ignores --json flag when --id filter is used (apache#8500) 08c0ab8 HDDS-13075. Fix default value in description of container placement policy configs (apache#8511) 58c87a8 HDDS-12177. Set runtime scope where missing (apache#8513) 10c470d HDDS-12817. Add EC block index in the ozone debug replicas chunk-info (apache#8515) 7027ab7 HDDS-13124. Respect config hdds.datanode.use.datanode.hostname when reading from datanode (apache#8518) b8b226c HDDS-12928. datanode min free space configuration (apache#8388) fd3d70c HDDS-13026. KeyDeletingService should also delete RenameEntries (apache#8447) 4c1c6cf HDDS-12714. Create acceptance test framework for debug and repair tools (apache#8510) fff80fc HDDS-13118. Remove duplicate mockito-core dependency from hdds-test-utils (apache#8508) 10d5555 HDDS-13115. Bump awssdk to 2.31.50 (apache#8505) 360d139 HDDS-13017. Fix warnings due to non-test scoped test dependencies (apache#8479) 1db1cca HDDS-13116. Bump jline to 3.30.3 (apache#8504) 322ca93 HDDS-13025. Refactor KeyDeletingService to use ReclaimableKeyFilter (apache#8450) 988b447 HDDS-5287. Document S3 ACL classes (apache#8501) 64bb29d HDDS-12777. Use module-specific name for generated config files (apache#8475) 54ed115 HDDS-9210. Update snapshot chain restore test to incorporate snapshot delete. (apache#8484) 87dfa5a HDDS-13014. Improve PrometheusMetricsSink#normalizeName performance (apache#8438) 7cdc865 HDDS-13100. ozone admin datanode list --json should output a newline at the end (apache#8499) 9cc4194 HDDS-13089. [snapshot] Add an integration test to verify snapshotted data can be read by S3 SDK client (apache#8495) cb9867b HDDS-13065. Refactor SnapshotCache to return AutoCloseSupplier instead of ReferenceCounted (apache#8473) a88ff71 HDDS-10979. Support STANDARD_IA S3 storage class to accept EC replication config (apache#8399) 6ec8f85 HDDS-13080. Improve delete metrics to show number of timeout DN command from SCM (apache#8497) 3bb8858 HDDS-12378. Change default hdds.scm.safemode.min.datanode to 3 (apache#8331) 0171bef HDDS-13073. Set pipeline ID in checksums verifier to avoid cached pipeline with different node (apache#8480) 5c7726a HDDS-11539. OzoneClientCache `@PreDestroy` is never called (apache#8493) a8ed19b HDDS-13031. Implement a Flat Lock resource in OzoneManagerLock (apache#8446) e9e8b30 HDDS-12935. Support unsigned chunked upload with STREAMING-UNSIGNED-PAYLOAD-TRAILER (apache#8366) 7590268 HDDS-13079. Improve logging in DN for delete operation. (apache#8489) 435fe7e HDDS-12870. Fix listObjects corner cases (apache#8307) eb5dabd HDDS-12926. Remove *.tmp.* exclusion in DU (apache#8486) eeb98c7 HDDS-13030. Snapshot Purge should unset deep cleaning flag for next 2 snapshots in the chain (apache#8451) 6bf121e HDDS-13032. Support proper S3OwnerId representation (apache#8478) 5d1b43d HDDS-13076. Refactor OzoneManagerLock class to rename Resource class to LeveledResource (apache#8482) bafe6d9 HDDS-13064. [snapshot] Add test coverage for SnapshotUtils.isBlockLocationInfoSame() (apache#8476) 7035846 HDDS-13040. Add user doc highlighting the difference between Ozone ACL and S3 ACL. (apache#8457) 1825cdf HDDS-13049. Deprecate VolumeName & BucketName in OmKeyPurgeRequest and prevent Key version purge on Block Deletion Failure (apache#8463) 211c76c HDDS-13060. Change NodeManager.addDatanodeCommand(..) to use DatanodeID (apache#8471) f410238 HDDS-13061. Add test for key ACL operations without permission (apache#8472) d1a2f48 HDDS-13057. Increment block delete processed transaction counts regardless of log level (apache#8466) 0cc6fcc HDDS-13043. Replace != with assertNotEquals in TestSCMContainerPlacementRackAware (apache#8470) e1c779a HDDS-13051. Use DatanodeID in server-scm. (apache#8465) 35e1126 HDDS-13042. [snapshot] Add future proofing test cases for unsupported file system API (apache#8458) 619c05d HDDS-13008. Exclude same SST files when calculating full snapdiff (apache#8423) 21b49d3 HDDS-12965. Fix warnings about "used undeclared" dependencies (apache#8468) 8136119 HDDS-13048. Create new module for Recon integration tests (apache#8464) Conflicts: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java


What changes were proposed in this pull request?
Add maxKey validation for S3 listObjects function
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12870
How was this patch tested?
Unit Test and Smoke Test
Latest Github Action Run Result
Latest Github Action Run ResultGithub Action Run Success
Smoke Test result pass:
