Merge pull request #409 from cumulus-nasa/CUMULUS-748

Cumulus 748
nasa · Jul 5, 2018 · 007801e · 007801e
2 parents 617f378 + 3467547
commit 007801e
Show file tree

Hide file tree

Showing 5 changed files with 126 additions and 81 deletions.
diff --git a/.eslint-ratchet-high-water-mark b/.eslint-ratchet-high-water-mark
@@ -1 +1 @@
-670
+466
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
     - update the StreamSpecification DynamoDB tables to have StreamViewType: "NEW_AND_OLD_IMAGES"
     - delete granule files in s3
 - **CUMULUS-398** - Fix not able to filter executions bu workflow
+- **CUMULUS-748** - Fix invalid lambda .zip files being validated/uploaded to AWS
 - **CUMULUS-544** - Post to CMR task has UAT URL hard-coded
   - Made configurable: PostToCmr now requires CMR_ENVIRONMENT env to be set to 'SIT' or 'OPS' for those CMR environments. Default is UAT.
 
@@ -373,4 +374,3 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 [v1.1.0]: https://github.com/cumulus-nasa/cumulus/compare/v1.0.1...v1.1.0
 [v1.0.1]: https://github.com/cumulus-nasa/cumulus/compare/v1.0.0...v1.0.1
 [v1.0.0]: https://github.com/cumulus-nasa/cumulus/compare/pre-v1-release...v1.0.0
-
diff --git a/packages/deployment/lib/lambda.js b/packages/deployment/lib/lambda.js
@@ -4,7 +4,10 @@
 
 const fs = require('fs-extra');
 const path = require('path');
+const util = require('util');
 const utils = require('kes').utils;
+const yauzl = require('yauzl');
+
 const { Lambda } = require('kes');
 
 /**
@@ -27,22 +30,31 @@ class UpdatedLambda extends Lambda {
     super(config);
     this.config = config;
   }
+
   /**
    * Copies the source code of a given lambda function, zips it, calculates
    * the hash of the source code and updates the lambda object with
-   * the hash, local and remote locations of the code
+   * the hash, local and remote locations of the code.
    *
    * @param {Object} lambda - the lambda object
    * @returns {Promise} returns the updated lambda object
    */
