Allow redirect for 400 - IllegalLocationConstraintException in s3 cro…

…ss region client (#5678) * add 400 IllegalLocationConstraint to redirect error * add 400 IllegalLocationConstraint to redirect error * changelog * checkstyle * Added test for 400 with IllegalLocationConstrain error code
aws · Oct 25, 2024 · 105d545 · 105d545
1 parent 9ab4e22
commit 105d545
Show file tree

Hide file tree

Showing 7 changed files with 135 additions and 32 deletions.
diff --git a/.changes/next-release/bugfix-AmazonS3-527812c.json b/.changes/next-release/bugfix-AmazonS3-527812c.json
@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "Amazon S3",
+    "contributor": "",
+    "description": "Update the S3 client to correctly handle redirect cases for opt-in regions when crossRegionAccessEnabled is used."
+}
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
@@ -36,6 +36,7 @@
 public final class CrossRegionUtils {
     public static final int REDIRECT_STATUS_CODE = 301;
     public static final int TEMPORARY_REDIRECT_STATUS_CODE = 307;
+    public static final String ILLEGAL_LOCATION_CONSTRAINT_EXCEPTION_ERROR_CODE = "IllegalLocationConstraintException";
     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);
@@ -59,7 +60,16 @@ public static boolean isS3RedirectException(Throwable exception) {
     }
 
     private static boolean isRedirectError(S3Exception exceptionToBeChecked) {
-        if (REDIRECT_STATUS_CODES.stream().anyMatch(status -> status.equals(exceptionToBeChecked.statusCode()))) {
+        boolean matchRedirectStatusCode =
+            REDIRECT_STATUS_CODES.stream()
+                                 .anyMatch(status -> status.equals(exceptionToBeChecked.statusCode()));
+        if (matchRedirectStatusCode) {
+            return true;
+        }
+        boolean is400IllegalLocationConstraintException =
+            exceptionToBeChecked.statusCode() == 400
+            && ILLEGAL_LOCATION_CONSTRAINT_EXCEPTION_ERROR_CODE.equals(exceptionToBeChecked.awsErrorDetails().errorCode());
+        if (is400IllegalLocationConstraintException) {
             return true;
         }
         return getBucketRegionFromException(exceptionToBeChecked).isPresent();

diff --git a/.../amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java b/.../amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java
@@ -49,8 +49,10 @@ public void setup() {
     @Override
     protected void stubRedirectSuccessSuccess(Integer redirect) {
         when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
-            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(),
-                                                                                                      null, null))))
+            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect,
+                                                                                                      CROSS_REGION.id(),
+                                                                                                      errorCodeFromRedirect(redirect),
+                                                                                                      null))))
             .thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()))
             .thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()));
     }
@@ -77,7 +79,10 @@ protected void stubServiceClientConfiguration() {
     @Override
     protected void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse(Integer redirect) {
         when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
-           .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(), null, null))))
+           .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect,
+                                                                                                     CROSS_REGION.id(),
+                                                                                                     errorCodeFromRedirect(redirect),
+                                                                                                     null))))
             .thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()
             ));
     }
@@ -112,15 +117,20 @@ protected void stubHeadBucketRedirect() {
     @Override
     protected void stubRedirectWithNoRegionAndThenSuccess(Integer redirect) {
         when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
-            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, null, null, null))))
+            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect,
+                                                                                                      null,
+                                                                                                      errorCodeFromRedirect(redirect),
+                                                                                                      null))))
             .thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()))
             .thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()));
     }
 
     @Override
     protected void stubRedirectThenError(Integer redirect) {
         when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
-            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(), null,
+            .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect,
+                                                                                                      CROSS_REGION.id(),
+                                                                                                      errorCodeFromRedirect(redirect),
                                                                                                       null))))
             .thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(400, null,
                                                                                                        "InvalidArgument", "Invalid id"))));

