Cumulus 748 #409

Jkovarik · 2018-06-29T15:14:04Z

Summary: Resolves deployment issue where truncated lambda .zip files were being pushed out to AWS

Addresses [CUMULUS-748]

Changes

Adds validation function/check to ensure invalid lambda zip archives aren't considered valid
by filename hash alone

Test Plan

Things that should succeed before merging.

Unit tests
Adhoc testing
Update CHANGELOG
Run ./bin/eslint-ratchet and verify that eslint errors have not increased
Commit .eslint-ratchet-high-water-mark if the score has improved

Release PR

If this is a release PR, make sure you branch name starts with release- prefix, e.g. release-v-1.1.2.

Reviewers: @tag-your-friends

We need to be sure there isn't an invalid package file left over from a previous aborted kes run - as the hash is generated and written as the filestream is opened in kes.utils.zip, and kes is capable of async calling process.exit, it's possible to have truncated/0 byte zip files. This validator ensures that the file is a legitimate zip archive in addition to a matching hash filename.

This is a fairly large number, however that seems to also be the case on master

Jkovarik · 2018-06-29T15:57:19Z

Ratchet updates were incredibly large given the size of this PR, however the difference is also in master....

laurenfrederick · 2018-07-02T11:03:57Z

packages/deployment/lib/lambda.js

