From 9eaefc247b4659f91fc9b416beef8794c8596b0c Mon Sep 17 00:00:00 2001 From: siddsriv Date: Wed, 27 Sep 2023 18:08:36 +0000 Subject: [PATCH] fix(middleware-sdk-s3): bug fix for middleware and test refactor --- clients/client-s3/src/S3Client.ts | 2 + .../aws/typescript/codegen/AddS3Config.java | 5 + .../region-redirect-endpoint-middleware.ts | 12 +- .../region-redirect-middleware.e2e.spec.ts | 140 ++++++++++++------ .../src/region-redirect-middleware.ts | 9 +- 5 files changed, 111 insertions(+), 57 deletions(-) diff --git a/clients/client-s3/src/S3Client.ts b/clients/client-s3/src/S3Client.ts index 57d8b7200ed1..6f72b42688f8 100644 --- a/clients/client-s3/src/S3Client.ts +++ b/clients/client-s3/src/S3Client.ts @@ -9,6 +9,7 @@ import { import { getLoggerPlugin } from "@aws-sdk/middleware-logger"; import { getRecursionDetectionPlugin } from "@aws-sdk/middleware-recursion-detection"; import { + getRegionRedirectMiddlewarePlugin, getValidateBucketNamePlugin, resolveS3Config, S3InputConfig, @@ -778,6 +779,7 @@ export class S3Client extends __Client< this.middlewareStack.use(getAwsAuthPlugin(this.config)); this.middlewareStack.use(getValidateBucketNamePlugin(this.config)); this.middlewareStack.use(getAddExpectContinuePlugin(this.config)); + this.middlewareStack.use(getRegionRedirectMiddlewarePlugin(this.config)); this.middlewareStack.use(getUserAgentPlugin(this.config)); } diff --git a/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java b/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java index 4d86227d61c6..39fa5aea7299 100644 --- a/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java +++ b/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java @@ -226,6 +226,11 @@ && isS3(s)) && isS3(s) && !isEndpointsV2Service(s) && containsInputMembers(m, o, BUCKET_ENDPOINT_INPUT_KEYS)) + .build(), + RuntimeClientPlugin.builder() + .withConventions(AwsDependency.S3_MIDDLEWARE.dependency, "RegionRedirectMiddleware", + HAS_MIDDLEWARE) + .servicePredicate((m, s) -> isS3(s)) .build() ); } diff --git a/packages/middleware-sdk-s3/src/region-redirect-endpoint-middleware.ts b/packages/middleware-sdk-s3/src/region-redirect-endpoint-middleware.ts index 902f4b37e3fd..ece8a964ace1 100644 --- a/packages/middleware-sdk-s3/src/region-redirect-endpoint-middleware.ts +++ b/packages/middleware-sdk-s3/src/region-redirect-endpoint-middleware.ts @@ -1,7 +1,6 @@ import { HandlerExecutionContext, MetadataBearer, - Pluggable, RelativeMiddlewareOptions, SerializeHandler, SerializeHandlerArguments, @@ -21,8 +20,10 @@ export const regionRedirectEndpointMiddleware = (config: PreviouslyResolved): Se ): SerializeHandler => async (args: SerializeHandlerArguments): Promise> => { const originalRegion = await config.region(); + const regionProviderRef = config.region; if (context.__s3RegionRedirect) { config.region = async () => { + config.region = regionProviderRef; return context.__s3RegionRedirect; }; } @@ -47,12 +48,3 @@ export const regionRedirectEndpointMiddlewareOptions: RelativeMiddlewareOptions relation: "before", toMiddleware: "endpointV2Middleware", }; - -/** - * @internal - */ -export const getRegionRedirectEndpointMiddlewarePlugin = (clientConfig: PreviouslyResolved): Pluggable => ({ - applyToStack: (clientStack) => { - clientStack.addRelativeTo(regionRedirectEndpointMiddleware(clientConfig), regionRedirectEndpointMiddlewareOptions); - }, -}); diff --git a/packages/middleware-sdk-s3/src/region-redirect-middleware.e2e.spec.ts b/packages/middleware-sdk-s3/src/region-redirect-middleware.e2e.spec.ts index de23996d17ff..7d68644570d0 100644 --- a/packages/middleware-sdk-s3/src/region-redirect-middleware.e2e.spec.ts +++ b/packages/middleware-sdk-s3/src/region-redirect-middleware.e2e.spec.ts @@ -1,58 +1,106 @@ -import { - CreateBucketCommand, - DeleteBucketCommand, - DeleteObjectCommand, - GetObjectCommand, - PutObjectCommand, - S3Client, -} from "@aws-sdk/client-s3"; -import { STS } from "@aws-sdk/client-sts"; - -const regionConfigs = [ - { region: "us-east-1", followRegionRedirects: true }, - { region: "eu-west-1", followRegionRedirects: true }, - { region: "us-west-2", followRegionRedirects: true }, -]; - -const s3Clients = regionConfigs.map((config) => new S3Client(config)); +import { S3 } from "@aws-sdk/client-s3"; +import { GetCallerIdentityCommandOutput, STS } from "@aws-sdk/client-sts"; const testValue = "Hello S3 global client!"; -async function testS3GlobalClient() { +describe("S3 Global Client Test", () => { + const regionConfigs = [ + { region: "us-east-1", followRegionRedirects: true }, + { region: "eu-west-1", followRegionRedirects: true }, + { region: "us-west-2", followRegionRedirects: true }, + ]; + const s3Clients = regionConfigs.map((config) => new S3(config)); const stsClient = new STS({}); - const callerID = await stsClient.getCallerIdentity({}); - - const bucketNames = regionConfigs.map((config) => `${callerID.Account}-redirect-${config.region}`); - await Promise.all( - bucketNames.map((bucketName, index) => s3Clients[index].send(new CreateBucketCommand({ Bucket: bucketName }))) - ); - // Upload objects to each bucket - for (const bucketName of bucketNames) { - for (const s3Client of s3Clients) { - const objKey = `object-from-${await s3Client.config.region()}-client`; - await s3Client.send(new PutObjectCommand({ Bucket: bucketName, Key: objKey, Body: testValue })); + let callerID = null as unknown as GetCallerIdentityCommandOutput; + let bucketNames = [] as string[]; + + beforeAll(async () => { + jest.setTimeout(500000); + callerID = await stsClient.getCallerIdentity({}); + bucketNames = regionConfigs.map((config) => `${callerID.Account}-redirect-${config.region}`); + await Promise.all(bucketNames.map((bucketName, index) => deleteBucket(s3Clients[index], bucketName))); + await Promise.all(bucketNames.map((bucketName, index) => s3Clients[index].createBucket({ Bucket: bucketName }))); + }); + + afterAll(async () => { + await Promise.all(bucketNames.map((bucketName, index) => deleteBucket(s3Clients[index], bucketName))); + }); + + it("Should be able to put objects following region redirect", async () => { + // Upload objects to each bucket + for (const bucketName of bucketNames) { + for (const s3Client of s3Clients) { + const objKey = `object-from-${await s3Client.config.region()}-client`; + await s3Client.putObject({ Bucket: bucketName, Key: objKey, Body: testValue }); + } } - } + }, 50000); - // Fetch, assert, and delete objects - for (const bucketName of bucketNames) { - for (const s3Client of s3Clients) { - const objKey = `object-from-${await s3Client.config.region()}-client`; - const { Body } = await s3Client.send(new GetObjectCommand({ Bucket: bucketName, Key: objKey })); - const data = await Body?.transformToString(); - expect(data).toEqual(testValue); - await s3Client.send(new DeleteObjectCommand({ Bucket: bucketName, Key: objKey })); + it("Should be able to get objects following region redirect", async () => { + // Fetch and assert objects + for (const bucketName of bucketNames) { + for (const s3Client of s3Clients) { + const objKey = `object-from-${await s3Client.config.region()}-client`; + const { Body } = await s3Client.getObject({ Bucket: bucketName, Key: objKey }); + const data = await Body?.transformToString(); + expect(data).toEqual(testValue); + } } + }, 50000); + + it("Should delete objects following region redirect", async () => { + for (const bucketName of bucketNames) { + for (const s3Client of s3Clients) { + const objKey = `object-from-${await s3Client.config.region()}-client`; + await s3Client.deleteObject({ Bucket: bucketName, Key: objKey }); + } + } + }, 50000); +}); + +async function deleteBucket(s3: S3, bucketName: string) { + const Bucket = bucketName; + + try { + await s3.headBucket({ + Bucket, + }); + } catch (e) { + return; } - for (let i = 0; i < s3Clients.length; ++i) { - await s3Clients[i].send(new DeleteBucketCommand({ Bucket: bucketNames[(i + 1) % bucketNames.length] })); + const list = await s3 + .listObjects({ + Bucket, + }) + .catch((e) => { + if (!String(e).includes("NoSuchBucket")) { + throw e; + } + return { + Contents: [], + }; + }); + + const promises = [] as any[]; + for (const key of list.Contents ?? []) { + promises.push( + s3.deleteObject({ + Bucket, + Key: key.Key, + }) + ); } -} + await Promise.all(promises); -describe("S3 Global Client Test", () => { - it("Can perform all operations cross-regionally by following region redirect", async () => { - await testS3GlobalClient(); - }, 50000); -}); + try { + return await s3.deleteBucket({ + Bucket, + }); + } catch (e) { + if (!String(e).includes("NoSuchBucket")) { + throw e; + } + } +} diff --git a/packages/middleware-sdk-s3/src/region-redirect-middleware.ts b/packages/middleware-sdk-s3/src/region-redirect-middleware.ts index 408b0f1c119e..f16bfd7465ff 100644 --- a/packages/middleware-sdk-s3/src/region-redirect-middleware.ts +++ b/packages/middleware-sdk-s3/src/region-redirect-middleware.ts @@ -10,6 +10,11 @@ import { Provider, } from "@smithy/types"; +import { + regionRedirectEndpointMiddleware, + regionRedirectEndpointMiddlewareOptions, +} from "./region-redirect-endpoint-middleware"; + /** * @internal */ @@ -28,8 +33,9 @@ export function regionRedirectMiddleware(clientConfig: PreviouslyResolved): Init ): InitializeHandler => async (args: InitializeHandlerArguments): Promise> => { try { - return next(args); + return await next(args); } catch (err) { + // console.log("Region Redirect", clientConfig.followRegionRedirects, err.name, err.$metadata.httpStatusCode); if ( clientConfig.followRegionRedirects && err.name === "PermanentRedirect" && @@ -66,5 +72,6 @@ export const regionRedirectMiddlewareOptions: InitializeHandlerOptions = { export const getRegionRedirectMiddlewarePlugin = (clientConfig: PreviouslyResolved): Pluggable => ({ applyToStack: (clientStack) => { clientStack.add(regionRedirectMiddleware(clientConfig), regionRedirectMiddlewareOptions); + clientStack.addRelativeTo(regionRedirectEndpointMiddleware(clientConfig), regionRedirectEndpointMiddlewareOptions); }, });