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.
aws · Jan 25, 2024 · 2cc8431 · 2cc8431
1 parent 0a6e10f
commit 2cc8431
Show file tree

Hide file tree

Showing 13 changed files with 170 additions and 180 deletions.
diff --git a/.changes/next-release/bugfix-AmazonSimpleStorageService-4b6b8dd.json b/.changes/next-release/bugfix-AmazonSimpleStorageService-4b6b8dd.json
@@ -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."
+}
diff --git a/...software/amazon/awssdk/services/s3/crossregion/S3CrossRegionAsyncIntegrationTestBase.java b/...software/amazon/awssdk/services/s3/crossregion/S3CrossRegionAsyncIntegrationTestBase.java
@@ -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;
@@ -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();
+    }
 }
diff --git a/...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionIntegrationTestBase.java b/...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionIntegrationTestBase.java
@@ -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;
@@ -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(
@@ -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();
 

diff --git a/...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionSyncIntegrationTest.java b/...java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionSyncIntegrationTest.java
@@ -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;
@@ -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;

diff --git a/...ava/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClient.java b/...ava/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClient.java
@@ -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(
+            requestWithDecoratedEndpointProvider(userAgentUpdatedRequest,
+                                                 () -> bucketToRegionCache.get(bucketName),
+                                                 serviceClientConfiguration().endpointProvider().get())
+        );
         apiOperationFuture.whenComplete(redirectToCrossRegionIfRedirectException(operation,
                                                                                  userAgentUpdatedRequest,
                                                                                  bucketName,

diff --git a/...java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClient.java b/...java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClient.java
@@ -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);

diff --git a/...azon/awssdk/services/s3/internal/crossregion/endpointprovider/BucketEndpointProvider.java b/...azon/awssdk/services/s3/internal/crossregion/endpointprovider/BucketEndpointProvider.java
@@ -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)));
     }
 }
 
diff --git a/.../java/software/amazon/awssdk/services/s3/internal/crossregion/utils/CrossRegionUtils.java b/.../java/software/amazon/awssdk/services/s3/internal/crossregion/utils/CrossRegionUtils.java
@@ -41,7 +41,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);
 
@@ -65,12 +64,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")

diff --git a/.../amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java b/.../amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java
@@ -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() {
 
@@ -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)

diff --git a/...software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java b/...software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java
@@ -17,9 +17,10 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.CHANGED_CROSS_REGION;
 import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.CROSS_REGION;
-import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.CROSS_REGION_BUCKET;
 import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.OVERRIDE_CONFIGURED_REGION;
 import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.X_AMZ_BUCKET_REGION;
 
@@ -37,21 +38,29 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
 import software.amazon.awssdk.core.ResponseBytes;
 import software.amazon.awssdk.core.async.AsyncResponseTransformer;
 import software.amazon.awssdk.core.interceptor.Context;
 import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
 import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
 import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
+import software.amazon.awssdk.endpoints.Endpoint;
 import software.amazon.awssdk.endpoints.EndpointProvider;
 import software.amazon.awssdk.http.AbortableInputStream;
 import software.amazon.awssdk.http.HttpExecuteResponse;
 import software.amazon.awssdk.http.SdkHttpFullResponse;
 import software.amazon.awssdk.http.SdkHttpMethod;
 import software.amazon.awssdk.http.SdkHttpRequest;
 import software.amazon.awssdk.http.SdkHttpResponse;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3AsyncClient;
 import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams;
+import software.amazon.awssdk.services.s3.endpoints.S3EndpointProvider;
 import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
 import software.amazon.awssdk.services.s3.internal.crossregion.endpointprovider.BucketEndpointProvider;
 import software.amazon.awssdk.services.s3.model.GetObjectRequest;
@@ -87,16 +96,13 @@ private static Stream<Arguments> stubSuccessfulRedirectResponses() {
         Consumer<MockAsyncHttpClient> successStubConsumer = mockAsyncHttpClient ->
             mockAsyncHttpClient.stubResponses(successHttpResponse(), successHttpResponse());
 
-        Consumer<MockAsyncHttpClient> malFormerAuthError = mockAsyncHttpClient ->
-            mockAsyncHttpClient.stubResponses(
-                customHttpResponse(400, "AuthorizationHeaderMalformed", null),
-                customHttpResponse(400, "AuthorizationHeaderMalformed", CROSS_REGION_BUCKET),
-                successHttpResponse());
+        Consumer<MockAsyncHttpClient> tempRedirectStubConsumer = mockAsyncHttpClient ->
+            mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(307, CROSS_REGION.id()), successHttpResponse());
 
         return Stream.of(
             Arguments.of(redirectStubConsumer, BucketEndpointProvider.class, "Redirect Error with region in x-amz-bucket-header"),
-            Arguments.of(successStubConsumer, DefaultS3EndpointProvider.class, "Success response" ),
-            Arguments.of(malFormerAuthError, BucketEndpointProvider.class, "Authorization Malformed Error with region in x-amz-bucket-header in Head bucket response" )
+            Arguments.of(successStubConsumer, BucketEndpointProvider.class, "Success response" ),
+            Arguments.of(tempRedirectStubConsumer, BucketEndpointProvider.class, "Permanent redirect Error with region in x-amz-bucket-header" )
         );
     }
 