-  zipLambda(lambda) {
-    let msg = `Zipping ${lambda.local}`;
+
+  async zipLambda(lambda) {
     // skip if the file with the same hash is zipped
-    if (fs.existsSync(lambda.local)) {
-      return Promise.resolve(lambda);
+    // and is a valid zip file
+    if (await fs.pathExists(lambda.local)) {
+      try {
+        await (util.promisify(yauzl.open))(lambda.local); // Verify yauzl can open the .zip file
+        return Promise.resolve(lambda);
+      }
+      catch (e) {
+        console.log(`${lambda.local} is invalid and will be rebuilt`);
+      }
     }
-    const fileList = [lambda.source];
 
+    let msg = `Zipping ${lambda.local}`;
+    const fileList = [lambda.source];
     if (lambda.useMessageAdapter) {
       const kesFolder = path.join(this.config.kesFolder, 'build', 'adapter');
       fileList.push(kesFolder);
@@ -51,7 +63,15 @@ class UpdatedLambda extends Lambda {
 
     console.log(`${msg} for ${lambda.name}`);
 
-    return utils.zip(lambda.local, fileList).then(() => lambda);
+    try {
+      await utils.zip(lambda.local, fileList);
+    }
+    catch (e) {
+      console.log(`Error zipping ${e}`);
+      throw e;
+    }
+
+    return lambda;
   }
 
   /**

diff --git a/packages/deployment/test/fixtures/zipfile-fixture.zip b/packages/deployment/test/fixtures/zipfile-fixture.zip
diff --git a/packages/deployment/test/test_lambda.js b/packages/deployment/test/test_lambda.js
@@ -3,14 +3,17 @@
 
 'use strict';
 
-const os = require('os');
 const fs = require('fs-extra');
+const os = require('os');
 const path = require('path');
 const test = require('ava');
+
 const Lambda = require('../lib/lambda');
 const { fetchMessageAdapter } = require('../lib/adapter');
 
 const gitPath = 'cumulus-nasa/cumulus-message-adapter';
+const zipFixturePath = 'test/fixtures/zipfile-fixture.zip';
+const zipFixtureSize = fs.statSync(zipFixturePath).size;
 
 test.beforeEach(async (t) => {
   t.context.temp = fs.mkdtempSync(`${os.tmpdir()}${path.sep}`);
@@ -25,7 +28,6 @@ test.beforeEach(async (t) => {
     bucket: 'testbucket',
     stack: 'teststack'
   };
-
   t.context.lambda = {
     handler: 'index.handler',
     name: 'lambda-example',
@@ -62,8 +64,8 @@ test.serial('zipLambda: works for lambda not using message adapter', async (t) =
   t.context.lambda.useMessageAdapter = false;
   const lambdaLocalOrigin = t.context.lambda.local;
   const lambdaRemoteOrigin = t.context.lambda.remote;
-  const l = new Lambda(t.context.config)
-  await l.zipLambda(l.buildS3Path(t.context.lambda));
+  const testLambda = new Lambda(t.context.config);
+  await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
   t.truthy(fs.statSync(t.context.lambda.local));
   t.is(t.context.lambda.local, lambdaLocalOrigin);
   t.is(t.context.lambda.remote, lambdaRemoteOrigin);
@@ -73,8 +75,8 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
   t.context.lambda.useMessageAdapter = true;
   const lambdaLocalOrigin = t.context.lambda.local;
   const lambdaRemoteOrigin = t.context.lambda.remote;
-  const l = new Lambda(t.context.config)
-  await l.zipLambda(l.buildS3Path(t.context.lambda));
+  const testLambda = new Lambda(t.context.config);
+  await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
   t.truthy(fs.statSync(t.context.lambda.local));
   t.is(
     path.basename(t.context.lambda.local),
@@ -86,69 +88,92 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
   );
 });
 
-test.serial(
-  `zipLambda: for lambda using message adapter, no new file is generated 
-  if the task and message adapter are not updated`,
-  async (t) => {
-    t.context.lambda.useMessageAdapter = true;
-
-    // put a lambda zip file there as the result of the previous run
-    const existingLambdaLocal = path.join(
-      path.dirname(t.context.lambda.local),
-      `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
-    );
-
-    const existingLambdaRemote = path.join(
-      path.dirname(t.context.lambda.remote),
-      `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}`
-    );
-
-    fs.writeFileSync(existingLambdaLocal, 'hello');
-    t.is(fs.statSync(existingLambdaLocal).size, 5);
-
-    const l = new Lambda(t.context.config)
-    await l.zipLambda(l.buildS3Path(t.context.lambda));
-    t.truthy(fs.statSync(t.context.lambda.local));
-    t.is(fs.statSync(t.context.lambda.local).size, 5);
-    t.is(t.context.lambda.local, existingLambdaLocal);
-    t.is(t.context.lambda.remote, existingLambdaRemote);
-  }
-);
-
-test.serial(
-  `zipLambda: for lambda using message adapter, a new file is created 
-  if the message adapter is updated`,
-  async (t) => {
-    t.context.lambda.useMessageAdapter = true;
-    const lambdaLocalOrigin = t.context.lambda.local;
-    const lambdaRemoteOrigin = t.context.lambda.remote;
-
-    // put an empty lambda zip file there as the result of the previous run
-    const existingLambdaLocal = path.join(
-      path.dirname(t.context.lambda.local),
-      `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
-    );
-
-    fs.writeFileSync(existingLambdaLocal, 'hello');
-    t.is(fs.statSync(existingLambdaLocal).size, 5);
-
-    // message adapter is updated, a new lambda zip file is generated
-    const adapterHashOrigin = Lambda.messageAdapterZipFileHash;
-    Lambda.messageAdapterZipFileHash = `${adapterHashOrigin}123`;
-
-    const l = new Lambda(t.context.config)
-    await l.zipLambda(l.buildS3Path(t.context.lambda));
-    t.truthy(fs.statSync(t.context.lambda.local));
-    t.true(fs.statSync(t.context.lambda.local).size > 5);
-    t.not(t.context.lambda.local, existingLambdaLocal);
-
-    t.is(
-      path.basename(t.context.lambda.local),
-      `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
-    );
-    t.is(
-      path.basename(t.context.lambda.remote),
-      `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
-    );
-  }
-);
+test.serial('zipLambda: given an invalid zip file generated from a previous run, a new valid lambda file is generated', async (t) => { // eslint-disable-line max-len
+  t.context.lambda.useMessageAdapter = true;
+  const lambdaLocalOrigin = t.context.lambda.local;
+  const lambdaRemoteOrigin = t.context.lambda.remote;
+
+  // put an empty lambda zip file there as the result of the previous run
+  const existingLambdaLocal = path.join(
+    path.dirname(t.context.lambda.local),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
+  );
+
+  await fs.writeFile(existingLambdaLocal, 'hello');
+  t.is(fs.statSync(existingLambdaLocal).size, 5);
+
+  const testLambda = new Lambda(t.context.config);
+  await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
+  t.truthy(fs.statSync(t.context.lambda.local));
+  t.true(fs.statSync(t.context.lambda.local).size > 5);
+  t.is(t.context.lambda.local, existingLambdaLocal);
+
+  t.is(
+    path.basename(t.context.lambda.local),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
+  );
+  t.is(
+    path.basename(t.context.lambda.remote),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
+  );
+});
+
+
+test.serial('zipLambda: for lambda using message adapter, no new file is generated if the task and message adapter are not updated', async (t) => { // eslint-disable-line max-len
+  t.context.lambda.useMessageAdapter = true;
+
+  // put a lambda zip file there as the result of the previous run
+  const existingLambdaLocal = path.join(
+    path.dirname(t.context.lambda.local),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
+  );
+
+  const existingLambdaRemote = path.join(
+    path.dirname(t.context.lambda.remote),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}`
+  );
+
+  await fs.copy(zipFixturePath, existingLambdaLocal);
+  t.is(fs.statSync(existingLambdaLocal).size, zipFixtureSize);
+
+  const testLambda = new Lambda(t.context.config);
+  await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
+  t.truthy(fs.statSync(t.context.lambda.local));
+  t.is(fs.statSync(t.context.lambda.local).size, zipFixtureSize);
+  t.is(t.context.lambda.local, existingLambdaLocal);
+  t.is(t.context.lambda.remote, existingLambdaRemote);
+});
+
+test.serial('zipLambda: for lambda using message adapter, a new file is created if the message adapter is updated', async (t) => { // eslint-disable-line max-len
+  t.context.lambda.useMessageAdapter = true;
+  const lambdaLocalOrigin = t.context.lambda.local;
+  const lambdaRemoteOrigin = t.context.lambda.remote;
+
+  // put an empty lambda zip file there as the result of the previous run
+  const existingLambdaLocal = path.join(
+    path.dirname(t.context.lambda.local),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
+  );
+
+  await fs.writeFile(existingLambdaLocal, 'hello');
+  t.is(fs.statSync(existingLambdaLocal).size, 5);
+
+  // message adapter is updated, a new lambda zip file is generated
+  const adapterHashOrigin = Lambda.messageAdapterZipFileHash;
+  Lambda.messageAdapterZipFileHash = `${adapterHashOrigin}123`;
+
+  const testLambda = new Lambda(t.context.config);
+  await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
+  t.truthy(fs.statSync(t.context.lambda.local));
+  t.true(fs.statSync(t.context.lambda.local).size > 5);
+  t.not(t.context.lambda.local, existingLambdaLocal);
+
+  t.is(
+    path.basename(t.context.lambda.local),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
+  );
+  t.is(
+    path.basename(t.context.lambda.remote),
+    `${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
+  );
+});