feat(events-targets): add CloudWatch LogGroup Target #10598

DaWyz · 2020-09-30T07:36:17Z

Implementation

Update package @aws-cdk/aws-events-targets to include support for CloudWatch LogGroups.
The CloudWatchLogGroup event target must add a resource policy to CloudWatch LogGroups to allow events to be posted to the LogGroup. It requires a CustomResource to do so as it's not supported by CloudFormation.

The log-group-resource-policy.ts file should be moved to another module related to LogGroups so it can be easily shared. At the time of this pull request, it is not possible to add it into the aws-logs module because of a circular dependency issue.

Closes #9953

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

DaWyz · 2020-10-01T00:25:46Z

Still working on it. Just realized we need a CloudWatch log group with a name that starts with /aws/events/. Probably worth adding some validation for that.

…name required prefix

DaWyz · 2020-10-01T02:07:54Z

The LogGroup name is a token so the validation doesn't work. Adding comments in the docstring and in the README.md. Please, let me know if there is a way to validate it!

I also have a question about the InputTransformer. It seems It's getting configured correctly but It doesn't log any events in the LogGroup. I can't seem to make it work when setting it up manually using the AWS Console as well.

shivlaks · 2020-10-07T23:12:00Z

The LogGroup name is a token so the validation doesn't work. Adding comments in the docstring and in the README.md. Please, let me know if there is a way to validate it!

I also have a question about the InputTransformer. It seems It's getting configured correctly but It doesn't log any events in the LogGroup. I can't seem to make it work when setting it up manually using the AWS Console as well.

@DaWyz for tokens, we skip validation since they're late bound values. if you do a quick search for Token.isUnresolved you'll see a few examples where we only proceed with validation if the value to be examined/evaluated is not a token. Let me know if that helps!

re: inputTransformer, I also admittedly don't know this domain very well yet. Perhaps you could share a gist of a minimal repro and I can help dig through it.

DaWyz · 2020-10-09T18:29:57Z

@shivlaks , I updated the code and added the check on the LogGroup name.

About the inputTransformer, here is the code example:

import * as events from '@aws-cdk/aws-events';
import * as logs from '@aws-cdk/aws-logs';
import * as cdk from '@aws-cdk/core';
import { RemovalPolicy } from '@aws-cdk/core';


// I copied the logGroup Target file locally. You will need to change this.
import { LogGroup } from '../lib/loggroup';

const app = new cdk.App();

const stack = new cdk.Stack(app, 'log-group-events');

const rule = new events.Rule(stack, 'rule', {
  eventPattern: {
    source: [stack.account],
  },
});

const logGroup = new logs.LogGroup(stack, 'MyLogGroup', {
  logGroupName: '/aws/events/MyLogGroup',
  removalPolicy: RemovalPolicy.DESTROY,
});

rule.addTarget(
  new LogGroup(logGroup, {
    event: events.RuleTargetInput.fromObject({
      status: events.EventField.fromPath('$.detail.status'),
      instanceId: events.EventField.fromPath('$.detail.instance-id'),
    }),
  }),
);

It is generating the following template:

Resources:
  ruleF2C1DCDC:
    Type: AWS::Events::Rule
    Properties:
      EventPattern:
        source:
          - Ref: AWS::AccountId
      State: ENABLED
      Targets:
        - Arn:
            Fn::Join:
              - ""
              - - "arn:aws:logs:"
                - Ref: AWS::Region
                - ":"
                - Ref: AWS::AccountId
                - ":log-group:"
                - Ref: MyLogGroup5C0DAD85
          Id: Target0
          InputTransformer:
            InputPathsMap:
              detail-status: $.detail.status
              detail-instance-id: $.detail.instance-id
            InputTemplate: '{"status":<detail-status>,"instanceId":<detail-instance-id>}'
    Metadata:
      aws:cdk:path: log-group-events/rule/Resource
  MyLogGroup5C0DAD85:
    Type: AWS::Logs::LogGroup
    Properties:
      LogGroupName: /aws/events/MyLogGroup
      RetentionInDays: 731
    UpdateReplacePolicy: Delete
    DeletionPolicy: Delete
    Metadata:
      aws:cdk:path: log-group-events/MyLogGroup/Resource

The rule is triggered but It's reported as failed invocation. Creating everything from the AWS Console has the same result. I didn't find a way to investigate the failed invocation. Can't seem to find it in CloudTrail.

Let me know if you need anything else.

shivlaks · 2020-10-20T05:39:40Z

