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

[aws-events-targets] Support CloudWatch Logs as a target #9953

Closed
2 tasks
otterley opened this issue Aug 25, 2020 · 12 comments · Fixed by #10598
Closed
2 tasks

[aws-events-targets] Support CloudWatch Logs as a target #9953

otterley opened this issue Aug 25, 2020 · 12 comments · Fixed by #10598
Assignees
Labels
@aws-cdk/aws-events-targets effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@otterley
Copy link
Contributor

Although it's not well-documented yet, CloudWatch Logs is a supported target for event rules. The ARN is the log group ARN.

Use Case

This is useful for, among other things, recording ECS task state changes that are otherwise difficult to observe.

Proposed Solution

Here's an example. Note, the log group name must start with /aws/events/ because EventBridge relies on an unpublished CloudWatch Logs resource policy in order to work properly.

Caller:

    const logGroup = new logs.LogGroup(this, 'EcsEventLog', {
      // must start with /aws/events/
      logGroupName: `/aws/events/ecs-${cluster.clusterName}` 
    });

    new cdk.CfnOutput(this, 'LogGroupName', {
      value: logGroup.logGroupName
    });

    const clusterEvents = new events.Rule(this, 'EcsEvents', {
      eventPattern: {
        source: ['aws.ecs'],
        detail: {
          clusterArn: [cluster.clusterArn]
        }
      },
      targets: [new CloudWatchLogsTarget(this, 'LogsTarget', logGroup)]
    });

Implementation:

export class CloudWatchLogsTarget extends cdk.Construct {
    private logGroup: logs.ILogGroup;

    constructor(scope: cdk.Construct, id: string, logGroup: logs.ILogGroup) {
        super(scope, id);
        this.logGroup = logGroup;
    }

    public bind(rule: events.IRule, id?: string): events.RuleTargetConfig {
        return {
            id: '',
            // we can't use logGroup.logArn because it has `:*` on the end, and it's a token
            // so we can't just remove the suffix with string replacement operations
            arn: `arn:aws:logs:${this.logGroup.stack.region}:${this.logGroup.stack.account}:log-group:${this.logGroup.logGroupName}`,
        }
    }
}

Ideally, we'd go one step further and implement event handler declarations on ECS Cluster objects, but that's a story for another day.

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@otterley otterley added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 25, 2020
@demus
Copy link

demus commented Aug 25, 2020

Thanks for making this issue! I was looking to do this today as well.

Here's a slight modification of the code sample in this issue that I have working by implementing IRuleTarget:

// Source: https://github.com/aws/aws-cdk/issues/9953
import * as events from "@aws-cdk/aws-events";
import * as logs from "@aws-cdk/aws-logs";

export class CloudWatchLogsTarget implements events.IRuleTarget {
  private logGroup: logs.ILogGroup;

  constructor(logGroup: logs.ILogGroup) {
    this.logGroup = logGroup;
  }

  public bind(_rule: events.IRule, _id?: string): events.RuleTargetConfig {
    return {
      id: "",
      // we can't use logGroup.logArn because it has `:*` on the end, and it's a token
      // so we can't just remove the suffix with string replacement operations
      arn: `arn:aws:logs:${this.logGroup.stack.region}:${this.logGroup.stack.account}:log-group:${this.logGroup.logGroupName}`,
    };
  }
}

@otterley
Copy link
Contributor Author

Looks good @demus. One runtime check I might add is to verify that the log group name starts with /aws/events/ and raise a runtime error if not.

@shivlaks shivlaks added p1 effort/small Small work item – less than a day of effort labels Aug 26, 2020
@fedher
Copy link

fedher commented Sep 1, 2020

@demus have you been able to make it work with custom events, sent from a lambda for example, or does it only work with aws events?

@otterley
Copy link
Contributor Author

otterley commented Sep 1, 2020

@jfenderico I have not tried! But if you do, please let us know. :)

@demus
Copy link

demus commented Sep 1, 2020

@demus have you been able to make it work with custom events, sent from a lambda for example, or does it only work with aws events?

@jfenderico I have not tried! But if you do, please let us know. :)

