chore: prevent empty zip uploads (#18487)

rix0rrr · web-flow · commit 2415a802a4fb · 2022-01-18T21:50:22.000Z
Due to something we haven't completely figured out yet, our asset packaging sometimes produces empty zip files, leading to an error like this uploading code Lambda: ``` Uploaded file must be a non-empty zip ``` Do the following to address this: * If any empty zip files already ended up in S3, do not regard this as a cache hit. Rebuild the asset and upload it again. We do this by checking the item's size: this may be overly pessimistic, but if it is we're probably not wasting a lot of time rebuilding and uploading a tiny file. * If a fresh asset build is producing an empty zip file, loudly complain and ask the user to come to our bug tracker, so we can figure out where these spurious issues are coming from. Relates to #18459. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
diff --git a/packages/cdk-assets/lib/private/handlers/files.ts b/packages/cdk-assets/lib/private/handlers/files.ts
@@ -10,6 +10,13 @@ import { pathExists } from '../fs-extra';
 import { replaceAwsPlaceholders } from '../placeholders';
 import { shell } from '../shell';
 
+/**
+ * The size of an empty zip file is 22 bytes
+ *
+ * Ref: https://en.wikipedia.org/wiki/ZIP_(file_format)
+ */
+const EMPTY_ZIP_FILE_SIZE = 22;
+
 export class FileAssetHandler implements IAssetHandler {
   private readonly fileCacheRoot: string;
 
@@ -74,6 +81,34 @@ export class FileAssetHandler implements IAssetHandler {
     const publishFile = this.asset.source.executable ?
       await this.externalPackageFile(this.asset.source.executable) : await this.packageFile(this.asset.source);
 
+    // Add a validation to catch the cases where we're accidentally producing an empty ZIP file (or worse,
+    // an empty file)
+    if (publishFile.contentType === 'application/zip') {
+      const fileSize = (await fs.stat(publishFile.packagedPath)).size;
+      if (fileSize <= EMPTY_ZIP_FILE_SIZE) {
+        const message = [
+          '🚨 WARNING: EMPTY ZIP FILE 🚨',
+          '',
+          'Zipping this asset produced an empty zip file. We do not know the root cause for this yet, and we need your help tracking it down.',
+          '',
+          'Please visit https://github.com/aws/aws-cdk/issues/18459 and tell us:',
+          'Your OS version, Nodejs version, CLI version, package manager, what the asset is supposed to contain, whether',
+          'or not this error is reproducible, what files are in your cdk.out directory, if you recently changed anything,',
+          'and anything else you think might be relevant.',
+          '',
+          'The deployment will continue, but it may fail. You can try removing the cdk.out directory and running the command',
+          'again; let us know if that resolves it.',
+          '',
+          'If you meant to produce an empty asset file on purpose, you can add an empty dotfile to the asset for now',
+          'to disable this notice.',
+        ];
+
+        for (const line of message) {
+          this.host.emitMessage(EventType.FAIL, line);
+        }
+      }
+    }
+
     this.host.emitMessage(EventType.UPLOAD, `Upload ${s3Url}`);
 
     const params = Object.assign({}, {
@@ -101,11 +136,11 @@ export class FileAssetHandler implements IAssetHandler {
       const packagedPath = path.join(this.fileCacheRoot, `${this.asset.id.assetId}.zip`);
 
       if (await pathExists(packagedPath)) {
-        this.host.emitMessage(EventType.CACHED, `From cache ${path}`);
+        this.host.emitMessage(EventType.CACHED, `From cache ${packagedPath}`);
         return { packagedPath, contentType };
       }
 
-      this.host.emitMessage(EventType.BUILD, `Zip ${fullPath} -> ${path}`);
+      this.host.emitMessage(EventType.BUILD, `Zip ${fullPath} -> ${packagedPath}`);
       await zipDirectory(fullPath, packagedPath);
       return { packagedPath, contentType };
     } else {
@@ -149,9 +184,19 @@ async function objectExists(s3: AWS.S3, bucket: string, key: string) {
    * prefix, and limiting results to 1. Since the list operation returns keys ordered by binary
    * UTF-8 representation, the key we are looking for is guaranteed to always be the first match
    * returned if it exists.
+   *
+   * If the file is too small, we discount it as a cache hit. There is an issue
+   * somewhere that sometimes produces empty zip files, and we would otherwise
+   * never retry building those assets without users having to manually clear
+   * their bucket, which is a bad experience.
    */
   const response = await s3.listObjectsV2({ Bucket: bucket, Prefix: key, MaxKeys: 1 }).promise();
-  return response.Contents != null && response.Contents.some(object => object.Key === key);
+  return (
+    response.Contents != null &&
+    response.Contents.some(
+      (object) => object.Key === key && (object.Size == null || object.Size > EMPTY_ZIP_FILE_SIZE),
+    )
+  );
 }
 
 
diff --git a/packages/cdk-assets/test/fake-listener.ts b/packages/cdk-assets/test/fake-listener.ts
@@ -1,6 +1,7 @@
 import { IPublishProgressListener, EventType, IPublishProgress } from '../lib/progress';
 
 export class FakeListener implements IPublishProgressListener {
+  public readonly types = new Array<EventType>();
   public readonly messages = new Array<string>();
 
   constructor(private readonly doAbort = false) {
diff --git a/packages/cdk-assets/test/files.test.ts b/packages/cdk-assets/test/files.test.ts
@@ -2,13 +2,20 @@ jest.mock('child_process');
 
 import { Manifest } from '@aws-cdk/cloud-assembly-schema';
 import * as mockfs from 'mock-fs';
-import { AssetManifest, AssetPublishing } from '../lib';
+import { AssetPublishing, AssetManifest } from '../lib';
 import { FakeListener } from './fake-listener';
 import { mockAws, mockedApiFailure, mockedApiResult, mockUpload } from './mock-aws';
 import { mockSpawn } from './mock-child_process';
 
 const ABS_PATH = '/simple/cdk.out/some_external_file';
 
+const DEFAULT_DESTINATION = {
+  region: 'us-north-50',
+  assumeRoleArn: 'arn:aws:role',
+  bucketName: 'some_bucket',
+  objectKey: 'some_key',
+};
+
 let aws: ReturnType<typeof mockAws>;
 beforeEach(() => {
   jest.resetAllMocks();
@@ -21,34 +28,20 @@ beforeEach(() => {
           source: {
             path: 'some_file',
           },
-          destinations: {
-            theDestination: {
-              region: 'us-north-50',
-              assumeRoleArn: 'arn:aws:role',
-              bucketName: 'some_bucket',
-              objectKey: 'some_key',
-            },
-          },
+          destinations: { theDestination: DEFAULT_DESTINATION },
         },
       },
     }),
     '/simple/cdk.out/some_file': 'FILE_CONTENTS',
-    [ABS_PATH]: 'FILE_CONTENTS',
+    [ABS_PATH]: 'ZIP_FILE_THAT_IS_DEFINITELY_NOT_EMPTY',
     '/abs/cdk.out/assets.json': JSON.stringify({
       version: Manifest.version(),
       files: {
         theAsset: {
           source: {
             path: '/simple/cdk.out/some_file',
           },
-          destinations: {
-            theDestination: {
-              region: 'us-north-50',
-              assumeRoleArn: 'arn:aws:role',
-              bucketName: 'some_other_bucket',
-              objectKey: 'some_key',
-            },
-          },
+          destinations: { theDestination: { ...DEFAULT_DESTINATION, bucketName: 'some_other_bucket' } },
         },
       },
     }),
@@ -59,14 +52,7 @@ beforeEach(() => {
           source: {
             executable: ['sometool'],
           },
-          destinations: {
-            theDestination: {
-              region: 'us-north-50',
-              assumeRoleArn: 'arn:aws:role',
-              bucketName: 'some_external_bucket',
-              objectKey: 'some_key',
-            },
-          },
+          destinations: { theDestination: { ...DEFAULT_DESTINATION, bucketName: 'some_external_bucket' } },
         },
       },
     }),
@@ -77,32 +63,31 @@ beforeEach(() => {
           source: {
             path: 'plain_text.txt',
           },
-          destinations: {
-            theDestination: {
-              region: 'us-north-50',
-              assumeRoleArn: 'arn:aws:role',
-              bucketName: 'some_bucket',
-              objectKey: 'some_key.txt',
-            },
-          },
+          destinations: { theDestination: { ...DEFAULT_DESTINATION, objectKey: 'some_key.txt' } },
         },
         theImageAsset: {
           source: {
             path: 'image.png',
           },
-          destinations: {
-            theDestination: {
-              region: 'us-north-50',
-              assumeRoleArn: 'arn:aws:role',
-              bucketName: 'some_bucket',
-              objectKey: 'some_key.png',
-            },
-          },
+          destinations: { theDestination: { ...DEFAULT_DESTINATION, objectKey: 'some_key.png' } },
         },
       },
     }),
     '/types/cdk.out/plain_text.txt': 'FILE_CONTENTS',
     '/types/cdk.out/image.png': 'FILE_CONTENTS',
+    '/emptyzip/cdk.out/assets.json': JSON.stringify({
+      version: Manifest.version(),
+      files: {
+        theTextAsset: {
+          source: {
+            path: 'empty_dir',
+            packaging: 'zip',
+          },
+          destinations: { theDestination: DEFAULT_DESTINATION },
+        },
+      },
+    }),
+    '/emptyzip/cdk.out/empty_dir': { }, // Empty directory
   });
 
   aws = mockAws();
@@ -114,6 +99,7 @@ afterEach(() => {
 
 test('pass destination properties to AWS client', async () => {
   const pub = new AssetPublishing(AssetManifest.fromPath('/simple/cdk.out'), { aws, throwOnError: false });
+  aws.mockS3.listObjectsV2 = mockedApiResult({});
 
   await pub.publish();
 
@@ -127,15 +113,29 @@ test('Do nothing if file already exists', async () => {
   const pub = new AssetPublishing(AssetManifest.fromPath('/simple/cdk.out'), { aws });
 
   aws.mockS3.listObjectsV2 = mockedApiResult({ Contents: [{ Key: 'some_key' }] });
+  aws.mockS3.upload = mockUpload();
   await pub.publish();
 
   expect(aws.mockS3.listObjectsV2).toHaveBeenCalledWith(expect.objectContaining({
     Bucket: 'some_bucket',
     Prefix: 'some_key',
     MaxKeys: 1,
   }));
+  expect(aws.mockS3.upload).not.toHaveBeenCalled();
+});
+
+test('tiny file does not count as cache hit', async () => {
+  const pub = new AssetPublishing(AssetManifest.fromPath('/simple/cdk.out'), { aws });
+
+  aws.mockS3.listObjectsV2 = mockedApiResult({ Contents: [{ Key: 'some_key', Size: 5 }] });
+  aws.mockS3.upload = mockUpload();
+
+  await pub.publish();
+
+  expect(aws.mockS3.upload).toHaveBeenCalled();
 });
 
+
 test('upload file if new (list returns other key)', async () => {
   const pub = new AssetPublishing(AssetManifest.fromPath('/simple/cdk.out'), { aws });
 
@@ -310,6 +310,18 @@ test('correctly identify asset path if path is absolute', async () => {
   expect(true).toBeTruthy(); // No exception, satisfy linter
 });
 
+test('empty directory prints failures', async () => {
+  const progressListener = new FakeListener();
+  const pub = new AssetPublishing(AssetManifest.fromPath('/emptyzip/cdk.out'), { aws, progressListener });
+
+  aws.mockS3.listObjectsV2 = mockedApiResult({ Contents: undefined });
+  aws.mockS3.upload = mockUpload(); // Should not be hit
+
+  await pub.publish();
+
+  expect(progressListener.messages).toContainEqual(expect.stringContaining('EMPTY ZIP FILE'));
+});
+
 describe('external assets', () => {
   let pub: AssetPublishing;
   beforeEach(() => {
@@ -330,7 +342,7 @@ describe('external assets', () => {
 
   test('upload external asset correctly', async () => {
     aws.mockS3.listObjectsV2 = mockedApiResult({ Contents: undefined });
-    aws.mockS3.upload = mockUpload('FILE_CONTENTS');
+    aws.mockS3.upload = mockUpload('ZIP_FILE_THAT_IS_DEFINITELY_NOT_EMPTY');
     const expectAllSpawns = mockSpawn({ commandLine: ['sometool'], stdout: ABS_PATH });
 
     await pub.publish();
