feat(aws-logs-destinations): add Firehose Log Subscription Destination #14918

yerzhan7 · 2021-05-29T22:49:38Z

Add FIrehose CW Logs Subscription Destination for delivering logs from CW directly into Firehose (in batches, gziped).

Currently, this package contains only Kinesis Logs Subscription Destination, but it's possible to send directly to Firehose without Kinesis stream. See https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html#FirehoseExample

Implementation is based on these official instructions.

~~Also, Firehose does not have high level CDK constructs, so I used native Cfn level stuff.~~

Tested by deploying test stack and sending logs to the log stream. Validated that logs are delivered to S3 via Firehose.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-05-29T22:49:40Z

BenChaimberg · 2021-06-01T16:23:14Z

Thank you very much for your contribution (especially the documentation)! We are currently working on the design for the Firehose L2 (see #10810) which should be used here. To avoid rewriting the API in the future, I am going to mark this as blocked for now. Please reach out here if you have questions!

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

BenChaimberg · 2021-06-01T16:52:38Z

packages/@aws-cdk/aws-logs-destinations/README.md

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


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

Will wait for L2

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

BenChaimberg · 2021-06-01T16:56:31Z

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

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


Shouldn't be necessary when the L2 is created, but I'd prefer

role.addToPrincipalPolicy({ ... })

Will wait for L2

packages/@aws-cdk/aws-logs-destinations/test/integ.kinesisfirehose-to-s3.ts

BenChaimberg · 2021-06-01T17:04:45Z

packages/@aws-cdk/aws-logs-destinations/test/kinesisfirehose.test.ts

+  });
+
+  // THEN: we have a single role to write to the Firehose
+  expect(stack).toCountResources('AWS::IAM::Role', 2); // TODO: Fix to 1 - somehow it still creates 2 roles...


Please change this to 1 so the build will fail until this has been fixed.

Any tips on what might be the issue here? I have no idea...

Oh this is actually correct, since you create two separate log groups and create the role under the log group scope. You should create a separate test that attaches two destinations to the same log group and asserts that only one role is created.

But in my first revision it was not working when I used scope of Stack.

My initial intention was to have a single role for each Firehose destination. (i.e. 2 log groups will re-use the same role). What scope is the correct one to use for this role?

Pull request has been modified.

BenChaimberg · 2021-06-01T22:22:36Z

In the future, please add more commits onto your working branch instead of amending the old commit and force-pushing. This makes it easier to see what you've changed :)

yerzhan7 · 2021-06-02T11:55:57Z

In the future, please add more commits onto your working branch instead of amending the old commit and force-pushing. This makes it easier to see what you've changed :)

Will do, thanks. I presumed github can do diffs between revisions.

So, do I still need to squash commits after approval?

BenChaimberg · 2021-06-02T15:10:26Z

One would hope but doesn't seem to be the case...or I just haven't found the right buttons to click. No need to squash or manually merge – our workflows will take care of that.

yerzhan7 · 2021-06-15T22:46:13Z

@BenChaimberg any ETA for Firehose L2? I saw #14860 got closed.

Is it possible to merge this PR without Firehose L2?

Thanks

BenChaimberg · 2021-06-15T22:51:26Z

The PR in question was closed in favour of a different branch, but the work is still actively ongoing! We are hoping to open a first PR for review by 6/17. Because the changes in this PR would be immediately overwritten and aws-logs-destinations is a stable module, we will not be merging this in its current state. Thanks for your patience!

BenChaimberg

we're almost there! #15544 is nearing completion and we can restart this work when it's complete

requesting changes just to get this off my queue

BenChaimberg · 2021-07-23T02:00:23Z

Hello! #15544 has been merged and the DeliveryStream L2 is ready and raring to go! Please go take a look – it should have all we need to complete this PR.

yerzhan7 · 2021-07-23T14:09:32Z

Awesome! Thanks, will post an update in a few days

Pull request has been modified.

BenChaimberg · 2021-07-26T17:37:50Z

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

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


Suggested change

