Skip to content

Disable MD5 checksum at S3 CRT client creation time and propagate checksum algorithm #3309

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Disable MD5 checksum at S3 CRT client creation time and propagate checksum algorithm #3309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...rc/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import static software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute.SDK_HTTP_EXECUTION_ATTRIBUTES;
import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.OPERATION_NAME;
import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.CHECKSUM_SPECS;

import java.net.URI;
import software.amazon.awssdk.annotations.SdkInternalApi;
Expand Down Expand Up @@ -160,10 +161,11 @@ public void afterMarshalling(Context.AfterMarshalling context,
SdkHttpExecutionAttributes.builder()
.put(OPERATION_NAME,
executionAttributes.getAttribute(SdkExecutionAttribute.OPERATION_NAME))
.put(CHECKSUM_SPECS,
executionAttributes.getAttribute(SdkExecutionAttribute.RESOLVED_CHECKSUM_SPECS))
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we do a

getAttribute(SdkExecutionAttribute.RESOLVED_CHECKSUM_SPECS)

where is SdkExecutionAttribute.RESOLVED_CHECKSUM_SPECS updated with resolved in case of code flow for transfer manager execution ?

.build();

executionAttributes.putAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES,
attributes);
executionAttributes.putAttribute(SDK_HTTP_EXECUTION_ATTRIBUTES, attributes);
}
}

Expand Down
29 changes: 28 additions & 1 deletion ...3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package software.amazon.awssdk.services.s3.internal.crt;

import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.CHECKSUM_SPECS;
import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.OPERATION_NAME;

