Skip to content
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

Fix for issue #4720 , Cross Region enabled Clients created in US-EAST-1 will by internally disable global endpoint and do a regional endpoint call. #4849

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Fix for issue #4720 , Cross Region enabled Clients created in US-EAST-1 will by internally disable global endpoint and do a regional endpoint call. #4849

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4adf3f2
Fix for issue #4720 , Cross Region enabled Clients created in US-EAST…
joviegas Jan 25, 2024
d2c2409
Handled Zoe's comments
joviegas Jan 26, 2024
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: 6 additions & 0 deletions .changes/next-release/bugfix-AmazonSimpleStorageService-4b6b8dd.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "bugfix",
"category": "Amazon Simple Storage Service",
"contributor": "",
"description": "Fix for issue [#4720](https://github.com/aws/aws-sdk-java-v2/issues/4720) , Cross Region enabled Clients created in US-EAST-1 will by internally disable global endpoint and do a regional endpoint call."
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, suggestion:
"S3 client configured with crossRegionEnabled(true) will now use us-east-1 regional endpoint instead of the global endpoint. See #4720"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Zoe

}
7 changes: 7 additions & 0 deletions ...software/amazon/awssdk/services/s3/crossregion/S3CrossRegionAsyncIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
import software.amazon.awssdk.services.s3.model.HeadBucketRequest;
import software.amazon.awssdk.services.s3.model.HeadBucketResponse;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
import software.amazon.awssdk.services.s3.model.PutObjectResponse;
Expand Down Expand Up @@ -74,4 +76,9 @@ protected PutObjectResponse putAPICall(PutObjectRequest putObjectRequest, String
protected ResponseBytes<GetObjectResponse> getAPICall(GetObjectRequest getObjectRequest) {
return crossRegionS3Client.getObject(getObjectRequest, AsyncResponseTransformer.toBytes()).join();
}

@Override
protected HeadObjectResponse headObjectAPICall(HeadObjectRequest headObjectRequest) {
return crossRegionS3Client.headObject(headObjectRequest).join();
}
}
13 changes: 13 additions & 0 deletions ...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionIntegrationTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
import software.amazon.awssdk.services.s3.model.HeadBucketRequest;
import software.amazon.awssdk.services.s3.model.HeadBucketResponse;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
import software.amazon.awssdk.services.s3.model.PutObjectResponse;
Expand All @@ -58,6 +60,16 @@ void getApi_CrossRegionCall() {
assertThat(new String(response.asByteArray())).isEqualTo("TEST_STRING");
}

@Test
void headObjectApi_CrossRegionCall() {
s3.putObject(p -> p.bucket(bucketName()).checksumAlgorithm(ChecksumAlgorithm.CRC32).key(KEY), RequestBody.fromString(
"TEST_STRING"));
HeadObjectRequest headObjectRequest =
HeadObjectRequest.builder().bucket(bucketName()).checksumMode(ChecksumMode.ENABLED).key(KEY).build();
HeadObjectResponse response = headObjectAPICall(headObjectRequest);
assertThat(response.contentLength()).isEqualTo("TEST_STRING".length());
}