diff --git a/...software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java b/...software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java
@@ -26,11 +26,13 @@
 import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionRedirectTestBase.X_AMZ_BUCKET_REGION;
 
 import java.net.URI;
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -80,6 +82,7 @@
 
 class S3CrossRegionAsyncClientTest {
 
+    private static final String ILLEGAL_LOCATION_CONSTRAINT_EXCEPTION_ERROR_CODE = "IllegalLocationConstraintException";
     private static final String ERROR_RESPONSE_FORMAT = "<Error>\\n\\t<Code>%s</Code>\\n</Error>";
     private static final String BUCKET = "bucket";
     private static final String KEY = "key";
@@ -97,26 +100,38 @@ void setUp() {
 
     private static Stream<Arguments> stubSuccessfulRedirectResponses() {
         Consumer<MockAsyncHttpClient> redirectStubConsumer = mockAsyncHttpClient ->
-            mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(301, CROSS_REGION.id()), successHttpResponse());
+            mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(301, CROSS_REGION.id()),
+                                              successHttpResponse());
 
         Consumer<MockAsyncHttpClient> successStubConsumer = mockAsyncHttpClient ->
             mockAsyncHttpClient.stubResponses(successHttpResponse(), successHttpResponse());
 
         Consumer<MockAsyncHttpClient> tempRedirectStubConsumer = mockAsyncHttpClient ->
-            mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(307, CROSS_REGION.id()), successHttpResponse());
+            mockAsyncHttpClient.stubResponses(customHttpResponseWithUnknownErrorCode(307, CROSS_REGION.id()),
+                                              successHttpResponse());
+
+        Consumer<MockAsyncHttpClient> locationConstraintExceptionConsumer = mockAsyncHttpClient ->
+            mockAsyncHttpClient.stubResponses(customHttpResponse(400,
+                                                                 ILLEGAL_LOCATION_CONSTRAINT_EXCEPTION_ERROR_CODE,
+                                                                 CROSS_REGION.id()), successHttpResponse());
+
 
         return Stream.of(
             Arguments.of(redirectStubConsumer, BucketEndpointProvider.class, "Redirect Error with region in x-amz-bucket-header"),
-            Arguments.of(successStubConsumer, BucketEndpointProvider.class, "Success response" ),
-            Arguments.of(tempRedirectStubConsumer, BucketEndpointProvider.class, "Permanent redirect Error with region in x-amz-bucket-header" )
+            Arguments.of(successStubConsumer, BucketEndpointProvider.class, "Success response"),
+            Arguments.of(tempRedirectStubConsumer, BucketEndpointProvider.class,
+                         "Permanent redirect Error with region in x-amz-bucket-header"),
+            Arguments.of(locationConstraintExceptionConsumer, BucketEndpointProvider.class,
+                         "Redirect error: 400 IllegalLocationConstraintException with region in x-amz-bucket-header")
         );
     }
 
 
     private static Stream<Arguments> stubFailureResponses() {
 
         List<SdkHttpMethod> noregionOnHeadBucketHttpMethodListMethodList = Arrays.asList(SdkHttpMethod.GET, SdkHttpMethod.HEAD);
-        List<SdkHttpMethod> regionOnHeadBucketHttpMethodList = Arrays.asList(SdkHttpMethod.GET, SdkHttpMethod.HEAD, SdkHttpMethod.GET);
+        List<SdkHttpMethod> regionOnHeadBucketHttpMethodList = Arrays.asList(SdkHttpMethod.GET, SdkHttpMethod.HEAD,
+                                                                             SdkHttpMethod.GET);
         List<String> noRegionOnHeadBucket = Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
                                                           OVERRIDE_CONFIGURED_REGION.toString());
 
@@ -129,7 +144,8 @@ private static Stream<Arguments> stubFailureResponses() {
                                               customHttpResponseWithUnknownErrorCode(301, null),
                                               successHttpResponse(), successHttpResponse());
         return Stream.of(
-            Arguments.of(redirectFailedWithNoRegionFailure, 301, 2, noRegionOnHeadBucket, noregionOnHeadBucketHttpMethodListMethodList)
+            Arguments.of(redirectFailedWithNoRegionFailure, 301, 2, noRegionOnHeadBucket,
+                         noregionOnHeadBucketHttpMethodListMethodList)
         );
     }
 
