Fix for Issue #4912 where Cross Region S3 Access using AWS_GLOBAL Endpoint failed. (#4921)

joviegas · web-flow · commit f42ebdd98f88 · 2024-02-13T15:53:35.000-08:00
diff --git a/.changes/next-release/bugfix-AmazonSimpleStorageService-7648336.json b/.changes/next-release/bugfix-AmazonSimpleStorageService-7648336.json
@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "Amazon Simple Storage Service",
+    "contributor": "",
+    "description": "Fix for Issue [#4912](https://github.com/aws/aws-sdk-java-v2/issues/4912) where client region with AWS_GLOBAL calls failed for cross region access."
+}
diff --git a/services/s3/src/it/java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionCrtIntegrationTest.java b/services/s3/src/it/java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionCrtIntegrationTest.java
@@ -46,7 +46,7 @@ static void clearClass() {
     @BeforeEach
     public void initialize() {
         crossRegionS3Client = S3AsyncClient.crtBuilder()
-                                           .region(CROSS_REGION)
+                                           .region(Region.AWS_GLOBAL)
                                            .crossRegionAccessEnabled(true)
                                              .build();
     }
diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crossregion/endpointprovider/BucketEndpointProvider.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crossregion/endpointprovider/BucketEndpointProvider.java
@@ -43,9 +43,20 @@ public static BucketEndpointProvider create(S3EndpointProvider delegateEndPointP
     @Override
     public CompletableFuture<Endpoint> resolveEndpoint(S3EndpointParams endpointParams) {
         Region crossRegion = regionSupplier.get();
-        return delegateEndPointProvider.resolveEndpoint(
-            endpointParams.copy(c -> c.region(crossRegion == null ? endpointParams.region() : crossRegion)
-                                      .useGlobalEndpoint(false)));
+        S3EndpointParams.Builder endpointParamsBuilder = endpointParams.toBuilder();
+        // Check if cross-region resolution has already occurred.
+        if (crossRegion != null) {
+            endpointParamsBuilder.region(crossRegion);
+        } else {
+            // For global regions, set the region to "us-east-1" to use regional endpoints.
+            if (Region.AWS_GLOBAL.equals(endpointParams.region())) {
+                endpointParamsBuilder.region(Region.US_EAST_1);
+            }
+            // Disable the global endpoint as S3 can properly redirect regions in the 'x-amz-bucket-region' header
+            // only for regional endpoints.
+            endpointParamsBuilder.useGlobalEndpoint(false);
+        }
+        return delegateEndPointProvider.resolveEndpoint(endpointParamsBuilder.build());
     }
 }
 
diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java
@@ -429,7 +429,7 @@ void given_US_EAST_1_Client_resolveToRegionalEndpoints_when_crossRegion_is_True(
     }
 
     @ParameterizedTest
-    @ValueSource(strings = {"us-east-1", "us-east-2", "us-west-1", "aws-global"})
+    @ValueSource(strings = {"us-east-1", "us-east-2", "us-west-1"})
     void given_AnyRegion_Client_Updates_the_useGlobalEndpointFlag_asFalse(String region) {
         mockAsyncHttpClient.stubResponses(successHttpResponse());
         S3EndpointProvider mockEndpointProvider = Mockito.mock(S3EndpointProvider.class);
@@ -450,6 +450,28 @@ void given_AnyRegion_Client_Updates_the_useGlobalEndpointFlag_asFalse(String reg
         });
     }
 
+    @Test
+    void given_globalRegion_Client_Updates_region_to_useast1_and_useGlobalEndpointFlag_as_False() {
+        String region = Region.AWS_GLOBAL.id();
+        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, atLeastOnce()).resolveEndpoint(collectionCaptor.capture());
+        collectionCaptor.getAllValues().forEach(resolvedParams -> {
+            assertThat(resolvedParams.region()).isEqualTo(Region.US_EAST_1);
+            assertThat(resolvedParams.useGlobalEndpoint()).isFalse();
+        });
+    }
+
     private S3AsyncClientBuilder clientBuilder() {
         return S3AsyncClient.builder()
                             .httpClient(mockAsyncHttpClient)
diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClientTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClientTest.java
@@ -256,7 +256,7 @@ void given_US_EAST_1_Client_resolveToRegionalEndpoints_when_crossRegion_is_True(
     }
 
     @ParameterizedTest
-    @ValueSource(strings = {"us-east-1", "us-east-2", "us-west-1", "aws-global"})
+    @ValueSource(strings = {"us-east-1", "us-east-2", "us-west-1"})
     void given_AnyRegion_Client_Updates_the_useGlobalEndpointFlag_asFalse(String region) {
         mockSyncHttpClient.stubResponses(successHttpResponse());
         S3EndpointProvider mockEndpointProvider = Mockito.mock(S3EndpointProvider.class);
@@ -277,6 +277,28 @@ void given_AnyRegion_Client_Updates_the_useGlobalEndpointFlag_asFalse(String reg
         });
     }
 
+    @Test
+    void given_globalRegion_Client_Updates_region_to_useast1_and_useGlobalEndpointFlag_as_False() {
+        String region = Region.AWS_GLOBAL.id();
+        mockSyncHttpClient.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()));
+
+        S3Client s3Client = clientBuilder().crossRegionAccessEnabled(true)
+                                           .region(Region.of(region))
+                                           .endpointProvider(mockEndpointProvider).build();
+        s3Client.getObject(getObjectBuilder().build());
+        assertThat(captureInterceptor.endpointProvider).isInstanceOf(BucketEndpointProvider.class);
+        ArgumentCaptor<S3EndpointParams> collectionCaptor = ArgumentCaptor.forClass(S3EndpointParams.class);
+        verify(mockEndpointProvider,  atLeastOnce()).resolveEndpoint(collectionCaptor.capture());
+        collectionCaptor.getAllValues().forEach(resolvedParams ->{
+            assertThat(resolvedParams.region()).isEqualTo(Region.US_EAST_1);
+            assertThat(resolvedParams.useGlobalEndpoint()).isFalse();
+        });
+    }
+
     private static GetObjectRequest.Builder getObjectBuilder() {
         return GetObjectRequest.builder()
                                .bucket(BUCKET)

Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ static void clearClass() {`
`46`	`46`	`@BeforeEach`
`47`	`47`	`public void initialize() {`
`48`	`48`	`crossRegionS3Client = S3AsyncClient.crtBuilder()`
`49`		`- .region(CROSS_REGION)`
	`49`	`+ .region(Region.AWS_GLOBAL)`
`50`	`50`	`.crossRegionAccessEnabled(true)`
`51`	`51`	`.build();`
`52`	`52`	`}`