@DaWyz thanks for sharing the snippet - I'll prioritize taking a look at this error tomorrow. It will probably be a good exercise to get caught up on this PR and close the loop on that issue.

shivlaks

need to resolve the inputTransformer issue described in the comments

DaWyz · 2020-10-20T15:48:26Z

@shivlaks , just an FYI, I added a DLQ to my Event Rule to see if I would get more information but it doesn't help... See below.
I also think it is an issue with the Event Rule itself, not CDK. I have the same issue when only using the AWS Console.

I'm going to fix the conflicts later today or tomorrow.

gitpod-io · 2020-10-21T02:39:44Z

Pull request has been modified.

DaWyz · 2020-10-26T01:00:53Z

@shivlaks I updated the PR so we can target any log groups. Someone in the issue pointed out it's possible to make it work as long as we add a resource policy. Please, check this out.

Also, If you agree, I would like to remove the Input part for the rule as it doesn't seem to work anyway regardless of CDK. This way, we can make it available sooner. Let me know what you think.

shivlaks · 2020-10-27T16:56:40Z

@shivlaks I updated the PR so we can target any log groups. Someone in the issue pointed out it's possible to make it work as long as we add a resource policy. Please, check this out.

Also, If you agree, I would like to remove the Input part for the rule as it doesn't seem to work anyway regardless of CDK. This way, we can make it available sooner. Let me know what you think.

@DaWyz ack! taking a look at this today!!

shivlaks

hey @DaWyz - took another pass through the code, getting closer!

Do you mind also filling in the implementation details in the commit body? I think it's important to capture the context and the decisions we're making in the PR.

packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts

shivlaks · 2020-10-28T04:13:05Z

packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts

+  /**
+   * The log group resource policy name
+   */
+  readonly policyName: string;


does this need to be required? - I think there are a few places in the repo where we automatically generate if not provided (by using the logical ID of the policy resource)

I can't get the logical ID until the parent constructor is called (super()). And I have to pass the policyName as part of the parent constructor parameters. Tried scope.node.uniqueId() but it didn't work. I'm afraid it needs to be mandatory.

gotcha, it's worth a comment so this context isn't lost

So, I didn't know about cdk.Lazy.stringValue. Using it, I can generate a uniqueId as a default policyName.

let policyName = props.policyName || cdk.Lazy.stringValue({ produce: () => cdk.Names.uniqueId(this) });

I updated the code accordingly and removed the value I had initially created for the policyName!

packages/@aws-cdk/aws-events-targets/lib/log-group.ts

Pull request has been modified.

DaWyz · 2020-10-28T06:29:48Z

@shivlaks Thanks for the review.

Just a quick note on the CustomResource created (forgot to mentioned it in the previous changes). I copied the code from the aws-elasticsearch implementation. I'm not proud of this... I tried to extract it to the aws-logs module but it created a circular dependency (cloudformation -> aws-logs -> custom-resource -> cloudformation).

Let me know if you have a better idea about this.

DaWyz · 2020-11-10T17:13:34Z

@shivlaks any chance you would have time to look into the PR this week ?

shivlaks · 2020-11-12T07:44:12Z

@shivlaks any chance you would have time to look into the PR this week ?

@DaWyz yup, this is among the top of things on my list of things to check out tomorrow. stay tuned!!

shivlaks

@DaWyz - I think this is shaping up well and we're close. I had some minor suggestions, but looks great overall!

re: custom resource - since this is borrowed from the elasticsearch module which is experimental and this module is stable, we should ensure we address whatever sticks out as something we're not proud of. what stood out as potential problems / poor structure to you? perhaps we can improve on it without over-indexing on it to find a nice balance.

is there anything that you feel might result in us needing to make a breaking change? if we're good with the API itself, the implementation details can always be smoothed out in the future.

let me know what you think!!

packages/@aws-cdk/aws-events-targets/README.md

shivlaks · 2020-11-13T06:23:45Z

packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts

+  /**
+   * The log group resource policy name
+   */
+  readonly policyName: string;


gotcha, it's worth a comment so this context isn't lost

packages/@aws-cdk/aws-events-targets/lib/log-group.ts

shivlaks · 2020-11-13T06:30:44Z

packages/@aws-cdk/aws-events-targets/lib/log-group.ts

