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(aws-logs-destinations): add Firehose Log Subscription Destination #14918

Closed
wants to merge 11 commits into from
51 changes: 49 additions & 2 deletions packages/@aws-cdk/aws-logs-destinations/README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# CDK Construct Libray for AWS XXX
# CDK Construct Library for AWS CloudWatch Logs Subscription Destinations
<!--BEGIN STABILITY BANNER-->

---
Expand All @@ -9,4 +9,51 @@

<!--END STABILITY BANNER-->

A short description here.
This library supplies constructs for working with CloudWatch Logs Subscription Destinations.

## Logs Destinations
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're feeling up for it, it would be great to have these snippets compile in their first iteration. I'm thinking an initial snippet showing how to set up a log group, then each individual destination uses a fixture seeded with a log group to create the destination. Though ellipses are supported, giving a full specification of the destination resource would be ideal. See: CONTRIBUTING#documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will wait for L2


Log events matching a particular filter can be sent to Lambda, Kinesis or Kinesis Data Firehose.

Logs that are sent to a receiving service through a subscription filter are Base64 encoded and compressed with the gzip format. For further information, see the [Using CloudWatch Logs Subscription Filters](https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html).

Create a `SubscriptionFilter`, initialize it with an appropriate `Pattern` and supply the intended destination.

### Lambda Destination

```ts
const fn = new lambda.Function(this, 'Lambda', { ... });
const logGroup = new LogGroup(this, 'LogGroup', { ... });

new SubscriptionFilter(this, 'Subscription', {
logGroup,
destination: new LogsDestinations.LambdaDestination(fn),
filterPattern: FilterPattern.allTerms("ERROR", "MainThread")
});
```

### Kinesis Destination

```ts
const stream = new kinesis.Stream(stack, 'KinesisStream', { ... });
const logGroup = new LogGroup(this, 'LogGroup', { ... });

new SubscriptionFilter(this, 'Subscription', {
logGroup,
destination: new LogsDestinations.KinesisDestination(stream),
filterPattern: FilterPattern.allTerms("ERROR", "MainThread")
});
```

### Kinesis Firehose Destination

```ts
const deliveryStream = new firehose.DeliveryStream(stack, 'Firehose', { ... });
const logGroup = new LogGroup(this, 'LogGroup', { ... });

new SubscriptionFilter(this, 'Subscription', {
logGroup,
destination: new LogsDestinations.KinesisFirehoseDestination(deliveryStream),
filterPattern: FilterPattern.allTerms("ERROR", "MainThread")
});
```
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-logs-destinations/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './lambda';
export * from './kinesis';
export * from './kinesisfirehose';
35 changes: 35 additions & 0 deletions packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import * as iam from '@aws-cdk/aws-iam';
import * as firehose from '@aws-cdk/aws-kinesisfirehose';
import * as logs from '@aws-cdk/aws-logs';
import { Stack } from '@aws-cdk/core';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from '@aws-cdk/core';

/**
* Use a Kinesis Firehose as the destination for a log subscription
*/
export class KinesisFirehoseDestination implements logs.ILogSubscriptionDestination {
constructor(private readonly deliveryStream: firehose.DeliveryStream) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constructor(private readonly deliveryStream: firehose.DeliveryStream) {
constructor(private readonly deliveryStream: firehose.IDeliveryStream) {

}

public bind(_scope: Construct, _sourceLogGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public bind(_scope: Construct, _sourceLogGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {
public bind(scope: Construct, _sourceLogGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {

const { region } = Stack.of(this.deliveryStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { region } = Stack.of(this.deliveryStream);
const region = this.deliveryStream.env.region;


// Following example from https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html#FirehoseExample
// Create a role to be assumed by CWL that can write to this Firehose.
const roleId = 'CloudWatchLogsCanPutRecordsIntoKinesisFirehose';
const role = this.deliveryStream.node.tryFindChild(roleId) as iam.IRole ??
new iam.Role(this.deliveryStream, roleId, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const role = this.deliveryStream.node.tryFindChild(roleId) as iam.IRole ??
new iam.Role(this.deliveryStream, roleId, {
const role = scope.node.tryFindChild(roleId) as iam.IRole ??
new iam.Role(scope, roleId, {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With your suggested change we would create 2 identical IAM roles that will fail my unit test in https://github.com/aws/aws-cdk/pull/14918/files#diff-4dab4ab34393d725389edf0c2c55b924e6e9c569d86f78b21ad5d36b8f29b076R104

Do we really want to create 2 identical roles in that unit test case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do. There should be a logical mapping between the actor and the role, which would be violated if we create one role per delivery stream (as that is the resource). The test case can be modified to demonstrate that we re-use the role if there are multiple Firehose subscriptions from the same log group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case can be modified to demonstrate that we re-use the role if there are multiple Firehose subscriptions from the same log group

This would still create 2 separate roles because scope would be different for 2 separate subscriptions even if they are applied to the same log group.

So, it looks like scope.node.tryFindChild(roleId) as iam.IRole ?? is a redundant check as that condition will never be true.

Anyway, I will just remove this unit test case for now as it's impossible to test that condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah because the destination is bound under SubscriptionFilter, not LogGroup. Okay, no problem with having multiple roles then

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, yes we should remove the tryFindChild and always create a new role

assumedBy: new iam.ServicePrincipal(`logs.${region}.amazonaws.com`),
});

this.deliveryStream.grantPutRecords(role);

return {
arn: this.deliveryStream.deliveryStreamArn,
role,
};
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-logs-destinations/lib/lambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Construct } from '@aws-cdk/core';
/**
* Options that may be provided to LambdaDestination
*/
export interface LambdaDestinationOptions{
export interface LambdaDestinationOptions {
/** Whether or not to add Lambda Permissions.
* @default true
*/
Expand Down
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-logs-destinations/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,14 @@
"cfn2ts": "0.0.0",
"jest": "^26.6.3",
"pkglint": "0.0.0",
"@aws-cdk/assert-internal": "0.0.0"
"@aws-cdk/assert-internal": "0.0.0",
"@aws-cdk/aws-kinesisfirehose-destinations": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0"
},
"dependencies": {
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kinesis": "0.0.0",
"@aws-cdk/aws-kinesisfirehose": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-logs": "0.0.0",
"@aws-cdk/core": "0.0.0",
Expand All @@ -84,6 +87,7 @@
"peerDependencies": {
"@aws-cdk/aws-iam": "0.0.0",
"@aws-cdk/aws-kinesis": "0.0.0",
"@aws-cdk/aws-kinesisfirehose": "0.0.0",
"@aws-cdk/aws-lambda": "0.0.0",
"@aws-cdk/aws-logs": "0.0.0",
"@aws-cdk/core": "0.0.0",
Expand Down
Loading