@@ -159,8 +175,8 @@ public static HttpExecuteResponse customHttpResponse(int statusCode, String erro
     @ParameterizedTest(name = "{index} - {2}.")
     @MethodSource("stubSuccessfulRedirectResponses")
     void given_CrossRegionClientWithNoOverrideConfig_when_StandardOperationIsPerformed_then_SuccessfullyIntercepts(Consumer<MockAsyncHttpClient> stubConsumer,
-                                                                              Class<?> endpointProviderType,
-                                                                              String testCase) {
+                                                                                                                   Class<?> endpointProviderType,
+                                                                                                                   String testCase) {
         stubConsumer.accept(mockAsyncHttpClient);
         S3AsyncClient crossRegionClient = new S3CrossRegionAsyncClient(s3Client);
         crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
@@ -170,8 +186,9 @@ void given_CrossRegionClientWithNoOverrideConfig_when_StandardOperationIsPerform
     @ParameterizedTest(name = "{index} - {2}.")
     @MethodSource("stubSuccessfulRedirectResponses")
     void given_CrossRegionClientWithExistingOverrideConfig_when_StandardOperationIsPerformed_then_SuccessfullyIntercepts(Consumer<MockAsyncHttpClient> stubConsumer,
-                                                                                    Class<?> endpointProviderType,
-                                                                                    String testCase) {
+                                                                                                                         Class<
+                                                                                                                             ?> endpointProviderType,
+                                                                                                                         String testCase) {
         stubConsumer.accept(mockAsyncHttpClient);
         S3AsyncClient crossRegionClient = new S3CrossRegionAsyncClient(s3Client);
         GetObjectRequest request = GetObjectRequest.builder()
@@ -187,8 +204,8 @@ void given_CrossRegionClientWithExistingOverrideConfig_when_StandardOperationIsP
     @ParameterizedTest(name = "{index} - {2}.")
     @MethodSource("stubSuccessfulRedirectResponses")
     void given_CrossRegionClient_when_PaginatedOperationIsPerformed_then_DoesNotIntercept(Consumer<MockAsyncHttpClient> stubConsumer,
-                                                     Class<?> endpointProviderType,
-                                                     String testCase) throws Exception {
+                                                                                          Class<?> endpointProviderType,
+                                                                                          String testCase) throws Exception {
         stubConsumer.accept(mockAsyncHttpClient);
         S3AsyncClient crossRegionClient = new S3CrossRegionAsyncClient(s3Client);
         ListObjectsV2Publisher publisher =
@@ -201,8 +218,8 @@ void given_CrossRegionClient_when_PaginatedOperationIsPerformed_then_DoesNotInte
     @ParameterizedTest(name = "{index} - {2}.")
     @MethodSource("stubSuccessfulRedirectResponses")
     void given_CrossRegionClientCreatedWithWrapping_when_OperationIsPerformed_then_SuccessfullyIntercepts(Consumer<MockAsyncHttpClient> stubConsumer,
-                                                                      Class<?> endpointProviderType,
-                                                                      String testCase) {
+                                                                                                          Class<?> endpointProviderType,
+                                                                                                          String testCase) {
         stubConsumer.accept(mockAsyncHttpClient);
         S3AsyncClient crossRegionClient = clientBuilder().crossRegionAccessEnabled(true).build();
         crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
@@ -264,7 +281,7 @@ void given_CrossRegionClient_when_CallsHeadObjectErrors_then_ShouldTerminateTheA
 
         String errorMessage = String.format("software.amazon.awssdk.services.s3.model.S3Exception: "
                                             + "(Service: S3, Status Code: %d, Request ID: null)"
-                , statusCode);
+            , statusCode);
         assertThatExceptionOfType(CompletionException.class)
             .isThrownBy(() -> crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join())
             .withMessageContaining(errorMessage);
@@ -395,7 +412,6 @@ void given_CrossRegionClient_when_StandardOperation_then_ContainsUserAgent() {
     }
 
 
-
     @ParameterizedTest(name = "{index} - {2}.")
     @MethodSource("stubSuccessfulRedirectResponses")
     void given_CrossRegionAccessEnabled_when_SuccessfulResponse_then_EndpointIsUpdated(Consumer<MockAsyncHttpClient> stubConsumer,
@@ -416,7 +432,7 @@ void given_SimpleClient_when_StandardOperation_then_DoesNotContainCrossRegionUse
     }
 
     @Test
-    void given_US_EAST_1_Client_resolvesToGlobalEndpoints_when_crossRegion_is_False(){
+    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();
@@ -425,7 +441,7 @@ void given_US_EAST_1_Client_resolvesToGlobalEndpoints_when_crossRegion_is_False(
     }
 
     @Test
-    void given_US_EAST_1_Client_resolveToRegionalEndpoints_when_crossRegion_is_True(){
+    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();
@@ -478,6 +494,30 @@ void given_globalRegion_Client_Updates_region_to_useast1_and_useGlobalEndpointFl
 
     }
 
+    @Test
+    void given_CrossRegionClient_when400Error_WithoutIllegalLocationConstraint_DoesNotRedirect() {
+        String region = Region.AWS_GLOBAL.id();
+        mockAsyncHttpClient.stubResponses(customHttpResponse(400, "UnknownError", null));
+        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();
+
+        CompletableFuture<ResponseBytes<GetObjectResponse>> f =
+            s3Client.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes());
+
+        assertThat(f).failsWithin(Duration.ofSeconds(5))
+                     .withThrowableOfType(ExecutionException.class)
+                     .withCauseInstanceOf(S3Exception.class)
+                     .withMessageContaining("Status Code: 400");
+
+        assertThat(mockAsyncHttpClient.getRequests()).hasSize(1);
+    }
+
+
     private S3AsyncClientBuilder clientBuilder() {
         return S3AsyncClient.builder()
                             .httpClient(mockAsyncHttpClient)