import java.net.URI;
Expand All @@ -25,8 +26,11 @@
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.annotations.SdkTestInternalApi;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
import software.amazon.awssdk.core.checksums.Algorithm;
import software.amazon.awssdk.core.checksums.ChecksumSpecs;
import software.amazon.awssdk.crt.http.HttpHeader;
import software.amazon.awssdk.crt.http.HttpRequest;
import software.amazon.awssdk.crt.s3.ChecksumAlgorithm;
import software.amazon.awssdk.crt.s3.S3Client;
import software.amazon.awssdk.crt.s3.S3ClientOptions;
import software.amazon.awssdk.crt.s3.S3MetaRequest;
Expand Down Expand Up @@ -58,6 +62,7 @@ private S3CrtAsyncHttpClient(Builder builder) {
.signingRegion(builder.region == null ? null : builder.region.id())
.endpointOverride(builder.endpointOverride)
.credentialsProvider(builder.credentialsProvider)
.contentMd5(Boolean.FALSE)
.build();

S3ClientOptions s3ClientOptions =
Expand All @@ -67,7 +72,7 @@ private S3CrtAsyncHttpClient(Builder builder) {
.withCredentialsProvider(s3NativeClientConfiguration.credentialsProvider())
.withClientBootstrap(s3NativeClientConfiguration.clientBootstrap())
.withPartSize(s3NativeClientConfiguration.partSizeBytes())
.withComputeContentMd5(true)
.withComputeContentMd5(s3NativeClientConfiguration.isContentMd5())
.withThroughputTargetGbps(s3NativeClientConfiguration.targetThroughputInGbps());
this.crtS3Client = new S3Client(s3ClientOptions);
}
Expand All @@ -87,10 +92,12 @@ public CompletableFuture<Void> execute(AsyncExecuteRequest asyncRequest) {
new S3CrtResponseHandlerAdapter(executeFuture, asyncRequest.responseHandler());

S3MetaRequestOptions.MetaRequestType requestType = requestType(asyncRequest);
ChecksumAlgorithm checksumAlgorithm = crtChecksumAlgorithm(asyncRequest);

S3MetaRequestOptions requestOptions = new S3MetaRequestOptions()
.withHttpRequest(httpRequest)
.withMetaRequestType(requestType)
.withChecksumAlgorithm(checksumAlgorithm)
.withResponseHandler(responseHandler)
.withEndpoint(s3NativeClientConfiguration.endpointOverride());

Expand Down Expand Up @@ -123,6 +130,26 @@ private static S3MetaRequestOptions.MetaRequestType requestType(AsyncExecuteRequ
return S3MetaRequestOptions.MetaRequestType.DEFAULT;
}

private static ChecksumAlgorithm crtChecksumAlgorithm(AsyncExecuteRequest asyncRequest) {
ChecksumSpecs checksumSpecs = asyncRequest.httpExecutionAttributes().getAttribute(CHECKSUM_SPECS);
if (checksumSpecs != null && checksumSpecs.algorithm() != null) {
Algorithm checksumAlgorithm = checksumSpecs.algorithm();
switch (checksumAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Enum.fromValue(str.toUpper/LowerCase) here instead of switch case ?

case CRC32:
return ChecksumAlgorithm.CRC32;
case CRC32C:
return ChecksumAlgorithm.CRC32C;
case SHA1:
return ChecksumAlgorithm.SHA1;
case SHA256:
return ChecksumAlgorithm.SHA256;
default:
throw new IllegalStateException("Checksum algorithm not translatable: " + checksumAlgorithm);
}
}
return ChecksumAlgorithm.CRC32;
}

private static void closeResourcesWhenComplete(CompletableFuture<Void> executeFuture,
S3MetaRequest s3MetaRequest,
S3CrtResponseHandlerAdapter responseHandler) {
Expand Down
8 changes: 8 additions & 0 deletions .../software/amazon/awssdk/services/s3/internal/crt/S3InternalSdkHttpExecutionAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package software.amazon.awssdk.services.s3.internal.crt;

import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.checksums.Algorithm;
import software.amazon.awssdk.core.checksums.ChecksumSpecs;
import software.amazon.awssdk.http.SdkHttpExecutionAttribute;

@SdkInternalApi
Expand All @@ -27,6 +29,12 @@ public final class S3InternalSdkHttpExecutionAttribute<T> extends SdkHttpExecuti
public static final S3InternalSdkHttpExecutionAttribute<String> OPERATION_NAME =
new S3InternalSdkHttpExecutionAttribute<>(String.class);

/**
* The key to indicate the name of the operation
*/
public static final S3InternalSdkHttpExecutionAttribute<ChecksumSpecs> CHECKSUM_SPECS =
new S3InternalSdkHttpExecutionAttribute<>(ChecksumSpecs.class);

private S3InternalSdkHttpExecutionAttribute(Class<T> valueClass) {
super(valueClass);
}
Expand Down
13 changes: 13 additions & 0 deletions ...ain/java/software/amazon/awssdk/services/s3/internal/crt/S3NativeClientConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public class S3NativeClientConfiguration implements SdkAutoCloseable {
private final int maxConcurrency;
private final URI endpointOverride;
private final Executor futureCompletionExecutor;
private final boolean contentMd5;

public S3NativeClientConfiguration(Builder builder) {
this.signingRegion = builder.signingRegion == null ? DefaultAwsRegionProviderChain.builder().build().getRegion().id() :
Expand All @@ -76,6 +77,8 @@ public S3NativeClientConfiguration(Builder builder) {
this.endpointOverride = builder.endpointOverride;

this.futureCompletionExecutor = resolveAsyncFutureCompletionExecutor(builder.asynConfiguration);

this.contentMd5 = builder.contentMd5 == null ? false : builder.contentMd5;
}

public static Builder builder() {
Expand Down Expand Up @@ -114,6 +117,10 @@ public Executor futureCompletionExecutor() {
return futureCompletionExecutor;
}

public boolean isContentMd5() {
return contentMd5;
}

/**
* Finalize which async executor service will be used for the created client. The default async executor
* service has at least 8 core threads and can scale up to at least 64 threads when needed depending
Expand Down Expand Up @@ -164,6 +171,7 @@ public static final class Builder {
private Integer maxConcurrency;
private URI endpointOverride;
private ClientAsyncConfiguration asynConfiguration;
private Boolean contentMd5;

private Builder() {
}
Expand Down Expand Up @@ -203,6 +211,11 @@ public Builder asyncConfiguration(ClientAsyncConfiguration asyncConfiguration) {
return this;
}

public Builder contentMd5(Boolean contentMd5) {
this.contentMd5 = contentMd5;
return this;
}

public S3NativeClientConfiguration build() {
return new S3NativeClientConfiguration(this);
}
Expand Down
24 changes: 24 additions & 0 deletions ...c/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtAsyncHttpClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static software.amazon.awssdk.http.Header.CONTENT_LENGTH;
import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.CHECKSUM_SPECS;
import static software.amazon.awssdk.services.s3.internal.crt.S3InternalSdkHttpExecutionAttribute.OPERATION_NAME;

import java.net.URI;
Expand All @@ -28,12 +29,17 @@
import java.util.concurrent.CompletableFuture;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
import software.amazon.awssdk.core.checksums.Algorithm;
import software.amazon.awssdk.core.checksums.ChecksumSpecs;
import software.amazon.awssdk.crt.http.HttpRequest;
import software.amazon.awssdk.crt.s3.ChecksumAlgorithm;
import software.amazon.awssdk.crt.s3.S3Client;
import software.amazon.awssdk.crt.s3.S3MetaRequest;
import software.amazon.awssdk.crt.s3.S3MetaRequestOptions;
Expand Down Expand Up @@ -85,6 +91,7 @@ public void defaultRequest_shouldSetMetaRequestOptionsCorrectly() {
assertThat(actual.getMetaRequestType()).isEqualTo(S3MetaRequestOptions.MetaRequestType.DEFAULT);
assertThat(actual.getCredentialsProvider()).isNull();
assertThat(actual.getEndpoint().equals(DEFAULT_ENDPOINT));
assertThat(actual.getChecksumAlgorithm()).isEqualTo(ChecksumAlgorithm.CRC32);

HttpRequest httpRequest = actual.getHttpRequest();
assertThat(httpRequest.getEncodedPath()).isEqualTo("/key");
Expand Down Expand Up @@ -133,6 +140,23 @@ public void putObject_shouldSetMetaRequestTypeCorrectly() {
assertThat(actual.getMetaRequestType()).isEqualTo(S3MetaRequestOptions.MetaRequestType.PUT_OBJECT);
}

@Test
public void putObject_shouldSetChecksumAlgorithmCorrectly() {
ChecksumSpecs checksumSpecs = ChecksumSpecs.builder().algorithm(Algorithm.SHA1).build();
AsyncExecuteRequest asyncExecuteRequest = getExecuteRequestBuilder().putHttpExecutionAttribute(CHECKSUM_SPECS,
checksumSpecs).build();

ArgumentCaptor<S3MetaRequestOptions> s3MetaRequestOptionsArgumentCaptor =
ArgumentCaptor.forClass(S3MetaRequestOptions.class);

asyncHttpClient.execute(asyncExecuteRequest);

verify(s3Client).makeMetaRequest(s3MetaRequestOptionsArgumentCaptor.capture());

S3MetaRequestOptions actual = s3MetaRequestOptionsArgumentCaptor.getValue();
assertThat(actual.getChecksumAlgorithm()).isEqualTo(ChecksumAlgorithm.SHA1);
}

@Test
public void cancelRequest_shouldForwardCancellation() {
AsyncExecuteRequest asyncExecuteRequest = getExecuteRequestBuilder().build();
Expand Down