@Test
void putApi_CrossRegionCall() {
s3.putObject(p -> p.bucket(bucketName()).checksumAlgorithm(ChecksumAlgorithm.CRC32).key(KEY), RequestBody.fromString(
Expand Down Expand Up @@ -136,6 +148,7 @@ void headApi_CrossRegionCall() {
protected abstract PutObjectResponse putAPICall(PutObjectRequest putObjectRequest, String testString);

protected abstract ResponseBytes<GetObjectResponse> getAPICall(GetObjectRequest getObjectRequest);
protected abstract HeadObjectResponse headObjectAPICall(HeadObjectRequest headObjectRequest);

protected abstract String bucketName();

Expand Down
7 changes: 7 additions & 0 deletions ...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionSyncIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
import software.amazon.awssdk.services.s3.model.HeadBucketRequest;
import software.amazon.awssdk.services.s3.model.HeadBucketResponse;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.ListObjectsV2Request;
import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
Expand Down Expand Up @@ -101,6 +103,11 @@ protected ResponseBytes<GetObjectResponse> getAPICall(GetObjectRequest getObject
return crossRegionS3Client.getObject(getObjectRequest, ResponseTransformer.toBytes());
}

@Override
protected HeadObjectResponse headObjectAPICall(HeadObjectRequest headObjectRequest) {
return crossRegionS3Client.headObject(headObjectRequest);
}

@Override
protected String bucketName() {
return BUCKET;
Expand Down
16 changes: 5 additions & 11 deletions ...ava/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,17 @@ protected <T extends S3Request, ReturnT> CompletableFuture<ReturnT> invokeOperat

AwsRequestOverrideConfiguration overrideConfiguration = updateUserAgentInConfig(request);
T userAgentUpdatedRequest = (T) request.toBuilder().overrideConfiguration(overrideConfiguration).build();

if (!bucket.isPresent()) {
return operation.apply(userAgentUpdatedRequest);
}
String bucketName = bucket.get();

CompletableFuture<ReturnT> returnFuture = new CompletableFuture<>();
CompletableFuture<ReturnT> apiOperationFuture = bucketToRegionCache.containsKey(bucketName) ?
operation.apply(
requestWithDecoratedEndpointProvider(
userAgentUpdatedRequest,
() -> bucketToRegionCache.get(bucketName),
serviceClientConfiguration().endpointProvider().get()
)
) :
operation.apply(userAgentUpdatedRequest);

CompletableFuture<ReturnT> apiOperationFuture = operation.apply(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing bucketToRegionCache.containsKey(bucketName) check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier, for the first calls, since CachedBucket was null, we used to skip overriding the EndpointProvider because we were making the calls as they were. Now, since we need to update the 'useGlobalEndpoint' even for the first call, we need to override the EndpointProvider

requestWithDecoratedEndpointProvider(userAgentUpdatedRequest,
() -> bucketToRegionCache.get(bucketName),
serviceClientConfiguration().endpointProvider().get())
);
apiOperationFuture.whenComplete(redirectToCrossRegionIfRedirectException(operation,
userAgentUpdatedRequest,
bucketName,
Expand Down
11 changes: 4 additions & 7 deletions ...java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,10 @@ protected <T extends S3Request, ReturnT> ReturnT invokeOperation(T request, Func
}
String bucketName = bucketRequest.get();
try {
if (bucketToRegionCache.containsKey(bucketName)) {
return operation.apply(
requestWithDecoratedEndpointProvider(userAgentUpdatedRequest,
() -> bucketToRegionCache.get(bucketName),
serviceClientConfiguration().endpointProvider().get()));
}
return operation.apply(userAgentUpdatedRequest);
return operation.apply(
requestWithDecoratedEndpointProvider(userAgentUpdatedRequest,
() -> bucketToRegionCache.get(bucketName),
serviceClientConfiguration().endpointProvider().get()));
} catch (S3Exception exception) {
if (isS3RedirectException(exception)) {
updateCacheFromRedirectException(exception, bucketName);
Expand Down
3 changes: 2 additions & 1 deletion ...azon/awssdk/services/s3/internal/crossregion/endpointprovider/BucketEndpointProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public static BucketEndpointProvider create(S3EndpointProvider delegateEndPointP
public CompletableFuture<Endpoint> resolveEndpoint(S3EndpointParams endpointParams) {
Region crossRegion = regionSupplier.get();
return delegateEndPointProvider.resolveEndpoint(
crossRegion != null ? endpointParams.copy(c -> c.region(crossRegion)) : endpointParams);
endpointParams.copy(c -> c.region(crossRegion == null ? endpointParams.region() : crossRegion)
.useGlobalEndpoint(false)));
}
}

10 changes: 1 addition & 9 deletions .../java/software/amazon/awssdk/services/s3/internal/crossregion/utils/CrossRegionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@


import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.CompletionException;
import java.util.function.Consumer;
import java.util.function.Supplier;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.awscore.AwsRequestOverrideConfiguration;
import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
import software.amazon.awssdk.core.ApiName;
import software.amazon.awssdk.endpoints.EndpointProvider;
import software.amazon.awssdk.regions.Region;
Expand All @@ -41,7 +39,6 @@ public final class CrossRegionUtils {
public static final String AMZ_BUCKET_REGION_HEADER = "x-amz-bucket-region";
private static final List<Integer> REDIRECT_STATUS_CODES =
Arrays.asList(REDIRECT_STATUS_CODE, TEMPORARY_REDIRECT_STATUS_CODE);
private static final List<String> REDIRECT_ERROR_CODES = Collections.singletonList("AuthorizationHeaderMalformed");
private static final ApiName API_NAME = ApiName.builder().version("cross-region").name("hll").build();
private static final Consumer<AwsRequestOverrideConfiguration.Builder> USER_AGENT_APPLIER = b -> b.addApiName(API_NAME);

Expand All @@ -65,12 +62,7 @@ private static boolean isRedirectError(S3Exception exceptionToBeChecked) {
if (REDIRECT_STATUS_CODES.stream().anyMatch(status -> status.equals(exceptionToBeChecked.statusCode()))) {
return true;
}
if (getBucketRegionFromException(exceptionToBeChecked).isPresent()) {
return true;
}
AwsErrorDetails awsErrorDetails = exceptionToBeChecked.awsErrorDetails();
return awsErrorDetails != null
&& REDIRECT_ERROR_CODES.stream().anyMatch(code -> code.equals(awsErrorDetails.errorCode()));
return getBucketRegionFromException(exceptionToBeChecked).isPresent();
}

@SuppressWarnings("unchecked")
Expand Down
18 changes: 0 additions & 18 deletions .../amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,6 @@ protected void verifyHeadBucketServiceCall(int times) {
verify(mockDelegateAsyncClient, times(times)).headBucket(any(Consumer.class));
}

@Override
protected void stubApiWithAuthorizationHeaderMalformedError() {
when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(
new CompletionException(redirectException(400, null, "AuthorizationHeaderMalformed", null))))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()));
}

@Override
protected void stubApiWithAuthorizationHeaderWithInternalSoftwareError() {

Expand All @@ -150,16 +142,6 @@ protected void stubApiWithAuthorizationHeaderWithInternalSoftwareError() {
"InternalError", null))));
}

@Override
protected void stubHeadBucketRedirectWithAuthorizationHeaderMalformed() {
when(mockDelegateAsyncClient.headBucket(any(HeadBucketRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(
new CompletionException(redirectException(400,CROSS_REGION.id(), "AuthorizationHeaderMalformed", null))));
when(mockDelegateAsyncClient.headBucket(any(Consumer.class)))
.thenReturn(CompletableFutureUtils.failedFuture(
new CompletionException(redirectException(400,CROSS_REGION.id(), "AuthorizationHeaderMalformed", null))));
}

@Override
protected void verifyNoBucketCall() {
assertThatExceptionOfType(CompletionException.class)
Expand Down
Loading
Loading