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

feat(cloudtrail): user specified log group #8079

Merged
merged 7 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 24 additions & 8 deletions packages/@aws-cdk/aws-cloudtrail/lib/cloudtrail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,19 @@ export interface TrailProps {
readonly sendToCloudWatchLogs?: boolean;

/**
* How long to retain logs in CloudWatchLogs. Ignored if sendToCloudWatchLogs is false
* How long to retain logs in CloudWatchLogs.
* Ignored if sendToCloudWatchLogs is false or if cloudWatchLogGroup is set.
*
* @default logs.RetentionDays.OneYear
nija-at marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly cloudWatchLogsRetention?: logs.RetentionDays;

/**
* Log Group to which CloudTrail to push logs to. Ignored if sendToCloudWatchLogs is set to false.
* @default - a new log group is created and used.
*/
readonly cloudWatchLogGroup?: logs.ILogGroup;

/** The AWS Key Management Service (AWS KMS) key ID that you want to use to encrypt CloudTrail logs.
*
* @default - No encryption.
Expand Down Expand Up @@ -154,6 +161,12 @@ export class Trail extends Resource {
*/
public readonly trailSnsTopicArn: string;

/**
* The CloudWatch log group to which CloudTrail events are sent.
* `undefined` if `sendToCloudWatchLogs` property is false.
*/
public readonly logGroup: logs.ILogGroup | undefined;
nija-at marked this conversation as resolved.
Show resolved Hide resolved

private s3bucket: s3.IBucket;
private eventSelectors: EventSelector[] = [];

Expand Down Expand Up @@ -183,19 +196,22 @@ export class Trail extends Resource {
},
}));

let logGroup: logs.CfnLogGroup | undefined;
let logsRole: iam.IRole | undefined;

if (props.sendToCloudWatchLogs) {
logGroup = new logs.CfnLogGroup(this, 'LogGroup', {
retentionInDays: props.cloudWatchLogsRetention || logs.RetentionDays.ONE_YEAR,
});
if (props.cloudWatchLogGroup) {
this.logGroup = props.cloudWatchLogGroup;
} else {
this.logGroup = new logs.LogGroup(this, 'LogGroup', {
retention: props.cloudWatchLogsRetention ?? logs.RetentionDays.ONE_YEAR,
});
}

logsRole = new iam.Role(this, 'LogsRole', { assumedBy: cloudTrailPrincipal });

logsRole.addToPolicy(new iam.PolicyStatement({
actions: ['logs:PutLogEvents', 'logs:CreateLogStream'],
resources: [logGroup.attrArn],
resources: [this.logGroup.logGroupArn],
}));
}

