fix(logs): Cannot set log removalPolicy: destroy to more than one L…

…ogRetention resources (aws#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*
brennanho · Feb 22, 2023 · 0bc7d83 · 0bc7d83
1 parent 6e9daad
commit 0bc7d83
Show file tree

Hide file tree

Showing 10 changed files with 377 additions and 157 deletions.
diff --git a/packages/@aws-cdk/aws-logs/lib/log-retention.ts b/packages/@aws-cdk/aws-logs/lib/log-retention.ts
@@ -85,6 +85,11 @@ export class LogRetention extends Construct {
     // Custom resource provider
     const provider = this.ensureSingletonLogRetentionFunction(props);
 
+    // if removalPolicy is DESTROY, add action for DeleteLogGroup
+    if (props.removalPolicy === cdk.RemovalPolicy.DESTROY) {
+      provider.grantDeleteLogGroup(props.logGroupName);
+    }
+
     // Need to use a CfnResource here to prevent lerna dependency cycles
     // @aws-cdk/aws-cloudformation -> @aws-cdk/aws-lambda -> @aws-cdk/aws-cloudformation
     const retryOptions = props.logRetentionRetryOptions;
@@ -137,15 +142,15 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
 
   public readonly tags: cdk.TagManager = new cdk.TagManager(cdk.TagType.KEY_VALUE, 'AWS::Lambda::Function');
 
+  private readonly role: iam.IRole;
+
   constructor(scope: Construct, id: string, props: LogRetentionProps) {
     super(scope, id);
 
-    // Code
     const asset = new s3_assets.Asset(this, 'Code', {
       path: path.join(__dirname, 'log-retention-provider'),
     });
 
-    // Role
     const role = props.role || new iam.Role(this, 'ServiceRole', {
       assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
       managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')],
@@ -158,29 +163,7 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
       // creates a CF circular dependency.
       resources: ['*'],
     }));
-    // if removalPolicy is DESTROY, add action for DeleteLogGroup and DeleteLogStream
-    if (props.removalPolicy === cdk.RemovalPolicy.DESTROY) {
-      role.addToPrincipalPolicy(new iam.PolicyStatement({
-        actions: ['logs:DeleteLogGroup'],
-        //Only allow deleting the specific log group.
-        resources: [cdk.Stack.of(this).formatArn({
-          service: 'logs',
-          resource: 'log-group',
-          resourceName: `${props.logGroupName}:*`,
-          arnFormat: ArnFormat.COLON_RESOURCE_NAME,
-        })],
-      }));
-      role.addToPrincipalPolicy(new iam.PolicyStatement({
-        actions: ['logs:DeleteLogStream'],
-        //Only allow deleting the specific log group.
-        resources: [cdk.Stack.of(this).formatArn({
-          service: 'logs',
-          resource: `log-group:${props.logGroupName}:log-stream`,
-          resourceName: '*',
-          arnFormat: ArnFormat.COLON_RESOURCE_NAME,
-        })],
-      }));
-    }
+    this.role = role;
 
     // Lambda function
     const resource = new cdk.CfnResource(this, 'Resource', {
@@ -210,4 +193,20 @@ class LogRetentionFunction extends Construct implements cdk.ITaggable {
       }
     });
   }
+
+  /**
+   * @internal
+   */
+  public grantDeleteLogGroup(logGroupName: string) {
+    this.role.addToPrincipalPolicy(new iam.PolicyStatement({
+      actions: ['logs:DeleteLogGroup'],
+      //Only allow deleting the specific log group.
+      resources: [cdk.Stack.of(this).formatArn({
+        service: 'logs',
+        resource: 'log-group',
+        resourceName: `${logGroupName}:*`,
+        arnFormat: ArnFormat.COLON_RESOURCE_NAME,
+      })],
+    }));
+  }
 }
