Skip to content

Commit

Permalink
fix: half-written asset zips can be uploaded if process is interrupted (
Browse files Browse the repository at this point in the history
aws#22393)

Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip":

- Bundling starts
- A partial zip file is written
- The process is interrupted (`^C` or similar) while writing.
- On next run, the destination file already exists, is taken to be complete, and uploaded.

In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario.

Attempts to fix aws#18459.

I don't have tests for this, I don't know how to write them properly. To my mind, if the code looks good and we pass integration tests, that should be sufficient. Add a global asset salting mechanism so that we know all asset hashes are always unique.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored and arewa committed Oct 8, 2022
1 parent 863945b commit 2a3cfcb
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 9 deletions.
10 changes: 9 additions & 1 deletion packages/@aws-cdk/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { Stage } from './stage';

const ARCHIVE_EXTENSIONS = ['.tar.gz', '.zip', '.jar', '.tar', '.tgz'];

const ASSET_SALT_CONTEXT_KEY = '@aws-cdk/core:assetHashSalt';

/**
* A previously staged asset
*/
Expand Down Expand Up @@ -160,8 +162,13 @@ export class AssetStaging extends Construct {
constructor(scope: Construct, id: string, props: AssetStagingProps) {
super(scope, id);

const salt = this.node.tryGetContext(ASSET_SALT_CONTEXT_KEY);

this.sourcePath = path.resolve(props.sourcePath);
this.fingerprintOptions = props;
this.fingerprintOptions = {
...props,
extraHash: props.extraHash || salt ? `${props.extraHash ?? ''}${salt ?? ''}` : undefined,
};

if (!fs.existsSync(this.sourcePath)) {
throw new Error(`Cannot find asset at ${this.sourcePath}`);
Expand Down Expand Up @@ -329,6 +336,7 @@ export class AssetStaging extends Construct {

// Calculate assetHash afterwards if we still must
assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundledAsset.path);

const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension));

this.stageAsset(bundledAsset.path, stagedPath, 'move');
Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/core/test/staging.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,24 @@ describe('staging', () => {
expect(withExtra.assetHash).toEqual('c95c915a5722bb9019e2c725d11868e5a619b55f36172f76bcbcaa8bb2d10c5f');
});

test('can specify extra asset salt via context key', () => {
// GIVEN
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

const app = new App();
const stack = new Stack(app, 'stack');

const saltedApp = new App({ context: { '@aws-cdk/core:assetHashSalt': 'magic' } });
const saltedStack = new Stack(saltedApp, 'stack');

// WHEN
const asset = new AssetStaging(stack, 'X', { sourcePath: directory });
const saltedAsset = new AssetStaging(saltedStack, 'X', { sourcePath: directory });

// THEN
expect(asset.assetHash).not.toEqual(saltedAsset.assetHash);
});

test('with bundling', () => {
// GIVEN
const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
Expand Down
6 changes: 5 additions & 1 deletion packages/aws-cdk/test/integ/cli/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,11 @@ class BuiltinLambdaStack extends cdk.Stack {
}
}

const app = new cdk.App();
const app = new cdk.App({
context: {
'@aws-cdk/core:assetHashSalt': `${Date.now()}`, // Force all assets to be unique
},
});

const defaultEnv = {
account: process.env.CDK_DEFAULT_ACCOUNT,
Expand Down
54 changes: 52 additions & 2 deletions packages/cdk-assets/lib/private/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,17 @@ import * as glob from 'glob';
// eslint-disable-next-line @typescript-eslint/no-require-imports
const archiver = require('archiver');

export function zipDirectory(directory: string, outputFile: string): Promise<void> {
type Logger = (x: string) => void;

export async function zipDirectory(directory: string, outputFile: string, logger: Logger): Promise<void> {
// We write to a temporary file and rename at the last moment. This is so that if we are
// interrupted during this process, we don't leave a half-finished file in the target location.
const temporaryOutputFile = `${outputFile}._tmp`;
await writeZipFile(directory, temporaryOutputFile);
await moveIntoPlace(temporaryOutputFile, outputFile, logger);
}

function writeZipFile(directory: string, outputFile: string): Promise<void> {
return new Promise(async (ok, fail) => {
// The below options are needed to support following symlinks when building zip files:
// - nodir: This will prevent symlinks themselves from being copied into the zip.
Expand Down Expand Up @@ -44,6 +54,46 @@ export function zipDirectory(directory: string, outputFile: string): Promise<voi
}

await archive.finalize();

});
}

/**
* Rename the file to the target location, taking into account that we may see EPERM on Windows
* while an Antivirus scanner still has the file open, so retry a couple of times.
*/
async function moveIntoPlace(source: string, target: string, logger: Logger) {
let delay = 100;
let attempts = 5;
while (true) {
try {
if (await pathExists(target)) {
await fs.unlink(target);
}
await fs.rename(source, target);
return;
} catch (e) {
if (e.code !== 'EPERM' || attempts-- <= 0) {
throw e;
}
logger(e.message);
await sleep(Math.floor(Math.random() * delay));
delay *= 2;
}
}
}

function sleep(ms: number) {
return new Promise(ok => setTimeout(ok, ms));
}

async function pathExists(x: string) {
try {
await fs.stat(x);
return true;
} catch (e) {
if (e.code === 'ENOENT') {
return false;
}
throw e;
}
}
2 changes: 1 addition & 1 deletion packages/cdk-assets/lib/private/handlers/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ export class FileAssetHandler implements IAssetHandler {
}

this.host.emitMessage(EventType.BUILD, `Zip ${fullPath} -> ${packagedPath}`);
await zipDirectory(fullPath, packagedPath);
await zipDirectory(fullPath, packagedPath, (m) => this.host.emitMessage(EventType.DEBUG, m));
return { packagedPath, contentType };
} else {
const contentType = mime.getType(fullPath) ?? 'application/octet-stream';
Expand Down
13 changes: 9 additions & 4 deletions packages/cdk-assets/test/archive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,18 @@ import { rmRfSync } from '../lib/private/fs-extra';
const exec = promisify(_exec);
const pathExists = promisify(exists);

function logger(x: string) {
// eslint-disable-next-line no-console
console.log(x);
}

test('zipDirectory can take a directory and produce a zip from it', async () => {
const stagingDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive'));
const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive.extract'));
try {
const zipFile = path.join(stagingDir, 'output.zip');
const originalDir = path.join(__dirname, 'test-archive');
await zipDirectory(originalDir, zipFile);
await zipDirectory(originalDir, zipFile, logger);

// unzip and verify that the resulting tree is the same
await exec(`unzip ${zipFile}`, { cwd: extractDir });
Expand Down Expand Up @@ -46,9 +51,9 @@ test('md5 hash of a zip stays consistent across invocations', async () => {
const zipFile1 = path.join(stagingDir, 'output.zip');
const zipFile2 = path.join(stagingDir, 'output.zip');
const originalDir = path.join(__dirname, 'test-archive');
await zipDirectory(originalDir, zipFile1);
await zipDirectory(originalDir, zipFile1, logger);
await new Promise(ok => setTimeout(ok, 2000)); // wait 2s
await zipDirectory(originalDir, zipFile2);
await zipDirectory(originalDir, zipFile2, logger);

const hash1 = contentHash(await fs.readFile(zipFile1));
const hash2 = contentHash(await fs.readFile(zipFile2));
Expand All @@ -75,7 +80,7 @@ test('zipDirectory follows symlinks', async () => {
const originalDir = path.join(__dirname, 'test-archive-follow', 'data');
const zipFile = path.join(stagingDir, 'output.zip');

await expect(zipDirectory(originalDir, zipFile)).resolves.toBeUndefined();
await expect(zipDirectory(originalDir, zipFile, logger)).resolves.toBeUndefined();
await expect(exec(`unzip ${zipFile}`, { cwd: extractDir })).resolves.toBeDefined();
await expect(exec(`diff -bur ${originalDir} ${extractDir}`)).resolves.toBeDefined();
} finally {
Expand Down

0 comments on commit 2a3cfcb

Please sign in to comment.