From 09642e9636f5c32634f3030ca1c656ba3cca418f Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 16 Aug 2020 16:47:36 -0400 Subject: [PATCH 01/15] feat(s3): add option to auto delete objects upon bucket removal --- packages/@aws-cdk/aws-s3/README.md | 17 ++ .../auto-delete-objects-handler.ts | 189 ++++++++++++++++++ .../lib/auto-delete-objects-handler/index.ts | 1 + packages/@aws-cdk/aws-s3/lib/bucket.ts | 32 ++- .../aws-s3/test/test.auto-delete-objects.ts | 28 +++ 5 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts create mode 100644 packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts create mode 100644 packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md index 01165967f3dbc..ae029aaae8a97 100644 --- a/packages/@aws-cdk/aws-s3/README.md +++ b/packages/@aws-cdk/aws-s3/README.md @@ -312,3 +312,20 @@ const bucket = new Bucket(this, 'MyRedirectedBucket', { To put files into a bucket as part of a deployment (for example, to host a website), see the `@aws-cdk/aws-s3-deployment` package, which provides a resource that can do just that. + +### Bucket deletion + +By default, when a bucket is removed from a stack (or the stack is deleted), +the S3 bucket will be removed according to its removal policy (which by +default, will simply orphan the bucket and leave it in your AWS account). +However, even if the removal policy is set to DESTROY, the bucket will get +skipped by default if it still has objects inside. + +To override this, enable the `autoDeleteObjects` property. + +```ts +const bucket = new Bucket(this, 'MyTempFileBucket', { + removalPolicy: RemovalPolicy.DESTROY, + autoDeleteObjects: true, +}); +``` diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts new file mode 100644 index 0000000000000..1bb32b7ce17f6 --- /dev/null +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts @@ -0,0 +1,189 @@ +import * as iam from '@aws-cdk/aws-iam'; +import * as cdk from '@aws-cdk/core'; + +/** + * A Lambda-based custom resource handler that deletes objects from an S3 + * bucket whenever a bucket is removed from the stack. + * + * This is required to force buckets to be deleted since the 'destroy' removal + * policy will not delete a bucket if the bucket still has objects. + * + * The resource property schema is: + * + * { + * BucketName: string + * } + * + * Sadly, we can't use @aws-cdk/aws-lambda as it will introduce a dependency + * cycle, so this uses raw `cdk.Resource`s. + */ +export class AutoDeleteObjectsResourceHandler extends cdk.Construct { + /** + * Defines a stack-singleton lambda function with the logic for a + * CloudFormation custom resource that deletes a bucket's S3 objects when the + * bucket is targeted for deletion. + * + * @returns The custom resource lambda function. + */ + public static singleton(context: cdk.Construct) { + const root = cdk.Stack.of(context); + + // well-known logical id to ensure stack singletonity + const logicalId = 'AutoDeleteObjectsHandler321d6b9b694d3476b0a14f1c09870ea4'; + let lambda = root.node.tryFindChild(logicalId) as AutoDeleteObjectsResourceHandler; + if (!lambda) { + lambda = new AutoDeleteObjectsResourceHandler(root, logicalId); + } + + return lambda; + } + + /** + * The ARN of the handler's lambda function. Used as a service token in the + * custom resource. + */ + public readonly functionArn: string; + + public readonly role: iam.Role; + + constructor(scope: cdk.Construct, id: string) { + super(scope, id); + + this.role = new iam.Role(this, 'Role', { + assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), + managedPolicies: [ + iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), + ], + }); + + const resourceType = 'AWS::Lambda::Function'; + class InLineLambda extends cdk.CfnResource { + public readonly tags: cdk.TagManager = new cdk.TagManager(cdk.TagType.STANDARD, resourceType); + + protected renderProperties(properties: any): { [key: string]: any } { + properties.Tags = cdk.listMapper( + cdk.cfnTagToCloudFormation)(this.tags.renderTags()); + delete properties.tags; + return properties; + } + } + const resource = new InLineLambda(this, 'Resource', { + type: resourceType, + properties: { + Description: 'AWS CloudFormation handler for "Custom::AutoDeleteBucketObjects" resources (@aws-cdk/aws-s3)', + Code: { ZipFile: `exports.handler = ${handler.toString()};` }, + Handler: 'index.handler', + Role: this.role.roleArn, + Runtime: 'nodejs12.x', + Timeout: 900, + }, + }); + + resource.node.addDependency(this.role); + + this.functionArn = resource.getAtt('Arn').toString(); + } +} + +/* eslint-disable no-console */ + +/** + * Lambda to handle deleting all objects in a bucket when the bucket is + * scheduled for deletion. This function will be inlined using .toString() + * as Lambda code. + */ +/* istanbul ignore next */ +const handler = async function (event: any, context: any) { + console.log('REQUEST RECEIVED:\n' + JSON.stringify(event)); + const responseData = {}; + let physicalResourceId; + + /** + * Upload a CloudFormation response for a Custom Resourcenection error or HTTP error response + */ + const report = function (evt: any, ctx: any, respStatus: any, physResourceId: any, respData: any, reason?: any) { + return new Promise((resolve, reject) => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const https = require('https'); + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { URL } = require('url'); + + const responseBody = JSON.stringify({ + Status: respStatus, + Reason: reason || 'See the details in CloudWatch Log Stream: ' + ctx.logStreamName, + PhysicalResourceId: physResourceId || ctx.logStreamName, + StackId: evt.StackId, + RequestId: evt.RequestId, + LogicalResourceId: evt.LogicalResourceId, + Data: respData, + }); + + const parsedUrl = new URL(evt.ResponseURL); + const options = { + hostname: parsedUrl.hostname, + port: 443, + path: parsedUrl.pathname + parsedUrl.search, + method: 'PUT', + headers: { + 'Content-Type': '', + 'Content-Length': responseBody.length, + }, + }; + + https.request(options) + .on('error', reject) + .on('response', (res: any) => { + res.resume(); + if (res.statusCode >= 400) { + reject(new Error(`Server returned error ${res.statusCode}: ${res.statusMessage}`)); + } else { + resolve(); + } + }) + .end(responseBody, 'utf8'); + }); + }; + + /** + * Recursively delete all items in the bucket + * + * @param {AWS.S3} s3 the S3 client + * @param {*} bucketName the bucket name + */ + const emptyBucket = async function(s3: any, bucketName: string) { + const listedObjects = await s3.listObjectVersions({ Bucket: bucketName }).promise(); + const contents = (listedObjects.Versions || []).concat(listedObjects.DeleteMarkers || []); + if (contents.length === 0) return; + + let records = contents.map((record: any) => ({ Key: record.Key, VersionId: record.VersionId })); + await s3.deleteObjects({ Bucket: bucketName, Delete: { Objects: records } }).promise(); + + if (listedObjects?.IsTruncated === 'true' ) await emptyBucket(s3, bucketName); + }; + + try { + const bucketName = event.ResourceProperties?.BucketName; + if (!bucketName) throw new Error('BucketName is required'); + physicalResourceId = `${bucketName}-${event.LogicalResourceId}`; + + switch (event.RequestType) { + case 'Create': + case 'Update': + await report(event, context, 'SUCCESS', physicalResourceId, responseData); + break; + case 'Delete': + // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies + const s3 = new (require('aws-sdk').S3)(); + await emptyBucket(s3, bucketName); + await report(event, context, 'SUCCESS', physicalResourceId, responseData); + break; + default: + throw new Error(`Unsupported request type ${event.RequestType}`); + } + + console.log('Done!'); + } catch (err) { + console.log(`Caught error ${err}.`); + await report(event, context, 'FAILED', physicalResourceId, null, err.message); + } +}; diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts new file mode 100644 index 0000000000000..65f7dab23c1b4 --- /dev/null +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -0,0 +1 @@ +export * from './auto-delete-objects-handler'; diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index acb1949b1cb2e..642a9bc4710c0 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -2,7 +2,8 @@ import { EOL } from 'os'; import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, CfnResource } from '@aws-cdk/core'; +import { AutoDeleteObjectsResourceHandler } from './auto-delete-objects-handler/auto-delete-objects-handler'; import { BucketPolicy } from './bucket-policy'; import { IBucketNotificationDestination } from './destination'; import { BucketNotifications } from './notifications-resource'; @@ -995,6 +996,17 @@ export interface BucketProps { */ readonly removalPolicy?: RemovalPolicy; + /** + * Whether all objects should be automatically deleted when the bucket is + * removed from the stack. Prevents the bucket deletion from getting skipped + * because there are still objects inside. + * + * Requires the removal policy to be set to destroy. + * + * @default false + */ + readonly autoDeleteObjects?: boolean; + /** * Whether this bucket should have versioning turned on or not. * @@ -1267,6 +1279,24 @@ export class Bucket extends BucketBase { if (props.publicReadAccess) { this.grantPublicAccess(); } + + if (props.autoDeleteObjects) { + if (props.removalPolicy !== RemovalPolicy.DESTROY) { + throw new Error("Cannot use 'autoDeleteObjects' property on a bucket without setting removal policy to 'destroy'."); + } + + const handler = AutoDeleteObjectsResourceHandler.singleton(this); + + new CfnResource(this, 'AutoDeleteBucketResource', { + type: 'Custom::AutoDeleteBucketObjects', + properties: { + ServiceToken: handler.functionArn, + BucketName: this.bucketName, + }, + }); + + this.grantReadWrite(handler.role); + } } /** diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts new file mode 100644 index 0000000000000..b1258dc828855 --- /dev/null +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -0,0 +1,28 @@ +import { expect, haveResource } from '@aws-cdk/assert'; +import * as cdk from '@aws-cdk/core'; +import { Test } from 'nodeunit'; +import * as s3 from '../lib'; + +export = { + 'when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it'(test: Test) { + const stack = new cdk.Stack(); + + new s3.Bucket(stack, 'MyBucket', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + + expect(stack).to(haveResource('AWS::S3::Bucket')); + expect(stack).to(haveResource('Custom::AutoDeleteBucketObjects')); + + test.done(); + }, + + 'when autoDeleteObjects is enabled, throws if removalPolicy is not specified'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true }), /removal policy/); + + test.done(); + }, +}; From 2d42b2e841a2f25aa64b7b19446e8367a4542850 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Mon, 12 Oct 2020 21:44:01 -0400 Subject: [PATCH 02/15] Refactor to use CustomResourceProvider --- packages/@aws-cdk/aws-s3/README.md | 15 +- .../auto-delete-objects-handler.ts | 189 ------------------ .../lib/auto-delete-objects-handler/index.ts | 45 ++++- packages/@aws-cdk/aws-s3/lib/bucket.ts | 36 +++- .../aws-s3/test/test.auto-delete-objects.ts | 2 +- 5 files changed, 78 insertions(+), 209 deletions(-) delete mode 100644 packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md index a828861739142..9de58f73e916c 100644 --- a/packages/@aws-cdk/aws-s3/README.md +++ b/packages/@aws-cdk/aws-s3/README.md @@ -344,13 +344,14 @@ bucket.virtualHostedUrlForObject('objectname', { regional: false }); // Virtual ### Bucket deletion -By default, when a bucket is removed from a stack (or the stack is deleted), -the S3 bucket will be removed according to its removal policy (which by -default, will simply orphan the bucket and leave it in your AWS account). -However, even if the removal policy is set to DESTROY, the bucket will get -skipped by default if it still has objects inside. - -To override this, enable the `autoDeleteObjects` property. +When a bucket is removed from a stack (or the stack is deleted), the S3 +bucket will be removed according to its removal policy (which by default will +simply orphan the bucket and leave it in your AWS account). If the removal +policy is set to `RemovalPolicy.DESTROY`, the bucket will be deleted as long +as it does not contain any objects. + +To override this and force all objects to get deleted during bucket deletion, +enable the`autoDeleteObjects` option. ```ts const bucket = new Bucket(this, 'MyTempFileBucket', { diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts deleted file mode 100644 index 1bb32b7ce17f6..0000000000000 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/auto-delete-objects-handler.ts +++ /dev/null @@ -1,189 +0,0 @@ -import * as iam from '@aws-cdk/aws-iam'; -import * as cdk from '@aws-cdk/core'; - -/** - * A Lambda-based custom resource handler that deletes objects from an S3 - * bucket whenever a bucket is removed from the stack. - * - * This is required to force buckets to be deleted since the 'destroy' removal - * policy will not delete a bucket if the bucket still has objects. - * - * The resource property schema is: - * - * { - * BucketName: string - * } - * - * Sadly, we can't use @aws-cdk/aws-lambda as it will introduce a dependency - * cycle, so this uses raw `cdk.Resource`s. - */ -export class AutoDeleteObjectsResourceHandler extends cdk.Construct { - /** - * Defines a stack-singleton lambda function with the logic for a - * CloudFormation custom resource that deletes a bucket's S3 objects when the - * bucket is targeted for deletion. - * - * @returns The custom resource lambda function. - */ - public static singleton(context: cdk.Construct) { - const root = cdk.Stack.of(context); - - // well-known logical id to ensure stack singletonity - const logicalId = 'AutoDeleteObjectsHandler321d6b9b694d3476b0a14f1c09870ea4'; - let lambda = root.node.tryFindChild(logicalId) as AutoDeleteObjectsResourceHandler; - if (!lambda) { - lambda = new AutoDeleteObjectsResourceHandler(root, logicalId); - } - - return lambda; - } - - /** - * The ARN of the handler's lambda function. Used as a service token in the - * custom resource. - */ - public readonly functionArn: string; - - public readonly role: iam.Role; - - constructor(scope: cdk.Construct, id: string) { - super(scope, id); - - this.role = new iam.Role(this, 'Role', { - assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), - managedPolicies: [ - iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), - ], - }); - - const resourceType = 'AWS::Lambda::Function'; - class InLineLambda extends cdk.CfnResource { - public readonly tags: cdk.TagManager = new cdk.TagManager(cdk.TagType.STANDARD, resourceType); - - protected renderProperties(properties: any): { [key: string]: any } { - properties.Tags = cdk.listMapper( - cdk.cfnTagToCloudFormation)(this.tags.renderTags()); - delete properties.tags; - return properties; - } - } - const resource = new InLineLambda(this, 'Resource', { - type: resourceType, - properties: { - Description: 'AWS CloudFormation handler for "Custom::AutoDeleteBucketObjects" resources (@aws-cdk/aws-s3)', - Code: { ZipFile: `exports.handler = ${handler.toString()};` }, - Handler: 'index.handler', - Role: this.role.roleArn, - Runtime: 'nodejs12.x', - Timeout: 900, - }, - }); - - resource.node.addDependency(this.role); - - this.functionArn = resource.getAtt('Arn').toString(); - } -} - -/* eslint-disable no-console */ - -/** - * Lambda to handle deleting all objects in a bucket when the bucket is - * scheduled for deletion. This function will be inlined using .toString() - * as Lambda code. - */ -/* istanbul ignore next */ -const handler = async function (event: any, context: any) { - console.log('REQUEST RECEIVED:\n' + JSON.stringify(event)); - const responseData = {}; - let physicalResourceId; - - /** - * Upload a CloudFormation response for a Custom Resourcenection error or HTTP error response - */ - const report = function (evt: any, ctx: any, respStatus: any, physResourceId: any, respData: any, reason?: any) { - return new Promise((resolve, reject) => { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const https = require('https'); - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { URL } = require('url'); - - const responseBody = JSON.stringify({ - Status: respStatus, - Reason: reason || 'See the details in CloudWatch Log Stream: ' + ctx.logStreamName, - PhysicalResourceId: physResourceId || ctx.logStreamName, - StackId: evt.StackId, - RequestId: evt.RequestId, - LogicalResourceId: evt.LogicalResourceId, - Data: respData, - }); - - const parsedUrl = new URL(evt.ResponseURL); - const options = { - hostname: parsedUrl.hostname, - port: 443, - path: parsedUrl.pathname + parsedUrl.search, - method: 'PUT', - headers: { - 'Content-Type': '', - 'Content-Length': responseBody.length, - }, - }; - - https.request(options) - .on('error', reject) - .on('response', (res: any) => { - res.resume(); - if (res.statusCode >= 400) { - reject(new Error(`Server returned error ${res.statusCode}: ${res.statusMessage}`)); - } else { - resolve(); - } - }) - .end(responseBody, 'utf8'); - }); - }; - - /** - * Recursively delete all items in the bucket - * - * @param {AWS.S3} s3 the S3 client - * @param {*} bucketName the bucket name - */ - const emptyBucket = async function(s3: any, bucketName: string) { - const listedObjects = await s3.listObjectVersions({ Bucket: bucketName }).promise(); - const contents = (listedObjects.Versions || []).concat(listedObjects.DeleteMarkers || []); - if (contents.length === 0) return; - - let records = contents.map((record: any) => ({ Key: record.Key, VersionId: record.VersionId })); - await s3.deleteObjects({ Bucket: bucketName, Delete: { Objects: records } }).promise(); - - if (listedObjects?.IsTruncated === 'true' ) await emptyBucket(s3, bucketName); - }; - - try { - const bucketName = event.ResourceProperties?.BucketName; - if (!bucketName) throw new Error('BucketName is required'); - physicalResourceId = `${bucketName}-${event.LogicalResourceId}`; - - switch (event.RequestType) { - case 'Create': - case 'Update': - await report(event, context, 'SUCCESS', physicalResourceId, responseData); - break; - case 'Delete': - // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies - const s3 = new (require('aws-sdk').S3)(); - await emptyBucket(s3, bucketName); - await report(event, context, 'SUCCESS', physicalResourceId, responseData); - break; - default: - throw new Error(`Unsupported request type ${event.RequestType}`); - } - - console.log('Done!'); - } catch (err) { - console.log(`Caught error ${err}.`); - await report(event, context, 'FAILED', physicalResourceId, null, err.message); - } -}; diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts index 65f7dab23c1b4..7d46fee748060 100644 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -1 +1,44 @@ -export * from './auto-delete-objects-handler'; +export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { + if (event.RequestType === 'Create') { return onCreate(event); } + if (event.RequestType === 'Update') { return onUpdate(event); } + if (event.RequestType === 'Delete') { return onDelete(event); } + throw new Error('Invalid request type.'); +} + +/** + * Recursively delete all items in the bucket + * + * @param {AWS.S3} s3 the S3 client + * @param {*} bucketName the bucket name + */ +async function emptyBucket(s3: any, bucketName: string) { + const listedObjects = await s3.listObjectVersions({ Bucket: bucketName }).promise(); + const contents = (listedObjects.Versions || []).concat(listedObjects.DeleteMarkers || []); + if (contents.length === 0) { + return; + }; + + let records = contents.map((record: any) => ({ Key: record.Key, VersionId: record.VersionId })); + await s3.deleteObjects({ Bucket: bucketName, Delete: { Objects: records } }).promise(); + + if (listedObjects?.IsTruncated === 'true' ) await emptyBucket(s3, bucketName); +} + +async function onCreate(_event: AWSLambda.CloudFormationCustomResourceCreateEvent) { + return; +} + +async function onUpdate(_event: AWSLambda.CloudFormationCustomResourceUpdateEvent) { + return; +} + +async function onDelete(deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { + const bucketName = deleteEvent.ResourceProperties?.BucketName; + if (!bucketName) { + throw new Error('No BucketName was provided.'); + } + + // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies + const s3 = new (require('aws-sdk').S3)(); + await emptyBucket(s3, bucketName); +} diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index ebf10b2674392..216f55bf5f6d0 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1,10 +1,10 @@ import { EOL } from 'os'; +import * as path from 'path'; import * as events from '@aws-cdk/aws-events'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, CfnResource } from '@aws-cdk/core'; +import { Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, CustomResource, CustomResourceProvider, CustomResourceProviderRuntime } from '@aws-cdk/core'; import { Construct } from 'constructs'; -import { AutoDeleteObjectsResourceHandler } from './auto-delete-objects-handler/auto-delete-objects-handler'; import { BucketPolicy } from './bucket-policy'; import { IBucketNotificationDestination } from './destination'; import { BucketNotifications } from './notifications-resource'; @@ -13,6 +13,8 @@ import { LifecycleRule } from './rule'; import { CfnBucket } from './s3.generated'; import { parseBucketArn, parseBucketName } from './util'; +const AUTO_DELETE_OBJECTS_RESOURCE_TYPE = 'Custom::AutoDeleteObjects'; + export interface IBucket extends IResource { /** * The ARN of the bucket. @@ -1029,8 +1031,7 @@ export interface BucketProps { /** * Whether all objects should be automatically deleted when the bucket is - * removed from the stack. Prevents the bucket deletion from getting skipped - * because there are still objects inside. + * removed from the stack. * * Requires the removal policy to be set to destroy. * @@ -1319,17 +1320,13 @@ export class Bucket extends BucketBase { throw new Error("Cannot use 'autoDeleteObjects' property on a bucket without setting removal policy to 'destroy'."); } - const handler = AutoDeleteObjectsResourceHandler.singleton(this); - - new CfnResource(this, 'AutoDeleteBucketResource', { - type: 'Custom::AutoDeleteBucketObjects', + new CustomResource(this, 'AutoDeleteObjectsResource', { + resourceType: AUTO_DELETE_OBJECTS_RESOURCE_TYPE, + serviceToken: this.getOrCreateAutoDeleteObjectsResource(), properties: { - ServiceToken: handler.functionArn, BucketName: this.bucketName, }, }); - - this.grantReadWrite(handler.role); } } @@ -1722,6 +1719,23 @@ export class Bucket extends BucketBase { }; }); } + + private getOrCreateAutoDeleteObjectsResource() { + return CustomResourceProvider.getOrCreate(this, AUTO_DELETE_OBJECTS_RESOURCE_TYPE, { + codeDirectory: path.join(__dirname, 'auto-delete-objects-handler'), + runtime: CustomResourceProviderRuntime.NODEJS_12, + policyStatements: [ + { + Effect: 'Allow', + Resource: '*', + Action: [ + // TODO: add key perms? + ...perms.BUCKET_READ_ACTIONS.concat(perms.BUCKET_WRITE_ACTIONS), + ], + }, + ], + }); + } } /** diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index b1258dc828855..a9ca0edbb079e 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -13,7 +13,7 @@ export = { }); expect(stack).to(haveResource('AWS::S3::Bucket')); - expect(stack).to(haveResource('Custom::AutoDeleteBucketObjects')); + expect(stack).to(haveResource('Custom::AutoDeleteObjects')); test.done(); }, From 56a48d76de4349fe646ce9b481d085e13baa08ea Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Mon, 12 Oct 2020 22:51:55 -0400 Subject: [PATCH 03/15] Update unit tests --- .../aws-s3/test/test.auto-delete-objects.ts | 73 ++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index a9ca0edbb079e..c02bc7c9ab215 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -13,7 +13,78 @@ export = { }); expect(stack).to(haveResource('AWS::S3::Bucket')); - expect(stack).to(haveResource('Custom::AutoDeleteObjects')); + expect(stack).to(haveResource('AWS::Lambda::Function')); + expect(stack).to(haveResource('Custom::AutoDeleteObjects', + { BucketName: { Ref: 'MyBucketF68F3FF0' } })); + + test.done(); + }, + + 'two buckets with autoDeleteObjects enabled share the same cr provider'(test: Test) { + const app = new cdk.App(); + const stack = new cdk.Stack(app); + + new s3.Bucket(stack, 'MyBucket1', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + new s3.Bucket(stack, 'MyBucket2', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + + const template = app.synth().getStackArtifact(stack.artifactId).template; + const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); + + test.deepEqual(resourceTypes, [ + // custom resource provider resources + 'AWS::IAM::Role', + 'AWS::Lambda::Function', + + // buckets + 'AWS::S3::Bucket', + 'AWS::S3::Bucket', + + // auto delete object resources + 'Custom::AutoDeleteObjects', + 'Custom::AutoDeleteObjects', + ]); + + test.done(); + }, + + 'iam policy'(test: Test) { + const stack = new cdk.Stack(); + + new s3.Bucket(stack, 'MyBucket', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + + expect(stack).to(haveResource('AWS::IAM::Role', { + Policies: [ + { + PolicyName: 'Inline', + PolicyDocument: { + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Resource: '*', + Action: [ + 's3:GetObject*', + 's3:GetBucket*', + 's3:List*', + 's3:DeleteObject*', + 's3:PutObject*', + 's3:Abort*', + ], + }, + ], + }, + }, + ], + })); test.done(); }, From 809ea22c993d41931f3fd8ca1ade413080caa801 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 18 Oct 2020 00:55:38 -0400 Subject: [PATCH 04/15] Add feedback from jogold --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 5 +++-- packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 216f55bf5f6d0..f5446660efc09 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1727,10 +1727,11 @@ export class Bucket extends BucketBase { policyStatements: [ { Effect: 'Allow', - Resource: '*', + Resource: this.bucketArn, Action: [ // TODO: add key perms? - ...perms.BUCKET_READ_ACTIONS.concat(perms.BUCKET_WRITE_ACTIONS), + ...perms.BUCKET_READ_ACTIONS, + ...perms.BUCKET_WRITE_ACTIONS, ], }, ], diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index c02bc7c9ab215..59671fb50488a 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -70,7 +70,12 @@ export = { Statement: [ { Effect: 'Allow', - Resource: '*', + Resource: { + 'Fn::GetAtt': [ + 'MyBucketF68F3FF0', + 'Arn', + ], + }, Action: [ 's3:GetObject*', 's3:GetBucket*', From 83463a4d6edc6d45e28731dffe7b6481394f0f22 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 18 Oct 2020 02:42:39 -0400 Subject: [PATCH 05/15] Add unit tests for handler and ServiceToken --- .../lib/auto-delete-objects-handler/index.ts | 19 ++- packages/@aws-cdk/aws-s3/package.json | 3 +- .../aws-s3/test/test.auto-delete-objects.ts | 116 +++++++++++++++++- 3 files changed, 126 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts index 7d46fee748060..d69075a91fdf3 100644 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -1,7 +1,9 @@ -export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { - if (event.RequestType === 'Create') { return onCreate(event); } - if (event.RequestType === 'Update') { return onUpdate(event); } - if (event.RequestType === 'Delete') { return onDelete(event); } +export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent, s3?: any) { + // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies + if (!s3) { s3 = new (require('aws-sdk').S3)(); } + if (event.RequestType === 'Create') { return onCreate(s3, event); } + if (event.RequestType === 'Update') { return onUpdate(s3, event); } + if (event.RequestType === 'Delete') { return onDelete(s3, event); } throw new Error('Invalid request type.'); } @@ -24,21 +26,18 @@ async function emptyBucket(s3: any, bucketName: string) { if (listedObjects?.IsTruncated === 'true' ) await emptyBucket(s3, bucketName); } -async function onCreate(_event: AWSLambda.CloudFormationCustomResourceCreateEvent) { +async function onCreate(_s3: any, _event: AWSLambda.CloudFormationCustomResourceCreateEvent) { return; } -async function onUpdate(_event: AWSLambda.CloudFormationCustomResourceUpdateEvent) { +async function onUpdate(_s3: any, _event: AWSLambda.CloudFormationCustomResourceUpdateEvent) { return; } -async function onDelete(deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { +async function onDelete(s3: any, deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { const bucketName = deleteEvent.ResourceProperties?.BucketName; if (!bucketName) { throw new Error('No BucketName was provided.'); } - - // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies - const s3 = new (require('aws-sdk').S3)(); await emptyBucket(s3, bucketName); } diff --git a/packages/@aws-cdk/aws-s3/package.json b/packages/@aws-cdk/aws-s3/package.json index fec39ed3100c9..611bd74aafb9a 100644 --- a/packages/@aws-cdk/aws-s3/package.json +++ b/packages/@aws-cdk/aws-s3/package.json @@ -76,7 +76,8 @@ "cdk-integ-tools": "0.0.0", "cfn2ts": "0.0.0", "nodeunit": "^0.11.3", - "pkglint": "0.0.0" + "pkglint": "0.0.0", + "sinon": "9.2.0" }, "dependencies": { "@aws-cdk/aws-events": "0.0.0", diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index 59671fb50488a..7d030d2153659 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -1,7 +1,9 @@ import { expect, haveResource } from '@aws-cdk/assert'; import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; +import * as sinon from 'sinon'; import * as s3 from '../lib'; +import { handler } from '../lib/auto-delete-objects-handler/index'; export = { 'when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it'(test: Test) { @@ -15,7 +17,15 @@ export = { expect(stack).to(haveResource('AWS::S3::Bucket')); expect(stack).to(haveResource('AWS::Lambda::Function')); expect(stack).to(haveResource('Custom::AutoDeleteObjects', - { BucketName: { Ref: 'MyBucketF68F3FF0' } })); + { + BucketName: { Ref: 'MyBucketF68F3FF0' }, + ServiceToken: { + 'Fn::GetAtt': [ + 'CustomAutoDeleteObjectsCustomResourceProviderHandlerE060A45A', + 'Arn', + ], + }, + })); test.done(); }, @@ -101,4 +111,108 @@ export = { test.done(); }, + + 'custom resource handler': { + + async 'does nothing on create event'(test: Test) { + sinon.reset(); + const s3Mock = newS3ClientMock(); + + const event: Partial = { + RequestType: 'Create', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.notCalled(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + + test.done(); + }, + + async 'does nothing on update event'(test: Test) { + sinon.reset(); + + const s3Mock = newS3ClientMock(); + + const event: Partial = { + RequestType: 'Update', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.notCalled(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + + test.done(); + }, + + async 'deletes no objects on delete event when bucket has no objects'(test: Test) { + sinon.reset(); + + const listObjectVersionsMock = sinon.fake.returns({ Versions: [] }); + const s3Mock = newS3ClientMock(listObjectVersionsMock); + + const event: Partial = { + RequestType: 'Delete', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.calledOnce(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + + test.done(); + }, + + async 'deletes all objects on delete event'(test: Test) { + sinon.reset(); + + const listObjectVersionsMock = sinon.fake.returns({ + Versions: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + }); + const s3Mock = newS3ClientMock(listObjectVersionsMock); + + const event: Partial = { + RequestType: 'Delete', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.calledOnce(s3Mock.listObjectVersions); + sinon.assert.calledOnce(s3Mock.deleteObjects); + + test.done(); + }, + }, }; + +async function invokeHandler(event: Partial, s3client: any) { + return handler(event as any, s3client); +} + +function newS3ClientMock(listObjectVersionsMock?: any, deleteObjectsMock?: any) { + return { + listObjectVersions: sinon.stub().returns({ + promise: listObjectVersionsMock ?? sinon.fake.returns({}), + }), + deleteObjects: sinon.stub().returns({ + promise: deleteObjectsMock ?? sinon.fake.returns({}), + }), + }; +} From 6ff4becfa5f4cfe116f7169912924754f11782ef Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 18 Oct 2020 18:03:01 -0400 Subject: [PATCH 06/15] revert changes to handler permissions since it is singleton --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index f5446660efc09..57f838a335810 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1727,7 +1727,7 @@ export class Bucket extends BucketBase { policyStatements: [ { Effect: 'Allow', - Resource: this.bucketArn, + Resource: '*', Action: [ // TODO: add key perms? ...perms.BUCKET_READ_ACTIONS, diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index 7d030d2153659..8d4a2f16525a3 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -80,12 +80,7 @@ export = { Statement: [ { Effect: 'Allow', - Resource: { - 'Fn::GetAtt': [ - 'MyBucketF68F3FF0', - 'Arn', - ], - }, + Resource: '*', Action: [ 's3:GetObject*', 's3:GetBucket*', From 885b1787be10cef03581dffe5af926c045eede52 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 18 Oct 2020 18:52:49 -0400 Subject: [PATCH 07/15] add unit test --- .../aws-s3/test/test.auto-delete-objects.ts | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index 8d4a2f16525a3..2c70a1759c724 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -1,4 +1,4 @@ -import { expect, haveResource } from '@aws-cdk/assert'; +import { expect, haveResource, not } from '@aws-cdk/assert'; import * as cdk from '@aws-cdk/core'; import { Test } from 'nodeunit'; import * as sinon from 'sinon'; @@ -34,11 +34,11 @@ export = { const app = new cdk.App(); const stack = new cdk.Stack(app); - new s3.Bucket(stack, 'MyBucket1', { + new s3.Bucket(stack, 'MyBucketOne', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, }); - new s3.Bucket(stack, 'MyBucket2', { + new s3.Bucket(stack, 'MyBucketTwo', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, }); @@ -63,6 +63,46 @@ export = { test.done(); }, + 'when only one bucket has autoDeleteObjects enabled, only that bucket gets assigned a custom resource'(test: Test) { + const app = new cdk.App(); + const stack = new cdk.Stack(app); + + new s3.Bucket(stack, 'MyBucketOne', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + new s3.Bucket(stack, 'MyBucketTwo', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: false, + }); + + const template = app.synth().getStackArtifact(stack.artifactId).template; + const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); + + test.deepEqual(resourceTypes, [ + // custom resource provider resources + 'AWS::IAM::Role', + 'AWS::Lambda::Function', + + // buckets + 'AWS::S3::Bucket', + 'AWS::S3::Bucket', + + // auto delete object resource + 'Custom::AutoDeleteObjects', + ]); + + // custom resource for MyBucket1 is present + expect(stack).to(haveResource('Custom::AutoDeleteObjects', + { BucketName: { Ref: 'MyBucketOneA6BE54C9' } })); + + // custom resource for MyBucket2 is not present + expect(stack).to(not(haveResource('Custom::AutoDeleteObjects', + { BucketName: { Ref: 'MyBucketTwoC7437026' } }))); + + test.done(); + }, + 'iam policy'(test: Test) { const stack = new cdk.Stack(); From 00f1b65d3a57f7b03d940b668daa9fea19c45728 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 18 Oct 2020 19:03:02 -0400 Subject: [PATCH 08/15] remove todo comment --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 57f838a335810..b83236ef17e19 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1729,7 +1729,6 @@ export class Bucket extends BucketBase { Effect: 'Allow', Resource: '*', Action: [ - // TODO: add key perms? ...perms.BUCKET_READ_ACTIONS, ...perms.BUCKET_WRITE_ACTIONS, ], From 6762518a088290a5f2507c65b62d2d3688057e59 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Mon, 19 Oct 2020 22:18:09 -0400 Subject: [PATCH 09/15] remove unneeded PutObject permissions --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index b83236ef17e19..f70d0e7f853de 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1730,7 +1730,7 @@ export class Bucket extends BucketBase { Resource: '*', Action: [ ...perms.BUCKET_READ_ACTIONS, - ...perms.BUCKET_WRITE_ACTIONS, + ...perms.BUCKET_DELETE_ACTIONS, ], }, ], diff --git a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts index 2c70a1759c724..0483340dcdd36 100644 --- a/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts +++ b/packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts @@ -126,8 +126,6 @@ export = { 's3:GetBucket*', 's3:List*', 's3:DeleteObject*', - 's3:PutObject*', - 's3:Abort*', ], }, ], From 2d41d98f2f7746a3458af33bc272c60253460758 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sat, 7 Nov 2020 00:30:34 -0500 Subject: [PATCH 10/15] convert test file to jest --- .../aws-s3/test/auto-delete-objects.test.ts | 419 +++++++++--------- 1 file changed, 199 insertions(+), 220 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index ea1adac79ac7a..f1d40e433756a 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -1,238 +1,217 @@ -import { expect, haveResource, not } from '@aws-cdk/assert'; +import '@aws-cdk/assert/jest'; import * as cdk from '@aws-cdk/core'; -import { nodeunitShim, Test } from 'nodeunit-shim'; import * as sinon from 'sinon'; import * as s3 from '../lib'; import { handler } from '../lib/auto-delete-objects-handler/index'; -nodeunitShim({ - 'when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it'(test: Test) { - const stack = new cdk.Stack(); - - new s3.Bucket(stack, 'MyBucket', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: true, - }); - - expect(stack).to(haveResource('AWS::S3::Bucket')); - expect(stack).to(haveResource('AWS::Lambda::Function')); - expect(stack).to(haveResource('Custom::AutoDeleteObjects', - { - BucketName: { Ref: 'MyBucketF68F3FF0' }, - ServiceToken: { - 'Fn::GetAtt': [ - 'CustomAutoDeleteObjectsCustomResourceProviderHandlerE060A45A', - 'Arn', - ], - }, - })); - - test.done(); - }, - - 'two buckets with autoDeleteObjects enabled share the same cr provider'(test: Test) { - const app = new cdk.App(); - const stack = new cdk.Stack(app); - - new s3.Bucket(stack, 'MyBucketOne', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: true, - }); - new s3.Bucket(stack, 'MyBucketTwo', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: true, - }); - - const template = app.synth().getStackArtifact(stack.artifactId).template; - const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); - - test.deepEqual(resourceTypes, [ - // custom resource provider resources - 'AWS::IAM::Role', - 'AWS::Lambda::Function', - - // buckets - 'AWS::S3::Bucket', - 'AWS::S3::Bucket', - - // auto delete object resources - 'Custom::AutoDeleteObjects', - 'Custom::AutoDeleteObjects', - ]); - - test.done(); - }, - - 'when only one bucket has autoDeleteObjects enabled, only that bucket gets assigned a custom resource'(test: Test) { - const app = new cdk.App(); - const stack = new cdk.Stack(app); - - new s3.Bucket(stack, 'MyBucketOne', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: true, - }); - new s3.Bucket(stack, 'MyBucketTwo', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: false, - }); - - const template = app.synth().getStackArtifact(stack.artifactId).template; - const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); - - test.deepEqual(resourceTypes, [ - // custom resource provider resources - 'AWS::IAM::Role', - 'AWS::Lambda::Function', - - // buckets - 'AWS::S3::Bucket', - 'AWS::S3::Bucket', - - // auto delete object resource - 'Custom::AutoDeleteObjects', - ]); - - // custom resource for MyBucket1 is present - expect(stack).to(haveResource('Custom::AutoDeleteObjects', - { BucketName: { Ref: 'MyBucketOneA6BE54C9' } })); - - // custom resource for MyBucket2 is not present - expect(stack).to(not(haveResource('Custom::AutoDeleteObjects', - { BucketName: { Ref: 'MyBucketTwoC7437026' } }))); - - test.done(); - }, - - 'iam policy'(test: Test) { - const stack = new cdk.Stack(); - - new s3.Bucket(stack, 'MyBucket', { - removalPolicy: cdk.RemovalPolicy.DESTROY, - autoDeleteObjects: true, +test('when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it', () => { + const stack = new cdk.Stack(); + + new s3.Bucket(stack, 'MyBucket', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + + expect(stack).toHaveResource('AWS::S3::Bucket'); + expect(stack).toHaveResource('AWS::Lambda::Function'); + expect(stack).toHaveResource('Custom::AutoDeleteObjects', + { + BucketName: { Ref: 'MyBucketF68F3FF0' }, + ServiceToken: { + 'Fn::GetAtt': [ + 'CustomAutoDeleteObjectsCustomResourceProviderHandlerE060A45A', + 'Arn', + ], + }, }); +}); - expect(stack).to(haveResource('AWS::IAM::Role', { - Policies: [ - { - PolicyName: 'Inline', - PolicyDocument: { - Version: '2012-10-17', - Statement: [ - { - Effect: 'Allow', - Resource: '*', - Action: [ - 's3:GetObject*', - 's3:GetBucket*', - 's3:List*', - 's3:DeleteObject*', - ], - }, - ], - }, - }, - ], - })); - - test.done(); - }, - - 'when autoDeleteObjects is enabled, throws if removalPolicy is not specified'(test: Test) { - const stack = new cdk.Stack(); - - test.throws(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true }), /removal policy/); - - test.done(); - }, - - 'custom resource handler': { - - async 'does nothing on create event'(test: Test) { - sinon.reset(); - const s3Mock = newS3ClientMock(); - - const event: Partial = { - RequestType: 'Create', - ResourceProperties: { - ServiceToken: 'Foo', - BucketName: 'MyBucket', - }, - }; - await invokeHandler(event, s3Mock); - - sinon.assert.notCalled(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); - - test.done(); - }, - - async 'does nothing on update event'(test: Test) { - sinon.reset(); - - const s3Mock = newS3ClientMock(); - - const event: Partial = { - RequestType: 'Update', - ResourceProperties: { - ServiceToken: 'Foo', - BucketName: 'MyBucket', - }, - }; - await invokeHandler(event, s3Mock); - - sinon.assert.notCalled(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); +test('two buckets with autoDeleteObjects enabled share the same cr provider', () => { + const app = new cdk.App(); + const stack = new cdk.Stack(app); + + new s3.Bucket(stack, 'MyBucketOne', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + new s3.Bucket(stack, 'MyBucketTwo', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + + const template = app.synth().getStackArtifact(stack.artifactId).template; + const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); + + expect(resourceTypes).toStrictEqual([ + // custom resource provider resources + 'AWS::IAM::Role', + 'AWS::Lambda::Function', + + // buckets + 'AWS::S3::Bucket', + 'AWS::S3::Bucket', + + // auto delete object resources + 'Custom::AutoDeleteObjects', + 'Custom::AutoDeleteObjects', + ]); +}); - test.done(); - }, +test('when only one bucket has autoDeleteObjects enabled, only that bucket gets assigned a custom resource', () => { + const app = new cdk.App(); + const stack = new cdk.Stack(app); + + new s3.Bucket(stack, 'MyBucketOne', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); + new s3.Bucket(stack, 'MyBucketTwo', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: false, + }); + + const template = app.synth().getStackArtifact(stack.artifactId).template; + const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); + + expect(resourceTypes).toStrictEqual([ + // custom resource provider resources + 'AWS::IAM::Role', + 'AWS::Lambda::Function', + + // buckets + 'AWS::S3::Bucket', + 'AWS::S3::Bucket', + + // auto delete object resource + 'Custom::AutoDeleteObjects', + ]); + + // custom resource for MyBucket1 is present + expect(stack).toHaveResource('Custom::AutoDeleteObjects', + { BucketName: { Ref: 'MyBucketOneA6BE54C9' } }); + + // custom resource for MyBucket2 is not present + expect(stack).not.toHaveResource('Custom::AutoDeleteObjects', + { BucketName: { Ref: 'MyBucketTwoC7437026' } }); +}); - async 'deletes no objects on delete event when bucket has no objects'(test: Test) { - sinon.reset(); +test('iam policy is created', () => { + const stack = new cdk.Stack(); - const listObjectVersionsMock = sinon.fake.returns({ Versions: [] }); - const s3Mock = newS3ClientMock(listObjectVersionsMock); + new s3.Bucket(stack, 'MyBucket', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + autoDeleteObjects: true, + }); - const event: Partial = { - RequestType: 'Delete', - ResourceProperties: { - ServiceToken: 'Foo', - BucketName: 'MyBucket', + expect(stack).toHaveResource('AWS::IAM::Role', { + Policies: [ + { + PolicyName: 'Inline', + PolicyDocument: { + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Resource: '*', + Action: [ + 's3:GetObject*', + 's3:GetBucket*', + 's3:List*', + 's3:DeleteObject*', + ], + }, + ], }, - }; - await invokeHandler(event, s3Mock); - - sinon.assert.calledOnce(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); - - test.done(); - }, - - async 'deletes all objects on delete event'(test: Test) { - sinon.reset(); + }, + ], + }); +}); - const listObjectVersionsMock = sinon.fake.returns({ - Versions: [ - { Key: 'Key1', VersionId: 'VersionId1' }, - { Key: 'Key2', VersionId: 'VersionId2' }, - ], - }); - const s3Mock = newS3ClientMock(listObjectVersionsMock); - - const event: Partial = { - RequestType: 'Delete', - ResourceProperties: { - ServiceToken: 'Foo', - BucketName: 'MyBucket', - }, - }; - await invokeHandler(event, s3Mock); +test('when autoDeleteObjects is enabled, throws if removalPolicy is not specified', () => { + const stack = new cdk.Stack(); - sinon.assert.calledOnce(s3Mock.listObjectVersions); - sinon.assert.calledOnce(s3Mock.deleteObjects); + expect(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true })).toThrowError(/removal policy/); +}); - test.done(); - }, - }, +describe('custom resource handler', () => { + + test('does nothing on create event', async () => { + sinon.reset(); + const s3Mock = newS3ClientMock(); + + const event: Partial = { + RequestType: 'Create', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.notCalled(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + }); + + test('does nothing on update event', async () => { + sinon.reset(); + + const s3Mock = newS3ClientMock(); + + const event: Partial = { + RequestType: 'Update', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.notCalled(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + }); + + test('deletes no objects on delete event when bucket has no objects', async () => { + sinon.reset(); + + const listObjectVersionsMock = sinon.fake.returns({ Versions: [] }); + const s3Mock = newS3ClientMock(listObjectVersionsMock); + + const event: Partial = { + RequestType: 'Delete', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.calledOnce(s3Mock.listObjectVersions); + sinon.assert.notCalled(s3Mock.deleteObjects); + }); + + test('deletes all objects on delete event', async () => { + sinon.reset(); + + const listObjectVersionsMock = sinon.fake.returns({ + Versions: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + }); + const s3Mock = newS3ClientMock(listObjectVersionsMock); + + const event: Partial = { + RequestType: 'Delete', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event, s3Mock); + + sinon.assert.calledOnce(s3Mock.listObjectVersions); + sinon.assert.calledOnce(s3Mock.deleteObjects); + }); }); async function invokeHandler(event: Partial, s3client: any) { From 4e2e2ff4691e57779bbb1c88615de3a43c5b074d Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sat, 7 Nov 2020 00:41:04 -0500 Subject: [PATCH 11/15] rewrite mocking with jest to remove sinon dependency --- packages/@aws-cdk/aws-s3/package.json | 3 +- .../aws-s3/test/auto-delete-objects.test.ts | 40 ++++++++----------- 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/package.json b/packages/@aws-cdk/aws-s3/package.json index bf471134c4a54..c886e31837d75 100644 --- a/packages/@aws-cdk/aws-s3/package.json +++ b/packages/@aws-cdk/aws-s3/package.json @@ -77,8 +77,7 @@ "cdk-integ-tools": "0.0.0", "cfn2ts": "0.0.0", "pkglint": "0.0.0", - "nodeunit-shim": "0.0.0", - "sinon": "^9.2.1" + "nodeunit-shim": "0.0.0" }, "dependencies": { "@aws-cdk/aws-events": "0.0.0", diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index f1d40e433756a..0dfb10e8d848e 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -1,6 +1,5 @@ import '@aws-cdk/assert/jest'; import * as cdk from '@aws-cdk/core'; -import * as sinon from 'sinon'; import * as s3 from '../lib'; import { handler } from '../lib/auto-delete-objects-handler/index'; @@ -43,7 +42,7 @@ test('two buckets with autoDeleteObjects enabled share the same cr provider', () const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); expect(resourceTypes).toStrictEqual([ - // custom resource provider resources + // custom resource provider resources (shared) 'AWS::IAM::Role', 'AWS::Lambda::Function', @@ -127,7 +126,7 @@ test('iam policy is created', () => { }); }); -test('when autoDeleteObjects is enabled, throws if removalPolicy is not specified', () => { +test('throws if autoDeleteObjects is enabled but if removalPolicy is not specified', () => { const stack = new cdk.Stack(); expect(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true })).toThrowError(/removal policy/); @@ -136,7 +135,6 @@ test('when autoDeleteObjects is enabled, throws if removalPolicy is not specifie describe('custom resource handler', () => { test('does nothing on create event', async () => { - sinon.reset(); const s3Mock = newS3ClientMock(); const event: Partial = { @@ -148,13 +146,11 @@ describe('custom resource handler', () => { }; await invokeHandler(event, s3Mock); - sinon.assert.notCalled(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); + expect(s3Mock.listObjectVersions).not.toHaveBeenCalled(); + expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); }); test('does nothing on update event', async () => { - sinon.reset(); - const s3Mock = newS3ClientMock(); const event: Partial = { @@ -166,14 +162,12 @@ describe('custom resource handler', () => { }; await invokeHandler(event, s3Mock); - sinon.assert.notCalled(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); + expect(s3Mock.listObjectVersions).not.toHaveBeenCalled(); + expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); }); test('deletes no objects on delete event when bucket has no objects', async () => { - sinon.reset(); - - const listObjectVersionsMock = sinon.fake.returns({ Versions: [] }); + const listObjectVersionsMock = jest.fn().mockReturnValue({ Versions: [] }); const s3Mock = newS3ClientMock(listObjectVersionsMock); const event: Partial = { @@ -185,14 +179,12 @@ describe('custom resource handler', () => { }; await invokeHandler(event, s3Mock); - sinon.assert.calledOnce(s3Mock.listObjectVersions); - sinon.assert.notCalled(s3Mock.deleteObjects); + expect(s3Mock.listObjectVersions).toHaveBeenCalled(); + expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); }); test('deletes all objects on delete event', async () => { - sinon.reset(); - - const listObjectVersionsMock = sinon.fake.returns({ + const listObjectVersionsMock = jest.fn().mockReturnValue({ Versions: [ { Key: 'Key1', VersionId: 'VersionId1' }, { Key: 'Key2', VersionId: 'VersionId2' }, @@ -209,8 +201,8 @@ describe('custom resource handler', () => { }; await invokeHandler(event, s3Mock); - sinon.assert.calledOnce(s3Mock.listObjectVersions); - sinon.assert.calledOnce(s3Mock.deleteObjects); + expect(s3Mock.listObjectVersions).toHaveBeenCalled(); + expect(s3Mock.deleteObjects).toHaveBeenCalled(); }); }); @@ -220,11 +212,11 @@ async function invokeHandler(event: Partial Date: Sat, 7 Nov 2020 01:50:26 -0500 Subject: [PATCH 12/15] refactor tests to mock S3 client cleaner, add one test --- .../lib/auto-delete-objects-handler/index.ts | 4 +- .../aws-s3/test/auto-delete-objects.test.ts | 94 ++++++++++++------- 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts index d69075a91fdf3..0cb97126040a1 100644 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -1,6 +1,6 @@ -export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent, s3?: any) { +export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies - if (!s3) { s3 = new (require('aws-sdk').S3)(); } + const s3 = new (require('aws-sdk').S3)(); if (event.RequestType === 'Create') { return onCreate(s3, event); } if (event.RequestType === 'Update') { return onUpdate(s3, event); } if (event.RequestType === 'Delete') { return onDelete(s3, event); } diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index 0dfb10e8d848e..120c893a9676d 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -134,9 +134,23 @@ test('throws if autoDeleteObjects is enabled but if removalPolicy is not specifi describe('custom resource handler', () => { - test('does nothing on create event', async () => { - const s3Mock = newS3ClientMock(); + const mockS3Client = { + listObjectVersions: jest.fn().mockReturnThis(), + deleteObjects: jest.fn().mockReturnThis(), + promise: jest.fn(), + }; + + jest.mock('aws-sdk', () => { + return { S3: jest.fn(() => mockS3Client) }; + }); + beforeEach(() => { + mockS3Client.deleteObjects.mockClear(); + mockS3Client.listObjectVersions.mockClear(); + mockS3Client.promise.mockClear(); + }); + + test('does nothing on create event', async () => { const event: Partial = { RequestType: 'Create', ResourceProperties: { @@ -144,15 +158,13 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; - await invokeHandler(event, s3Mock); + await invokeHandler(event); - expect(s3Mock.listObjectVersions).not.toHaveBeenCalled(); - expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); + expect(mockS3Client.listObjectVersions).not.toHaveBeenCalled(); + expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); }); test('does nothing on update event', async () => { - const s3Mock = newS3ClientMock(); - const event: Partial = { RequestType: 'Update', ResourceProperties: { @@ -160,15 +172,14 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; - await invokeHandler(event, s3Mock); + await invokeHandler(event); - expect(s3Mock.listObjectVersions).not.toHaveBeenCalled(); - expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); + expect(mockS3Client.listObjectVersions).not.toHaveBeenCalled(); + expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); }); test('deletes no objects on delete event when bucket has no objects', async () => { - const listObjectVersionsMock = jest.fn().mockReturnValue({ Versions: [] }); - const s3Mock = newS3ClientMock(listObjectVersionsMock); + mockS3Client.listObjectVersions().promise.mockResolvedValue({ Versions: [] }); // this counts as one call const event: Partial = { RequestType: 'Delete', @@ -177,20 +188,19 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; - await invokeHandler(event, s3Mock); + await invokeHandler(event); - expect(s3Mock.listObjectVersions).toHaveBeenCalled(); - expect(s3Mock.deleteObjects).not.toHaveBeenCalled(); + expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(2); + expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); }); test('deletes all objects on delete event', async () => { - const listObjectVersionsMock = jest.fn().mockReturnValue({ + mockS3Client.listObjectVersions().promise.mockResolvedValue({ // this counts as one call Versions: [ { Key: 'Key1', VersionId: 'VersionId1' }, { Key: 'Key2', VersionId: 'VersionId2' }, ], }); - const s3Mock = newS3ClientMock(listObjectVersionsMock); const event: Partial = { RequestType: 'Delete', @@ -199,24 +209,42 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; - await invokeHandler(event, s3Mock); + await invokeHandler(event); - expect(s3Mock.listObjectVersions).toHaveBeenCalled(); - expect(s3Mock.deleteObjects).toHaveBeenCalled(); + expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(2); + expect(mockS3Client.deleteObjects.mock.calls.length).toEqual(1); }); -}); -async function invokeHandler(event: Partial, s3client: any) { - return handler(event as any, s3client); -} + test('delete event where bucket has many objects does recurse appropriately', async () => { + mockS3Client.listObjectVersions().promise.mockResolvedValueOnce({ // this counts as one call + Versions: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + IsTruncated: 'true', + }).mockResolvedValueOnce({ + Versions: [ + { Key: 'Key3', VersionId: 'VersionId3' }, + { Key: 'Key4', VersionId: 'VersionId4' }, + ], + }); -function newS3ClientMock(listObjectVersionsMock?: any, deleteObjectsMock?: any) { - return { - listObjectVersions: jest.fn().mockReturnValue({ - promise: listObjectVersionsMock ?? jest.fn().mockReturnValue({}), - }), - deleteObjects: jest.fn().mockReturnValue({ - promise: deleteObjectsMock ?? jest.fn().mockReturnValue({}), - }), - }; + const event: Partial = { + RequestType: 'Delete', + ResourceProperties: { + ServiceToken: 'Foo', + BucketName: 'MyBucket', + }, + }; + await invokeHandler(event); + + expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(3); + expect(mockS3Client.deleteObjects.mock.calls.length).toEqual(2); + }); +}); + +// helper function to get around TypeScript expecting a complete event object, +// even though our tests require some of the fields +async function invokeHandler(event: Partial) { + return handler(event as AWSLambda.CloudFormationCustomResourceEvent); } From 99ab9c1871dad6639c275f4ea07f5ad723479388 Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 8 Nov 2020 00:55:15 -0500 Subject: [PATCH 13/15] fixup tests and other nitpicks --- .../lib/auto-delete-objects-handler/index.ts | 25 ++-- .../aws-s3/test/auto-delete-objects.test.ts | 126 ++++++++++++++---- 2 files changed, 108 insertions(+), 43 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts index 0cb97126040a1..b95231417d4f1 100644 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -1,10 +1,13 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies const s3 = new (require('aws-sdk').S3)(); - if (event.RequestType === 'Create') { return onCreate(s3, event); } - if (event.RequestType === 'Update') { return onUpdate(s3, event); } - if (event.RequestType === 'Delete') { return onDelete(s3, event); } - throw new Error('Invalid request type.'); + switch (event.RequestType) { + case 'Create': + case 'Update': + return; + case 'Delete': + return onDelete(s3, event); + } } /** @@ -20,18 +23,12 @@ async function emptyBucket(s3: any, bucketName: string) { return; }; - let records = contents.map((record: any) => ({ Key: record.Key, VersionId: record.VersionId })); + const records = contents.map((record: any) => ({ Key: record.Key, VersionId: record.VersionId })); await s3.deleteObjects({ Bucket: bucketName, Delete: { Objects: records } }).promise(); - if (listedObjects?.IsTruncated === 'true' ) await emptyBucket(s3, bucketName); -} - -async function onCreate(_s3: any, _event: AWSLambda.CloudFormationCustomResourceCreateEvent) { - return; -} - -async function onUpdate(_s3: any, _event: AWSLambda.CloudFormationCustomResourceUpdateEvent) { - return; + if (listedObjects?.IsTruncated) { + await emptyBucket(s3, bucketName); + } } async function onDelete(s3: any, deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index 120c893a9676d..e279500786fc8 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -1,16 +1,19 @@ import '@aws-cdk/assert/jest'; import * as cdk from '@aws-cdk/core'; import * as s3 from '../lib'; -import { handler } from '../lib/auto-delete-objects-handler/index'; +import { handler } from '../lib/auto-delete-objects-handler'; test('when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it', () => { + // GIVEN const stack = new cdk.Stack(); + // WHEN new s3.Bucket(stack, 'MyBucket', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, }); + // THEN expect(stack).toHaveResource('AWS::S3::Bucket'); expect(stack).toHaveResource('AWS::Lambda::Function'); expect(stack).toHaveResource('Custom::AutoDeleteObjects', @@ -26,9 +29,11 @@ test('when autoDeleteObjects is enabled, a custom resource is provisioned + a la }); test('two buckets with autoDeleteObjects enabled share the same cr provider', () => { + // GIVEN const app = new cdk.App(); const stack = new cdk.Stack(app); + // WHEN new s3.Bucket(stack, 'MyBucketOne', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, @@ -38,6 +43,7 @@ test('two buckets with autoDeleteObjects enabled share the same cr provider', () autoDeleteObjects: true, }); + // THEN const template = app.synth().getStackArtifact(stack.artifactId).template; const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); @@ -57,9 +63,11 @@ test('two buckets with autoDeleteObjects enabled share the same cr provider', () }); test('when only one bucket has autoDeleteObjects enabled, only that bucket gets assigned a custom resource', () => { + // GIVEN const app = new cdk.App(); const stack = new cdk.Stack(app); + // WHEN new s3.Bucket(stack, 'MyBucketOne', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, @@ -69,6 +77,7 @@ test('when only one bucket has autoDeleteObjects enabled, only that bucket gets autoDeleteObjects: false, }); + // THEN const template = app.synth().getStackArtifact(stack.artifactId).template; const resourceTypes = Object.values(template.Resources).map((r: any) => r.Type).sort(); @@ -95,13 +104,16 @@ test('when only one bucket has autoDeleteObjects enabled, only that bucket gets }); test('iam policy is created', () => { + // GIVEN const stack = new cdk.Stack(); + // WHEN new s3.Bucket(stack, 'MyBucket', { removalPolicy: cdk.RemovalPolicy.DESTROY, autoDeleteObjects: true, }); + // THEN expect(stack).toHaveResource('AWS::IAM::Role', { Policies: [ { @@ -127,8 +139,11 @@ test('iam policy is created', () => { }); test('throws if autoDeleteObjects is enabled but if removalPolicy is not specified', () => { + // GIVEN const stack = new cdk.Stack(); + // WHEN + // THEN expect(() => new s3.Bucket(stack, 'MyBucket', { autoDeleteObjects: true })).toThrowError(/removal policy/); }); @@ -145,12 +160,19 @@ describe('custom resource handler', () => { }); beforeEach(() => { - mockS3Client.deleteObjects.mockClear(); - mockS3Client.listObjectVersions.mockClear(); - mockS3Client.promise.mockClear(); + // must reset implementations otherwise it's possible for + // mockResolvedValue values to leak between tests + mockS3Client.promise.mockReset(); + mockS3Client.listObjectVersions.mockReset(); + mockS3Client.deleteObjects.mockReset(); + + mockS3Client.listObjectVersions.mockReturnThis(); + mockS3Client.deleteObjects.mockReturnThis(); }); test('does nothing on create event', async () => { + // GIVEN + // WHEN const event: Partial = { RequestType: 'Create', ResourceProperties: { @@ -160,11 +182,14 @@ describe('custom resource handler', () => { }; await invokeHandler(event); - expect(mockS3Client.listObjectVersions).not.toHaveBeenCalled(); - expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); + // THEN + expect(mockS3Client.listObjectVersions).toHaveBeenCalledTimes(0); + expect(mockS3Client.deleteObjects).toHaveBeenCalledTimes(0); }); test('does nothing on update event', async () => { + // GIVEN + // WHEN const event: Partial = { RequestType: 'Update', ResourceProperties: { @@ -174,13 +199,16 @@ describe('custom resource handler', () => { }; await invokeHandler(event); - expect(mockS3Client.listObjectVersions).not.toHaveBeenCalled(); - expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); + // THEN + expect(mockS3Client.listObjectVersions).toHaveBeenCalledTimes(0); + expect(mockS3Client.deleteObjects).toHaveBeenCalledTimes(0); }); test('deletes no objects on delete event when bucket has no objects', async () => { - mockS3Client.listObjectVersions().promise.mockResolvedValue({ Versions: [] }); // this counts as one call + // GIVEN + mockS3Client.promise.mockResolvedValue({ Versions: [] }); // listObjectVersions() call + // WHEN const event: Partial = { RequestType: 'Delete', ResourceProperties: { @@ -190,18 +218,22 @@ describe('custom resource handler', () => { }; await invokeHandler(event); - expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(2); - expect(mockS3Client.deleteObjects).not.toHaveBeenCalled(); + // THEN + expect(mockS3Client.listObjectVersions).toHaveBeenCalledTimes(1); + expect(mockS3Client.listObjectVersions).toHaveBeenCalledWith({ Bucket: 'MyBucket' }); + expect(mockS3Client.deleteObjects).toHaveBeenCalledTimes(0); }); test('deletes all objects on delete event', async () => { - mockS3Client.listObjectVersions().promise.mockResolvedValue({ // this counts as one call + // GIVEN + mockS3Client.promise.mockResolvedValue({ // listObjectVersions() call Versions: [ { Key: 'Key1', VersionId: 'VersionId1' }, { Key: 'Key2', VersionId: 'VersionId2' }, ], }); + // WHEN const event: Partial = { RequestType: 'Delete', ResourceProperties: { @@ -211,24 +243,40 @@ describe('custom resource handler', () => { }; await invokeHandler(event); - expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(2); - expect(mockS3Client.deleteObjects.mock.calls.length).toEqual(1); + // THEN + expect(mockS3Client.listObjectVersions).toHaveBeenCalledTimes(1); + expect(mockS3Client.listObjectVersions).toHaveBeenCalledWith({ Bucket: 'MyBucket' }); + expect(mockS3Client.deleteObjects).toHaveBeenCalledTimes(1); + expect(mockS3Client.deleteObjects).toHaveBeenCalledWith({ + Bucket: 'MyBucket', + Delete: { + Objects: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + }, + }); }); test('delete event where bucket has many objects does recurse appropriately', async () => { - mockS3Client.listObjectVersions().promise.mockResolvedValueOnce({ // this counts as one call - Versions: [ - { Key: 'Key1', VersionId: 'VersionId1' }, - { Key: 'Key2', VersionId: 'VersionId2' }, - ], - IsTruncated: 'true', - }).mockResolvedValueOnce({ - Versions: [ - { Key: 'Key3', VersionId: 'VersionId3' }, - { Key: 'Key4', VersionId: 'VersionId4' }, - ], - }); + // GIVEN + mockS3Client.promise // listObjectVersions() call + .mockResolvedValueOnce({ + Versions: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + IsTruncated: true, + }) + .mockResolvedValueOnce(undefined) // deleteObjects() call + .mockResolvedValueOnce({ // listObjectVersions() call + Versions: [ + { Key: 'Key3', VersionId: 'VersionId3' }, + { Key: 'Key4', VersionId: 'VersionId4' }, + ], + }); + // WHEN const event: Partial = { RequestType: 'Delete', ResourceProperties: { @@ -238,13 +286,33 @@ describe('custom resource handler', () => { }; await invokeHandler(event); - expect(mockS3Client.listObjectVersions.mock.calls.length).toEqual(3); - expect(mockS3Client.deleteObjects.mock.calls.length).toEqual(2); + // THEN + expect(mockS3Client.listObjectVersions).toHaveBeenCalledTimes(2); + expect(mockS3Client.listObjectVersions).toHaveBeenCalledWith({ Bucket: 'MyBucket' }); + expect(mockS3Client.deleteObjects).toHaveBeenCalledTimes(2); + expect(mockS3Client.deleteObjects).toHaveBeenNthCalledWith(1, { + Bucket: 'MyBucket', + Delete: { + Objects: [ + { Key: 'Key1', VersionId: 'VersionId1' }, + { Key: 'Key2', VersionId: 'VersionId2' }, + ], + }, + }); + expect(mockS3Client.deleteObjects).toHaveBeenNthCalledWith(2, { + Bucket: 'MyBucket', + Delete: { + Objects: [ + { Key: 'Key3', VersionId: 'VersionId3' }, + { Key: 'Key4', VersionId: 'VersionId4' }, + ], + }, + }); }); }); // helper function to get around TypeScript expecting a complete event object, -// even though our tests require some of the fields +// even though our tests only need some of the fields async function invokeHandler(event: Partial) { return handler(event as AWSLambda.CloudFormationCustomResourceEvent); } From d35592add968cd9fbe9e2ff507aa65f40a718d3c Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 8 Nov 2020 01:41:34 -0500 Subject: [PATCH 14/15] hoist aws-sdk import to global scope --- .../lib/auto-delete-objects-handler/index.ts | 17 +++++---- .../aws-s3/test/auto-delete-objects.test.ts | 37 ++++++++++--------- 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts index b95231417d4f1..c27e346615775 100644 --- a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts +++ b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts @@ -1,12 +1,15 @@ +// eslint-disable-next-line import/no-extraneous-dependencies +import * as AWS from 'aws-sdk'; + +const s3 = new AWS.S3(); + export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) { - // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies - const s3 = new (require('aws-sdk').S3)(); switch (event.RequestType) { case 'Create': case 'Update': return; case 'Delete': - return onDelete(s3, event); + return onDelete(event); } } @@ -16,7 +19,7 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent * @param {AWS.S3} s3 the S3 client * @param {*} bucketName the bucket name */ -async function emptyBucket(s3: any, bucketName: string) { +async function emptyBucket(bucketName: string) { const listedObjects = await s3.listObjectVersions({ Bucket: bucketName }).promise(); const contents = (listedObjects.Versions || []).concat(listedObjects.DeleteMarkers || []); if (contents.length === 0) { @@ -27,14 +30,14 @@ async function emptyBucket(s3: any, bucketName: string) { await s3.deleteObjects({ Bucket: bucketName, Delete: { Objects: records } }).promise(); if (listedObjects?.IsTruncated) { - await emptyBucket(s3, bucketName); + await emptyBucket(bucketName); } } -async function onDelete(s3: any, deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { +async function onDelete(deleteEvent: AWSLambda.CloudFormationCustomResourceDeleteEvent) { const bucketName = deleteEvent.ResourceProperties?.BucketName; if (!bucketName) { throw new Error('No BucketName was provided.'); } - await emptyBucket(s3, bucketName); + await emptyBucket(bucketName); } diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index e279500786fc8..f9a1cd9e2adab 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -1,8 +1,19 @@ +// must be at the top of the file for successful jest mocking +const mockS3Client = { + listObjectVersions: jest.fn().mockReturnThis(), + deleteObjects: jest.fn().mockReturnThis(), + promise: jest.fn(), +}; + import '@aws-cdk/assert/jest'; import * as cdk from '@aws-cdk/core'; import * as s3 from '../lib'; import { handler } from '../lib/auto-delete-objects-handler'; +jest.mock('aws-sdk', () => { + return { S3: jest.fn(() => mockS3Client) }; +}); + test('when autoDeleteObjects is enabled, a custom resource is provisioned + a lambda handler for it', () => { // GIVEN const stack = new cdk.Stack(); @@ -149,30 +160,17 @@ test('throws if autoDeleteObjects is enabled but if removalPolicy is not specifi describe('custom resource handler', () => { - const mockS3Client = { - listObjectVersions: jest.fn().mockReturnThis(), - deleteObjects: jest.fn().mockReturnThis(), - promise: jest.fn(), - }; - - jest.mock('aws-sdk', () => { - return { S3: jest.fn(() => mockS3Client) }; - }); - beforeEach(() => { - // must reset implementations otherwise it's possible for - // mockResolvedValue values to leak between tests - mockS3Client.promise.mockReset(); - mockS3Client.listObjectVersions.mockReset(); - mockS3Client.deleteObjects.mockReset(); - mockS3Client.listObjectVersions.mockReturnThis(); mockS3Client.deleteObjects.mockReturnThis(); }); + afterEach(() => { + jest.resetAllMocks(); + }); + test('does nothing on create event', async () => { // GIVEN - // WHEN const event: Partial = { RequestType: 'Create', ResourceProperties: { @@ -180,6 +178,8 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; + + // WHEN await invokeHandler(event); // THEN @@ -189,7 +189,6 @@ describe('custom resource handler', () => { test('does nothing on update event', async () => { // GIVEN - // WHEN const event: Partial = { RequestType: 'Update', ResourceProperties: { @@ -197,6 +196,8 @@ describe('custom resource handler', () => { BucketName: 'MyBucket', }, }; + + // WHEN await invokeHandler(event); // THEN From 21f5a31429f3b6dd8566b0b991f05aad21f72b7f Mon Sep 17 00:00:00 2001 From: Chriscbr Date: Sun, 8 Nov 2020 01:45:07 -0500 Subject: [PATCH 15/15] reorder imports --- packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts index f9a1cd9e2adab..3c705a8de62dd 100644 --- a/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts +++ b/packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts @@ -1,13 +1,13 @@ -// must be at the top of the file for successful jest mocking +import '@aws-cdk/assert/jest'; +import * as cdk from '@aws-cdk/core'; +import * as s3 from '../lib'; + const mockS3Client = { listObjectVersions: jest.fn().mockReturnThis(), deleteObjects: jest.fn().mockReturnThis(), promise: jest.fn(), }; -import '@aws-cdk/assert/jest'; -import * as cdk from '@aws-cdk/core'; -import * as s3 from '../lib'; import { handler } from '../lib/auto-delete-objects-handler'; jest.mock('aws-sdk', () => {