From 2b9a7a31bf16b2441cc46e334670b75a86b2fc0a Mon Sep 17 00:00:00 2001 From: Josh Balfour Date: Thu, 2 Feb 2023 14:04:24 +0000 Subject: [PATCH 1/3] chore(@aws-cdk/core): add failing test --- .../cross-region-ssm-writer-handler.test.ts | 53 ++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts b/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts index e5079668b3b8c..ea81a19bb8de0 100644 --- a/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts +++ b/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts @@ -31,7 +31,10 @@ beforeEach(() => { jest.spyOn(console, 'info').mockImplementation(() => {}); jest.spyOn(console, 'error').mockImplementation(() => {}); mockPutParameter = jest.fn(); - mockDeleteParameters = jest.fn().mockImplementation(() => { + mockDeleteParameters = jest.fn().mockImplementation(({ Names }: { Names: string[] }) => { + if (Names.length > 10) { + throw new Error('SSM DeleteParameters: Names.length cannot be more than 10'); + } return {}; }); mocklistTagsForResource = jest.fn().mockImplementation(() => { @@ -223,6 +226,54 @@ describe('cross-region-ssm-writer entrypoint', () => { }); }); + test('>10 removed exports are deleted without throwing', async () => { + // GIVEN + const event = makeEvent({ + RequestType: 'Update', + OldResourceProperties: { + ServiceToken: '', + WriterProps: { + region: 'us-east-1', + exports: { + '/cdk/exports/MyStack/ExistingExport': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport1': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport2': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport3': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport4': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport5': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport6': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport7': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport8': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport9': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport10': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport11': 'MyExistingValue', + }, + }, + }, + ResourceProperties: { + ServiceToken: '', + WriterProps: { + region: 'us-east-1', + exports: { + '/cdk/exports/MyStack/ExistingExport': 'MyExistingValue', + }, + }, + }, + }); + + // WHEN + await handler(event); + + let err; + try { + await handler(event); + } catch (e) { + err = e; + } + // THEN + expect(err).toBeUndefined(); + }); + test('update throws if params already exist', async () => { // GIVEN const event = makeEvent({ From 1dd1eb0bd65858cf9259eba95edc0f8b29b33b56 Mon Sep 17 00:00:00 2001 From: Josh Balfour Date: Thu, 2 Feb 2023 14:12:25 +0000 Subject: [PATCH 2/3] fix(@aws-cdk/core): batch deleteParameters to SSM --- .../cross-region-ssm-writer-handler/index.ts | 32 +++++++--- .../cross-region-ssm-writer-handler.test.ts | 61 +++++++++++++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-export-providers/cross-region-ssm-writer-handler/index.ts b/packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-export-providers/cross-region-ssm-writer-handler/index.ts index 9f1f2d62d7b99..f5f084db43810 100644 --- a/packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-export-providers/cross-region-ssm-writer-handler/index.ts +++ b/packages/@aws-cdk/core/lib/custom-resource-provider/cross-region-export-providers/cross-region-ssm-writer-handler/index.ts @@ -1,6 +1,6 @@ /*eslint-disable no-console*/ /* eslint-disable import/no-extraneous-dependencies */ -import { SSM } from 'aws-sdk'; +import { AWSError, SSM } from 'aws-sdk'; import { CrossRegionExports, ExportWriterCRProps } from '../types'; export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { @@ -33,9 +33,7 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent // skip if no export names are to be deleted const removedExportsNames = Object.keys(removedExports); if (removedExportsNames.length > 0) { - await ssm.deleteParameters({ - Names: removedExportsNames, - }).promise(); + await deleteParameters(ssm, removedExportsNames); } // also throw an error if we are creating a new export that already exists for some reason @@ -48,9 +46,7 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent // the stack deletion. await throwIfAnyInUse(ssm, exports); // if none are in use then delete all of them - await ssm.deleteParameters({ - Names: Object.keys(exports), - }).promise(); + await deleteParameters(ssm, Object.keys(exports)); return; default: return; @@ -61,6 +57,26 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent } }; +function chunkArray(arr: T[], size: number): T[][] { + return arr.length > size + ? [arr.slice(0, size), ...chunkArray(arr.slice(size), size)] + : [arr]; +} + +async function deleteParameters(ssm: SSM, names: string[]): Promise { + if (names.length === 0) { + return; + } + + await Promise.all( + chunkArray(names, 10) + .map((namesChunk) => { + return ssm.deleteParameters({ + Names: namesChunk, + }).promise(); + })); +} + /** * Create parameters for existing exports */ @@ -113,7 +129,7 @@ async function isInUse(ssm: SSM, parameterName: string): Promise> { } catch (e) { // an InvalidResourceId means that the parameter doesn't exist // which we should ignore since that means it's not in use - if (e.code === 'InvalidResourceId') { + if ((e as AWSError).code === 'InvalidResourceId') { return new Set(); } throw e; diff --git a/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts b/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts index ea81a19bb8de0..8ab019bccae9a 100644 --- a/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts +++ b/packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts @@ -274,6 +274,67 @@ describe('cross-region-ssm-writer entrypoint', () => { expect(err).toBeUndefined(); }); + test('>10 removed exports are deleted by batching to SSM', async () => { + // GIVEN + const event = makeEvent({ + RequestType: 'Update', + OldResourceProperties: { + ServiceToken: '', + WriterProps: { + region: 'us-east-1', + exports: { + '/cdk/exports/MyStack/ExistingExport': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport1': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport2': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport3': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport4': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport5': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport6': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport7': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport8': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport9': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport10': 'MyExistingValue', + '/cdk/exports/MyStack/RemovedExport11': 'MyExistingValue', + }, + }, + }, + ResourceProperties: { + ServiceToken: '', + WriterProps: { + region: 'us-east-1', + exports: { + '/cdk/exports/MyStack/ExistingExport': 'MyExistingValue', + }, + }, + }, + }); + + // WHEN + await handler(event); + + // THEN + expect(mockDeleteParameters).toHaveBeenCalledTimes(2); + expect(mockDeleteParameters).toHaveBeenCalledWith({ + Names: [ + '/cdk/exports/MyStack/RemovedExport1', + '/cdk/exports/MyStack/RemovedExport2', + '/cdk/exports/MyStack/RemovedExport3', + '/cdk/exports/MyStack/RemovedExport4', + '/cdk/exports/MyStack/RemovedExport5', + '/cdk/exports/MyStack/RemovedExport6', + '/cdk/exports/MyStack/RemovedExport7', + '/cdk/exports/MyStack/RemovedExport8', + '/cdk/exports/MyStack/RemovedExport9', + '/cdk/exports/MyStack/RemovedExport10', + ], + }); + expect(mockDeleteParameters).toHaveBeenCalledWith({ + Names: [ + '/cdk/exports/MyStack/RemovedExport11', + ], + }); + }); + test('update throws if params already exist', async () => { // GIVEN const event = makeEvent({ From 6d1b873066a095e879305a7922c5334274e4a694 Mon Sep 17 00:00:00 2001 From: Josh Balfour Date: Thu, 2 Feb 2023 15:51:24 +0000 Subject: [PATCH 3/3] start writing integration test --- .../test/integ.core-cross-region-references.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-region-references.ts b/packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-region-references.ts index 5e6408139889d..b1966476d9118 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-region-references.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-region-references.ts @@ -12,6 +12,7 @@ const app = new App({ class ProducerStack extends Stack { public readonly queue: IQueue; public readonly nestedQueue: IQueue; + public readonly manyQueues: IQueue[]; constructor(scope: Construct, id: string) { super(scope, id, { env: { @@ -22,6 +23,7 @@ class ProducerStack extends Stack { const nested = new NestedStack(this, 'IntegNested'); this.queue = new Queue(this, 'IntegQueue'); this.nestedQueue = new Queue(nested, 'NestedIntegQueue'); + this.manyQueues = [0,1,2,3,4,5,6,7,8,9,10].map((i) => new Queue(this, `IntegQueue-${i}`)) } } @@ -59,7 +61,7 @@ class TestCase extends Construct { super(scope, id); this.producer = new ProducerStack(app, 'cross-region-producer'); this.testCase = new ConsumerStack(app, 'cross-region-consumer', { - queues: [this.producer.queue, this.producer.nestedQueue], + queues: [this.producer.queue, this.producer.nestedQueue, ...this.producer.manyQueues], }); } } @@ -71,7 +73,6 @@ const integ = new IntegTest(app, 'cross-region-references', { stackUpdateWorkflow: false, }); - /** * Test that if the references are still in use, deleting the producer * stack will fail @@ -94,3 +95,9 @@ integ.assertions.awsApiCall('CloudFormation', 'deleteStack', { ]), })).waitForAssertions(), ); + +/** + * Test that if we delete more than 10 exports that the + * stack will update correctly. + */ +// TODO: how can I do this?