Same here, I have not tried either

@fedher
Copy link

fedher commented Sep 2, 2020

I could send events from a lambda and it worked well. I had to add some permissions to the target. I found this workaround in another post:

  // Source: https://github.com/aws-cloudformation/aws-cloudformation-coverage-roadmap/issues/351#issuecomment-664493982
  public bind(rule: IRule, id: string): RuleTargetConfig {
      // Ugly hack to grant a permission for allowing EventBridge to store logs in CloudWatch
      const policyName = `${rule.ruleName}-CloudWatchPolicy`
      new AwsCustomResource(this.stack, "CloudwatchLogResourcePolicy", {
          resourceType: "Custom::CloudwatchLogResourcePolicy",
          onUpdate: {
              service: "CloudWatchLogs",
              action: "putResourcePolicy",
              parameters: {
                  policyName,
                  policyDocument: JSON.stringify({
                      Version: "2012-10-17",
                      Statement: [
                          {
                              Sid: policyName,
                              Effect: "Allow",
                              Principal: {
                                  Service: ["events.amazonaws.com"]
                              },
                              Action: ["logs:CreateLogStream", "logs:PutLogEvents"],
                              Resource: this.logGroup.logGroupArn
                          }
                      ]
                  })
              },
              physicalResourceId: PhysicalResourceId.of(policyName),
          },
          onDelete: {
              service: "CloudWatchLogs",
              action: "deleteResourcePolicy",
              parameters: {
                  policyName
              }
          },
          policy: AwsCustomResourcePolicy.fromStatements([
              new PolicyStatement({
                  actions: ["logs:PutResourcePolicy", "logs:DeleteResourcePolicy"],
                  resources: ["*"]
              })
          ])
      });

      return {
          id: '',
          arn: this.logGroup.logGroupArn,
          targetResource: this.logGroup
      }
  }

@otterley
Copy link
Contributor Author

otterley commented Sep 2, 2020

@jfenderico The resource policy should not be needed as long as the target Log Group prefix is /aws/events/. This policy already exists by default in AWS accounts.

@leantorres73
Copy link

This is related to this issue in cloudformation coverage roadmap:

aws-cloudformation/cloudformation-coverage-roadmap#351

@aripalo
Copy link

aripalo commented Sep 21, 2020

I guess this is due to that CloudFormation issue, but just today found this out since I had created CloudWatch Log Group as EventBridge Rule target manually in the console, but could not replicate it with CDK.

Just my two cents, it'd be a really useful feature to have!

@SomayaB SomayaB added in-progress This issue is being actively worked on. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 30, 2020
@BenChaimberg
Copy link
Contributor

@jfenderico The resource policy should not be needed as long as the target Log Group prefix is /aws/events/. This policy already exists by default in AWS accounts.

I don't believe that this is correct. When creating an event rule with a log group target in the console, this resource policy is created automatically. However, the policy does not exist for brand-new accounts and would not be created if the only event rules with log group targets are created via CDK (or the API, for that matter). See: https://docs.aws.amazon.com/eventbridge/latest/userguide/resource-based-policies-eventbridge.html#cloudwatchlogs-permissions

(cc: @DaWyz since you are working on this feature)

@DaWyz
Copy link
Contributor

DaWyz commented Oct 22, 2020

@jfenderico The resource policy should not be needed as long as the target Log Group prefix is /aws/events/. This policy already exists by default in AWS accounts.

I don't believe that this is correct. When creating an event rule with a log group target in the console, this resource policy is created automatically. However, the policy does not exist for brand-new accounts and would not be created if the only event rules with log group targets are created via CDK (or the API, for that matter). See: https://docs.aws.amazon.com/eventbridge/latest/userguide/resource-based-policies-eventbridge.html#cloudwatchlogs-permissions

(cc: @DaWyz since you are working on this feature)

I didn't know that. Thanks for the update @BenChaimberg , I will try and update the PR accordingly !

@mergify mergify bot closed this as completed in #10598 Nov 18, 2020
mergify bot pushed a commit that referenced this issue Nov 18, 2020
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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-events-targets effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants