fix(events-targets): CloudWatch Log Group Resource Policy should be disabled (under feature flag) #27569

aritola-sxm · 2023-10-17T01:55:33Z

This is a proposed solution to #17002. In summary, aws-events-targets.CloudWatchLogGroup will always create a CloudWatch Log Group Resource Policy, which have a max limit of 10 per AWS environment. This behavior is untenable in large environments and instead a single Resource Policy should be created separately, similar to how the AWS Console will do it.

I want to run the approach by the CDK team before I do the polish work.

TODOs:

Update FF README
Write and run tests

Closes #17002.

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

…n should be disabled via feature flag

aws-cdk-automation

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.

aws-cdk-automation · 2023-10-17T02:05:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e3c442d
Result: FAILED
Build Logs (available for 30 days)

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

aritola-sxm · 2023-10-18T21:36:51Z

Clarification Request

scanlonp

This looks good! Please go ahead with the readme and tests.

However, I do not think this needs to be behind a feature flag. The new property will allow the user to control if the policy is created or not, and keeping the default behavior consistent with the current behavior will not be breaking. If you have a reason for keeping this behind a feature flag, I am open to discussion.

scanlonp · 2023-10-18T23:15:48Z

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

@@ -5,7 +5,8 @@ import { RuleTargetInputProperties, RuleTargetInput, EventField, IRule } from '.
 import * as iam from '../../aws-iam';
 import * as logs from '../../aws-logs';
 import * as cdk from '../../core';
-import { ArnFormat, Stack } from '../../core';
+import { ArnFormat, Stack, FeatureFlags} from '../../core';


Suggested change

import { ArnFormat, Stack, FeatureFlags} from '../../core';

This was not needed even before your change. We can get rid of it now.

scanlonp · 2023-10-18T23:15:53Z

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

+  /**
+   * Automatically configure an AWS CloudWatch Log Group Resource Policy for Events Target.
+   *
+   * @default true
+   * @default - false if `@aws-cdk/aws-events:eventsTargetDisableLogGroupResourcePolicy` is enabled, true otherwise
+   */
+  readonly logGroupResourcePolicy?: boolean;


Suggested change

/**

* Automatically configure an AWS CloudWatch Log Group Resource Policy for Events Target.

*

* @default true

* @default - false if `@aws-cdk/aws-events:eventsTargetDisableLogGroupResourcePolicy` is enabled, true otherwise

*/

readonly logGroupResourcePolicy?: boolean;

/**

* Automatically configure an AWS CloudWatch Log Group Resource Policy for Events Target.

*

* Note: if set to true, you must create and configure a CloudWatch Log Group Resource Policy separately.

*

* Can be used to avoid hitting the limit of 10 CloudWatch Log Group Resources in an AWS environment.

*

* @default false

*/

readonly disableLogGroupResourcePolicyCreation?: boolean;

Open to discussion on the prop name + doc string, but I think this is closer to what we want to convey.

aritola-sxm · 2023-10-19T20:35:47Z

Hey @scanlonp, I appreciate you taking an early look! I will make time in the next several business days to add the tests, README, and naming changes.

If you have a reason for keeping this behind a feature flag, I am open to discussion.

IMO the feature flag is important to be able to change what the default behavior is, similar to how the @aws-cdk/aws-apigateway:disableCloudWatchRole FF works. Large AWS environments can easily end up hitting the limit.

If you disagree with this reasoning, I'm happy to remove the FF because it's an improvement regardless.

scanlonp · 2023-10-24T23:46:41Z

@aritola-sxm, after some discussion with the team, I think that there are 3 states for how we handle policy creation:

Create policy for each log group (legacy default)
Create 1 policy for all log groups in a stack (possible feature flag default)
Disable creation of policies; depend on user to configure their own policy

State 1 is what we currently have, and it will stay as the default for backwards compatibility.

State 2 may be a more ideal default. We can check if a log group policy has been made in the stack, and add new log groups to that policy automatically. It looks like that may have been the intention of the current logic, but looking for the resourcePolicyId in different scopes (this.logGroup.node.tryFindChild rather than this.logGroupStack.node.tryFindChild) may be why it is not working. If you would like to implement this as well, that would be terrific! This could be the default behind a feature flag.

State 3 is your proposal. This would take the guardrails off policy creation. This will not be a default (even behind a feature flag), but will cover the cases described in the original issue.

This will require your new optional prop to be an enum type rather than a boolean (even if state 2 is not implemented right now).
If you have any questions, feel free to ask!

aritola-sxm · 2023-10-27T00:20:30Z

@scanlonp Thanks for your response.

All 3 states make sense to me. I'm happy to take a stab at fixing state 2 per your recommendation. In my context, we require state 3 as the resource policy will generally not exist in the stack(s) using this trigger.

Could you explain more of the rationalization behind why making state 3 a configurable default is not tenable? It means without additional code changes, teams cannot deploy more than 10 distinct stacks in a given AWS environment containing this trigger. That limit seems a bit low to me, even for companies not using shared AWS environments.

Regardless, I have blocked off time early next week to implement the changes we're aligned on.

aws-cdk-automation · 2023-11-02T00:08:44Z