HDDS-16115. Reject invalid lifecycle rule Status values - #10980
HDDS-16115. Reject invalid lifecycle rule Status values#10980F64116045 wants to merge 2 commits into
Conversation
| String status = rule.getStatus(); | ||
| if (status == null || status.isEmpty()) { | ||
| throw S3ErrorTable.newError(S3ErrorTable.MALFORMED_XML, | ||
| "The Status element is required in LifecycleConfiguration"); |
There was a problem hiding this comment.
The second String argument here is actually resource, and OS3Exception serializes it as <Resource>. So this text does not replace <Message>; MALFORMED_XML already provides that. I think the one-argument overload is enough here.
This is in the existing missing-Status path, so I plan to follow it up separately with response coverage. If it makes more sense to include it here, I can update the patch.
ashishkumar50
left a comment
ashishkumar50
left a comment
There was a problem hiding this comment.
@F64116045 Thanks for the patch, mostly looks fine to me. Here are few minor suggestions.
| throw S3ErrorTable.newError(S3ErrorTable.MALFORMED_XML, | ||
| "The Status element is required in LifecycleConfiguration"); | ||
| } | ||
| if (!"Enabled".equals(status) && !"Disabled".equals(status)) { |
There was a problem hiding this comment.
Instead of hardcoding, good to define constant and use everywhere for Enabled/Disabled.
There was a problem hiding this comment.
Updated to use STATUS_ENABLED and STATUS_DISABLED in the validation and builder mapping. Thanks.
| @Test | ||
| public void testPutValidLifecycleConfiguration() throws Exception { | ||
| assertEquals(HTTP_OK, bucketEndpoint.put("bucket1", onePrefix()).getStatus()); | ||
| assertEquals(HTTP_OK, bucketEndpoint.put("bucket1", withStatus("Disabled")).getStatus()); |
There was a problem hiding this comment.
Verify valid input for both Enabled and Disabled.
There was a problem hiding this comment.
Added the Enabled test, thanks.
| private OmLCRule convertToOmRule(Rule rule) throws OMException, OS3Exception { | ||
| if (rule.getStatus() == null || rule.getStatus().isEmpty()) { | ||
| String status = rule.getStatus(); | ||
| if (status == null || status.isEmpty()) { |
There was a problem hiding this comment.
You can use StringUtils.isEmpty(status)
There was a problem hiding this comment.
Updated to use StringUtils.isEmpty(status). Thanks.
|
Thanks @ashishkumar50 for the review. I’ve addressed the suggestions. |
3 participants
What changes were proposed in this pull request?
According to the Amazon S3 LifecycleRule API, lifecycle rule
Statusis required and only accepts the exact valuesEnabledorDisabled. This behavior is covered bytest_lifecycle_invalid_statusin ceph/s3-tests.S3 Gateway previously checked only whether
Statuswas missing or empty. It then converted the value using"Enabled".equals(status), causing every other non-empty value, includingenabled,disabled, andinvalid, to be accepted and interpreted as a disabled rule.This pull request validates
Statusagainst the two values defined by Amazon S3 before converting the lifecycle rule. Parameterized tests cover invalid values, while the valid path explicitly verifies bothEnabledandDisabled.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16115
How was this patch tested?
mvn -pl hadoop-ozone/s3gateway -DskipDocs=true -DskipShade \ -Dtest=TestS3LifecycleConfigurationPut testmvn -pl hadoop-ozone/s3gateway -DskipDocs=true -DskipShade testCeph S3 compatibility test against a locally built packaged Ozone cluster: