diff --git a/packages/core/__tests__/clients/middleware/retry/defaultRetryDecider.test.ts b/packages/core/__tests__/clients/middleware/retry/defaultRetryDecider.test.ts index e5e4b6bfa1f..b33b4e27f4d 100644 --- a/packages/core/__tests__/clients/middleware/retry/defaultRetryDecider.test.ts +++ b/packages/core/__tests__/clients/middleware/retry/defaultRetryDecider.test.ts @@ -4,15 +4,10 @@ import { HttpResponse } from '../../../../src/clients'; import { getRetryDecider } from '../../../../src/clients/middleware/retry'; import { isClockSkewError } from '../../../../src/clients/middleware/retry/isClockSkewError'; -import { isCredentialsExpiredError as isCredentialsExpiredErrorValidator } from '../../../../src/clients/middleware/retry/isCredentialsExpiredError'; jest.mock('../../../../src/clients/middleware/retry/isClockSkewError'); -jest.mock('../../../../src/clients/middleware/retry/isCredentialsExpiredError'); const mockIsClockSkewError = jest.mocked(isClockSkewError); -const mockIsCredentialsExpiredError = jest.mocked( - isCredentialsExpiredErrorValidator, -); describe('getRetryDecider', () => { const mockErrorParser = jest.fn(); @@ -35,7 +30,6 @@ describe('getRetryDecider', () => { const { retryable, isCredentialsExpiredError } = await retryDecider( mockHttpResponse, connectionError, - {}, ); expect(retryable).toBe(true); expect(isCredentialsExpiredError).toBeFalsy(); @@ -65,7 +59,6 @@ describe('getRetryDecider', () => { const { retryable, isCredentialsExpiredError } = await retryDecider( mockHttpResponse, undefined, - {}, ); expect(retryable).toBe(true); expect(isCredentialsExpiredError).toBeFalsy(); @@ -83,7 +76,6 @@ describe('getRetryDecider', () => { statusCode: 429, }, undefined, - {}, ); expect(retryable).toBe(true); expect(isInvalidCredentialsError).toBeFalsy(); @@ -101,7 +93,6 @@ describe('getRetryDecider', () => { const { retryable, isCredentialsExpiredError } = await retryDecider( mockHttpResponse, undefined, - {}, ); expect(retryable).toBe(true); expect(isCredentialsExpiredError).toBeFalsy(); @@ -122,7 +113,6 @@ describe('getRetryDecider', () => { statusCode, }, undefined, - {}, ); expect(retryable).toBe(true); expect(isCredentialsExpiredError).toBeFalsy(); @@ -138,54 +128,9 @@ describe('getRetryDecider', () => { const { retryable, isCredentialsExpiredError } = await retryDecider( mockHttpResponse, undefined, - {}, ); expect(retryable).toBe(true); expect(isCredentialsExpiredError).toBeFalsy(); }, ); - - describe('handling expired token errors', () => { - const mockErrorName = 'ExpiredToken'; - const mockErrorMessage = 'Token expired'; - it.each([ - { - code: mockErrorName, - message: mockErrorMessage, - }, - { - name: mockErrorName, - message: mockErrorMessage, - }, - ])('should retry if expired credentials error %o', async parsedError => { - expect.assertions(3); - mockErrorParser.mockResolvedValue(parsedError); - mockIsCredentialsExpiredError.mockReturnValue(true); - const retryDecider = getRetryDecider(mockErrorParser); - const { retryable, isCredentialsExpiredError } = await retryDecider( - mockHttpResponse, - undefined, - {}, - ); - expect(retryable).toBe(true); - expect(isCredentialsExpiredError).toBe(true); - expect(mockIsCredentialsExpiredError).toHaveBeenCalledWith( - mockErrorName, - mockErrorMessage, - ); - }); - - it('should not retry if invalid credentials error has been retried previously', async () => { - expect.assertions(2); - mockIsCredentialsExpiredError.mockReturnValue(true); - const retryDecider = getRetryDecider(mockErrorParser); - const { retryable, isCredentialsExpiredError } = await retryDecider( - mockHttpResponse, - undefined, - { isCredentialsExpired: true }, - ); - expect(retryable).toBe(false); - expect(isCredentialsExpiredError).toBe(true); - }); - }); }); diff --git a/packages/core/__tests__/clients/middleware/retry/isCredentialsExpiredError.test.ts b/packages/core/__tests__/clients/middleware/retry/isCredentialsExpiredError.test.ts deleted file mode 100644 index 272442e0950..00000000000 --- a/packages/core/__tests__/clients/middleware/retry/isCredentialsExpiredError.test.ts +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { isCredentialsExpiredError } from '../../../../src/clients/middleware/retry/isCredentialsExpiredError'; - -describe('isInvalidCredentialsError', () => { - test('returns true if error code is an invalid token error', () => { - expect(isCredentialsExpiredError('RequestExpired')).toBe(true); - expect(isCredentialsExpiredError('ExpiredTokenException')).toBe(true); - expect(isCredentialsExpiredError('ExpiredToken')).toBe(true); - }); - - test('returns false if error code is not an invalid token error', () => { - expect(isCredentialsExpiredError('Foo')).toBe(false); - }); - - test('returns true if error message indicates invalid credentials', () => { - expect( - isCredentialsExpiredError( - 'InvalidSignature', - 'Auth token in request is expired.', - ), - ).toBe(true); - }); -}); diff --git a/packages/core/src/clients/middleware/retry/defaultRetryDecider.ts b/packages/core/src/clients/middleware/retry/defaultRetryDecider.ts index 516b492dd95..edec193ebf1 100644 --- a/packages/core/src/clients/middleware/retry/defaultRetryDecider.ts +++ b/packages/core/src/clients/middleware/retry/defaultRetryDecider.ts @@ -2,10 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 import { ErrorParser, HttpResponse } from '../../types'; -import { MiddlewareContext } from '../../types/core'; import { isClockSkewError } from './isClockSkewError'; -import { isCredentialsExpiredError } from './isCredentialsExpiredError'; import { RetryDeciderOutput } from './types'; /** @@ -17,32 +15,22 @@ export const getRetryDecider = async ( response?: HttpResponse, error?: unknown, - middlewareContext?: MiddlewareContext, ): Promise => { const parsedError = (error as Error & { code: string }) ?? (await errorParser(response)) ?? undefined; const errorCode = parsedError?.code || parsedError?.name; - const errorMessage = parsedError?.message; const statusCode = response?.statusCode; - const isCredentialsExpired = isCredentialsExpiredError( - errorCode, - errorMessage, - ); const isRetryable = isConnectionError(error) || isThrottlingError(statusCode, errorCode) || isClockSkewError(errorCode) || - isServerSideError(statusCode, errorCode) || - // If we know the previous retry attempt sets isCredentialsInvalid in the - // middleware context, we don't want to retry anymore. - (isCredentialsExpired && !middlewareContext?.isCredentialsExpired); + isServerSideError(statusCode, errorCode); return { retryable: isRetryable, - isCredentialsExpiredError: isCredentialsExpired, }; }; diff --git a/packages/core/src/clients/middleware/retry/isCredentialsExpiredError.ts b/packages/core/src/clients/middleware/retry/isCredentialsExpiredError.ts deleted file mode 100644 index 497f60f9d88..00000000000 --- a/packages/core/src/clients/middleware/retry/isCredentialsExpiredError.ts +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Ref: https://github.com/aws/aws-sdk-js/blob/54829e341181b41573c419bd870dd0e0f8f10632/lib/event_listeners.js#L522-L541 -const INVALID_TOKEN_ERROR_CODES = [ - 'RequestExpired', - 'ExpiredTokenException', - 'ExpiredToken', -]; - -/** - * Given an error code, returns true if it is related to invalid credentials. - * - * @param errorCode String representation of some error. - * @returns True if given error indicates the credentials used to authorize request - * are invalid. - * - * @internal - */ -export const isCredentialsExpiredError = ( - errorCode?: string, - errorMessage?: string, -) => { - const isExpiredTokenError = - !!errorCode && INVALID_TOKEN_ERROR_CODES.includes(errorCode); - // Ref: https://github.com/aws/aws-sdk-js/blob/54829e341181b41573c419bd870dd0e0f8f10632/lib/event_listeners.js#L536-L539 - const isExpiredSignatureError = - !!errorCode && - !!errorMessage && - errorCode.includes('Signature') && - errorMessage.includes('expired'); - - return isExpiredTokenError || isExpiredSignatureError; -}; diff --git a/packages/core/src/clients/types/index.ts b/packages/core/src/clients/types/index.ts index e2b8953a4d2..0ee905fb162 100644 --- a/packages/core/src/clients/types/index.ts +++ b/packages/core/src/clients/types/index.ts @@ -4,6 +4,7 @@ export { Middleware, MiddlewareHandler, + MiddlewareContext, Request, Response, TransferHandler, diff --git a/packages/storage/__tests__/providers/s3/apis/copy.test.ts b/packages/storage/__tests__/providers/s3/apis/copy.test.ts index 56f46927b30..56104e84d17 100644 --- a/packages/storage/__tests__/providers/s3/apis/copy.test.ts +++ b/packages/storage/__tests__/providers/s3/apis/copy.test.ts @@ -384,7 +384,6 @@ describe('copy API', () => { }, }); } catch (error: any) { - console.log(error); expect(error).toBeInstanceOf(StorageError); expect(error.name).toBe( StorageValidationErrorCode.InvalidCopyOperationStorageBucket, @@ -403,7 +402,6 @@ describe('copy API', () => { }, }); } catch (error: any) { - console.log(error); expect(error).toBeInstanceOf(StorageError); expect(error.name).toBe( StorageValidationErrorCode.InvalidCopyOperationStorageBucket, diff --git a/packages/storage/__tests__/providers/s3/utils/client/S3/utils/retryDecider.test.ts b/packages/storage/__tests__/providers/s3/utils/client/S3/utils/retryDecider.test.ts new file mode 100644 index 00000000000..5e1801c07db --- /dev/null +++ b/packages/storage/__tests__/providers/s3/utils/client/S3/utils/retryDecider.test.ts @@ -0,0 +1,103 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +import { + HttpResponse, + getRetryDecider as getDefaultRetryDecider, +} from '@aws-amplify/core/internals/aws-client-utils'; + +import { retryDecider } from '../../../../../../../src/providers/s3/utils/client/utils'; +import { parseXmlError } from '../../../../../../../src/providers/s3/utils/client/utils/parsePayload'; + +jest.mock( + '../../../../../../../src/providers/s3/utils/client/utils/parsePayload', +); +jest.mock('@aws-amplify/core/internals/aws-client-utils'); + +const mockErrorParser = jest.mocked(parseXmlError); + +describe('retryDecider', () => { + const mockHttpResponse: HttpResponse = { + statusCode: 200, + headers: {}, + body: 'body' as any, + }; + + beforeEach(() => { + jest.mocked(getDefaultRetryDecider).mockReturnValue(async () => { + return { retryable: false }; + }); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should invoke the default retry decider', async () => { + expect.assertions(3); + const { retryable, isCredentialsExpiredError } = await retryDecider( + mockHttpResponse, + undefined, + {}, + ); + expect(getDefaultRetryDecider).toHaveBeenCalledWith(mockErrorParser); + expect(retryable).toBe(false); + expect(isCredentialsExpiredError).toBeFalsy(); + }); + + describe('handling expired token errors', () => { + const mockErrorMessage = 'Token expired'; + it.each(['RequestExpired', 'ExpiredTokenException', 'ExpiredToken'])( + 'should retry if expired credentials error name %s', + async errorName => { + expect.assertions(2); + const parsedError = { + name: errorName, + message: mockErrorMessage, + $metadata: {}, + }; + mockErrorParser.mockResolvedValue(parsedError); + const { retryable, isCredentialsExpiredError } = await retryDecider( + { ...mockHttpResponse, statusCode: 400 }, + undefined, + {}, + ); + expect(retryable).toBe(true); + expect(isCredentialsExpiredError).toBe(true); + }, + ); + + it('should retry if error message indicates invalid credentials', async () => { + expect.assertions(2); + const parsedError = { + name: 'InvalidSignature', + message: 'Auth token in request is expired.', + $metadata: {}, + }; + mockErrorParser.mockResolvedValue(parsedError); + const { retryable, isCredentialsExpiredError } = await retryDecider( + { ...mockHttpResponse, statusCode: 400 }, + undefined, + {}, + ); + expect(retryable).toBe(true); + expect(isCredentialsExpiredError).toBe(true); + }); + + it('should not retry if invalid credentials error has been retried previously', async () => { + expect.assertions(2); + const parsedError = { + name: 'RequestExpired', + message: mockErrorMessage, + $metadata: {}, + }; + mockErrorParser.mockResolvedValue(parsedError); + const { retryable, isCredentialsExpiredError } = await retryDecider( + { ...mockHttpResponse, statusCode: 400 }, + undefined, + { isCredentialsExpired: true }, + ); + expect(retryable).toBe(false); + expect(isCredentialsExpiredError).toBe(true); + }); + }); +}); diff --git a/packages/storage/src/providers/s3/utils/client/s3control/base.ts b/packages/storage/src/providers/s3/utils/client/s3control/base.ts index 380488bf4ac..a40f9f6a5dd 100644 --- a/packages/storage/src/providers/s3/utils/client/s3control/base.ts +++ b/packages/storage/src/providers/s3/utils/client/s3control/base.ts @@ -8,11 +8,10 @@ import { import { EndpointResolverOptions, getDnsSuffix, - getRetryDecider, jitteredBackoff, } from '@aws-amplify/core/internals/aws-client-utils'; -import { parseXmlError } from '../utils'; +import { retryDecider } from '../utils'; /** * The service name used to sign requests if the API requires authentication. @@ -64,7 +63,7 @@ const endpointResolver = ( export const defaultConfig = { service: SERVICE_NAME, endpointResolver, - retryDecider: getRetryDecider(parseXmlError), + retryDecider, computeDelay: jitteredBackoff, userAgentValue: getAmplifyUserAgent(), uriEscapePath: false, // Required by S3. See https://github.com/aws/aws-sdk-js-v3/blob/9ba012dfa3a3429aa2db0f90b3b0b3a7a31f9bc3/packages/signature-v4/src/SignatureV4.ts#L76-L83 diff --git a/packages/storage/src/providers/s3/utils/client/s3data/base.ts b/packages/storage/src/providers/s3/utils/client/s3data/base.ts index a31d6d5a2f1..d51c3a18a11 100644 --- a/packages/storage/src/providers/s3/utils/client/s3data/base.ts +++ b/packages/storage/src/providers/s3/utils/client/s3data/base.ts @@ -8,11 +8,10 @@ import { import { EndpointResolverOptions, getDnsSuffix, - getRetryDecider, jitteredBackoff, } from '@aws-amplify/core/internals/aws-client-utils'; -import { parseXmlError } from '../utils'; +import { retryDecider } from '../utils'; const DOMAIN_PATTERN = /^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$/; const IP_ADDRESS_PATTERN = /(\d+\.){3}\d+/; @@ -106,7 +105,7 @@ export const isDnsCompatibleBucketName = (bucketName: string): boolean => export const defaultConfig = { service: SERVICE_NAME, endpointResolver, - retryDecider: getRetryDecider(parseXmlError), + retryDecider, computeDelay: jitteredBackoff, userAgentValue: getAmplifyUserAgent(), useAccelerateEndpoint: false, diff --git a/packages/storage/src/providers/s3/utils/client/s3data/completeMultipartUpload.ts b/packages/storage/src/providers/s3/utils/client/s3data/completeMultipartUpload.ts index 4679c4c5694..1e399e824e7 100644 --- a/packages/storage/src/providers/s3/utils/client/s3data/completeMultipartUpload.ts +++ b/packages/storage/src/providers/s3/utils/client/s3data/completeMultipartUpload.ts @@ -5,6 +5,7 @@ import { Endpoint, HttpRequest, HttpResponse, + MiddlewareContext, RetryDeciderOutput, parseMetadata, } from '@aws-amplify/core/internals/aws-client-utils'; @@ -19,6 +20,7 @@ import { map, parseXmlBody, parseXmlError, + retryDecider, s3TransferHandler, serializePathnameObjectKey, validateS3RequiredParameter, @@ -137,6 +139,7 @@ const completeMultipartUploadDeserializer = async ( const retryWhenErrorWith200StatusCode = async ( response?: HttpResponse, error?: unknown, + middlewareContext?: MiddlewareContext, ): Promise => { if (!response) { return { retryable: false }; @@ -153,9 +156,7 @@ const retryWhenErrorWith200StatusCode = async ( return { retryable: false }; } - const defaultRetryDecider = defaultConfig.retryDecider; - - return defaultRetryDecider(response, error); + return retryDecider(response, error, middlewareContext); }; export const completeMultipartUpload = composeServiceApi( diff --git a/packages/storage/src/providers/s3/utils/client/utils/index.ts b/packages/storage/src/providers/s3/utils/client/utils/index.ts index abfe9328d45..423987699f8 100644 --- a/packages/storage/src/providers/s3/utils/client/utils/index.ts +++ b/packages/storage/src/providers/s3/utils/client/utils/index.ts @@ -25,3 +25,4 @@ export { serializePathnameObjectKey, validateS3RequiredParameter, } from './serializeHelpers'; +export { retryDecider } from './retryDecider'; diff --git a/packages/storage/src/providers/s3/utils/client/utils/retryDecider.ts b/packages/storage/src/providers/s3/utils/client/utils/retryDecider.ts new file mode 100644 index 00000000000..071402146a0 --- /dev/null +++ b/packages/storage/src/providers/s3/utils/client/utils/retryDecider.ts @@ -0,0 +1,81 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +import { + HttpResponse, + MiddlewareContext, + RetryDeciderOutput, + getRetryDecider, +} from '@aws-amplify/core/internals/aws-client-utils'; + +import { LocationCredentialsProvider } from '../../../types/options'; + +import { parseXmlError } from './parsePayload'; + +/** + * Function to decide if the S3 request should be retried. For S3 APIs, we support forceRefresh option + * for {@link LocationCredentialsProvider | LocationCredentialsProvider } option. It's set when S3 returns + * credentials expired error. In the retry decider, we detect this response and set flag to signify a retry + * attempt with forceRefresh option when invoking the LocationCredentialsProvider. + * + * @param response Optional response of the request. + * @param error Optional error thrown from previous attempts. + * @param middlewareContext Optional context object to store data between retries. + * @returns True if the request should be retried. + */ +export const retryDecider = async ( + response?: HttpResponse, + error?: unknown, + middlewareContext?: MiddlewareContext, +): Promise => { + const defaultRetryDecider = getRetryDecider(parseXmlError); + const defaultRetryDecision = await defaultRetryDecider(response, error); + if (!response || response.statusCode < 300) { + return { retryable: false }; + } + const parsedError = await parseXmlError(response); + const errorCode = parsedError?.name; + const errorMessage = parsedError?.message; + const isCredentialsExpired = isCredentialsExpiredError( + errorCode, + errorMessage, + ); + + return { + retryable: + defaultRetryDecision.retryable || + // If we know the previous retry attempt sets isCredentialsInvalid in the + // middleware context, we don't want to retry anymore. + !!(isCredentialsExpired && !middlewareContext?.isCredentialsExpired), + isCredentialsExpiredError: isCredentialsExpired, + }; +}; + +// Ref: https://github.com/aws/aws-sdk-js/blob/54829e341181b41573c419bd870dd0e0f8f10632/lib/event_listeners.js#L522-L541 +const INVALID_TOKEN_ERROR_CODES = [ + 'RequestExpired', + 'ExpiredTokenException', + 'ExpiredToken', +]; + +/** + * Given an error code, returns true if it is related to invalid credentials. + * + * @param errorCode String representation of some error. + * @returns True if given error indicates the credentials used to authorize request + * are invalid. + */ +const isCredentialsExpiredError = ( + errorCode?: string, + errorMessage?: string, +) => { + const isExpiredTokenError = + !!errorCode && INVALID_TOKEN_ERROR_CODES.includes(errorCode); + // Ref: https://github.com/aws/aws-sdk-js/blob/54829e341181b41573c419bd870dd0e0f8f10632/lib/event_listeners.js#L536-L539 + const isExpiredSignatureError = + !!errorCode && + !!errorMessage && + errorCode.includes('Signature') && + errorMessage.includes('expired'); + + return isExpiredTokenError || isExpiredSignatureError; +};