+        return Promise.resolve(lambda);
+      }
+      catch (e) {
+        console.log(`${lambda.local} appears to be an invalid zip file, and will be re-built`);


Do we actually retry it if it's invalid?

@laurenfrederick yes, the original code was skipping regeneration on L69, if the validator throws an error it logs and continues instead.

ifestus · 2018-07-02T17:35:25Z

packages/deployment/test/test_lambda.js

-    fs.writeFileSync(existingLambdaLocal, 'hello');
-    t.is(fs.statSync(existingLambdaLocal).size, 5);
+    fs.copySync(`${t.context.fixturedir}/zipfile-fixture.zip`, existingLambdaLocal);
+    t.is(fs.statSync(existingLambdaLocal).size, 180);


Since the 180 is the size of zipfile-fixture.zip, would it make sense to do a

let origSize = fs.statSync(`${t.context.fixturedir}`/zipfile-fixture.zip).size;

(or something)? While it does add another line, it would make the test a bit more resilient to changes to the zip file.

@ifestus that's .... probably preferable for a fixture like this, as the contents are largely irrelevant, and it eliminates the magic-numbery feeling. 👍

marchuffnagle · 2018-07-02T17:46:23Z

packages/deployment/lib/lambda.js

+  validateZipFile(lambda) {
+    return new Promise((resolve, reject) => {
+      yauzl.open(lambda.local, (err, _zipfile) => {
+        if (err) {


This should be:

if (err) return reject(err); return resolve();

If you don't return the rejection, it will continue on in the function and call resolve() as well. Speaking from experience, that's an annoying bug to track down.

On second thought, if you add const util = require('util'); to the top of this file, this entire function can be rewritten as:

return (util.promisify(yauzl.open))(lambda.local);

@yjpa7145 Nice, I like that far better. TIL. Thanks!

marchuffnagle · 2018-07-02T18:29:14Z

packages/deployment/lib/lambda.js

    if (lambda.useMessageAdapter) {
      const kesFolder = path.join(this.config.kesFolder, 'build', 'adapter');
      fileList.push(kesFolder);
      msg += ' and injecting message adapter';
    }

    console.log(`${msg} for ${lambda.name}`);
-
-    return utils.zip(lambda.local, fileList).then(() => lambda);
+    return utils.zip(lambda.local, fileList).then(() => lambda).catch((e) => console.log(`Error zipping ${e}`));


As discussed in chat, please put the .then() and .catch() statements on their own lines, and re-throw the exception in the catch block after logging it.

marchuffnagle · 2018-07-02T18:29:43Z

packages/deployment/test/test_lambda.js

@@ -62,7 +62,7 @@ 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)
+  const l = new Lambda(t.context.config);


Could this be given a more descriptive variable name?

Hmm, I don't think it's possible to have a less descriptive variable name. How about testLambda.

marchuffnagle · 2018-07-02T18:30:21Z

packages/deployment/test/test_lambda.js

@@ -87,7 +87,42 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
 });

 test.serial(
-  `zipLambda: for lambda using message adapter, no new file is generated 
+  `zipLambda: given an invalid zip file generated from a previous run,


If string interpolation is not being performed, use single quotes instead of backticks.

There's no interpolation, however it's a multi-line string due to line length. Do we have a project-standard (aside eslint) when it comes to that?

For the case of long, single-line strings, the project convention is to use single quotes. If eslint complains about the length of the line, add // eslint-disable-line max-len to the end of the line.

marchuffnagle · 2018-07-02T18:32:02Z

packages/deployment/test/test_lambda.js

+      `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
+    );
+
+    fs.writeFileSync(existingLambdaLocal, 'hello');


Instead of fs.writeFileSync, look at using the fs-extra module and await fs.writeFile.

@yjpa7145 out of curiosity, what is the benefit of doing an await fs.writeFile over fs.writeFileSync when on the next line the result of writeFile is being referenced?

oh, I think I see. If the next line is also async, then everything makes sense.

@ifestus If you use the Sync call, the entire process stops until that writeFile operation finishes. That means that any other background operations (any asynchronous code running in the process) will hang until this write operation finishes. If the async version is used then those other asynchronous operations can continue while this write is being performed.

marchuffnagle · 2018-07-02T18:32:30Z

packages/deployment/test/test_lambda.js

+    );
+
+    fs.writeFileSync(existingLambdaLocal, 'hello');
+    t.is(fs.statSync(existingLambdaLocal).size, 5);


Same comment as above, consider using the Promise-returning version from fs-extra.

marchuffnagle · 2018-07-02T18:33:00Z

packages/deployment/test/test_lambda.js

@@ -87,7 +87,42 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
 });

 test.serial(


Is there a reason that these tests can't be run in parallel?

Hmm. Probably not, the setup function tempdir should be unique, it doesn't appear anything here should collide, assuming I'm not missing anything in the underlying dependencies.... why was it implemented that way initially 🤔

Running them in parallel results in several failures. I can dig into that further and put up another PR if appropriate, or address it here if it feels like it should be sorted in this PR.

Sounds like it's a defensive thing due to localstack constraints as much as anything - see #416

marchuffnagle · 2018-07-02T18:33:26Z

packages/deployment/test/test_lambda.js

+
+
+test.serial(
+  `zipLambda: for lambda using message adapter, no new file is generated


Same comment about using single quotes instead of backticks.

marchuffnagle · 2018-07-02T18:33:47Z

packages/deployment/test/test_lambda.js

@@ -103,20 +138,20 @@ test.serial(
      `${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}`
    );

-    fs.writeFileSync(existingLambdaLocal, 'hello');
-    t.is(fs.statSync(existingLambdaLocal).size, 5);
+    fs.copySync(`${t.context.fixturedir}/zipfile-fixture.zip`, existingLambdaLocal);


Same comment about using fs-extra and not Sync version.

marchuffnagle · 2018-07-02T18:35:14Z

packages/deployment/test/test_lambda.js


-    const l = new Lambda(t.context.config)
+    const l = new Lambda(t.context.config);


A more descriptive variable name would be helpful.

marchuffnagle

Looks good overall, a few requested changes.

scisco

I don't have anything to add on top of Marc said. Great work!

Jkovarik · 2018-07-03T05:41:49Z

@yjpa7145 @ifestus I believe 7fe3b38 should resolve your comments, aside the comment about back-ticks. I'm happy to go with whatever convention is, if a change is needed. Thanks for the review!

ifestus

Looks good to me. It looks like the changes proposed in previous reviews have been addressed.

marchuffnagle · 2018-07-05T13:47:13Z

packages/deployment/test/test_lambda.js

 const fs = require('fs-extra');
+const os = require('os');


👍 Alphabetical

marchuffnagle

I created a PR of my own against your CUMULUS-748 PR with the last changes that I recommend. I’m going to mark your PR as approved, your discretion as to whether you want to pull my PR into yours first.

My PR - #422

Marc's final feedback on CUMULUS-748

Cumulus 748

Jkovarik added 5 commits June 29, 2018 08:40

Update CHANGELOG

b45a71c

Add test fixture

fb25317

Fix CHANGELOG

5000f71

Updating ratchet

aa3fa57

This is a fairly large number, however that seems to also be the case on master

laurenfrederick reviewed Jul 2, 2018

View reviewed changes

laurenfrederick requested a review from scisco July 2, 2018 11:04

ifestus self-assigned this Jul 2, 2018

scisco self-assigned this Jul 2, 2018

ifestus reviewed Jul 2, 2018

View reviewed changes

marchuffnagle reviewed Jul 2, 2018

View reviewed changes

marchuffnagle self-assigned this Jul 2, 2018

marchuffnagle reviewed Jul 2, 2018

View reviewed changes

marchuffnagle suggested changes Jul 2, 2018

View reviewed changes

scisco reviewed Jul 2, 2018

View reviewed changes

scisco removed their assignment Jul 2, 2018

scisco requested review from scisco and removed request for scisco July 2, 2018 22:41

Jkovarik added 2 commits July 2, 2018 22:40

Address PR commentary -

7fe3b38

re-ratchet post PR changes

56b83fc

Jkovarik added 2 commits July 3, 2018 08:58

Merge branch 'master' into CUMULUS-748

97f2c7d

Add uncommited hunk/update from PR changes

dd789e0

ifestus approved these changes Jul 3, 2018

View reviewed changes

Alireza and others added 3 commits July 3, 2018 13:10

Merge branch 'master' into CUMULUS-748

c9fb6f1

Merge branch 'master' into CUMULUS-748

d6cd99a

Merge branch 'master' into CUMULUS-748

49a48ea

marchuffnagle reviewed Jul 5, 2018

View reviewed changes

packages/deployment/test/test_lambda.js

const fs = require('fs-extra');

const os = require('os');

Copy link

Contributor

marchuffnagle Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Alphabetical

Marc Huffnagle and others added 2 commits July 5, 2018 09:52

CUMULUS-748 PR feedback changes

c77310d

Fix typo

bb3973b

marchuffnagle approved these changes Jul 5, 2018

View reviewed changes

Merge pull request #422 from cumulus-nasa/CUMULUS-748-marc

3467547

Marc's final feedback on CUMULUS-748

Jkovarik merged commit 007801e into master Jul 5, 2018

Jkovarik deleted the CUMULUS-748 branch July 5, 2018 15:28

jasonduley unassigned marchuffnagle and ifestus Jul 18, 2018

laurenfrederick pushed a commit that referenced this pull request Sep 24, 2018

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

46b651a

Cumulus 748

botanical mentioned this pull request May 20, 2024

[Snyk] Upgrade tough-cookie from 4.0.0 to 4.1.4 #3661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cumulus 748 #409

Cumulus 748 #409

Jkovarik commented Jun 29, 2018 •

edited

Loading

Jkovarik commented Jun 29, 2018

laurenfrederick Jul 2, 2018

Jkovarik Jul 2, 2018

scisco Jul 2, 2018

ifestus Jul 2, 2018 •

edited

Loading

Jkovarik Jul 2, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

Jkovarik Jul 2, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

Jkovarik Jul 2, 2018 •

edited

Loading

marchuffnagle Jul 2, 2018

Jkovarik Jul 2, 2018 •

edited

Loading

marchuffnagle Jul 5, 2018

marchuffnagle Jul 2, 2018

ifestus Jul 3, 2018

ifestus Jul 3, 2018

marchuffnagle Jul 5, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

Jkovarik Jul 2, 2018

Jkovarik Jul 2, 2018

Jkovarik Jul 3, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

marchuffnagle Jul 2, 2018

marchuffnagle left a comment

scisco left a comment

Jkovarik commented Jul 3, 2018

ifestus left a comment

marchuffnagle Jul 5, 2018

marchuffnagle left a comment •

edited

Loading

		@@ -87,7 +87,42 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
		});

		test.serial(



		test.serial(
		`zipLambda: for lambda using message adapter, no new file is generated


		const l = new Lambda(t.context.config)
		const l = new Lambda(t.context.config);

Cumulus 748 #409

Cumulus 748 #409

Conversation

Jkovarik commented Jun 29, 2018 • edited Loading

Changes

Test Plan

Release PR

Jkovarik commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifestus Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jkovarik Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jkovarik Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchuffnagle left a comment

Choose a reason for hiding this comment

scisco left a comment

Choose a reason for hiding this comment

Jkovarik commented Jul 3, 2018

ifestus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchuffnagle left a comment • edited Loading

Choose a reason for hiding this comment

Jkovarik commented Jun 29, 2018 •

edited

Loading

ifestus Jul 2, 2018 •

edited

Loading

Jkovarik Jul 2, 2018 •

edited

Loading

Jkovarik Jul 2, 2018 •

edited

Loading

marchuffnagle left a comment •

edited

Loading