Skip to content

Commit

Permalink
fix(ecr-assets): unable to use one Dockerfile to build multiple images (
Browse files Browse the repository at this point in the history
#5705)

* fix(ecr-assets): unable to use one Dockerfile to build multiple images

Since we use the source hash as the image ID, if we use different docker build options such as specifying a different `target`, `buildArgs` or a custom docker file name, we still get the same image ID. This means that if this image ID already exists, we skip the build. This makes it impossible to use the same docker build context to build multiple images.

This fix exposes the ability to include extra information to the source hash fingerprint algorithm (in `assets.Staging`), and utilizes this capability to "salt" the hash with build arguments if they are provided.

Fixes #5683

* code review feedback

- rename `extra` to `extraHash` so it makes more sense when extending `FingerprintOptions`.
- build `extraHash` as an object instead of an array.
- include `repositoryName` in fingerprint

* Update fingerprint.ts

* Update copy.ts

* fix build

* add test for repository name

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
Elad Ben-Israel and mergify[bot] committed Jan 8, 2020
1 parent b3e7978 commit ff3f27f
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 21 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/assets/lib/fs/copy.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as fs from 'fs';
import * as path from 'path';
import { CopyOptions } from './copy-options';
import { FollowMode } from './follow-mode';
import { CopyOptions } from './options';
import { shouldExclude, shouldFollow } from './utils';

export function copyDirectory(srcDir: string, destDir: string, options: CopyOptions = { }, rootDir?: string) {
Expand Down
12 changes: 2 additions & 10 deletions packages/@aws-cdk/assets/lib/fs/fingerprint.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,15 @@
import * as crypto from 'crypto';
import * as fs from 'fs';
import * as path from 'path';
import { CopyOptions } from './copy-options';
import { FollowMode } from './follow-mode';
import { FingerprintOptions } from './options';
import { shouldExclude, shouldFollow } from './utils';

const BUFFER_SIZE = 8 * 1024;
const CTRL_SOH = '\x01';
const CTRL_SOT = '\x02';
const CTRL_ETX = '\x03';

export interface FingerprintOptions extends CopyOptions {
/**
* Extra information to encode into the fingerprint (e.g. build instructions
* and other inputs)
*/
extra?: string;
}

/**
* Produces fingerprint based on the contents of a single file or an entire directory tree.
*
Expand All @@ -31,7 +23,7 @@ export interface FingerprintOptions extends CopyOptions {
*/
export function fingerprint(fileOrDirectory: string, options: FingerprintOptions = { }) {
const hash = crypto.createHash('sha256');
_hashField(hash, 'options.extra', options.extra || '');
_hashField(hash, 'options.extra', options.extraHash || '');
const follow = options.follow || FollowMode.EXTERNAL;
_hashField(hash, 'options.follow', follow);

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/assets/lib/fs/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from './copy';
export * from './copy-options';
export * from './fingerprint';
export * from './follow-mode';
export * from './options';
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,16 @@ export interface CopyOptions {
*/
readonly exclude?: string[];
}

/**
* Options related to calculating source hash.
*/
export interface FingerprintOptions extends CopyOptions {
/**
* Extra information to encode into the fingerprint (e.g. build instructions
* and other inputs)
*
* @default - hash is only based on source content
*/
readonly extraHash?: string;
}
4 changes: 2 additions & 2 deletions packages/@aws-cdk/assets/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from './fs/copy-options';
export * from './api';
export * from './fs/follow-mode';
export * from './fs/options';
export * from './staging';
export * from './api';
10 changes: 5 additions & 5 deletions packages/@aws-cdk/assets/lib/staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { Construct, ISynthesisSession } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import * as fs from 'fs';
import * as path from 'path';
import { copyDirectory, CopyOptions, fingerprint } from './fs';
import { copyDirectory, fingerprint, FingerprintOptions } from './fs';

export interface StagingProps extends CopyOptions {
export interface StagingProps extends FingerprintOptions {
readonly sourcePath: string;
}

Expand Down Expand Up @@ -46,15 +46,15 @@ export class Staging extends Construct {
*/
public readonly sourceHash: string;

private readonly copyOptions: CopyOptions;
private readonly fingerprintOptions: FingerprintOptions;

private readonly relativePath?: string;

constructor(scope: Construct, id: string, props: StagingProps) {
super(scope, id);

this.sourcePath = props.sourcePath;
this.copyOptions = props;
this.fingerprintOptions = props;
this.sourceHash = fingerprint(this.sourcePath, props);

const stagingDisabled = this.node.tryGetContext(cxapi.DISABLE_ASSET_STAGING_CONTEXT);
Expand Down Expand Up @@ -84,7 +84,7 @@ export class Staging extends Construct {
fs.copyFileSync(this.sourcePath, targetPath);
} else if (stat.isDirectory()) {
fs.mkdirSync(targetPath);
copyDirectory(this.sourcePath, targetPath, this.copyOptions);
copyDirectory(this.sourcePath, targetPath, this.fingerprintOptions);
} else {
throw new Error(`Unknown file type: ${this.sourcePath}`);
}
Expand Down
17 changes: 17 additions & 0 deletions packages/@aws-cdk/assets/test/test.staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,22 @@ export = {
'tree.json',
]);
test.done();
},

'allow specifying extra data to include in the source hash'(test: Test) {
// GIVEN
const app = new App();
const stack = new Stack(app, 'stack');
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const withoutExtra = new Staging(stack, 'withoutExtra', { sourcePath: directory });
const withExtra = new Staging(stack, 'withExtra', { sourcePath: directory, extraHash: 'boom' });

// THEN
test.notEqual(withoutExtra.sourceHash, withExtra.sourceHash);
test.deepEqual(withoutExtra.sourceHash, '2f37f937c51e2c191af66acf9b09f548926008ec68c575bd2ee54b6e997c0e00');
test.deepEqual(withExtra.sourceHash, 'c95c915a5722bb9019e2c725d11868e5a619b55f36172f76bcbcaa8bb2d10c5f');
test.done();
}
};
15 changes: 13 additions & 2 deletions packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as fs from 'fs';
import * as path from 'path';
import { AdoptedRepository } from './adopted-repository';

export interface DockerImageAssetProps extends assets.CopyOptions {
export interface DockerImageAssetProps extends assets.FingerprintOptions {
/**
* The directory where the Dockerfile is stored
*/
Expand Down Expand Up @@ -93,10 +93,21 @@ export class DockerImageAsset extends Construct implements assets.IAsset {
exclude = [...exclude, ...fs.readFileSync(ignore).toString().split('\n').filter(e => !!e)];
}

// include build context in "extra" so it will impact the hash
const extraHash: { [field: string]: any } = { };
if (props.extraHash) { extraHash.user = props.extraHash; }
if (props.buildArgs) { extraHash.buildArgs = props.buildArgs; }
if (props.target) { extraHash.target = props.target; }
if (props.file) { extraHash.file = props.file; }
if (props.repositoryName) { extraHash.repositoryName = props.repositoryName; }

const staging = new assets.Staging(this, 'Staging', {
...props,
exclude,
sourcePath: dir
sourcePath: dir,
extraHash: Object.keys(extraHash).length === 0
? undefined
: JSON.stringify(extraHash)
});

this.sourceHash = staging.sourceHash;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM python:3.6
EXPOSE 8000
WORKDIR /src
ADD . /src
CMD python3 index.py
23 changes: 23 additions & 0 deletions packages/@aws-cdk/aws-ecr-assets/test/test.image-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,29 @@ export = {
repositoryName: token
}), /Cannot use Token as value of 'repositoryName'/);

test.done();
},

'docker build options are included in the asset id'(test: Test) {
// GIVEN
const stack = new Stack();
const directory = path.join(__dirname, 'demo-image-custom-docker-file');

const asset1 = new DockerImageAsset(stack, 'Asset1', { directory });
const asset2 = new DockerImageAsset(stack, 'Asset2', { directory, file: 'Dockerfile.Custom' });
const asset3 = new DockerImageAsset(stack, 'Asset3', { directory, target: 'NonDefaultTarget' });
const asset4 = new DockerImageAsset(stack, 'Asset4', { directory, buildArgs: { opt1: '123', opt2: 'boom' } });
const asset5 = new DockerImageAsset(stack, 'Asset5', { directory, file: 'Dockerfile.Custom', target: 'NonDefaultTarget' });
const asset6 = new DockerImageAsset(stack, 'Asset6', { directory, extraHash: 'random-extra' });
const asset7 = new DockerImageAsset(stack, 'Asset7', { directory, repositoryName: 'foo' });

test.deepEqual(asset1.sourceHash, 'b84a5001da0f5714e484134e2471213d7e987e22ee6219469029f1779370cc2a');
test.deepEqual(asset2.sourceHash, 'c6568a7946e92a408c60278f70834b901638e71237d470ed1e5e6d707c55c0c9');
test.deepEqual(asset3.sourceHash, '963a5329c170c54bc667fddab8d9cc4cec4bffb65ce3a1f323bb5fbc1d268732');
test.deepEqual(asset4.sourceHash, '0e3eb87273509e0f0d45d67d40fa3080566aa22abd7f976e1ce7ea60a8ccd0a8');
test.deepEqual(asset5.sourceHash, 'de0fd4b2bff8c9f180351fd59c6f2e9409fa21366453e1e0b75fedbd93dda1fc');
test.deepEqual(asset6.sourceHash, '00879adf80f97271bf6d7e214b4fac8a043fc6e2661912cbf4d898ccb317d46c');
test.deepEqual(asset7.sourceHash, 'b8abda995e51bd1a47b2705fa40021f3e9619a334bddb96866e808b09303eff7');
test.done();
}
};

0 comments on commit ff3f27f

Please sign in to comment.