feat: put object with instruction file configured #466
Conversation
…r MPU + instructionFile
…tructionFileConfig
…equest to PutObjectRequest
| // Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
We need to include these lines in every file, they should not be deleted here.
| "To work around this issue you can disable multi part upload," + | ||
| "use the Async client, or not set this value on PutObject." + | ||
| "You may be able to update this value after the PutObject request completes." |
There was a problem hiding this comment.
| "To work around this issue you can disable multi part upload," + | |
| "use the Async client, or not set this value on PutObject." + | |
| "You may be able to update this value after the PutObject request completes." | |
| "To work around this issue you can disable Instruction File on PutObject or disable " + | |
| "multi part upload, or use the Async client, or not set this value on PutObject." + | |
| "You may be able to update this value after the request completes." |
The text should be slightly different as it only applies to cases where InstructionFile puts are enabled.
| // Rather than silently dropping the value, | ||
| // we loudly signal that we don't know how to handle this field. | ||
| throw new IllegalArgumentException( | ||
| f.locationName() + " is an unknown field. " + |
There was a problem hiding this comment.
| f.locationName() + " is an unknown field. " + | |
| f.memberName() + " is an unknown field. " + |
Please fix this in the other convertRequest method, we missed that originally.
|
|
||
| public class ConvertSDKRequests { | ||
|
|
||
| public static PutObjectRequest convertRequest(CreateMultipartUploadRequest request) { |
There was a problem hiding this comment.
nit: We should add some comments here, i.e. that this is used to set the optional fields for Instruction File puts when the request is a multipart upload, because the instruction file is Put with an ordinary PutObject request.
And likewise for the other convertRequest method which is used to set the optional fields on high-level Multipart PutObject requests.
src/main/java/software/amazon/encryption/s3/internal/MultipartUploadObjectPipeline.java
Show resolved
Hide resolved
| final InputStream objectStreamForResult = new BoundedInputStream(fileSizeLimit); | ||
|
|
||
| S3Client wrappedClient = S3Client.create(); | ||
| S3Client s3Client = S3EncryptionClient.builder() |
There was a problem hiding this comment.
We need to set .enableMultipartPutObject(true) to enable multipart put.
| .bucket(BUCKET) | ||
| .key(object_key + ".instruction") | ||
| .build()); | ||
| assertTrue(directInstGetResponse.response().metadata().containsKey("x-amz-crypto-instr-file")); |
There was a problem hiding this comment.
Let's also set StorageClass in the original put request and check for it here.
|
|
||
|
|
||
| CreateMultipartUploadResponse initiateResult = v3Client.createMultipartUpload(builder -> | ||
| builder.bucket(BUCKET).key(object_key)); |
There was a problem hiding this comment.
Let's also set StorageClass here and check for it in the directInstGetResponse later on.
| import software.amazon.awssdk.awscore.AwsRequestOverrideConfiguration; | ||
| import software.amazon.awssdk.services.s3.model.*; | ||
|
|
||
| import java.awt.event.ComponentListener; |
| } | ||
|
|
||
| @Test | ||
| public void testConversionAllFields() { |
There was a problem hiding this comment.
nit: Specify that this is for CreateMPUToPutObject
kessplas
left a comment
There was a problem hiding this comment.
Just a few minor things to fix
| /*Converts a CreateMultipartUploadRequest into a PutObjectRequest by setting optional fields needed for | ||
| putInstructionFile operation. | ||
| */ |
There was a problem hiding this comment.
This should be a Javadoc comment. Meaning, it comes before the method and it has this syntax (most importantly, the two *s in the first block):
/**
* Comment here
*/
public whatever methodName() {
There are many examples throughout the codebase. Your IDE should also render them as green rather than grey.
In terms of content, it is not really accurate. We should specify that the conversion is used when Instruction File PutObject is enabled and the customer performs a multipart upload. putInstructionFile doesn't exist in v3.
|
|
||
| public static CreateMultipartUploadRequest convertRequest(PutObjectRequest request) { | ||
|
|
||
| /*Converts a PutObjectRequest into a CreateMultipartUploadRequest by setting optional fields needed for high-level |
| import software.amazon.awssdk.services.s3.model.PutObjectResponse; | ||
| import software.amazon.awssdk.services.s3.model.S3Exception; | ||
| import software.amazon.awssdk.services.s3.model.StorageClass; | ||
| import software.amazon.awssdk.services.s3.model.StorageClassAnalysisDataExport; |
There was a problem hiding this comment.
I don't think this is used
kessplas
left a comment
There was a problem hiding this comment.
LGTM, lets get another review for this though.
lucasmcdonald3
left a comment
There was a problem hiding this comment.
Mostly looking for confirmation on whether we should include some fields in the ConvertSDKRequests, but the shape of this looks good
src/main/java/software/amazon/encryption/s3/internal/ConvertSDKRequests.java
Show resolved
Hide resolved
src/main/java/software/amazon/encryption/s3/internal/ConvertSDKRequests.java
Show resolved
Hide resolved
src/main/java/software/amazon/encryption/s3/internal/ConvertSDKRequests.java
Show resolved
Hide resolved
| .bucket(value) | ||
| .build(); |
There was a problem hiding this comment.
nit: Indentation; it looks like this file was using 2 spaces per indentation but you've changed it to 6 here. Was that intentional?
There was a problem hiding this comment.
That was an error on my part, will fix that now!
|
We have a use case that requires the addition of an instruction file. However, we haven't seen these changes included in the latest release. Could you please let us know when we can expect a new release that incorporates the changes from this PR? |
|
@kessplas We rely on separate instruction files for encrypted uploads (legacy setup), but they haven’t been showing up in S3 when using S3AsyncEncryptionClient. Looks like this PR fixes that. Any idea when the next release with this fix will be out? Thank You |
|
Hey @ashu027 and @GSudeendra , |
## [3.4.0](v3.3.5...v3.4.0) (2025-07-30) ### Features * put object with instruction file configured ([#466](#466)) ([99077dc](99077dc)) * reEncryptInstructionFile Implementation ([#475](#475)) ([ff66e72](ff66e72)) * reEncryptInstructionFile Implementation ([#478](#478)) ([f7e6fa5](f7e6fa5)) ### Fixes * Revert "feat: reEncryptInstructionFile Implementation ([#475](#475))" ([#477](#477)) ([6d45ec5](6d45ec5)) ### Maintenance * guard against properties conflicts ([#479](#479)) ([793c73b](793c73b)) * **pom:** fix scm url ([#469](#469)) ([1bc2ca3](1bc2ca3)) * **release:** Migrate release to Central Portal ([#468](#468)) ([da71231](da71231)) * validate against legacy wrapping on client but customer passes keyring with no legacy wrapping ([#473](#473)) ([bb898d1](bb898d1))
|
Hey @ashu027 and @GSudeendra , |
|
@kessplas thanks for the update |
Issue #, if available: #355
Description of changes:
*Adds ability to add instruction file configuration for putObject
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: