Skip to content

Commit fee2fa2

Browse files
tmokmsstmokmss
authored
fix(logs): Cannot set log removalPolicy: destroy to more than one LogRetention resources (#22755)
Currently the IAM policy for LogRetention custom resource Lambda function is set only when it is initialized. Because that lambda function is a singleton function, it is only initialized once and therefore the IAM policy to remove log groups is not configured properly. e.g. given we create two LogRetention resources with `removalPolicy: destroy`, the resulting IAM policy has only statement for log group `group1`. ```ts new LogRetention(stack, 'MyLambda1', { logGroupName: 'group1', retention: RetentionDays.ONE_DAY, removalPolicy: cdk.RemovalPolicy.DESTROY, }); new LogRetention(stack, 'MyLambda2', { logGroupName: 'group2', retention: RetentionDays.ONE_DAY, removalPolicy: cdk.RemovalPolicy.DESTROY, }); ``` Also I removed `logs:DeleteLogStream` allow statement because I confirmed it is not required to remove a log group. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 8b35b12 commit fee2fa2

10 files changed

+377
-157
lines changed

‎packages/@aws-cdk/aws-logs/lib/log-retention.ts

+24-25
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,11 @@ export class LogRetention extends Construct {
8585
// Custom resource provider
8686
const provider = this.ensureSingletonLogRetentionFunction(props);
8787

88+
// if removalPolicy is DESTROY, add action for DeleteLogGroup
89+
if (props.removalPolicy === cdk.RemovalPolicy.DESTROY) {
90+
provider.grantDeleteLogGroup(props.logGroupName);
91+
}
92+
8893
// Need to use a CfnResource here to prevent lerna dependency cycles
8994
// @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation
9095
const retryOptions = props.logRetentionRetryOptions;
@@ -137,15 +142,15 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
137142

138143
public readonly tags: cdk.TagManager = new cdk.TagManager(cdk.TagType.KEY_VALUE, 'AWS::Lambda::Function');
139144

145+
private readonly role: iam.IRole;
146+
140147
constructor(scope: Construct, id: string, props: LogRetentionProps) {
141148
super(scope, id);
142149

143-
// Code
144150
const asset = new s3_assets.Asset(this, 'Code', {
145151
path: path.join(__dirname, 'log-retention-provider'),
146152
});
147153

148-
// Role
149154
const role = props.role || new iam.Role(this, 'ServiceRole', {
150155
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
151156
managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')],
@@ -158,29 +163,7 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
158163
// creates a CF circular dependency.
159164
resources: ['*'],
160165
}));
161-
// if removalPolicy is DESTROY, add action for DeleteLogGroup and DeleteLogStream
162-
if (props.removalPolicy === cdk.RemovalPolicy.DESTROY) {
163-
role.addToPrincipalPolicy(new iam.PolicyStatement({
164-
actions: ['logs:DeleteLogGroup'],
165-
//Only allow deleting the specific log group.
166-
resources: [cdk.Stack.of(this).formatArn({
167-
service: 'logs',
168-
resource: 'log-group',
169-
resourceName: `${props.logGroupName}:*`,
170-
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
171-
})],
172-
}));
173-
role.addToPrincipalPolicy(new iam.PolicyStatement({
174-
actions: ['logs:DeleteLogStream'],
175-
//Only allow deleting the specific log group.
176-
resources: [cdk.Stack.of(this).formatArn({
177-
service: 'logs',
178-
resource: `log-group:${props.logGroupName}:log-stream`,
179-
resourceName: '*',
180-
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
181-
})],
182-
}));
183-
}
166+
this.role = role;
184167

185168
// Lambda function
186169
const resource = new cdk.CfnResource(this, 'Resource', {
@@ -210,4 +193,20 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
210193
}
211194
});
212195
}
196+
197+
/**
198+
* @internal
199+
*/
200+
public grantDeleteLogGroup(logGroupName: string) {
201+
this.role.addToPrincipalPolicy(new iam.PolicyStatement({
202+
actions: ['logs:DeleteLogGroup'],
203+
//Only allow deleting the specific log group.
204+
resources: [cdk.Stack.of(this).formatArn({
205+
service: 'logs',
206+
resource: 'log-group',
207+
resourceName: `${logGroupName}:*`,
208+
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
209+
})],
210+
}));
211+
}
213212
}

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/LogRetentionIntegDefaultTestDeployAssert6ACC5A74.assets.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"files": {
44
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
55
"source": {

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.assets.json

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"files": {
44
"d01c24641c7d8cb6488393ffceaefff282370a9a522bf9d77b21da73fa257347": {
55
"source": {
@@ -14,15 +14,15 @@
1414
}
1515
}
1616
},
17-
"918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2": {
17+
"0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6": {
1818
"source": {
1919
"path": "aws-cdk-log-retention-integ.template.json",
2020
"packaging": "file"
2121
},
2222
"destinations": {
2323
"current_account-current_region": {
2424
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
25-
"objectKey": "918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2.json",
25+
"objectKey": "0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6.json",
2626
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
2727
}
2828
}

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.template.json

+54-42
Original file line numberDiff line numberDiff line change
@@ -61,50 +61,48 @@
6161
{
6262
"Action": "logs:DeleteLogGroup",
6363
"Effect": "Allow",
64-
"Resource": {
65-
"Fn::Join": [
66-
"",
67-
[
68-
"arn:",
69-
{
70-
"Ref": "AWS::Partition"
71-
},
72-
":logs:",
73-
{
74-
"Ref": "AWS::Region"
75-
},
76-
":",
77-
{
78-
"Ref": "AWS::AccountId"
79-
},
80-
":log-group:logRetentionLogGroup:*"
64+
"Resource": [
65+
{
66+
"Fn::Join": [
67+
"",
68+
[
69+
"arn:",
70+
{
71+
"Ref": "AWS::Partition"
72+
},
73+
":logs:",
74+
{
75+
"Ref": "AWS::Region"
76+
},
77+
":",
78+
{
79+
"Ref": "AWS::AccountId"
80+
},
81+
":log-group:logRetentionLogGroup2:*"
82+
]
8183
]
82-
]
83-
}
84-
},
85-
{
86-
"Action": "logs:DeleteLogStream",
87-
"Effect": "Allow",
88-
"Resource": {
89-
"Fn::Join": [
90-
"",
91-
[
92-
"arn:",
93-
{
94-
"Ref": "AWS::Partition"
95-
},
96-
":logs:",
97-
{
98-
"Ref": "AWS::Region"
99-
},
100-
":",
101-
{
102-
"Ref": "AWS::AccountId"
103-
},
104-
":log-group:logRetentionLogGroup:log-stream:*"
84+
},
85+
{
86+
"Fn::Join": [
87+
"",
88+
[
89+
"arn:",
90+
{
91+
"Ref": "AWS::Partition"
92+
},
93+
":logs:",
94+
{
95+
"Ref": "AWS::Region"
96+
},
97+
":",
98+
{
99+
"Ref": "AWS::AccountId"
100+
},
101+
":log-group:logRetentionLogGroup:*"
102+
]
105103
]
106-
]
107-
}
104+
}
105+
]
108106
}
109107
],
110108
"Version": "2012-10-17"
@@ -139,6 +137,20 @@
139137
"LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB",
140138
"LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB"
141139
]
140+
},
141+
"MyLambda2254B54D5": {
142+
"Type": "Custom::LogRetention",
143+
"Properties": {
144+
"ServiceToken": {
145+
"Fn::GetAtt": [
146+
"LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A",
147+
"Arn"
148+
]
149+
},
150+
"LogGroupName": "logRetentionLogGroup2",
151+
"RetentionInDays": 1,
152+
"RemovalPolicy": "destroy"
153+
}
142154
}
143155
},
144156
"Parameters": {

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/cdk.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"version":"20.0.0"}
1+
{"version":"21.0.0"}

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/integ.json

+3-2
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"testCases": {
44
"LogRetentionInteg/DefaultTest": {
55
"stacks": [
66
"aws-cdk-log-retention-integ"
77
],
8-
"assertionStack": "LogRetentionInteg/DefaultTest/DeployAssert"
8+
"assertionStack": "LogRetentionInteg/DefaultTest/DeployAssert",
9+
"assertionStackName": "LogRetentionIntegDefaultTestDeployAssert6ACC5A74"
910
}
1011
}
1112
}

‎packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/manifest.json

+8-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "21.0.0",
33
"artifacts": {
44
"Tree": {
55
"type": "cdk:tree",
@@ -23,7 +23,7 @@
2323
"validateOnSynth": false,
2424
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
2525
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
26-
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2.json",
26+
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6.json",
2727
"requiresBootstrapStackVersion": 6,
2828
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
2929
"additionalDependencies": [
@@ -63,6 +63,12 @@
6363
"data": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A"
6464
}
6565
],
66+
"/aws-cdk-log-retention-integ/MyLambda2/Resource": [
67+
{
68+
"type": "aws:cdk:logicalId",
69+
"data": "MyLambda2254B54D5"
70+
}
71+
],
6672
"/aws-cdk-log-retention-integ/BootstrapVersion": [
6773
{
6874
"type": "aws:cdk:logicalId",

0 commit comments

Comments
 (0)