Skip to content

Fix to handle temporary Redirect 307 for S3 Cross Region client #4197

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

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Fix to handle temporary Redirect 307 for S3 Cross Region client #4197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67ed5a2
Fix to handle temporary Redirect 307 for S3 Cross Region client
joviegas Jul 18, 2023
1feb56f
Handled Review comments
joviegas Jul 19, 2023
bcf38ad
Merge branch 'master' into joviegas/bugfix_temp_redirect
joviegas Jul 19, 2023
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-AWSSDKforJavav2-4620073.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "bugfix",
"category": "Amazon S3",
"contributor": "",
"description": "Handle Temporary redirect error 307 when client configured in us-east-1 accesses cross region."
}
9 changes: 8 additions & 1 deletion .../java/software/amazon/awssdk/services/s3/crossregion/S3CrossRegionCrtIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.s3.S3AsyncClient;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.ListObjectsRequest;
import software.amazon.awssdk.services.s3.model.ListObjectsResponse;

public class S3CrossRegionCrtIntegrationTest extends S3CrossRegionAsyncIntegrationTestBase {
private static final String BUCKET = temporaryBucketName(S3CrossRegionCrtIntegrationTest.class);
Expand All @@ -41,7 +48,7 @@ public void initialize() {
crossRegionS3Client = S3AsyncClient.crtBuilder()
.region(CROSS_REGION)
.crossRegionAccessEnabled(true)
.build();
.build();
}
@Override
protected String bucketName() {
Expand Down
3 changes: 2 additions & 1 deletion ...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 @@ -26,6 +26,7 @@
import software.amazon.awssdk.core.ResponseBytes;
import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
import software.amazon.awssdk.services.s3.model.DeleteObjectResponse;
Expand Down Expand Up @@ -59,7 +60,7 @@ static void clearClass() {
@BeforeEach
public void initialize() {
crossRegionS3Client = S3Client.builder()
.region(CROSS_REGION)
.region(Region.US_EAST_1)
.crossRegionAccessEnabled(true)
.build();
}
Expand Down
10 changes: 8 additions & 2 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 @@ -33,6 +33,7 @@
@SdkInternalApi
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 AMZ_BUCKET_REGION_HEADER = "x-amz-bucket-region";
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 @@ -48,9 +49,14 @@ public static Optional<String> getBucketRegionFromException(S3Exception exceptio
}

public static boolean isS3RedirectException(Throwable exception) {
Throwable exceptionToBeChecked = exception instanceof CompletionException ? exception.getCause() : exception ;
Throwable exceptionToBeChecked = exception instanceof CompletionException ? exception.getCause() : exception;
return exceptionToBeChecked instanceof S3Exception
&& ((S3Exception) exceptionToBeChecked).statusCode() == REDIRECT_STATUS_CODE;
&& isRedirectError((S3Exception) exceptionToBeChecked);
}

private static boolean isRedirectError(S3Exception exceptionToBeChecked) {
int statusCode = exceptionToBeChecked.statusCode();
return statusCode == REDIRECT_STATUS_CODE || statusCode == TEMPORARY_REDIRECT_STATUS_CODE;
}


Expand Down
17 changes: 9 additions & 8 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 @@ -47,9 +47,10 @@ public void setup() {
}

@Override
protected void stubRedirectSuccessSuccess() {
protected void stubRedirectSuccessSuccess(Integer redirect) {
when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(301, CROSS_REGION.id(), null, null))))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(),
null, null))))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()));
}
Expand All @@ -74,9 +75,9 @@ protected void stubServiceClientConfiguration() {
}

@Override
protected void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse() {
protected void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse(Integer redirect) {
when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(301, CROSS_REGION.id(), null,
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(), null,
null))))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()
));
Expand Down Expand Up @@ -110,17 +111,17 @@ protected void stubHeadBucketRedirect() {
}