+/**
+ * Customize the CloudWatch LogGroup Event Target
+ */
+export interface LogGroupProps {


can a role also be supplied? - some of our other event targets accept a role / create a singleton role for the target.

It is not possible to pass a role when using a LogGroup as a target. I tried it initially and got an CloudFormation error back.

gotcha, thanks for clarifying

WojciechMatuszewski · 2020-11-14T15:37:36Z

packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts

+      policy: cr.AwsCustomResourcePolicy.fromSdkCalls({
+        // putResourcePolicy and deleteResourcePolicy don't support resource-level permissions. We must specify all resources ("*").
+        // https://docs.aws.amazon.com/IAM/latest/UserGuide/list_amazoncloudwatchlogs.html
+        resources: ['*'],


I think we could use AwsCustomResourcePolicy.ANY_RESOURCE here instead of *. This way, this piece of code would be aligned with AwsCustomResources readme.

https://docs.aws.amazon.com/cdk/api/latest/docs/custom-resources-readme.html#execution-policy-1

Pull request has been modified.

DaWyz · 2020-11-15T03:16:30Z

re: custom resource - since this is borrowed from the elasticsearch module which is experimental and this module is stable, we should ensure we address whatever sticks out as something we're not proud of. what stood out as potential problems / poor structure to you? perhaps we can improve on it without over-indexing on it to find a nice balance.

is there anything that you feel might result in us needing to make a breaking change? if we're good with the API itself, the implementation details can always be smoothed out in the future.

let me know what you think!!

@shivlaks I think it's pretty stable. The CustomResource module is stable so we should be fine. I was just concerned about duplicating the code.

So I'm guessing we should highlight the fact that log-group-resource-policy should probably be moved somewhere else so it can be re-used (in aws-events-targets and elasticsearch modules).

I'm happy to add a comment to this Pull Request and/or in the log-group-resource-policy.ts file.

I don't think anything would result in a breaking change here. And we could definitely move log-group-resource-policy.ts in another module in the future if needed without it being a breaking change.

Let me know if it make sense and I will highlight this in the PR/code.

shivlaks · 2020-11-18T02:54:25Z

So I'm guessing we should highlight the fact that log-group-resource-policy should probably be moved somewhere else so it can be re-used (in aws-events-targets and elasticsearch modules).

I'm happy to add a comment to this Pull Request and/or in the log-group-resource-policy.ts file.
Let me know if it make sense and I will highlight this in the PR/code.

@DaWyz go for it! let's ship this one out!!

DaWyz · 2020-11-18T05:53:50Z

So I'm guessing we should highlight the fact that log-group-resource-policy should probably be moved somewhere else so it can be re-used (in aws-events-targets and elasticsearch modules).
I'm happy to add a comment to this Pull Request and/or in the log-group-resource-policy.ts file.
Let me know if it make sense and I will highlight this in the PR/code.

@DaWyz go for it! let's ship this one out!!

@shivlaks I modified the PR text to highlight it. I didn't change the code since I'm not sure how/where to write this down (top of the file? top of the class? add a todo?). Please, feel free to add a comment in the file if you think it make sense.

Thanks again for your time reviewing this!

shivlaks

@DaWyz looks great!! thanks for working through this one with me. We really appreciate these contributions 😃

mergify · 2020-11-18T21:05:29Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-11-18T21:32:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 58c1cdb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-11-18T21:32:51Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

feat(aws-events-targets): add CloudWatch LogGroup Target

16820d7

SomayaB assigned shivlaks Sep 30, 2020

DaWyz marked this pull request as draft October 1, 2020 00:25

Rename handler prop to logGroup and add documentation about LogGroup …

1ebf625

…name required prefix

DaWyz force-pushed the DaWyz/events-target-loggroup branch from d8d2120 to 1ebf625 Compare October 1, 2020 02:02

DaWyz marked this pull request as ready for review October 1, 2020 02:03

Merge branch 'master' into DaWyz/events-target-loggroup

a837a96

DaWyz added 2 commits October 9, 2020 00:22

Add log group name validation

ba11ea0

Fixing documentation

30c045a

shivlaks previously requested changes Oct 20, 2020

View reviewed changes

Merge branch 'master' into DaWyz/events-target-loggroup

ca31660

DaWyz force-pushed the DaWyz/events-target-loggroup branch from 91cf3fb to ca31660 Compare October 21, 2020 02:40

DaWyz added 4 commits October 23, 2020 22:04

Merge branch 'master' into DaWyz/events-target-loggroup

d042c70

Merge branch 'master' into DaWyz/events-target-loggroup

36ef4d5

Add support for resource policy for LogGroup

b970107

fix integ json for loggroup target

4c71270

shivlaks changed the title ~~feat(aws-events-targets): add CloudWatch LogGroup Target~~ feat(events-targets): add CloudWatch LogGroup Target Oct 27, 2020

shivlaks previously requested changes Oct 28, 2020

View reviewed changes

Improve code and fix nits

3b59ec3

Remove like to issue as it's not related to events targets

9e9e425

DaWyz requested a review from shivlaks November 1, 2020 18:50

DaWyz added 2 commits November 5, 2020 18:27

Merge branch 'master' into DaWyz/events-target-loggroup

e41e241

Merge branch 'master' into DaWyz/events-target-loggroup

13c9ae6

shivlaks previously requested changes Nov 13, 2020

View reviewed changes

WojciechMatuszewski reviewed Nov 14, 2020

View reviewed changes

Fix nits and udpate aws-events-targets README.md

d1bf75b

fix readme

90c19bb

DaWyz force-pushed the DaWyz/events-target-loggroup branch from e58c248 to 90c19bb Compare November 15, 2020 03:17

DaWyz and others added 6 commits November 14, 2020 22:18

use node.addr for log-group-resource policyName

b334fda

default PolicyName to use cdk.Names.uniqueId() instead of node.addr()

9f0da1b

Removing policyName value in favor of the uniqueId

5cfa807

Merge branch 'master' into DaWyz/events-target-loggroup

4ec36bc

Merge branch 'master' into DaWyz/events-target-loggroup

fa8f30f

update integration test expectation

3251d58

DaWyz requested a review from shivlaks November 17, 2020 17:48

shivlaks approved these changes Nov 18, 2020

View reviewed changes

Merge branch 'master' into DaWyz/events-target-loggroup

58c1cdb

mergify bot merged commit 98e9b59 into aws:master Nov 18, 2020

seyeong mentioned this pull request Jun 24, 2022

events: log group target #20855

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events-targets): add CloudWatch LogGroup Target #10598

feat(events-targets): add CloudWatch LogGroup Target #10598

DaWyz commented Sep 30, 2020 •

edited

DaWyz commented Oct 1, 2020

DaWyz commented Oct 1, 2020

shivlaks commented Oct 7, 2020 •

edited

DaWyz commented Oct 9, 2020

shivlaks commented Oct 20, 2020

shivlaks left a comment

DaWyz commented Oct 20, 2020

gitpod-io bot commented Oct 21, 2020 •

edited

DaWyz commented Oct 26, 2020

shivlaks commented Oct 27, 2020

shivlaks left a comment

shivlaks Oct 28, 2020

DaWyz Oct 28, 2020

shivlaks Nov 13, 2020

DaWyz Nov 15, 2020

DaWyz commented Oct 28, 2020

DaWyz commented Nov 10, 2020

shivlaks commented Nov 12, 2020

shivlaks left a comment

shivlaks Nov 13, 2020

shivlaks Nov 13, 2020

DaWyz Nov 15, 2020

shivlaks Nov 18, 2020

WojciechMatuszewski Nov 14, 2020

DaWyz commented Nov 15, 2020

shivlaks commented Nov 18, 2020

DaWyz commented Nov 18, 2020

shivlaks left a comment

mergify bot commented Nov 18, 2020

aws-cdk-automation commented Nov 18, 2020

mergify bot commented Nov 18, 2020

feat(events-targets): add CloudWatch LogGroup Target #10598

feat(events-targets): add CloudWatch LogGroup Target #10598

Conversation

DaWyz commented Sep 30, 2020 • edited

DaWyz commented Oct 1, 2020

DaWyz commented Oct 1, 2020

shivlaks commented Oct 7, 2020 • edited

DaWyz commented Oct 9, 2020

shivlaks commented Oct 20, 2020

shivlaks left a comment

Choose a reason for hiding this comment

DaWyz commented Oct 20, 2020

gitpod-io bot commented Oct 21, 2020 • edited

DaWyz commented Oct 26, 2020

shivlaks commented Oct 27, 2020

shivlaks left a 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

DaWyz commented Oct 28, 2020

DaWyz commented Nov 10, 2020

shivlaks commented Nov 12, 2020

shivlaks left a 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

DaWyz commented Nov 15, 2020

shivlaks commented Nov 18, 2020

DaWyz commented Nov 18, 2020

shivlaks left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 18, 2020

aws-cdk-automation commented Nov 18, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 18, 2020

DaWyz commented Sep 30, 2020 •

edited

shivlaks commented Oct 7, 2020 •

edited

gitpod-io bot commented Oct 21, 2020 •

edited