From a3ba43ed302606aeffd78bb6c2ece23bfe2f3e71 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 18 Jan 2022 14:03:04 +0100 Subject: [PATCH] chore: prevent empty zip uploads 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. --- .../cdk-assets/lib/private/handlers/files.ts | 51 +++++++++- packages/cdk-assets/test/fake-listener.ts | 1 + packages/cdk-assets/test/files.test.ts | 98 +++++++++++-------- 3 files changed, 104 insertions(+), 46 deletions(-) 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();