diff --git a/...eg.log-retention.js.snapshot/LogRetentionIntegDefaultTestDeployAssert6ACC5A74.assets.json b/...eg.log-retention.js.snapshot/LogRetentionIntegDefaultTestDeployAssert6ACC5A74.assets.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "files": {
     "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
       "source": {

diff --git a/...cdk/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.assets.json b/...cdk/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.assets.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "files": {
     "d01c24641c7d8cb6488393ffceaefff282370a9a522bf9d77b21da73fa257347": {
       "source": {
@@ -14,15 +14,15 @@
         }
       }
     },
-    "918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2": {
+    "0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6": {
       "source": {
         "path": "aws-cdk-log-retention-integ.template.json",
         "packaging": "file"
       },
       "destinations": {
         "current_account-current_region": {
           "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
-          "objectKey": "918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2.json",
+          "objectKey": "0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6.json",
           "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
         }
       }

diff --git a/...k/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.template.json b/...k/aws-logs/test/integ.log-retention.js.snapshot/aws-cdk-log-retention-integ.template.json
@@ -61,50 +61,48 @@
       {
        "Action": "logs:DeleteLogGroup",
        "Effect": "Allow",
-       "Resource": {
-        "Fn::Join": [
-         "",
-         [
-          "arn:",
-          {
-           "Ref": "AWS::Partition"
-          },
-          ":logs:",
-          {
-           "Ref": "AWS::Region"
-          },
-          ":",
-          {
-           "Ref": "AWS::AccountId"
-          },
-          ":log-group:logRetentionLogGroup:*"
+       "Resource": [
+        {
+         "Fn::Join": [
+          "",
+          [
+           "arn:",
+           {
+            "Ref": "AWS::Partition"
+           },
+           ":logs:",
+           {
+            "Ref": "AWS::Region"
+           },
+           ":",
+           {
+            "Ref": "AWS::AccountId"
+           },
+           ":log-group:logRetentionLogGroup2:*"
+          ]
          ]
-        ]
-       }
-      },
-      {
-       "Action": "logs:DeleteLogStream",
-       "Effect": "Allow",
-       "Resource": {
-        "Fn::Join": [
-         "",
-         [
-          "arn:",
-          {
-           "Ref": "AWS::Partition"
-          },
-          ":logs:",
-          {
-           "Ref": "AWS::Region"
-          },
-          ":",
-          {
-           "Ref": "AWS::AccountId"
-          },
-          ":log-group:logRetentionLogGroup:log-stream:*"
+        },
+        {
+         "Fn::Join": [
+          "",
+          [
+           "arn:",
+           {
+            "Ref": "AWS::Partition"
+           },
+           ":logs:",
+           {
+            "Ref": "AWS::Region"
+           },
+           ":",
+           {
+            "Ref": "AWS::AccountId"
+           },
+           ":log-group:logRetentionLogGroup:*"
+          ]
          ]
-        ]
-       }
+        }
+       ]
       }
      ],
      "Version": "2012-10-17"
@@ -139,6 +137,20 @@
     "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRoleDefaultPolicyADDA7DEB",
     "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aServiceRole9741ECFB"
    ]
+  },
+  "MyLambda2254B54D5": {
+   "Type": "Custom::LogRetention",
+   "Properties": {
+    "ServiceToken": {
+     "Fn::GetAtt": [
+      "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A",
+      "Arn"
+     ]
+    },
+    "LogGroupName": "logRetentionLogGroup2",
+    "RetentionInDays": 1,
+    "RemovalPolicy": "destroy"
+   }
   }
  },
  "Parameters": {

diff --git a/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/cdk.out b/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/cdk.out
@@ -1 +1 @@
-{"version":"20.0.0"}
+{"version":"21.0.0"}
diff --git a/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/integ.json b/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/integ.json
@@ -1,11 +1,12 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "testCases": {
     "LogRetentionInteg/DefaultTest": {
       "stacks": [
         "aws-cdk-log-retention-integ"
       ],
-      "assertionStack": "LogRetentionInteg/DefaultTest/DeployAssert"
+      "assertionStack": "LogRetentionInteg/DefaultTest/DeployAssert",
+      "assertionStackName": "LogRetentionIntegDefaultTestDeployAssert6ACC5A74"
     }
   }
 }
diff --git a/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/manifest.json b/packages/@aws-cdk/aws-logs/test/integ.log-retention.js.snapshot/manifest.json
@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "21.0.0",
   "artifacts": {
     "Tree": {
       "type": "cdk:tree",
@@ -23,7 +23,7 @@
         "validateOnSynth": false,
         "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
         "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
-        "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/918f6261a4f427af36f4c1a810688fd80eafca576f152cb945787b6e916d00f2.json",
+        "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/0b36d56a8395399d5e1fa49430910ae11ecc0b6a6f1163109c595a563569dde6.json",
         "requiresBootstrapStackVersion": 6,
         "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
         "additionalDependencies": [
@@ -63,6 +63,12 @@
             "data": "LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A"
           }
         ],
+        "/aws-cdk-log-retention-integ/MyLambda2/Resource": [
+          {
+            "type": "aws:cdk:logicalId",
+            "data": "MyLambda2254B54D5"
+          }
+        ],
         "/aws-cdk-log-retention-integ/BootstrapVersion": [
           {
             "type": "aws:cdk:logicalId",