Skip to content

Commit

Permalink
fix(ecs): unnecessary CloudWatch logs ResourcePolicy (#28495)
Browse files Browse the repository at this point in the history
This PR modified to avoid creating unnecessary `ResourcePolicy` in CloudWatch Logs.

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

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.
https://github.com/aws/aws-cdk/blob/607dccb0fd920d25f0fe2613b83c9830322c439e/packages/aws-cdk-lib/aws-ecs/lib/log-drivers/aws-log-driver.ts#L138
https://github.com/aws/aws-cdk/blob/607dccb0fd920d25f0fe2613b83c9830322c439e/packages/aws-cdk-lib/aws-logs/lib/log-group.ts#L194
https://github.com/aws/aws-cdk/blob/607dccb0fd920d25f0fe2613b83c9830322c439e/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L122

`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.
https://github.com/aws/aws-cdk/blob/607dccb0fd920d25f0fe2613b83c9830322c439e/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L141

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
![スクリーンショット 2023-12-27 1 08 20](https://github.com/aws/aws-cdk/assets/58683719/5a17a041-d560-45fa-bac6-cdc3894b18bc)
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/ReleaseHistory.html

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

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*
  • Loading branch information
sakurai-ryo authored and GavinZZ committed Jan 11, 2024
1 parent 7d68fee commit c274045
Show file tree
Hide file tree
Showing 14 changed files with 36,693 additions and 4 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/github-merit-badger.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ jobs:
badges: '[beginning-contributor,repeat-contributor,valued-contributor,admired-contributor,star-contributor,distinguished-contributor]'
thresholds: '[0,3,6,13,25,50]'
badge-type: 'achievement'
ignore-usernames: '[rix0rrr,MrArnoldPalmer,iliapolo,otaviomacedo,madeline-k,kaizencc,comcalvi,TheRealAmazonKendra,vinayak-kukreja,mrgrain,pahud,cgarvis,kellertk,HBobertz,sumupitchayan,SankyRed,udaypant,colifran,khushail,scanlonp,mikewrighton,moelasmar,paulhcsun,awsmjs,evgenyka,aws-cdk-automation,dependabot[bot],mergify[bot]]'
ignore-usernames: '[rix0rrr,MrArnoldPalmer,iliapolo,otaviomacedo,madeline-k,kaizencc,comcalvi,TheRealAmazonKendra,vinayak-kukreja,mrgrain,pahud,cgarvis,kellertk,HBobertz,sumupitchayan,SankyRed,udaypant,colifran,khushail,scanlonp,mikewrighton,moelasmar,paulhcsun,awsmjs,evgenyka,GavinZZ,aws-cdk-automation,dependabot[bot],mergify[bot]]'
2 changes: 1 addition & 1 deletion .mergify.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pull_request_rules:
label:
add: [ contribution/core ]
conditions:
- author~=^(rix0rrr|MrArnoldPalmer|iliapolo|otaviomacedo|madeline-k|kaizencc|comcalvi|TheRealAmazonKendra|vinayak-kukreja|mrgrain|pahud|cgarvis|kellertk|HBobertz|sumupitchayan|SankyRed|udaypant|colifran|scanlonp|mikewrighton|moelasmar|paulhcsun|awsmjs|evgenyka)$
- author~=^(rix0rrr|MrArnoldPalmer|iliapolo|otaviomacedo|madeline-k|kaizencc|comcalvi|TheRealAmazonKendra|vinayak-kukreja|mrgrain|pahud|cgarvis|kellertk|HBobertz|sumupitchayan|SankyRed|udaypant|colifran|scanlonp|mikewrighton|moelasmar|paulhcsun|awsmjs|evgenyka|GavinZZ)$
- -label~="contribution/core"
- name: automatic merge
actions:
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit c274045

Please sign in to comment.