feat(toolkit): ensure consistent zip files #2931

jogold · 2019-06-19T14:59:32Z

Zip files were not consistent across deploys resulting in unnecessary S3 uploads and stack updates.

Ensure consistency by appending files in series (guarantees file ordering in the zip) and reseting
dates (guarantees same hash for same content).

Closes #1997, Closes #2759

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Zip files were not consistent across deploys resulting in unnecessary S3 uploads and stack updates. Ensure consistency by appending files in series (guarantees file ordering in the zip) and reseting dates (guarantees same hash for same content). Closes aws#1997, Closes aws#2759

jogold · 2019-06-19T15:01:10Z

Not 100% sure how to correctly unit test this... but I can confirm that I finally have the (no changes) message all the time from the cli across deploys for stacks containing Lambda functions.

jogold · 2019-06-19T15:12:01Z

Got inspiration from here https://github.com/serverless/serverless/blob/d69d57ea0976544ced0714104fb8774d41d72089/lib/plugins/package/lib/zipService.js

rix0rrr

Thanks for taking this on!

rix0rrr · 2019-06-20T08:35:02Z

packages/aws-cdk/lib/archive.ts

+    output.on('open', async () => {
+      archive.pipe(output);
+
+      const contents = await Promise.all(files.map(async (file) => {


This will load all files into memory concurrently. Not sure I like that for very big zips (tens to hundreds of megabytes).

Also, don't see the use of doing this in parallel since this task should be I/O bound, as far as I can estimate.

Can you test and verify whether the speed makes a difference? If it does, then I would prefer some kind of batching (chop the list into batches of 10 and do those in parallel) to keep memory consumption under control.

Understand, will have a look at it

jogold · 2019-06-20T08:44:44Z

packages/aws-cdk/tsconfig.json

@@ -2,7 +2,7 @@
    "compilerOptions": {
        "target":"ES2018",
        "module": "commonjs",
-        "lib": ["es2016", "es2017.object", "es2017.string"],
+        "lib": ["es2016", "es2017.object", "es2017.string", "dom"],


This is needed for @types/jszip

jogold · 2019-06-20T13:28:40Z

packages/aws-cdk/lib/archive.ts

+      const stream = fs.createReadStream(path.join(directory, file));
+      archive.append(stream, {
+        name: file,
+        date: new Date('1980-01-01T00:00:00.000Z'), // reset dates to get the same hash for the same content


on my system specifying new Date(0) here seems to be translated to 1980-01-01T00:00:00.000Z for the files in the zip... this means that I cannot write a unit test for the date reset => setting to 1980-01-01T00:00:00.000Z

rix0rrr · 2019-06-20T14:30:18Z

packages/aws-cdk/lib/archive.ts

+
+    files.forEach(file => { // Append files serially to ensure file order
+      const stream = fs.createReadStream(path.join(directory, file));
+      archive.append(stream, {


Aren't NodeJS streams async? Are we 100% positive that after this statement, the stream has been fully read and added? Or are we sure that .finalize() will synchronously read and flush all streams? Do we need to await finalize()?

It concerns me that I don't understand the API fully, and the documentation doesn't seem to be very explicit about it.

From the example in the doc: finalize the archive (ie we are done appending files but streams have to finish yet)

From the doc: Finalizes the instance and prevents further appending to the archive structure (queue will continue til drained).

I've successfully created zips containing multiple files of more than 50MB with this code (resulting in zip files of several hundreds of MB).

~~Looks like finalize() is a blocking statement.~~

zipDirectory returns a promise that is resolved only once we get the close event on output, this is fired only when archiver has been finalized and the output file descriptor has closed (output.once('close', () => ok());)

It's also possible to call archive.file() and let the library handle the read stream creation (need to check if the sort order is preserved across zips in this case).

Tested archive.file() and file order is not preserved across zips. If think that the only way to go here is to use archive.append() with fs.createReadStream().

Cool, yes you are right, I had missed this:

output.once('close', () => ok());

I'm just very scared of accidental asyncness where you're not expecting it :(

Thanks, will merge when the validation build finishes.

jogold requested a review from a team as a code owner June 19, 2019 14:59

simplify

cfaa40d

rix0rrr reviewed Jun 20, 2019

View reviewed changes

rix0rrr self-assigned this Jun 20, 2019

jogold added 3 commits June 20, 2019 10:41

test that dates are reset

8e6ce58

equal

379d246

Merge branch 'master' into consistent-zip-assets

4df0e13

jogold commented Jun 20, 2019

View reviewed changes

better with streams?

a8c12f6

jogold commented Jun 20, 2019

View reviewed changes

rix0rrr reviewed Jun 20, 2019

View reviewed changes

add comment

491b4c8

rix0rrr approved these changes Jun 21, 2019

View reviewed changes

rix0rrr merged commit 9101161 into aws:master Jun 21, 2019

jogold deleted the consistent-zip-assets branch June 30, 2019 16:31

eladb mentioned this pull request Jul 1, 2019

Deployment crashing while zipping assets #3145

Closed

1 task

aereal mentioned this pull request Jul 24, 2019

fix(toolkit): preserve asset's files mode #3413

Closed

4 tasks

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Dreamescaper mentioned this pull request Dec 2, 2021

'package' command should be able to create deterministic zip archives aws/aws-extensions-for-dotnet-cli#195

Open

NOUIY mentioned this pull request Oct 2, 2024

[Snyk] Upgrade @aws-cdk/aws-apigateway from 0.0.0 to 0.39.0 NOUIY/aws-solutions-constructs#132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(toolkit): ensure consistent zip files #2931

feat(toolkit): ensure consistent zip files #2931

jogold commented Jun 19, 2019 •

edited

Loading

jogold commented Jun 19, 2019 •

edited

Loading

jogold commented Jun 19, 2019

rix0rrr left a comment

rix0rrr Jun 20, 2019

jogold Jun 20, 2019

jogold Jun 20, 2019

jogold Jun 20, 2019 •

edited

Loading

rix0rrr Jun 20, 2019

jogold Jun 20, 2019 •

edited

Loading

jogold Jun 20, 2019

jogold Jun 21, 2019 •

edited

Loading

rix0rrr Jun 21, 2019

feat(toolkit): ensure consistent zip files #2931

feat(toolkit): ensure consistent zip files #2931

Conversation

jogold commented Jun 19, 2019 • edited Loading

Pull Request Checklist

jogold commented Jun 19, 2019 • edited Loading

jogold commented Jun 19, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Jun 20, 2019

Choose a reason for hiding this comment

jogold Jun 20, 2019

Choose a reason for hiding this comment

jogold Jun 20, 2019

Choose a reason for hiding this comment

jogold Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

rix0rrr Jun 20, 2019

Choose a reason for hiding this comment

jogold Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

jogold Jun 20, 2019

Choose a reason for hiding this comment

jogold Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

rix0rrr Jun 21, 2019

Choose a reason for hiding this comment

jogold commented Jun 19, 2019 •

edited

Loading

jogold commented Jun 19, 2019 •

edited

Loading

jogold Jun 20, 2019 •

edited

Loading

jogold Jun 20, 2019 •

edited

Loading

jogold Jun 21, 2019 •

edited

Loading