@@ -116,21 +122,8 @@ private static Stream<Arguments> stubFailureResponses() {
             mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(301, null),
                                               customHttpResponseWithUnknownErrorCode(301, null),
                                               successHttpResponse(), successHttpResponse());
-
-        Consumer<MockAsyncHttpClient> authMalformedWithNoRegion = mockAsyncHttpClient ->
-            mockAsyncHttpClient.stubResponses(customHttpResponse(400, "AuthorizationHeaderMalformed", null),
-                                              customHttpResponse(400, "AuthorizationHeaderMalformed", null));
-
-        Consumer<MockAsyncHttpClient> authMalformedAuthorizationFailureAfterRegionRetrieval = mockAsyncHttpClient ->
-            mockAsyncHttpClient.stubResponses(customHttpResponse(400, "AuthorizationHeaderMalformed", null),
-                                              customHttpResponse(400, "AuthorizationHeaderMalformed", CROSS_REGION.id()),
-                                              customHttpResponse(400, "AuthorizationHeaderMalformed", CROSS_REGION.id()));
-
         return Stream.of(
-            Arguments.of(redirectFailedWithNoRegionFailure, 301, 2, noRegionOnHeadBucket, noregionOnHeadBucketHttpMethodListMethodList),
-            Arguments.of(authMalformedWithNoRegion, 400, 2, noRegionOnHeadBucket, noregionOnHeadBucketHttpMethodListMethodList),
-            Arguments.of(authMalformedAuthorizationFailureAfterRegionRetrieval, 400, 3, regionOnHeadBucket,
-                         regionOnHeadBucketHttpMethodList)
+            Arguments.of(redirectFailedWithNoRegionFailure, 301, 2, noRegionOnHeadBucket, noregionOnHeadBucketHttpMethodListMethodList)
         );
     }
 
@@ -416,10 +409,49 @@ void given_SimpleClient_when_StandardOperation_then_DoesNotContainCrossRegionUse
         assertThat(mockAsyncHttpClient.getLastRequest().firstMatchingHeader("User-Agent").get()).doesNotContain("hll/cross");
     }
 
+    @Test
+    void given_US_EAST_1_Client_resolvesToGlobalEndpoints_when_crossRegion_is_False(){
+        mockAsyncHttpClient.stubResponses(successHttpResponse());
+        S3AsyncClient s3Client = clientBuilder().region(Region.US_EAST_1).build();
+        s3Client.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
+        assertThat(mockAsyncHttpClient.getLastRequest().host()).isEqualTo("bucket.s3.amazonaws.com");
+        assertThat(captureInterceptor.endpointProvider).isInstanceOf(DefaultS3EndpointProvider.class);
+    }
+
+    @Test
+    void given_US_EAST_1_Client_resolveToRegionalEndpoints_when_crossRegion_is_True(){
+        mockAsyncHttpClient.stubResponses(successHttpResponse());
+        S3AsyncClient s3Client = clientBuilder().crossRegionAccessEnabled(true).region(Region.US_EAST_1).build();
+        s3Client.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
+        assertThat(mockAsyncHttpClient.getLastRequest().host()).isEqualTo("bucket.s3.us-east-1.amazonaws.com");
+        assertThat(captureInterceptor.endpointProvider).isInstanceOf(BucketEndpointProvider.class);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"us-east-1", "us-east-2", "us-west-1", "aws-global"})
+    void given_AnyRegion_Client_Updates_the_useGlobalEndpointFlag_asFalse(String region) {
+        mockAsyncHttpClient.stubResponses(successHttpResponse());
+        S3EndpointProvider mockEndpointProvider = Mockito.mock(S3EndpointProvider.class);
+
+        when(mockEndpointProvider.resolveEndpoint(ArgumentMatchers.any(S3EndpointParams.class)))
+            .thenReturn(CompletableFuture.completedFuture(Endpoint.builder().url(URI.create("https://bucket.s3.amazonaws.com")).build()));
+
+        S3AsyncClient s3Client = clientBuilder().crossRegionAccessEnabled(true)
+                                           .region(Region.of(region))
+                                           .endpointProvider(mockEndpointProvider).build();
+        s3Client.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
+        assertThat(captureInterceptor.endpointProvider).isInstanceOf(BucketEndpointProvider.class);
+        ArgumentCaptor<S3EndpointParams> collectionCaptor = ArgumentCaptor.forClass(S3EndpointParams.class);
+        verify(mockEndpointProvider).resolveEndpoint(collectionCaptor.capture());
+        S3EndpointParams resolvedParams = collectionCaptor.getAllValues().get(0);
+        assertThat(resolvedParams.region()).isEqualTo(Region.of(region));
+        assertThat(resolvedParams.useGlobalEndpoint()).isEqualTo(false);
+    }
+
+
     private S3AsyncClientBuilder clientBuilder() {
         return S3AsyncClient.builder()
                             .httpClient(mockAsyncHttpClient)
-                            .endpointOverride(URI.create("http://localhost"))
                             .overrideConfiguration(c -> c.addExecutionInterceptor(captureInterceptor));
     }