fix(ecr): auto delete images on ECR repository containing manifest li…

…st (aws#25789) Fixes aws#24822 As I commented on aws#24822 (comment), auto delete container images in ECR repository fails when it has container manifest list. I fix custom resource Lambda function to delete tagged images first. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
0xdevalias · Jun 6, 2023 · 830e6d3 · 830e6d3
1 parent 1668dbd
commit 830e6d3
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 8 deletions.
diff --git a/...cr/test/integ.repository-auto-delete-images.js.snapshot/aws-ecr-integ-stack.template.json b/...cr/test/integ.repository-auto-delete-images.js.snapshot/aws-ecr-integ-stack.template.json
@@ -89,7 +89,7 @@
      "S3Bucket": {
       "Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
      },
-     "S3Key": "0bec74976eeec3c24fbc534a8e85197274c1c43a93018353f96c90cbd671cf14.zip"
+     "S3Key": "9064d7af3a637d340a1e36aada4ccade64a383701b3b15008043e12bbea5a67e.zip"
     },
     "Timeout": 900,
     "MemorySize": 128,

diff --git a/...@aws-cdk-testing/framework-integ/test/aws-ecr/test/integ.repository-auto-delete-images.ts b/...@aws-cdk-testing/framework-integ/test/aws-ecr/test/integ.repository-auto-delete-images.ts
@@ -17,4 +17,5 @@ new cdk.CfnOutput(stack, 'RepositoryURI', {
 
 new IntegTest(app, 'cdk-integ-auto-delete-images', {
   testCases: [stack],
+  diffAssets: true,
 });
diff --git a/packages/aws-cdk-lib/aws-ecr/lib/auto-delete-images-handler/index.ts b/packages/aws-cdk-lib/aws-ecr/lib/auto-delete-images-handler/index.ts
@@ -39,16 +39,34 @@ async function onUpdate(event: AWSLambda.CloudFormationCustomResourceEvent) {
 async function emptyRepository(params: ECR.ListImagesRequest) {
   const listedImages = await ecr.listImages(params).promise();
 
-  const imageIds = listedImages?.imageIds ?? [];
+  const imageIds: ECR.ImageIdentifier[] = [];
+  const imageIdsTagged: ECR.ImageIdentifier[] = [];
+  (listedImages.imageIds ?? []).forEach(imageId => {
+    if ('imageTag' in imageId) {
+      imageIdsTagged.push(imageId);
+    } else {
+      imageIds.push(imageId);
+    }
+  });
+
   const nextToken = listedImages.nextToken ?? null;
-  if (imageIds.length === 0) {
+  if (imageIds.length === 0 && imageIdsTagged.length === 0) {
     return;
   }
 
-  await ecr.batchDeleteImage({
-    repositoryName: params.repositoryName,
-    imageIds,
-  }).promise();
+  if (imageIdsTagged.length !== 0) {
+    await ecr.batchDeleteImage({
+      repositoryName: params.repositoryName,
+      imageIds: imageIdsTagged,
+    }).promise();
+  }
+
+  if (imageIds.length !== 0) {
+    await ecr.batchDeleteImage({
+      repositoryName: params.repositoryName,
+      imageIds: imageIds,
+    }).promise();
+  }
 
   if (nextToken) {
     await emptyRepository({

diff --git a/packages/aws-cdk-lib/aws-ecr/test/auto-delete-images-handler.test.ts b/packages/aws-cdk-lib/aws-ecr/test/auto-delete-images-handler.test.ts
@@ -157,6 +157,7 @@ test('deletes all objects when the name changes on update event', async () => {
   await invokeHandler(event);
 
   // THEN
+  expect(mockECRClient.describeRepositories).toHaveBeenCalledTimes(1);
   expect(mockECRClient.listImages).toHaveBeenCalledTimes(1);
   expect(mockECRClient.listImages).toHaveBeenCalledWith({ repositoryName: 'MyRepo' });
   expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(1);
@@ -167,7 +168,6 @@ test('deletes all objects when the name changes on update event', async () => {
       { imageDigest: 'ImageDigest2', imageTag: 'ImageTag2' },
     ],
   });
-  expect(mockECRClient.describeRepositories).toHaveBeenCalledTimes(1);
 });
 
 test('deletes no images on delete event when repository has no images', async () => {
@@ -366,6 +366,61 @@ test('does nothing when the repository does not exist', async () => {
   expect(mockECRClient.batchDeleteImage).not.toHaveBeenCalled();
 });
 
+test('delete event where repo has tagged images and untagged images', async () => {
+  // GIVEN
+  mockAwsPromise(mockECRClient.describeRepositories, {
+    repositories: [
+      { repositoryArn: 'RepositoryArn', respositoryName: 'MyRepo' },
+    ],
+  });
+
+  mockECRClient.promise // listedImages() call
+    .mockResolvedValueOnce({
+      imageIds: [
+        {
+          imageTag: 'tag1',
+          imageDigest: 'sha256-1',
+        },
+        {
+          imageDigest: 'sha256-2',
+        },
+      ],
+    });
+
+  // WHEN
+  const event: Partial<AWSLambda.CloudFormationCustomResourceDeleteEvent> = {
+    RequestType: 'Delete',
+    ResourceProperties: {
+      ServiceToken: 'Foo',
+      RepositoryName: 'MyRepo',
+    },
+  };
+  await invokeHandler(event);
+
+  // THEN
+  expect(mockECRClient.describeRepositories).toHaveBeenCalledTimes(1);
+  expect(mockECRClient.listImages).toHaveBeenCalledTimes(1);
+  expect(mockECRClient.listImages).toHaveBeenCalledWith({ repositoryName: 'MyRepo' });
+  expect(mockECRClient.batchDeleteImage).toHaveBeenCalledTimes(2);
+  expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(1, {
+    repositoryName: 'MyRepo',
+    imageIds: [
+      {
+        imageTag: 'tag1',
+        imageDigest: 'sha256-1',
+      },
+    ],
+  });
+  expect(mockECRClient.batchDeleteImage).toHaveBeenNthCalledWith(2, {
+    repositoryName: 'MyRepo',
+    imageIds: [
+      {
+        imageDigest: 'sha256-2',
+      },
+    ],
+  });
+});
+
 // helper function to get around TypeScript expecting a complete event object,
 // even though our tests only need some of the fields
 async function invokeHandler(event: Partial<AWSLambda.CloudFormationCustomResourceEvent>) {