ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3013645782
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1176,6 +1185,11 @@ message KeyInfo {
// This allows a key to be created an committed atomically if the original
has not
// been modified.
optional uint64 expectedDataGeneration = 22;
+
+ // expectedETag, when set, indicates that the existing key must have
+ // the given ETag for the operation to succeed. This is used for
+ // S3 conditional writes with the If-Match header.
+ optional string expectedETag = 23;
Review Comment:
Hm, I don't recall that we need to add `expectedDataGeneration` and
`expectedETag` in `KeyInfo`. Adding the two fields in KeyInfo requires
additional logic to null the two fields during commit to prevent space
overhead, but adding fields on two fields that will always be null in the
keyTable does not seem to be right. However, I don't think we can avoid it
since without setting the fields in the openKey, there might be concurrent
PutObjects that might violate the serial consistency guarantee. Ideally, we
should use a separate `OpenKeyInfo` for `openKeyTable` to differentiate with
the final `KeyInfo` in the `keyTable`, but I guess the ship has sailed a long
time ago.
Please let me know what you think. I'm OK if there are no other ways.
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java:
##########
@@ -377,6 +377,103 @@ public void testPutObject() {
assertEquals("37b51d194a7513e45b56f6524f2d51f2",
putObjectResult.getETag());
}
+ @Test
+ public void testPutObjectIfNoneMatch() {
+ final String bucketName = getBucketName();
+ final String keyName = getKeyName();
+ final String content = "bar";
+ s3Client.createBucket(bucketName);
+
+ InputStream is = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ ObjectMetadata metadata = new ObjectMetadata();
+ metadata.setHeader("If-None-Match", "*");
+
+ PutObjectResult putObjectResult = s3Client.putObject(bucketName, keyName,
is, metadata);
+ assertEquals("37b51d194a7513e45b56f6524f2d51f2",
putObjectResult.getETag());
+ }
+
+ @Test
+ public void testPutObjectIfNoneMatchFail() {
+ final String bucketName = getBucketName();
+ final String keyName = getKeyName();
+ final String content = "bar";
+ s3Client.createBucket(bucketName);
+
+ InputStream is = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ s3Client.putObject(bucketName, keyName, is, new ObjectMetadata());
+
+ InputStream is2 = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ ObjectMetadata metadata = new ObjectMetadata();
+ metadata.setHeader("If-None-Match", "*");
+
+ AmazonServiceException ase = assertThrows(AmazonServiceException.class,
+ () -> s3Client.putObject(bucketName, keyName, is2, metadata));
+
+ assertEquals(ErrorType.Client, ase.getErrorType());
+ assertEquals(412, ase.getStatusCode());
+ assertEquals("PreconditionFailed", ase.getErrorCode());
+ }
Review Comment:
Nit: Please also help to add the post validation suggested in the acceptance
tests for relevant integration test methods (V1 and V2). Thanks.
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# 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 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already
exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate
__import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" >
/tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
Review Comment:
Nit: Can compare the `result` with the previous `etag`.
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# 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 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already
exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate
__import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" >
/tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
+
+Conditional Put If-Match With Wrong ETag Fails
+ [Documentation] If-Match with wrong ETag should fail with 412
+ ${key} = Set Variable condput-ifmatch-fail
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Try to rewrite with a wrong ETag
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match wrong-etag
+ Should contain ${result} PreconditionFailed
Review Comment:
Nit: We can query the existing original key and ensure it's indeed not
rewritten (checking the ETag using HeadObject should be enough).
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# 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 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already
exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate
__import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" >
/tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
+
+Conditional Put If-Match With Wrong ETag Fails
+ [Documentation] If-Match with wrong ETag should fail with 412
+ ${key} = Set Variable condput-ifmatch-fail
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET}
--key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Try to rewrite with a wrong ETag
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match wrong-etag
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match On Non-Existent Key Fails
+ [Documentation] If-Match on a key that does not exist should fail with
412
+ ${key} = Set Variable condput-ifmatch-nonexistent
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli and ignore error put-object
--bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match some-etag
+ Should contain ${result} PreconditionFailed
Review Comment:
Nit: Check that the key does not exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