@Override
protected void stubRedirectWithNoRegionAndThenSuccess() {
protected void stubRedirectWithNoRegionAndThenSuccess(Integer redirect) {
when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(301, null, null, null))))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, null, null, null))))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()))
.thenReturn(CompletableFuture.completedFuture(ListObjectsResponse.builder().contents(S3_OBJECTS).build()));
}

@Override
protected void stubRedirectThenError() {
protected void stubRedirectThenError(Integer redirect) {
when(mockDelegateAsyncClient.listObjects(any(ListObjectsRequest.class)))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(301, CROSS_REGION.id(), null,
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(redirect, CROSS_REGION.id(), null,
null))))
.thenReturn(CompletableFutureUtils.failedFuture(new CompletionException(redirectException(400, null,
"InvalidArgument", "Invalid id"))));
Expand Down
14 changes: 7 additions & 7 deletions ...e/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClientRedirectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,23 @@ protected void stubHeadBucketRedirect() {
}

@Override
protected void stubRedirectWithNoRegionAndThenSuccess() {
protected void stubRedirectWithNoRegionAndThenSuccess(Integer redirect) {
when(mockDelegateClient.listObjects(any(ListObjectsRequest.class)))
.thenThrow(redirectException(301, null, null, null))
.thenThrow(redirectException(redirect, null, null, null))
.thenReturn(ListObjectsResponse.builder().contents(S3_OBJECTS).build());
}

@Override
protected void stubRedirectThenError() {
protected void stubRedirectThenError(Integer redirect) {
when(mockDelegateClient.listObjects(any(ListObjectsRequest.class)))
.thenThrow(redirectException(301, CROSS_REGION.id(), null, null))
.thenThrow(redirectException(redirect, CROSS_REGION.id(), null, null))
.thenThrow(redirectException(400, null, "InvalidArgument", "Invalid id"));
}

@Override
protected void stubRedirectSuccessSuccess() {
protected void stubRedirectSuccessSuccess(Integer redirect) {
when(mockDelegateClient.listObjects(any(ListObjectsRequest.class)))
.thenThrow(redirectException(301, CROSS_REGION.id(), null, null))
.thenThrow(redirectException(redirect, CROSS_REGION.id(), null, null))
.thenReturn(ListObjectsResponse.builder().contents(S3_OBJECTS).build())
.thenReturn(ListObjectsResponse.builder().contents(S3_OBJECTS).build());
}
Expand All @@ -120,7 +120,7 @@ protected void stubServiceClientConfiguration() {
}

@Override
protected void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse() {
protected void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse(Integer redirect) {
when(mockDelegateClient.listObjects(any(ListObjectsRequest.class)))
.thenThrow(redirectException(301, CROSS_REGION.id(), null, null))
.thenReturn(ListObjectsResponse.builder().contents(S3_OBJECTS).build());
Expand Down
45 changes: 26 additions & 19 deletions .../software/amazon/awssdk/services/s3/internal/crossregion/S3DecoratorRedirectTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.Collections;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.ArgumentCaptor;
import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
import software.amazon.awssdk.awscore.exception.AwsServiceException;
Expand Down Expand Up @@ -51,10 +53,11 @@ public abstract class S3DecoratorRedirectTestBase {
protected static final S3ServiceClientConfiguration CONFIGURED_ENDPOINT_PROVIDER =
S3ServiceClientConfiguration.builder().endpointProvider(S3EndpointProvider.defaultProvider()).build();

@Test
void decoratorAttemptsToRetryWithRegionNameInErrorResponse() throws Throwable {
@ParameterizedTest
@ValueSource(ints = {301, 307})
void decoratorAttemptsToRetryWithRegionNameInErrorResponse(Integer redirect) throws Throwable {
stubServiceClientConfiguration();
stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse();
stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse(redirect);
// Assert retrieved listObject
ListObjectsResponse listObjectsResponse = apiCallToService();
assertThat(listObjectsResponse.contents()).isEqualTo(S3_OBJECTS);
Expand All @@ -68,10 +71,11 @@ void decoratorAttemptsToRetryWithRegionNameInErrorResponse() throws Throwable {
verifyHeadBucketServiceCall(0);
}

@Test
void decoratorUsesCache_when_CrossRegionAlreadyPresent() throws Throwable {
@ParameterizedTest
@ValueSource(ints = {301, 307})
void decoratorUsesCache_when_CrossRegionAlreadyPresent(Integer redirect) throws Throwable {
stubServiceClientConfiguration();
stubRedirectSuccessSuccess();
stubRedirectSuccessSuccess(redirect);

ListObjectsResponse listObjectsResponse = apiCallToService();
assertThat(listObjectsResponse.contents()).isEqualTo(S3_OBJECTS);
Expand All @@ -93,20 +97,22 @@ void decoratorUsesCache_when_CrossRegionAlreadyPresent() throws Throwable {
* The redirected call fails because of incorrect parameters passed
* This exception should be reported correctly
*/
@Test
void apiCallFailure_when_CallFailsAfterRedirection() {
@ParameterizedTest
@ValueSource(ints = {301, 307})
void apiCallFailure_when_CallFailsAfterRedirection(Integer redirectError) {
stubServiceClientConfiguration();
stubRedirectThenError();
stubRedirectThenError(redirectError);
assertThatExceptionOfType(S3Exception.class)
.isThrownBy(() -> apiCallToService())
.withMessageContaining("Invalid id (Service: S3, Status Code: 400, Request ID: 1, Extended Request ID: A1)");
verifyHeadBucketServiceCall(0);
}

@Test
void headBucketCalled_when_RedirectDoesNotHasRegionName() throws Throwable {
@ParameterizedTest
@ValueSource(ints = {301, 307})
void headBucketCalled_when_RedirectDoesNotHasRegionName(Integer redirect) throws Throwable {
stubServiceClientConfiguration();
stubRedirectWithNoRegionAndThenSuccess();
stubRedirectWithNoRegionAndThenSuccess(redirect);
stubHeadBucketRedirect();
ListObjectsResponse listObjectsResponse = apiCallToService();
assertThat(listObjectsResponse.contents()).isEqualTo(S3_OBJECTS);
Expand All @@ -119,10 +125,11 @@ void headBucketCalled_when_RedirectDoesNotHasRegionName() throws Throwable {
verifyHeadBucketServiceCall(1);
}

@Test
void headBucketCalledAndCached__when_RedirectDoesNotHasRegionName() throws Throwable {
@ParameterizedTest
@ValueSource(ints = {301, 307})
void headBucketCalledAndCached__when_RedirectDoesNotHasRegionName(Integer redirect) throws Throwable {
stubServiceClientConfiguration();
stubRedirectWithNoRegionAndThenSuccess();
stubRedirectWithNoRegionAndThenSuccess(redirect);
stubHeadBucketRedirect();
ListObjectsResponse listObjectsResponse = apiCallToService();
assertThat(listObjectsResponse.contents()).isEqualTo(S3_OBJECTS);
Expand Down Expand Up @@ -165,11 +172,11 @@ void requestsAreNotOverridden_when_NoBucketInRequest() throws Throwable {

protected abstract void stubHeadBucketRedirect();

protected abstract void stubRedirectWithNoRegionAndThenSuccess();
protected abstract void stubRedirectWithNoRegionAndThenSuccess(Integer redirect);

protected abstract void stubRedirectThenError();
protected abstract void stubRedirectThenError(Integer redirect);

protected abstract void stubRedirectSuccessSuccess();
protected abstract void stubRedirectSuccessSuccess(Integer redirect);

protected AwsServiceException redirectException(int statusCode, String region, String errorCode, String errorMessage) {
SdkHttpFullResponse.Builder sdkHttpFullResponseBuilder = SdkHttpFullResponse.builder();
Expand Down Expand Up @@ -210,5 +217,5 @@ void verifyTheEndPointProviderOverridden(int attempt,

protected abstract void stubServiceClientConfiguration();

protected abstract void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse();
protected abstract void stubClientAPICallWithFirstRedirectThenSuccessWithRegionInErrorResponse(Integer redirect);
}