Skip to content

Commit 1418277

Browse files
authored
fix(pipelines): can't have the same asset display name 3 times (#34017)
When introducing friendly asset names in #33844, we made a mistake in calculating unique action names for distinct assets with the same display name. As a result, the same asset could be added to the pipeline twice, but not thrice. The simplest fix would have been to move the increment before the reassignment of `name`: ```ts // Before if (nameCount > 0) { name += `${nameCount + 1}`; } namesCtrs.set(name, nameCount + 1); // After namesCtrs.set(name, nameCount + 1); if (nameCount > 0) { name += `${nameCount + 1}`; } ``` But it occurred to me that that would still lead to problems if there were actions with display names that would conflict with the numbered names in that way (e.g., `['asdf', 'asdf', 'asdf2']`, leading to action names `['asdf', 'asdf2', 'asdf2']`). So what we do instead is linear probing: increment a counter for every name and pick the first name that is unused. Also in this PR: Stages can't have a name of only one character. Fix that. Closes #34004. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 009680d commit 1418277

File tree

7 files changed

+86
-40
lines changed

7 files changed

+86
-40
lines changed

packages/aws-cdk-lib/core/lib/stage.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ export class Stage extends Construct {
163163
constructor(scope: Construct, id: string, props: StageProps = {}) {
164164
super(scope, id);
165165

166-
if (id !== '' && !/^[a-z][a-z0-9\-\_\.]+$/i.test(id)) {
166+
if (id !== '' && !/^[a-z][a-z0-9\-\_\.]*$/i.test(id)) {
167167
throw new Error(`invalid stage name "${id}". Stage name must start with a letter and contain only alphanumeric characters, hypens ('-'), underscores ('_') and periods ('.')`);
168168
}
169169

packages/aws-cdk-lib/core/test/stage.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ describe('stage', () => {
288288
test('stage name validation', () => {
289289
const app = new App();
290290

291+
new Stage(app, 'a');
291292
new Stage(app, 'abcd');
292293
new Stage(app, 'abcd123');
293294
new Stage(app, 'abcd123-588dfjjk');
@@ -298,7 +299,6 @@ describe('stage', () => {
298299
expect(() => new Stage(app, 'abcd123-588dfjjk.sss_ajsid/dfo')).toThrow(/invalid stage name "abcd123-588dfjjk.sss_ajsid\/dfo"/);
299300
expect(() => new Stage(app, '&')).toThrow(/invalid stage name "&"/);
300301
expect(() => new Stage(app, '45hello')).toThrow(/invalid stage name "45hello"/);
301-
expect(() => new Stage(app, 'f')).toThrow(/invalid stage name "f"/);
302302
});
303303

304304
test('outdir cannot be specified for nested stages', () => {

packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts

+10-6
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ export class CodePipeline extends PipelineBase {
529529
const sharedParent = new GraphNodeCollection(flatten(tranches)).commonAncestor();
530530

531531
// If we produce the same action name for some actions, append a unique number
532-
let namesCtrs = new Map<string, number>();
532+
let namesUsed = new Set<string>();
533533

534534
let runOrder = 1;
535535
for (const tranche of tranches) {
@@ -540,12 +540,16 @@ export class CodePipeline extends PipelineBase {
540540
const nodeType = this.nodeTypeFromNode(node);
541541

542542
// Come up with a unique name for this action, incrementing a counter if necessary
543-
let name = actionName(node, sharedParent);
544-
const nameCount = namesCtrs.get(name) ?? 0;
545-
if (nameCount > 0) {
546-
name += `${nameCount + 1}`;
543+
const baseName = actionName(node, sharedParent);
544+
let name = baseName;
545+
for (let ctr = 1; ; ctr++) {
546+
const candidate = ctr > 1 ? `${name}${ctr}` : name;
547+
if (!namesUsed.has(candidate)) {
548+
name = candidate;
549+
break;
550+
}
547551
}
548-
namesCtrs.set(name, nameCount + 1);
552+
namesUsed.add(name);
549553

550554
const variablesNamespace = node.data?.type === 'step'
551555
? namespaceStepOutputs(node.data.step, pipelineStage, name)

packages/aws-cdk-lib/pipelines/test/codepipeline/codepipeline.test.ts

+36-12
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { Stack } from '../../../core';
1010
import * as cxapi from '../../../cx-api';
1111
import * as cdkp from '../../lib';
1212
import { CodePipeline } from '../../lib';
13-
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, TwoStackApp, StageWithStackOutput, TwoFileAssetsApp } from '../testhelpers';
13+
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, TwoStackApp, StageWithStackOutput, MultipleFileAssetsApp } from '../testhelpers';
1414

1515
let app: TestApp;
1616

@@ -438,31 +438,55 @@ test('display name can contain illegal characters which are sanitized for the pi
438438
});
439439
});
440440

441-
test('two assets can have same display name, which are reflected in the pipeline', () => {
441+
test.each([2, 3])('%p assets can have same display name, which are reflected in the pipeline', (n) => {
442442
const pipelineStack = new cdk.Stack(app, 'PipelineStack', { env: PIPELINE_ENV });
443443
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk', {
444444
crossAccountKeys: true,
445445
useChangeSets: false,
446446
});
447-
pipeline.addStage(new TwoFileAssetsApp(pipelineStack, 'App', {
448-
displayName1: 'asdf',
449-
displayName2: 'asdf',
447+
pipeline.addStage(new MultipleFileAssetsApp(pipelineStack, 'App', {
448+
n,
449+
displayNames: Array.from({ length: n }, () => 'asdf'),
450450
}));
451451

452452
// THEN
453453
const template = Template.fromStack(pipelineStack);
454454

455-
// We expect `asdf` and `asdf2` Actions in the pipeline
455+
// We expect at least `asdf` and `asdf2` Actions in the pipeline,
456456
template.hasResourceProperties('AWS::CodePipeline::Pipeline', {
457457
Stages: Match.arrayWith([{
458458
Name: 'Assets',
459459
Actions: Match.arrayWith([
460-
Match.objectLike({
461-
Name: 'asdf',
462-
}),
463-
Match.objectLike({
464-
Name: 'asdf2',
465-
}),
460+
Match.objectLike({ Name: 'asdf' }),
461+
Match.objectLike({ Name: 'asdf2' }),
462+
...(n == 3 ? [Match.objectLike({ Name: 'asdf3' })] : []),
463+
]),
464+
}]),
465+
});
466+
});
467+
468+
test('assets can have display names that conflict with calculated action names', () => {
469+
const pipelineStack = new cdk.Stack(app, 'PipelineStack', { env: PIPELINE_ENV });
470+
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk', {
471+
crossAccountKeys: true,
472+
useChangeSets: false,
473+
});
474+
pipeline.addStage(new MultipleFileAssetsApp(pipelineStack, 'App', {
475+
n: 3,
476+
displayNames: ['asdf', 'asdf', 'asdf2'], // asdf2 will conflict with the second generated name which will also be asdf2
477+
}));
478+
479+
// THEN
480+
const template = Template.fromStack(pipelineStack);
481+
482+
// We expect `asdf`, `asdf2` and `asdf22` Actions in the pipeline
483+
template.hasResourceProperties('AWS::CodePipeline::Pipeline', {
484+
Stages: Match.arrayWith([{
485+
Name: 'Assets',
486+
Actions: Match.arrayWith([
487+
Match.objectLike({ Name: 'asdf' }),
488+
Match.objectLike({ Name: 'asdf2' }),
489+
Match.objectLike({ Name: 'asdf22' }),
466490
]),
467491
}]),
468492
});

packages/aws-cdk-lib/pipelines/test/compliance/assets.test.ts

+6-6
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import * as cb from '../../../aws-codebuild';
55
import * as ec2 from '../../../aws-ec2';
66
import { Stack, Stage } from '../../../core';
77
import { CDKP_DEFAULT_CODEBUILD_IMAGE } from '../../lib/private/default-codebuild-image';
8-
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, MegaAssetsApp, TwoFileAssetsApp, DockerAssetApp, PlainStackApp, stringLike } from '../testhelpers';
8+
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, FileAssetApp, MegaAssetsApp, MultipleFileAssetsApp, DockerAssetApp, PlainStackApp, stringLike } from '../testhelpers';
99

1010
const FILE_ASSET_SOURCE_HASH = '8289faf53c7da377bb2b90615999171adef5e1d8f6b88810e5fef75e6ca09ba5';
1111
const FILE_ASSET_SOURCE_HASH2 = 'ac76997971c3f6ddf37120660003f1ced72b4fc58c498dfd99c78fa77e721e0e';
@@ -183,7 +183,7 @@ describe('basic pipeline', () => {
183183

184184
test('multiple assets are published in parallel', () => {
185185
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk');
186-
pipeline.addStage(new TwoFileAssetsApp(app, 'FileAssetApp'));
186+
pipeline.addStage(new MultipleFileAssetsApp(app, 'FileAssetApp', { n: 2 }));
187187

188188
Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodePipeline::Pipeline', {
189189
Stages: Match.arrayWith([{
@@ -519,13 +519,13 @@ describe('pipeline with single asset publisher', () => {
519519
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-1', {
520520
publishAssetsInParallel: false,
521521
});
522-
pipeline.addStage(new TwoFileAssetsApp(app, 'FileAssetApp'));
522+
pipeline.addStage(new MultipleFileAssetsApp(app, 'FileAssetApp', { n: 2 }));
523523

524524
const pipelineStack2 = new Stack(app, 'PipelineStack2', { env: PIPELINE_ENV });
525525
const otherPipeline = new ModernTestGitHubNpmPipeline(pipelineStack2, 'Cdk-2', {
526526
publishAssetsInParallel: false,
527527
});
528-
otherPipeline.addStage(new TwoFileAssetsApp(app, 'OtherFileAssetApp'));
528+
otherPipeline.addStage(new MultipleFileAssetsApp(app, 'OtherFileAssetApp', { n: 2 }));
529529
// THEN
530530
const buildSpecName1 = new Capture(stringLike('buildspec-*.yaml'));
531531
const buildSpecName2 = new Capture(stringLike('buildspec-*.yaml'));
@@ -585,7 +585,7 @@ describe('pipeline with single asset publisher', () => {
585585
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk', {
586586
publishAssetsInParallel: false,
587587
});
588-
pipeline.addStage(new TwoFileAssetsApp(app, 'FileAssetApp'));
588+
pipeline.addStage(new MultipleFileAssetsApp(app, 'FileAssetApp', { n: 2 }));
589589

590590
// THEN
591591
const buildSpecName = new Capture(stringLike('buildspec-*.yaml'));
@@ -634,7 +634,7 @@ describe('pipeline with custom asset publisher BuildSpec', () => {
634634
}),
635635
},
636636
});
637-
pipeline.addStage(new TwoFileAssetsApp(app, 'FileAssetApp'));
637+
pipeline.addStage(new MultipleFileAssetsApp(app, 'FileAssetApp', { n: 2 }));
638638

639639
const buildSpecName = new Capture(stringLike('buildspec-*'));
640640

Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
And a third.

packages/aws-cdk-lib/pipelines/test/testhelpers/test-app.ts

+31-14
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { Construct } from 'constructs';
44
import * as ecr_assets from '../../../aws-ecr-assets';
55
import * as s3 from '../../../aws-s3';
66
import * as s3_assets from '../../../aws-s3-assets';
7-
import { App, AppProps, Environment, CfnOutput, Stage, StageProps, Stack, StackProps } from '../../../core';
7+
import { App, AppProps, Environment, CfnOutput, Stage, StageProps, Stack, StackProps, ValidationError } from '../../../core';
88
import { assemblyBuilderOf } from '../../lib/private/construct-internals';
99

1010
export const PIPELINE_ENV: Environment = {
@@ -185,23 +185,40 @@ export class FileAssetApp extends Stage {
185185
}
186186
}
187187

188-
export interface TwoFileAssetsAppProps extends StageProps {
189-
readonly displayName1?: string;
190-
readonly displayName2?: string;
188+
export interface MultipleFileAssetsProps extends StageProps {
189+
readonly n: number;
190+
191+
/**
192+
* Up to 3 display names for equally many assets
193+
*/
194+
readonly displayNames?: string[];
191195
}
192196

193-
export class TwoFileAssetsApp extends Stage {
194-
constructor(scope: Construct, id: string, props?: TwoFileAssetsAppProps) {
197+
/**
198+
* Supports up to 3 file assets
199+
*/
200+
export class MultipleFileAssetsApp extends Stage {
201+
constructor(scope: Construct, id: string, props: MultipleFileAssetsProps) {
195202
super(scope, id, props);
196203
const stack = new Stack(this, 'Stack');
197-
new s3_assets.Asset(stack, 'Asset1', {
198-
path: path.join(__dirname, 'assets', 'test-file-asset.txt'),
199-
displayName: props?.displayName1,
200-
});
201-
new s3_assets.Asset(stack, 'Asset2', {
202-
path: path.join(__dirname, 'assets', 'test-file-asset-two.txt'),
203-
displayName: props?.displayName2,
204-
});
204+
205+
const fileNames = ['test-file-asset.txt', 'test-file-asset-two.txt', 'test-file-asset-three.txt'];
206+
if (props.displayNames && props.displayNames.length !== props.n) {
207+
throw new Error('Incorrect displayNames lenght');
208+
}
209+
210+
for (let i = 0; i < props.n; i++) {
211+
const displayName = props.displayNames ? props.displayNames[i] : undefined;
212+
const fn = fileNames[i];
213+
if (!fn) {
214+
throw new ValidationError(`Got more displayNames than we have fileNames: ${i + 1}`, this);
215+
}
216+
217+
new s3_assets.Asset(stack, `Asset${i + 1}`, {
218+
path: path.join(__dirname, 'assets', fn),
219+
displayName,
220+
});
221+
}
205222
}
206223
}
207224

0 commit comments

Comments
 (0)