Merge pull request #13 from nicklaw5/limit-to-100

Limit to 100 image per invocation, other various improvements
nib-health-funds · Mar 25, 2019 · 3b3b2e6 · 3b3b2e6
2 parents 091e701 + 8d3d425
commit 3b3b2e6
Show file tree

Hide file tree

Showing 3 changed files with 113 additions and 111 deletions.
diff --git a/README.md b/README.md
@@ -1,21 +1,27 @@
-# THIS IS NOW OBSOLETE - REPLACED BY [AWS ECR LIFECYCLE POLICIES](http://docs.aws.amazon.com/AmazonECR/latest/userguide/LifecyclePolicies.html)
 # Robin
 
+Note that [ECR Lifecycle Policies](http://docs.aws.amazon.com/AmazonECR/latest/userguide/LifecyclePolicies.html) may be a better fit for you use case.
+
 [![Build Status](https://travis-ci.org/nib-health-funds/robin.svg?branch=master)](https://travis-ci.org/nib-health-funds/robin)
 [![Dependencies](https://david-dm.org/nib-health-funds/robin.svg)](https://david-dm.org/nib-health-funds/robin)
 
 Batman's very capable side kick - deletes old ECR images.
-<center><img src="images/robin.jpg"</img></center>
+<center><img src="images/robin.jpg"></center>
 
-# Why Robin?
+## Why Robin?
 
 Images stored in ECR incur monthly data storage charges, this means paying to store old images that are no longer in use. Also, AWS ECR has a default limit of 1000 images. Therefore, it is desirable to ensure the ECR repositories are kept clean of unused images.
 
-# What we delete currently:
+## What we delete currently:
+
+Per Lambda invocation:
+
+- 100 images that are older than 30 days and that do not have tags that contain 'master'
 
-- Images older than 30 days that do not have tags that contain 'master'
+If you need to delete more than 100 images, rather than complicating this script so that it can paginate
+through all pages of images, we suggest you simply run the lambda multiple times.
 
-# Usage
+## Usage
 
 1. Authenticate and get AWS credentials via your preferred CLI, you may need to export the environment variables directly
 
@@ -50,10 +56,9 @@ $ npm run tail-logs
 ```
 
 
-# TODO
+## TODO
 
 - Only keep the last 10 master images (justification: we should be using the last images only, last 10 gives us something to rollback to if needed.)
 - Add some more documentation to this readme
 - Delete all untagged images
-- Make repo names configurable via env vars
 - Make tagging convention configurable
diff --git a/handler.js b/handler.js
@@ -26,32 +26,31 @@ function getAllImages(ecr, registryId, repoName) {
     maxResults: 100
   };
 
-  /**
-   * 'Follows' the result pagination in a sort of reduce method in which the results are continuely added to an array
-   */
-  function followPages(ecrSvc, allImages, data) {
-    const combinedImages = allImages.length === 0 ? [...data.imageDetails] : [...allImages, ...data.imageDetails];
-
-    if (data.nextToken) {
-      params.nextToken = data.nextToken;
-      return ecrSvc.describeImages(params)
-        .promise()
-        .then(res => followPages(ecrSvc, combinedImages, res));
-    }
-
-    return Promise.resolve(combinedImages);
-  }
-
   return ecr.describeImages(params).promise()
-    .then(data => followPages(ecr, [], data));
+    .then(data => Promise.resolve(data.imageDetails));
 }
 
-function buildReport(isDryRun, reposNotFound, reposWithUntaggedImages, reposWithDeletedImages) {
+function buildReport(
+  isDryRun,
+  reposNotFound,
+  reposWithUntaggedImages,
+  reposWithDeletedImagesDryRun,
+  reposWithDeletedImages,
+  reposWithImagesThatFailedToDelete
+) {
   const untaggedRepoKeys = Object.keys(reposWithUntaggedImages);
+  const deletedDryRunRepoKeys = Object.keys(reposWithDeletedImagesDryRun);
   const deletedRepoKeys = Object.keys(reposWithDeletedImages);
-
-  if (reposNotFound.length === 0 && untaggedRepoKeys.length === 0 && deletedRepoKeys.length === 0) {
-    return Promise.resolve('Robin ran but there was no vigilamnte justice was needed');
+  const failedToDeletedRepoKeys = Object.keys(reposWithImagesThatFailedToDelete);
+
+  if (
+    reposNotFound.length === 0
+    && untaggedRepoKeys.length === 0
+    && deletedDryRunRepoKeys === 0
+    && deletedRepoKeys.length === 0
+    && failedToDeletedRepoKeys === 0
+  ) {
+    return 'Robin ran but there no vigilamnte justice was needed';
   }
 
   const backticks = str => (`\`${str}\``);
@@ -60,32 +59,59 @@ function buildReport(isDryRun, reposNotFound, reposWithUntaggedImages, reposWith
   let text = 'Robin has attempted to clean up the streets!';
 
   if (reposNotFound.length !== 0) {
-    text += '\n\n\n====================================';
+    text += '\n\n\n===================================================';
     text += `\nRepositories not found (${reposNotFound.length})${dryRunText}`;
-    text += '\n====================================';
+    text += '\n===================================================';
     reposNotFound.forEach(repoName => {
       text += `\n${backticks(repoName)}`;
     });
   }
 
   if (untaggedRepoKeys.length !== 0) {
-    text += '\n\n\n====================================';
+    text += '\n\n\n===================================================';
     text += `\nRepositories with untagged images (${untaggedRepoKeys.length})${dryRunText}`;
-    text += '\n====================================';
+    text += '\n===================================================';
     untaggedRepoKeys.forEach(repoName => {
       text += `\n${backticks(repoName)} - ${reposWithUntaggedImages[repoName]} image${reposWithUntaggedImages[repoName] > 1 ? 's' : ''}`;
     });
   }
 
-  if (deletedRepoKeys.length !== 0) {
-    text += '\n\n\n====================================';
-    text += `\nRepositories with images deleted (${deletedRepoKeys.length})${dryRunText}`;
-    text += '\n====================================';
-    deletedRepoKeys.forEach(repoName => {
-      text += `\n${backticks(repoName)} (${reposWithDeletedImages[repoName].length} tags): ${reposWithDeletedImages[repoName].join(', ')}`;
-    });
+  if (isDryRun) {
+    text += '\n\n\n===================================================';
+    text += `\nRepositories with images deleted (${deletedDryRunRepoKeys.length})${dryRunText}`;
+    text += '\n===================================================';
+    if (deletedDryRunRepoKeys.length === 0) {
+      text += '\nNo images deleted';
+    } else {
+      deletedDryRunRepoKeys.forEach(repoName => {
+        // eslint-disable-next-line max-len
+        text += `\n${backticks(repoName)} (${reposWithDeletedImagesDryRun[repoName].length} tags): ${reposWithDeletedImagesDryRun[repoName].join(', ')}`;
+      });
+    }
+  } else {
+    text += '\n\n\n===================================================';
+    text += `\nRepositories with images deleted (${deletedRepoKeys.length})`;
+    text += '\n===================================================';
+    if (deletedRepoKeys.length === 0) {
+      text += '\nNo images deleted';
+    } else {
+      deletedRepoKeys.forEach(repoName => {
+        text += `\n${backticks(repoName)} (${reposWithDeletedImages[repoName].length} tags): ${reposWithDeletedImages[repoName].join(', ')}`;
+      });
+    }
+
+    if (failedToDeletedRepoKeys.length !== 0) {
+      text += '\n\n\n===================================================';
+      text += `\nRepositories with images that failed deleted (${failedToDeletedRepoKeys.length})`;
+      text += '\n===================================================';
+      deletedRepoKeys.forEach(repoName => {
+        // eslint-disable-next-line max-len
+        text += `\n${backticks(repoName)} (${reposWithImagesThatFailedToDelete[repoName].length} tags): ${reposWithImagesThatFailedToDelete[repoName].join(', ')}`;
+      });
+    }
   }
 
+
   return text;
 }
 
@@ -107,6 +133,8 @@ module.exports.cleanupImages = (event, context, callback) => {
   const reposNotFound = [];
   const reposWithUntaggedImages = {};
   const reposWithDeletedImages = {};
+  const reposWithDeletedImagesDryRun = {};
+  const reposWithImagesThatFailedToDelete = {};
 
   console.log('Robin is dealing out some of his own justice...');
   console.log('Robin is using ECR Region: ', ecrRegion);
@@ -147,28 +175,47 @@ module.exports.cleanupImages = (event, context, callback) => {
         console.log('Images to delete: ', toDelete);
 
         const convertedToDelete = toDelete.map(image => {
-          if (typeof reposWithDeletedImages[repoName] === 'undefined') {
-            reposWithDeletedImages[repoName] = [];
+          if (isDryRun) {
+            image.imageTags.forEach(tag => {
+              if (typeof reposWithDeletedImagesDryRun[repoName] === 'undefined') {
+                reposWithDeletedImagesDryRun[repoName] = [];
+              }
+              reposWithDeletedImagesDryRun[repoName].push(tag);
+            });
           }
 
-          image.imageTags.forEach(tag => {
-            reposWithDeletedImages[repoName].push(tag);
-          });
-
           return { imageDigest: image.imageDigest };
         });
 
+        if (isDryRun) {
+          return Promise.resolve({ imageIds: [], failures: [] });
+        }
+
         const deleteParams = {
           registryId: registry,
           repositoryName: repoName,
           imageIds: convertedToDelete
         };
 
-        if (isDryRun) {
-          return Promise.resolve({ imageIds: [], failures: []});
-        }
-
-        return ecr.batchDeleteImage(deleteParams).promise();
+        return ecr.batchDeleteImage(deleteParams).promise()
+          .then(({ failures, imageIds }) => {
+            console.log('failures: ', failures);
+            console.log('imageIds: ', imageIds);
+
+            failures.forEach(({ imageId }) => {
+              if (typeof reposWithImagesThatFailedToDelete[repoName] === 'undefined') {
+                reposWithImagesThatFailedToDelete[repoName] = [];
+              }
+              reposWithImagesThatFailedToDelete[repoName].push(imageId.imageTag);
+            });
+
+            imageIds.forEach(({ imageTag }) => {
+              if (typeof reposWithDeletedImages[repoName] === 'undefined') {
+                reposWithDeletedImages[repoName] = [];
+              }
+              reposWithDeletedImages[repoName].push(imageTag);
+            });
+          });
       })
       .catch(err => {
         if (err.code === 'RepositoryNotFoundException' && reposNotFound.indexOf(repoName) === -1) {
@@ -181,7 +228,14 @@ module.exports.cleanupImages = (event, context, callback) => {
 
   return Promise.all(promises)
     .then(() => {
-      const reportText = buildReport(isDryRun, reposNotFound, reposWithUntaggedImages, reposWithDeletedImages);
+      const reportText = buildReport(
+        isDryRun,
+        reposNotFound,
+        reposWithUntaggedImages,
+        reposWithDeletedImagesDryRun,
+        reposWithDeletedImages,
+        reposWithImagesThatFailedToDelete
+      );
 
       // Log Results
       console.log(reportText.replace(/`/g, '')); // strip backticks when logging to CloudWatch (backticks are for Slack!)

diff --git a/handler.test.js b/handler.test.js
@@ -68,13 +68,15 @@ describe('cleanupImages', () => {
   const cleanup = proxyquire('./handler.js', mocks);
 
   beforeEach(() => {
+    process.env.DRY_RUN = 'true';
     process.env.REPO_NAMES = 'test-repo';
     process.env.AWS_ACCOUNT_ID = '1234567890';
 
     sandbox.reset();
   });
 
   it('Should not remove master images', done => {
+    process.env.DRY_RUN = 'false';
     deleteStub.returns(deletePromise);
     cleanup.cleanupImages(null, null, () => {
       assert(describeImagesStub.called);
@@ -89,69 +91,10 @@ describe('cleanupImages', () => {
 
   it('Should not call delete if dry run', done => {
     deleteStub.returns(deletePromise);
-    process.env.DRY_RUN = true;
     cleanup.cleanupImages(null, null, () => {
       assert(describeImagesStub.called);
       assert(deleteStub.notCalled);
       done();
     });
   });
-
-  it('Should handle pagination using the nextToken', done => {
-
-    const stub = sandbox.stub();
-    stub.onCall(0).returns({
-      promise: () => Promise.resolve(
-        {
-          nextToken: 'next-please',
-          imageDetails: [{
-            registryId: '384553929753',
-            repositoryName: 'test-repo-1',
-            imageDigest: '4',
-            imageTags: ['1.0.0-other', 'dont-ignore-this'],
-            imageSizeInBytes: '1024',
-            imagePushedAt: moment().add(-31, 'days')
-          }]
-        })
-    });
-
-    stub.onCall(1).returns({
-      promise: () => Promise.resolve(
-        {
-          nextToken: null,
-          imageDetails: [{
-            registryId: '384553929754',
-            repositoryName: 'test-repo-2',
-            imageDigest: '4',
-            imageTags: ['1.0.0-other', 'dont-ignore-this'],
-            imageSizeInBytes: '1024',
-            imagePushedAt: moment().add(-31, 'days')
-          }]
-        })
-    });
-
-    const cleanupImagesProxy = proxyquire('./handler.js', {
-      'aws-sdk': {
-        ECR: function () { // eslint-disable-line
-          this.batchDeleteImage = deleteStub;
-          this.describeImages = stub;
-        }
-      }
-    });
-
-    process.env.DRY_RUN = false;
-
-    cleanupImagesProxy.cleanupImages(null, null, () => {
-      const secondCall = stub.getCall(1);
-
-      assert(secondCall.calledWithMatch({
-        registryId: '1234567890',
-        repositoryName: 'test-repo',
-        maxResults: 100,
-        nextToken: 'next-please'
-      }), 'describeImages was not called with the next token for the 2nd time');
-      assert(stub.callCount === 2, 'describeImages was not called 2 times');
-      done();
-    });
-  });
 });