Expand All @@ -217,8 +233,8 @@ export class Trail extends Resource {
kmsKeyId: props.kmsKey && props.kmsKey.keyArn,
s3BucketName: this.s3bucket.bucketName,
s3KeyPrefix: props.s3KeyPrefix,
cloudWatchLogsLogGroupArn: logGroup && logGroup.attrArn,
cloudWatchLogsRoleArn: logsRole && logsRole.roleArn,
cloudWatchLogsLogGroupArn: this.logGroup?.logGroupArn,
cloudWatchLogsRoleArn: logsRole?.roleArn,
nija-at marked this conversation as resolved.
Show resolved Hide resolved
snsTopicName: props.snsTopic,
eventSelectors: this.eventSelectors,
});
Expand Down
51 changes: 47 additions & 4 deletions packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect, haveResource, not, SynthUtils } from '@aws-cdk/assert';
import { expect, haveResource, haveResourceLike, not, SynthUtils } from '@aws-cdk/assert';
import * as iam from '@aws-cdk/aws-iam';
import * as lambda from '@aws-cdk/aws-lambda';
import { RetentionDays } from '@aws-cdk/aws-logs';
import { LogGroup, RetentionDays } from '@aws-cdk/aws-logs';
import * as s3 from '@aws-cdk/aws-s3';
import { Stack } from '@aws-cdk/core';
import { Test } from 'nodeunit';
Expand Down Expand Up @@ -160,10 +160,12 @@ export = {
'with cloud watch logs': {
'enabled'(test: Test) {
const stack = getTestStack();
new Trail(stack, 'MyAmazingCloudTrail', {
const t = new Trail(stack, 'MyAmazingCloudTrail', {
sendToCloudWatchLogs: true,
});

test.ok(t.logGroup);
test.deepEqual(stack.resolve(t.logGroup!.logGroupArn), { 'Fn::GetAtt': ['MyAmazingCloudTrailLogGroup2BE67F87', 'Arn'] });
Copy link
Contributor

Choose a reason for hiding this comment

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

how come !. - @eladb had mentioned in another PR that we generally try to avoid it in our codebase. Although this is a test, I think we should try and be consistent if it's a style we generally don't prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test verifies that t.logGroup is not undefined and returns an ARN (vs. the other case where it returns undefined).

How should I update my assertions?

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately, something a little more naive - here's the similar example in the PR i mentioned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not following what the change you're expecting here is. The unit testing in the linked PR is testing something quite different from what I'm trying to here.

Can you tell me what's wrong with the current assertion, and how you would write it?
I can either assert for it being not undefined or for the value it returns. I've chosen the latter. Is there a 3rd option?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do the former, but I'm not picky on this. go for it!

expect(stack).to(haveResource('AWS::CloudTrail::Trail'));
expect(stack).to(haveResource('AWS::S3::Bucket'));
expect(stack).to(haveResource('AWS::S3::BucketPolicy', ExpectedBucketPolicyProperties));
Expand All @@ -177,7 +179,7 @@ export = {
Effect: 'Allow',
Action: ['logs:PutLogEvents', 'logs:CreateLogStream'],
Resource: {
'Fn::GetAtt': ['MyAmazingCloudTrailLogGroupAAD65144', 'Arn'],
'Fn::GetAtt': ['MyAmazingCloudTrailLogGroup2BE67F87', 'Arn'],
},
}],
},
Expand All @@ -188,6 +190,7 @@ export = {
test.deepEqual(trail.DependsOn, [logsRolePolicyName, logsRoleName, 'MyAmazingCloudTrailS3Policy39C120B0']);
test.done();
},

'enabled and custom retention'(test: Test) {
const stack = getTestStack();
new Trail(stack, 'MyAmazingCloudTrail', {
Expand All @@ -207,6 +210,46 @@ export = {
test.deepEqual(trail.DependsOn, [logsRolePolicyName, logsRoleName, 'MyAmazingCloudTrailS3Policy39C120B0']);
test.done();
},

'enabled and with custom log group'(test: Test) {
nija-at marked this conversation as resolved.
Show resolved Hide resolved
const stack = getTestStack();
const cloudWatchLogGroup = new LogGroup(stack, 'MyLogGroup', {
retention: RetentionDays.FIVE_DAYS,
});
new Trail(stack, 'MyAmazingCloudTrail', {
sendToCloudWatchLogs: true,
cloudWatchLogsRetention: RetentionDays.ONE_WEEK,
cloudWatchLogGroup,
});

expect(stack).to(haveResource('AWS::Logs::LogGroup', {
RetentionInDays: 5,
}));

expect(stack).to(haveResource('AWS::CloudTrail::Trail', {
CloudWatchLogsLogGroupArn: stack.resolve(cloudWatchLogGroup.logGroupArn),
}));

expect(stack).to(haveResourceLike('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [{
Resource: stack.resolve(cloudWatchLogGroup.logGroupArn),
}],
},
}));
test.done();
},

'disabled'(test: Test) {
const stack = getTestStack();
const t = new Trail(stack, 'MyAmazingCloudTrail', {
sendToCloudWatchLogs: false,
cloudWatchLogsRetention: RetentionDays.ONE_WEEK,
});
test.equals(t.logGroup, undefined);
expect(stack).notTo(haveResource('AWS::Logs::LogGroup'));
test.done();
},
},
'with event selectors': {
'with default props'(test: Test) {
Expand Down