Skip to content

Conversation

@akareddy04
Copy link
Contributor

@akareddy04 akareddy04 commented May 30, 2025

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:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@akareddy04 akareddy04 requested a review from a team as a code owner May 30, 2025 18:47
Comment on lines 1 to 2
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to include these lines in every file, they should not be deleted here.

Comment on lines 127 to 129
"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."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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. " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

final InputStream objectStreamForResult = new BoundedInputStream(fileSizeLimit);

S3Client wrappedClient = S3Client.create();
S3Client s3Client = S3EncryptionClient.builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

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"));
Copy link
Contributor

Choose a reason for hiding this comment

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

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks unused

}

@Test
public void testConversionAllFields() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Specify that this is for CreateMPUToPutObject

Copy link
Contributor

@kessplas kessplas left a comment

Choose a reason for hiding this comment

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

Just a few minor things to fix

Comment on lines 16 to 18
/*Converts a CreateMultipartUploadRequest into a PutObjectRequest by setting optional fields needed for
putInstructionFile operation.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is used

kessplas
kessplas previously approved these changes Jun 6, 2025
Copy link
Contributor

@kessplas kessplas left a comment

Choose a reason for hiding this comment

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

LGTM, lets get another review for this though.

Copy link
Contributor

@lucasmcdonald3 lucasmcdonald3 left a comment

Choose a reason for hiding this comment

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

Mostly looking for confirmation on whether we should include some fields in the ConvertSDKRequests, but the shape of this looks good

Comment on lines 18 to 19
.bucket(value)
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Indentation; it looks like this file was using 2 spaces per indentation but you've changed it to 6 here. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an error on my part, will fix that now!

Copy link
Contributor

@lucasmcdonald3 lucasmcdonald3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@akareddy04 akareddy04 merged commit 99077dc into main Jun 13, 2025
45 of 56 checks passed
@ashu027
Copy link

ashu027 commented Jun 19, 2025

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?

@GSudeendra
Copy link

@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

@kessplas
Copy link
Contributor

Hey @ashu027 and @GSudeendra ,
Sorry for the late reply. This feature, along with functionality to re-encrypt/rotate keys using instruction files will be released in the coming weeks. Thanks.

aws-crypto-tools-ci-bot pushed a commit that referenced this pull request Jul 30, 2025
## [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))
@kessplas
Copy link
Contributor

kessplas commented Aug 1, 2025

Hey @ashu027 and @GSudeendra ,
This feature, along with functionality to re-encrypt/rotate keys using instruction files has been released in v3.4.0 and is available in Maven. Thanks!

@GSudeendra
Copy link

@kessplas thanks for the update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants