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.

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

T131 - Logging buckets #164

Merged
merged 5 commits into from
Apr 27, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export class CloudFrontToS3 extends Construct {
super(scope, id);
let bucket: s3.Bucket;

if (props.existingBucketObj && props.bucketProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

1 - Add a unit test case to assert this scenario
2 - Update the input props comments and README.md to reflect the new behavior, currently it implies that existingBucketObj takes precedence over bucketProps which no longer will be the case

Similar feedback for the rest of the updates in PR

throw new Error('Cannot specify both bucket properties and an existing bucket');
}

if (!props.existingBucketObj) {
[this.s3Bucket, this.s3LoggingBucket] = defaults.buildS3Bucket(this, {
bucketProps: props.bucketProps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,10 @@ const app = new App();
const stack = new Stack(app, 'test-cloudfront-s3-existing-bucket-stack');

let mybucket: s3.Bucket;
mybucket = defaults.CreateScrapBucket(stack, {});
mybucket = defaults.CreateScrapBucket(stack, { removalPolicy: RemovalPolicy.DESTROY });

const _construct = new CloudFrontToS3(stack, 'test-cloudfront-s3', {
existingBucketObj: mybucket,
bucketProps: {
removalPolicy: RemovalPolicy.DESTROY,
}
});

// Add Cache Policy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ export class EventsRuleToKinesisFirehoseToS3 extends Construct {
constructor(scope: Construct, id: string, props: EventsRuleToKinesisFirehoseToS3Props) {
super(scope, id);

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

// Set up the Kinesis Firehose using KinesisFirehoseToS3 construct
const firehoseToS3 = new KinesisFirehoseToS3(this, 'KinesisFirehoseToS3', {
kinesisFirehoseProps: props.kinesisFirehoseProps,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ export class IotToKinesisFirehoseToS3 extends Construct {
constructor(scope: Construct, id: string, props: IotToKinesisFirehoseToS3Props) {
super(scope, id);

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

const firehoseToS3 = new KinesisFirehoseToS3(this, 'KinesisFirehoseToS3', {
kinesisFirehoseProps: props.kinesisFirehoseProps,
existingBucketObj: props.existingBucketObj,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ export class KinesisFirehoseToAnalyticsAndS3 extends Construct {
constructor(scope: Construct, id: string, props: KinesisFirehoseToAnalyticsAndS3Props) {
super(scope, id);

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

// Setup the kinesisfirehose-s3 pattern
const kinesisFirehoseToS3Props: KinesisFirehoseToS3Props = {
kinesisFirehoseProps: props.kinesisFirehoseProps,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ export class KinesisFirehoseToS3 extends Construct {

let bucket: s3.IBucket;

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

// Setup S3 Bucket
if (!props.existingBucketObj) {
let { bucketProps } = props;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ export class KinesisStreamsToKinesisFirehoseToS3 extends Construct {
constructor(scope: Construct, id: string, props: KinesisStreamsToKinesisFirehoseToS3Props) {
super(scope, id);

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

// Setup the Kinesis Stream
this.kinesisStream = defaults.buildKinesisStream(this, {
existingStreamObj: props.existingStreamObj,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,10 @@ const app = new App();
const stack = new Stack(app, 'test-existing-bucket-firehose-s3-stack');
stack.templateOptions.description = 'Integration Test for aws-kinesisstreams-kinesisfirehose-s3';

const existingBucket = CreateScrapBucket(stack, {});
const existingBucket = CreateScrapBucket(stack, { removalPolicy: RemovalPolicy.DESTROY });
const mybucket: s3.IBucket = s3.Bucket.fromBucketName(stack, 'mybucket', existingBucket.bucketName);
new KinesisStreamsToKinesisFirehoseToS3(stack, 'test-existing-bucket-firehose-s3-stack', {
existingBucketObj: mybucket,
bucketProps: {
removalPolicy: RemovalPolicy.DESTROY,
}
});

// Synth
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ export class LambdaToS3 extends Construct {
super(scope, id);
let bucket: s3.IBucket;

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

if (props.deployVpc || props.existingVpc) {
if (props.deployVpc && props.existingVpc) {
throw new Error("More than 1 VPC specified in the properties");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { CreateScrapBucket } from '@aws-solutions-constructs/core';
const app = new App();
const stack = new Stack(app, 'test-lambda-s3-pre-existing-bucket');
stack.templateOptions.description = 'Integration Test for aws-lambda-s3';
const existingBucket = CreateScrapBucket(stack, {});
const existingBucket = CreateScrapBucket(stack, { removalPolicy: RemovalPolicy.DESTROY });

const mybucket: s3.IBucket = s3.Bucket.fromBucketName(stack, 'mybucket', existingBucket.bucketName);
// Definitions
Expand All @@ -33,9 +33,6 @@ const props: LambdaToS3Props = {
handler: 'index.handler',
code: lambda.Code.fromAsset(`${__dirname}/lambda`)
},
bucketProps: {
removalPolicy: RemovalPolicy.DESTROY,
}
};

new LambdaToS3(stack, 'test-lambda-s3-pre-existing-bucket', props);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ export class S3ToLambda extends Construct {
super(scope, id);
let bucket: s3.Bucket;

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

this.lambdaFunction = defaults.buildLambdaFunction(this, {
existingLambdaObj: props.existingLambdaObj,
lambdaFunctionProps: props.lambdaFunctionProps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ export class S3ToSqs extends Construct {
let bucket: s3.Bucket;
let enableEncryptionParam = props.enableEncryptionWithCustomerManagedKey;

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

if (props.enableEncryptionWithCustomerManagedKey === undefined ||
props.enableEncryptionWithCustomerManagedKey === true) {
enableEncryptionParam = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
"RestrictPublicBuckets": true
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete",
"Metadata": {
"cfn_nag": {
"rules_to_suppress": [
Expand Down Expand Up @@ -111,8 +111,8 @@
"Status": "Enabled"
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"S3BucketNotifications58B5AD06": {
"Type": "Custom::S3BucketNotifications",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,12 @@ const app = new App();

const stack = new Stack(app, 'test-s3-sqs-existing-bucket');

const [myBucket] = defaults.buildS3Bucket(stack, {});
const [myBucket] = defaults.buildS3Bucket(stack, { bucketProps: { removalPolicy: RemovalPolicy.DESTROY }, });

// Currently there is no way to customize the logging bucket, so this
// test will leave a bucket behind
const props: S3ToSqsProps = {
existingBucketObj: myBucket,
bucketProps: {
removalPolicy: RemovalPolicy.DESTROY,
}
};

new S3ToSqs(stack, 'test-s3-sqs', props);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ export class S3ToStepFunction extends Construct {
super(scope, id);
let bucket: s3.IBucket;

if (props.existingBucketObj && props.bucketProps) {
throw new Error('Cannot specify both bucket properties and an existing bucket');
}

if (!props.existingBucketObj) {
[this.s3Bucket, this.s3LoggingBucket] = defaults.buildS3Bucket(this, {
bucketProps: props.bucketProps
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ const props: S3ToStepFunctionProps = {
stateMachineProps: {
definition: startState
},
bucketProps: {
removalPolicy: RemovalPolicy.DESTROY,
},
logGroupProps: {
removalPolicy: RemovalPolicy.DESTROY
},
Expand Down