(aws-s3-assets): `Asset` produces a corrupt ZIP if target `path` contains zero-byte file

shellscape commented 1 year ago

Describe the bug

I found this bug completely by accident. The scenario is as follows:

Using an Nx generator to create a react application
Nx generator creates an assets directory with a zero-byte .gitkeep file
Using Nx to compile the app with webpack and output an assets directory with contents matching the prior bullet
Using @aws-cdk/aws-amplify-alpha to initiate an Amplify app and deploy the generated app

  const asset = new Asset(scope, 'Appt', { path: distPath });
  const app = new Amplify.App(scope, appName, {
    appName
  });
  const branch = app.addBranch(env, { asset });

  ...

Now, please don't be distracted by the scenario above - it only revealed the issue. In this scenario, Asset will create a zip file which contains the zero-byte file.

Expected Behavior

A zip file created that's compatible with all aspects of of AWS services. Or, alternatively a warning that a ZIP file may not be compatible with some aspects of AWS.

Current Behavior

When the errant zip file is used by CDK for deployment of the amplify app, the subsequent CloudFront deploy task fails with FAILED. The cloudfront logs for the deploy task contain the error: only DEFLATED entries can have EXT descriptor

It took some time to track this down to a zero byte file in the zip file. After some research, this seems to be quite a common problem with JVM apps which use ZipInputStream. Given AWS' proclivity for JVM use, I'd wager that some portion of the Amplify+CloudFront process is using a JVM app behind the scenes.

Reproduction Steps

Please see the description section.

Possible Solution

A workaround is to delete the zero-byte file manually. Or run a post-build or pre-deploy task that sanitizes the target directory.

An ideal solution would include one of or a combo of:

Asset refusing to create a zip that contains a zero-byte file
A warning when Asset is given a directory that contains a zero-byte file

Additional Information/Context

I was able to verify that Amplify+Cloudfront have this same issue if you manually create the zip with the zero-byte file locally, upload it to s3, and then point an existing Amplify app to deploy from that zip file in s3 (directly or with url)

CDK CLI Version

2.70.0

Framework Version

2.70.0

Node.js Version

18

OS

MacOS Ventura

Language

Typescript

Language Version

No response

Other information

No response

pahud commented 1 year ago

Thank you for the report. Sorry I am not familiar with using Nx to create react application. Would you mind sharing your commands step by step that I can follow and reproduce this in my account?

shellscape commented 1 year ago

It looks like you might not have understood the entirety of the issue I reported.

Now, please don't be distracted by the scenario above - it only revealed the issue. In this scenario, Asset will create a zip file which contains the zero-byte file.

I think I very clearly explained that this only revealed the issue, and then I explained what the root cause was.

The code block provided is your reproduction. Assert that distPath contains a zero-byte file. You can create one with the command touch .batman if you're unfamiliar with how to do so.

Of note: This is the second issue of mine that you've responded to in a curious way. I would greatly appreciate a third party perspective to chime in to let me know if I'm not communicating effectively

pahud commented 1 year ago

Hi @shellscape

Thank you for the clarification. I am clear now. In terms of the only DEFLATED entries can have EXT descriptor error, I will reach out to relevant team to discuss this, but I agree with you we probably should add a warning message from CDK. I am making it a feature request as it doesn't seem to be a CDK bug.

shellscape commented 1 year ago

Thanks. Please let me know if additional context is needed.

pahud commented 1 year ago

@shellscape No problem. While I will be reaching to relevant team internally, we welcome pull requests from your valuable feature requests. I am not sure if we really have to refuse the assets bundling with zero-byte file in the Asset but throwing a relevant warning makes perfect sense to me.

aws / aws-cdk