diff --git a/packages/cdk-assets/lib/private/handlers/files.ts b/packages/cdk-assets/lib/private/handlers/files.ts index 7433d7d9be517..cf77eda43dddc 100644 --- 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 index 46b7f31a3ecc7..7aef7fbe9d9f6 100644 --- 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(); public readonly messages = new Array(); constructor(private readonly doAbort = false) { diff --git a/packages/cdk-assets/test/files.test.ts b/packages/cdk-assets/test/files.test.ts index 7616482d43944..9ef52ef8a21a8 100644 --- 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; beforeEach(() => { jest.resetAllMocks(); @@ -21,19 +28,12 @@ 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: { @@ -41,14 +41,7 @@ beforeEach(() => { 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,6 +113,7 @@ 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({ @@ -134,8 +121,21 @@ test('Do nothing if file already exists', async () => { 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();