constructor(private readonly deliveryStream: firehose.DeliveryStream) {

constructor(private readonly deliveryStream: firehose.IDeliveryStream) {

BenChaimberg · 2021-07-26T17:38:20Z

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

+  }
+
+  public bind(_scope: Construct, _sourceLogGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {
+    const { region } = Stack.of(this.deliveryStream);


Suggested change

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

const region = this.deliveryStream.env.region;

BenChaimberg · 2021-07-26T17:38:49Z

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

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


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, {

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?

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

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.

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

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

BenChaimberg · 2021-07-26T17:38:54Z

packages/@aws-cdk/aws-logs-destinations/lib/kinesisfirehose.ts

+  constructor(private readonly deliveryStream: firehose.DeliveryStream) {
+  }
+
+  public bind(_scope: Construct, _sourceLogGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {


Suggested change

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

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

BenChaimberg · 2021-07-26T18:54:56Z

packages/@aws-cdk/aws-logs-destinations/test/integ.kinesisfirehose-to-s3.ts

+      removalPolicy: cdk.RemovalPolicy.DESTROY,
+    });
+
+    // TODO: For some reason creation of this resource fails: `Subscription (Subscription391C9821) destinationArn for vendor firehose cannot be used with roleArn (Service: AWSLogs; Status Code: 400; Error Code: InvalidParameterException; Request ID: 0e598426-5fcb-4fde-b9d3-11b14c129eb6; Proxy: null)`


CloudWatch Logs currently has a bug where destination ARNs that contain the string "destination" are rejected from creating a subscription. Workaround is to change the stack name below to remove the "destination" – maybe replace with "dests"

Internal reference: P48761216

What a weird bug! Thanks for digging into this.

Pull request has been modified.

yerzhan7 · 2021-07-26T21:50:14Z

@BenChaimberg , could you please help me with CodeBuild failure above? Thanks

aws-cdk-lib: �[96mlib/aws-logs-destinations/lib/kinesisfirehose.ts�[0m:�[93m11�[0m:�[93m59�[0m - �[91merror�[0m�[90m TS2694: �[0mNamespace '"/codebuild/output/src850447146/src/github.com/aws/aws-cdk/packages/aws-cdk-lib/lib/aws-kinesisfirehose/index"' has no exported member 'IDeliveryStream'.
aws-cdk-lib: �[7m11�[0m     constructor(private readonly deliveryStream: firehose.IDeliveryStream) {

BenChaimberg · 2021-07-27T20:14:59Z

aws-cdk-lib is the v2 library which removes experimental modules (except for the L1s), meaning stable modules that are included cannot currently depend on experimental modules. The team is currently working on this issue so we may have a fix in the pipeline, but this PR will be blocked until then.

nikovirtala · 2021-11-19T08:33:55Z

@BenChaimberg, is there anything that one could do to get this merged? – The issue with experimental modules with v2 is, in my understanding, solved.

aws-cdk-automation · 2021-11-19T19:49:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: d706e74
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

yerzhan7 · 2021-11-19T19:53:57Z

The issue with experimental modules with v2 is, in my understanding, solved.

@nikovirtala I've just merged latest stuff from master and, as you see above, the build still fails with the same error message.

How do you know that the issue is solved? Maybe I am missing some config

BenChaimberg · 2021-11-22T17:32:46Z

@kaizen3031593 can you please respond to the query above -- I have not been following the developments on experimental modules.

kaizencc · 2021-11-22T21:57:21Z

@nikovirtala my quick understanding of the issue is that stable modules cannot depend on experimental modules; this is by design. In that sense, there is nothing to resolve. Instead, we are waiting for kinesis-firehose to be stabilized, in which case we can take another look at this PR. I will double-check for you, but if you don't hear otherwise that is the signal we are looking for.

nikovirtala · 2021-11-24T12:41:48Z

@nikovirtala my quick understanding of the issue is that stable modules cannot depend on experimental modules; this is by design. In that sense, there is nothing to resolve. Instead, we are waiting for kinesis-firehose to be stabilized, in which case we can take another look at this PR. I will double-check for you, but if you don't hear otherwise that is the signal we are looking for.

It is not an option to release these changes as experimental too? – If that (release an experimental version of a currently stable package) is not possible we are going to find ourselves in this situation over and over again.

kaizencc · 2021-12-07T14:16:26Z

@nikovirtala we can release changes as experimental in a stable module by using the beta prefix (i.e. myExperimentalConstruct-Beta). However what we are talking about is having a stable module depend on an experimental one, which is a different ballgame (and not possible). It is possible to release this as an entirely new experimental construct that can then depend on both experimental and stable modules, but that is overkill here. I think it makes more sense to wait for kinesis-firehose to stabilize.

I am going to remove my assignment from this for now but feel free to ping me if you want my attention. When kinesis-firehose is stable I will revisit this PR.

aws-cdk-automation · 2022-04-10T14:24:22Z

This PR has been in BUILD FAILING for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it.

github-actions · 2022-04-20T07:31:44Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

yerzhan7 marked this pull request as ready for review May 29, 2021 22:57

BenChaimberg self-assigned this Jun 1, 2021

BenChaimberg added @aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose @aws-cdk/aws-logs-destinations p2 labels Jun 1, 2021

BenChaimberg added the pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason. label Jun 1, 2021

BenChaimberg previously requested changes Jun 1, 2021

View reviewed changes

feat(aws-logs-destinations): add Firehose Log Subscription Destination

8a294c8

BenChaimberg added p1 and removed p2 labels Jun 22, 2021

BenChaimberg previously requested changes Jul 21, 2021

View reviewed changes

BenChaimberg removed pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason. @aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose labels Jul 23, 2021

yerzhan7 added 2 commits July 24, 2021 11:57

Merge remote-tracking branch 'upstream/master'

95e9ca7

Fix single role scope issue

cbd9a16

yerzhan7 added 2 commits July 24, 2021 13:34

Migrate to L2 DeliveryStream

f34f5bf

Revert whitespace changes in rules.ts

9b3d542

BenChaimberg previously requested changes Jul 26, 2021

View reviewed changes

Fix integration test and address some comments

6d52cb2

yerzhan7 added 3 commits July 26, 2021 21:20

Change role scope and remove unit test case

0f33993

Remove tryFindChild and always create role

e59b712

Merge remote-tracking branch 'upstream/master'

6baf06f

BenChaimberg added the pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason. label Jul 27, 2021

BenChaimberg mentioned this pull request Jul 29, 2021

Stabilize Kinesis Firehose module #7536

Open

10 tasks

Merge remote-tracking branch 'upstream/master'

7aa9ac4

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

Merge remote-tracking branch 'upstream/master'

d706e74

kaizencc assigned kaizencc and unassigned BenChaimberg Nov 22, 2021

kaizencc removed their assignment Dec 7, 2021

github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 20, 2022

github-actions bot closed this Apr 20, 2022

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

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

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

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

feat(aws-logs-destinations): add Firehose Log Subscription Destination #14918

feat(aws-logs-destinations): add Firehose Log Subscription Destination #14918

Conversation

yerzhan7 commented May 29, 2021 • edited Loading

gitpod-io bot commented May 29, 2021

BenChaimberg commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenChaimberg commented Jun 1, 2021

yerzhan7 commented Jun 2, 2021

BenChaimberg commented Jun 2, 2021

yerzhan7 commented Jun 15, 2021

BenChaimberg commented Jun 15, 2021 • edited Loading

BenChaimberg left a comment

Choose a reason for hiding this comment

BenChaimberg commented Jul 23, 2021

yerzhan7 commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yerzhan7 commented Jul 26, 2021

BenChaimberg commented Jul 27, 2021

nikovirtala commented Nov 19, 2021

aws-cdk-automation commented Nov 19, 2021

AWS CodeBuild CI Report

yerzhan7 commented Nov 19, 2021 • edited Loading

BenChaimberg commented Nov 22, 2021

kaizencc commented Nov 22, 2021

nikovirtala commented Nov 24, 2021 • edited Loading

kaizencc commented Dec 7, 2021

aws-cdk-automation commented Apr 10, 2022

github-actions bot commented Apr 20, 2022

yerzhan7 commented May 29, 2021 •

edited

Loading

BenChaimberg commented Jun 15, 2021 •

edited

Loading

yerzhan7 commented Nov 19, 2021 •

edited

Loading

nikovirtala commented Nov 24, 2021 •

edited

Loading