chore: prevent empty zip uploads

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),
+    )
+  );
 }
 
 

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) {

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();