Skip to content
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

Jump to bottom

fix(core): allow cross region export provider to delete more than 10 exports #23969

Closed
wants to merge 5 commits into from
Closed

fix(core): allow cross region export provider to delete more than 10 exports #23969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b9a7a3
chore(@aws-cdk/core): add failing test
joshbalfour Feb 2, 2023
1dd1eb0
fix(@aws-cdk/core): batch deleteParameters to SSM
joshbalfour Feb 2, 2023
6d1b873
start writing integration test
joshbalfour Feb 2, 2023
9ddfe61
Merge branch 'main' into fix/cross-region-ssm-writer-deleteParameters…
joshbalfour Feb 2, 2023
7129a80
Merge branch 'main' into fix/cross-region-ssm-writer-deleteParameters…
joshbalfour Feb 28, 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
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-cloudformation/test/integ.core-cross-region-references.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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}`))
}
}

Expand Down Expand Up @@ -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],
});
}
}
Expand All @@ -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
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to add a new integration test (new file and everything) since this test scenario will probably conflict with the one above.

To test this scenario you can probably do a very similar test to the one above, but just invert the scenario. Instead of deleting the producer stack, delete the consumer and then perform a describeStacks and expect the delete to have succeeded.

*/
// TODO: how can I do this?
32 changes: 24 additions & 8 deletions ...-resource-provider/cross-region-export-providers/cross-region-ssm-writer-handler/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -61,6 +57,26 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
}
};

function chunkArray<T>(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<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here explaining why we are splitting it into groups of 10?

if (names.length === 0) {
return;
}

await Promise.all(
chunkArray(names, 10)
.map((namesChunk) => {
return ssm.deleteParameters({
Names: namesChunk,
}).promise();
}));
}

/**
* Create parameters for existing exports
*/
Expand Down Expand Up @@ -113,7 +129,7 @@ async function isInUse(ssm: SSM, parameterName: string): Promise<Set<string>> {
} 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;
Expand Down
114 changes: 113 additions & 1 deletion packages/@aws-cdk/core/test/custom-resource-provider/cross-region-ssm-writer-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -223,6 +226,115 @@ describe('cross-region-ssm-writer entrypoint', () => {
});
});

test('>10 removed exports are deleted without throwing', async () => {
// GIVEN
const event = makeEvent({
RequestType: 'Update',
OldResourceProperties: {
ServiceToken: '<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: '<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('>10 removed exports are deleted by batching to SSM', async () => {
// GIVEN
const event = makeEvent({
RequestType: 'Update',
OldResourceProperties: {
ServiceToken: '<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: '<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({
Expand Down