fix(ecs): unnecessary CloudWatch logs ResourcePolicy

[sakurai-ryo]

This PR modified to avoid creating unnecessary ResourcePolicy in CloudWatch Logs.

issue summary

The related issue reports an error when using the awslogs driver on ECS.
This error is caused by the creation of a ResourcePolicy in CloudWatch Logs that reaches the maximum number of ResourcePolicies.
https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/cloudwatch_limits_cwl.html

Current behavior

In some cases, this ResourcePolicy will be created and in other cases it will not be created.
Currently, Grant.addToPrincipalOrResource is used to grant permissions to ExecutionRole and Log Group in the ECS taskDef.

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/log-drivers/aws-log-driver.ts

Line 138 in 607dccb

this.logGroup.grantWrite(containerDefinition.taskDefinition.obtainExecutionRole());

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/log-group.ts

Line 194 in 607dccb

return iam.Grant.addToPrincipalOrResource({

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/grant.ts

Line 122 in 607dccb

public static addToPrincipalOrResource(options: GrantWithResourceOptions): Grant {

Grant.addToPrincipalOrResource first grants permissions to the Grantee (ExecutionRole) and creates a resource base policy for cross account access in cases where certain conditions are not met.
This condition is determined by the contents of the principalAccount of the ExecutionRole and the accountID in the env.account and whether or not these are Tokens, but in this scenario, cross account access is not necessary.

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/grant.ts

Line 141 in 607dccb

if (result.success && sameAccount) {

Also, when the LogGroup.grantWrite call was added to aws-log-driver.ts, the ResourcePolicy for logs could not be created from CFn and only granted to the ExecutionRole.
#1291

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/ReleaseHistory.html

Therefore, the resource base policy should not be necessary when using the awslogs driver.

Major changes

This PR changed to grant permissions only to ExecutionRole when using the awslogs driver.
With this fix, ResourcePolicy will no longer be created when using the awslogs driver.
I don't consider this a breaking change, as it changes the content of the generated template, but does not change the behavior of forwarding logs to CloudWatch Logs.
However, if this is a breaking change, I think it is necessary to use the feature flag.

fixes #22307, fixes #20313

---

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

[sakurai-ryo]

for example,

aws-cdk/packages/aws-cdk-lib/aws-batch/lib/ecs-container-definition.ts

Line 610 in 607dccb

taskDefinition: {

[paulhcsun]

Suggested change

	// In cases where `addToExecutionRolePolicy` is not implemented in some cases,
	// In cases where `addToExecutionRolePolicy` is not implemented,

[sakurai-ryo]

@paulhcsun
Thanks! fixed it.

[paulhcsun]

Overall this looks good to me. Your reasoning for why resource base policy shouldn't be needed for aws log driver makes sense. I just have one concern about cases where addToPrincipalPolicy is not implemented, if that isn't an issue then I'm good to approve.

[sakurai-ryo]

@paulhcsun
Thanks for your feedback!

In this case, I believe that addToPrincipalPolicy is always implemented.
This is because the obtainExecutionRole method creates and returns an ExecutionRole using the Role construct if one does not exist, and the Role construct implements the addToPrincipalPolicy method.

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

Line 712 in 65505f0

if (!this._executionRole) {

https://github.com/sakurai-ryo/aws-cdk/blob/10ed1948beb0f83c1b978da9c0a656aa01a382cb/packages/aws-cdk-lib/aws-iam/lib/role.ts#L521

[paulhcsun]

Thanks for clarifying @sakurai-ryo, I think I just read the method names incorrectly there for addToPrincipalPolicy and addExecutionRolePolicy. A couple more questions to help me understand things a bit better as I'm not too familiar with aws-log-driver:

1. Could you clarify how you reached this conclusion from the attached PR that

when the LogGroup.grantWrite call was added to aws-log-driver.ts, the ResourcePolicy for logs could not be created from CFn and only granted to the ExecutionRole. #1291

I don't see any explicit mention of that in the PR itself.

2. The image attached below the above quote mentions that AWS::Logs::ResourcePolicy resource is used to create a IAM policy. Is this the same workflow for which we are no longer creating the resource policy? And if so would this break anything for users that may be using the generated ResourcePolicy to create IAM policies?

Thanks in advance for taking the time to have this discussion with me to better understand the changes. I know this is will be a big win for many people dealing with this same issue, I just want to be sure that we won't need a feature flag for it.

[sakurai-ryo]

@paulhcsun
Thank you, too, for digging deeper into this issue.
The change in the IAM Policy is significant, and we should discuss it thoroughly.

The implementation for setting IAM Policy when using AWSLogs driver was added in 2018 (#1291), while AWS::Logs::ResourcePolicy was introduced in 2021 and implemented in the CDK (#17015).
Since no other commits have changed AWSLogs driver codes about granting policy, it should have worked without AWS::Logs::ResourcePolicy until 2021.

I do not think this PR breaks anything that may be a breaking change.
As per these documents, access is allowed if either the Identity base policy or the Resource base policy is Allow for access by the same account.
In addition, IAM's evaluation logic evaluates the Identity base policy after the Resource base policy, so even if log forwarding is denied by Permissions Boundary etc, if it is allowed by ExecutionRole, log forwarding will be permitted without any problem.
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_evaluation-logic.html#policy-eval-denyallow
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_evaluation-logic.html#policies_evaluation_example

Sorry if I misunderstood what you meant.

[paulhcsun]

@sakurai-ryo
Thanks for your response, those answer both of my questions and I'm comfortable moving forward with this without a feature flag. Thanks again for the contribution, this will be a very helpful win for the community!

[mergify]

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

[mergify]

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: f12e2f1
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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