feat(events-targets): add toggle to opt out of resource policy creation for targeted log group#32242
feat(events-targets): add toggle to opt out of resource policy creation for targeted log group#32242gwaltneyluke wants to merge 9 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
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. |
|
Comments on closed issues and PRs are hard for our team to see. |
aaythapa
left a comment
There was a problem hiding this comment.
Thanks for contributing! Changes look good, just a couple nits and a question. I see there are some merge conflicts as well.
| to set `installLatestAwsSdk` to `true`. This may be problematic for CN partition deployment. To | ||
| workaround this issue, set `installLatestAwsSdk` to `false`. | ||
| By default, the CloudWatch LogGroup target will create a LogGroup Resource Policy which allows | ||
| EventBridge to write to the target LogGroup. If you want to manage this trust relationship manually, |
There was a problem hiding this comment.
How is the trust relationship managed manually? Is there a way we can make it easier through a property? If there is then we should go with that and my thinking is if they set that property then we don't make a resource policy. So the property will not only opt users out of making resource policies but make it easier to manage the trust. What are your thoughts on this?
There was a problem hiding this comment.
I was thinking the trust relationship would be managed with a pre-existing Log Group Resource Policy that allows EventBridge to write to any log group with a specific prefix (ex: /aws/events/). This way we can have several rules write to different log groups and that trust is managed by a single policy. This might be a little too coarse a policy, though.
Another idea would be to provide an existing Log Group Resource Policy to this method, and the method can add a statement to that policy which allows EventBridge to write to the specified log group, rather than adding a whole new policy.
|
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. |
|
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. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Abogical
left a comment
There was a problem hiding this comment.
Hi @gwaltneyluke ! Thank you for your contribution.
This looks good, but there are currently conflicts with the main branch with the snapshot files. Try rebasing from main and regenerating the snapshots to fix the conflicts.
|
This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled. |
Issue # (if applicable)
Closes #31404.
Reason for this change
When a CloudWatch LogGroup is set as the target of an EventBridge rule, a custom resource creates a Log Resource Policy to establish trust so that EventBridge can write messages to CloudWatch. However, there is a strict limit of 10 CloudWatch Log Resource Policies per account per region. This therefore limits the amount of EventBridge rules an account can have writing to CloudWatch.
Description of changes
The optional property
createLogGroupResourcePolicyhas been added to theLogGroupPropsinterface. When omitted or set totrue, the Resource Policy is created just as the functionality exists today. When set tofalse, the Resource Policy is not created. The trust between EventBridge and CloudWatch must be established manually.Description of how you validated changes
Unit tests have been added and are passing. Existing integration tests are passing.
Code was also linked to an existing project, where the new property was toggled on and off. When on, the CloudWatch LogGroup Resource Policy was created, and messages sent to EventBridge were making it to the LogGroup. When off, the CloudWatch LogGroup Resource Policy was NOT created, but a custom Resource Policy still allowed messages sent to the EventBridge to end up in the LogGroup.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license