Skip to content

Commit 009d689

Browse files
rix0rrrrix0rrr
authored
fix(pipelines): undeployable due to dependency cycle (#18686)
A dependency cycle was inadvertently introduced to CDK Pipelines in #18492. Fix that dependency cycle, and also one in Cognito IdentityPools. Add facilities to the `assertions` library to automatically detect this in the future, to stop errors like this from slipping in. We could make it a separate assertion method (`Template.fromStack().assertNoCycles()`), but the only thing that will do is give you an opportunity to forget to put the test in. Instead, we just check it by default for every generated template. Fixes #18673. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 7ac7221 commit 009d689

File tree

17 files changed

+254
-113
lines changed

17 files changed

+254
-113
lines changed

Diff for: ‎packages/@aws-cdk/assertions/lib/private/conditions.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { filterLogicalId, formatFailure, matchSection } from './section';
22
import { Template } from './template';
33

44
export function findConditions(template: Template, logicalId: string, props: any = {}): { [key: string]: { [key: string]: any } } {
5-
const section: { [key: string] : {} } = template.Conditions;
5+
const section: { [key: string] : {} } = template.Conditions ?? {};
66
const result = matchSection(filterLogicalId(section, logicalId), props);
77

88
if (!result.match) {
@@ -13,7 +13,7 @@ export function findConditions(template: Template, logicalId: string, props: any
1313
}
1414

1515
export function hasCondition(template: Template, logicalId: string, props: any): string | void {
16-
const section: { [key: string] : {} } = template.Conditions;
16+
const section: { [key: string] : {} } = template.Conditions ?? {};
1717
const result = matchSection(filterLogicalId(section, logicalId), props);
1818
if (result.match) {
1919
return;

Diff for: ‎packages/@aws-cdk/assertions/lib/private/cyclic.ts

+175
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
import { Resource, Template } from './template';
2+
3+
/**
4+
* Check a template for cyclic dependencies
5+
*
6+
* This will make sure that we don't happily validate templates
7+
* in unit tests that wouldn't deploy to CloudFormation anyway.
8+
*/
9+
export function checkTemplateForCyclicDependencies(template: Template): void {
10+
const logicalIds = new Set(Object.keys(template.Resources ?? {}));
11+
12+
const dependencies = new Map<string, Set<string>>();
13+
for (const [logicalId, resource] of Object.entries(template.Resources ?? {})) {
14+
dependencies.set(logicalId, intersect(findResourceDependencies(resource), logicalIds));
15+
}
16+
17+
// We will now progressively remove entries from the map of 'dependencies' that have
18+
// 0 elements in them. If we can't do that anymore and the map isn't empty, we
19+
// have a cyclic dependency.
20+
while (dependencies.size > 0) {
21+
const free = Array.from(dependencies.entries()).filter(([_, deps]) => deps.size === 0);
22+
if (free.length === 0) {
23+
// Oops!
24+
const cycle = findCycle(dependencies);
25+
26+
const cycleResources: any = {};
27+
for (const logicalId of cycle) {
28+
cycleResources[logicalId] = template.Resources?.[logicalId];
29+
}
30+
31+
throw new Error(`Template is undeployable, these resources have a dependency cycle: ${cycle.join(' -> ')}:\n\n${JSON.stringify(cycleResources, undefined, 2)}`);
32+
}
33+
34+
for (const [logicalId, _] of free) {
35+
for (const deps of dependencies.values()) {
36+
deps.delete(logicalId);
37+
}
38+
dependencies.delete(logicalId);
39+
}
40+
}
41+
}
42+
43+
function findResourceDependencies(res: Resource): Set<string> {
44+
return new Set([
45+
...toArray(res.DependsOn ?? []),
46+
...findExpressionDependencies(res.Properties),
47+
]);
48+
}
49+
50+
function toArray<A>(x: A | A[]): A[] {
51+
return Array.isArray(x) ? x : [x];
52+
}
53+
54+
function findExpressionDependencies(obj: any): Set<string> {
55+
const ret = new Set<string>();
56+
recurse(obj);
57+
return ret;
58+
59+
function recurse(x: any): void {
60+
if (!x) { return; }
61+
if (Array.isArray(x)) {
62+
x.forEach(recurse);
63+
}
64+
if (typeof x === 'object') {
65+
const keys = Object.keys(x);
66+
if (keys.length === 1 && keys[0] === 'Ref') {
67+
ret.add(x[keys[0]]);
68+
} else if (keys.length === 1 && keys[0] === 'Fn::GetAtt') {
69+
ret.add(x[keys[0]][0]);
70+
} else if (keys.length === 1 && keys[0] === 'Fn::Sub') {
71+
const argument = x[keys[0]];
72+
const pattern = Array.isArray(argument) ? argument[0] : argument;
73+
for (const logId of logicalIdsInSubString(pattern)) {
74+
ret.add(logId);
75+
}
76+
const contextDict = Array.isArray(argument) ? argument[1] : undefined;
77+
if (contextDict) {
78+
Object.values(contextDict).forEach(recurse);
79+
}
80+
} else {
81+
Object.values(x).forEach(recurse);
82+
}
83+
}
84+
}
85+
}
86+
87+
/**
88+
* Return the logical IDs found in a {Fn::Sub} format string
89+
*/
90+
function logicalIdsInSubString(x: string): string[] {
91+
return analyzeSubPattern(x).flatMap((fragment) => {
92+
switch (fragment.type) {
93+
case 'getatt':
94+
case 'ref':
95+
return [fragment.logicalId];
96+
case 'literal':
97+
return [];
98+
}
99+
});
100+
}
101+
102+
103+
function analyzeSubPattern(pattern: string): SubFragment[] {
104+
const ret: SubFragment[] = [];
105+
let start = 0;
106+
107+
let ph0 = pattern.indexOf('${', start);
108+
while (ph0 > -1) {
109+
if (pattern[ph0 + 2] === '!') {
110+
// "${!" means "don't actually substitute"
111+
start = ph0 + 3;
112+
ph0 = pattern.indexOf('${', start);
113+
continue;
114+
}
115+
116+
const ph1 = pattern.indexOf('}', ph0 + 2);
117+
if (ph1 === -1) {
118+
break;
119+
}
120+
const placeholder = pattern.substring(ph0 + 2, ph1);
121+
122+
if (ph0 > start) {
123+
ret.push({ type: 'literal', content: pattern.substring(start, ph0) });
124+
}
125+
if (placeholder.includes('.')) {
126+
const [logicalId, attr] = placeholder.split('.');
127+
ret.push({ type: 'getatt', logicalId: logicalId!, attr: attr! });
128+
} else {
129+
ret.push({ type: 'ref', logicalId: placeholder });
130+
}
131+
132+
start = ph1 + 1;
133+
ph0 = pattern.indexOf('${', start);
134+
}
135+
136+
if (start < pattern.length - 1) {
137+
ret.push({ type: 'literal', content: pattern.substr(start) });
138+
}
139+
140+
return ret;
141+
}
142+
143+
type SubFragment =
144+
| { readonly type: 'literal'; readonly content: string }
145+
| { readonly type: 'ref'; readonly logicalId: string }
146+
| { readonly type: 'getatt'; readonly logicalId: string; readonly attr: string };
147+
148+
149+
function intersect<A>(xs: Set<A>, ys: Set<A>): Set<A> {
150+
return new Set<A>(Array.from(xs).filter(x => ys.has(x)));
151+
}
152+
153+
/**
154+
* Find cycles in a graph
155+
*
156+
* Not the fastest, but effective and should be rare
157+
*/
158+
function findCycle(deps: ReadonlyMap<string, ReadonlySet<string>>): string[] {
159+
for (const node of deps.keys()) {
160+
const cycle = recurse(node, [node]);
161+
if (cycle) { return cycle; }
162+
}
163+
throw new Error('No cycle found. Assertion failure!');
164+
165+
function recurse(node: string, path: string[]): string[] | undefined {
166+
for (const dep of deps.get(node) ?? []) {
167+
if (dep === path[0]) { return [...path, dep]; }
168+
169+
const cycle = recurse(dep, [...path, dep]);
170+
if (cycle) { return cycle; }
171+
}
172+
173+
return undefined;
174+
}
175+
}

Diff for: ‎packages/@aws-cdk/assertions/lib/private/mappings.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { filterLogicalId, formatFailure, matchSection } from './section';
22
import { Template } from './template';
33

44
export function findMappings(template: Template, logicalId: string, props: any = {}): { [key: string]: { [key: string]: any } } {
5-
const section: { [key: string] : {} } = template.Mappings;
5+
const section: { [key: string] : {} } = template.Mappings ?? {};
66
const result = matchSection(filterLogicalId(section, logicalId), props);
77

88
if (!result.match) {
@@ -13,7 +13,7 @@ export function findMappings(template: Template, logicalId: string, props: any =
1313
}
1414

1515
export function hasMapping(template: Template, logicalId: string, props: any): string | void {
16-
const section: { [key: string]: {} } = template.Mappings;
16+
const section: { [key: string]: {} } = template.Mappings ?? {};
1717
const result = matchSection(filterLogicalId(section, logicalId), props);
1818

1919
if (result.match) {

Diff for: ‎packages/@aws-cdk/assertions/lib/private/outputs.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { filterLogicalId, formatFailure, matchSection } from './section';
22
import { Template } from './template';
33

44
export function findOutputs(template: Template, logicalId: string, props: any = {}): { [key: string]: { [key: string]: any } } {
5-
const section = template.Outputs;
5+
const section = template.Outputs ?? {};
66
const result = matchSection(filterLogicalId(section, logicalId), props);
77

88
if (!result.match) {
@@ -13,7 +13,7 @@ export function findOutputs(template: Template, logicalId: string, props: any =
1313
}
1414

1515
export function hasOutput(template: Template, logicalId: string, props: any): string | void {
16-
const section: { [key: string]: {} } = template.Outputs;
16+
const section: { [key: string]: {} } = template.Outputs ?? {};
1717
const result = matchSection(filterLogicalId(section, logicalId), props);
1818
if (result.match) {
1919
return;

Diff for: ‎packages/@aws-cdk/assertions/lib/private/parameters.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { filterLogicalId, formatFailure, matchSection } from './section';
22
import { Template } from './template';
33

44
export function findParameters(template: Template, logicalId: string, props: any = {}): { [key: string]: { [key: string]: any } } {
5-
const section: { [key: string] : {} } = template.Parameters;
5+
const section: { [key: string] : {} } = template.Parameters ?? {};
66
const result = matchSection(filterLogicalId(section, logicalId), props);
77

88
if (!result.match) {
@@ -13,7 +13,7 @@ export function findParameters(template: Template, logicalId: string, props: any
1313
}
1414

1515
export function hasParameter(template: Template, logicalId: string, props: any): string | void {
16-
const section: { [key: string] : {} } = template.Parameters;
16+
const section: { [key: string] : {} } = template.Parameters ?? {};
1717
const result = matchSection(filterLogicalId(section, logicalId), props);
1818
if (result.match) {
1919
return;

Diff for: ‎packages/@aws-cdk/assertions/lib/private/resources.ts

+4-4
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { formatFailure, matchSection } from './section';
44
import { Resource, Template } from './template';
55

66
export function findResources(template: Template, type: string, props: any = {}): { [key: string]: { [key: string]: any } } {
7-
const section = template.Resources;
7+
const section = template.Resources ?? {};
88
const result = matchSection(filterType(section, type), props);
99

1010
if (!result.match) {
@@ -15,7 +15,7 @@ export function findResources(template: Template, type: string, props: any = {})
1515
}
1616

1717
export function hasResource(template: Template, type: string, props: any): string | void {
18-
const section = template.Resources;
18+
const section = template.Resources ?? {};
1919
const result = matchSection(filterType(section, type), props);
2020
if (result.match) {
2121
return;
@@ -46,14 +46,14 @@ export function hasResourceProperties(template: Template, type: string, props: a
4646
}
4747

4848
export function countResources(template: Template, type: string): number {
49-
const section = template.Resources;
49+
const section = template.Resources ?? {};
5050
const types = filterType(section, type);
5151

5252
return Object.entries(types).length;
5353
}
5454

5555
function addEmptyProperties(template: Template): Template {
56-
let section = template.Resources;
56+
let section = template.Resources ?? {};
5757

5858
Object.keys(section).map((key) => {
5959
if (!section[key].hasOwnProperty('Properties')) {

Diff for: ‎packages/@aws-cdk/assertions/lib/private/template.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
11
// Partial types for CloudFormation Template
22

33
export type Template = {
4-
Resources: { [logicalId: string]: Resource },
5-
Outputs: { [logicalId: string]: Output },
6-
Mappings: { [logicalId: string]: Mapping },
7-
Parameters: { [logicalId: string]: Parameter },
8-
Conditions: { [logicalId: string]: Condition },
4+
// In actuality this is not optional, but we sometimes don't generate it so we
5+
// need to account for that.
6+
Resources?: { [logicalId: string]: Resource },
7+
Outputs?: { [logicalId: string]: Output },
8+
Mappings?: { [logicalId: string]: Mapping },
9+
Parameters?: { [logicalId: string]: Parameter },
10+
Conditions?: { [logicalId: string]: Condition },
911
}
1012

1113
export type Resource = {
1214
Type: string;
15+
DependsOn?: string | string[];
16+
Properties?: { [key: string]: any };
1317
[key: string]: any;
1418
}
1519

Diff for: ‎packages/@aws-cdk/assertions/lib/template.ts

+2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import * as fs from 'fs-extra';
44
import { Match } from './match';
55
import { Matcher } from './matcher';
66
import { findConditions, hasCondition } from './private/conditions';
7+
import { checkTemplateForCyclicDependencies } from './private/cyclic';
78
import { findMappings, hasMapping } from './private/mappings';
89
import { findOutputs, hasOutput } from './private/outputs';
910
import { findParameters, hasParameter } from './private/parameters';
@@ -47,6 +48,7 @@ export class Template {
4748

4849
private constructor(template: { [key: string]: any }) {
4950
this.template = template as TemplateType;
51+
checkTemplateForCyclicDependencies(this.template);
5052
}
5153

5254
/**

Diff for: ‎packages/@aws-cdk/assertions/test/template.test.ts

+25-6
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@ import { Capture, Match, Template } from '../lib';
55
describe('Template', () => {
66
test('fromString', () => {
77
const template = Template.fromString(`{
8-
"Resources": {
9-
"Foo": {
8+
"Resources": {
9+
"Foo": {
1010
"Type": "Baz::Qux",
1111
"Properties": { "Fred": "Waldo" }
12-
}
12+
}
1313
}
1414
}`);
1515

@@ -79,11 +79,11 @@ describe('Template', () => {
7979
describe('fromString', () => {
8080
test('default', () => {
8181
const assertions = Template.fromString(`{
82-
"Resources": {
83-
"Foo": {
82+
"Resources": {
83+
"Foo": {
8484
"Type": "Baz::Qux",
8585
"Properties": { "Fred": "Waldo" }
86-
}
86+
}
8787
}
8888
}`);
8989
assertions.resourceCountIs('Baz::Qux', 1);
@@ -1084,6 +1084,25 @@ describe('Template', () => {
10841084
expect(Object.keys(result).length).toEqual(0);
10851085
});
10861086
});
1087+
1088+
test('throws when given a template with cyclic dependencies', () => {
1089+
expect(() => {
1090+
Template.fromJSON({
1091+
Resources: {
1092+
Res1: {
1093+
Type: 'Foo',
1094+
Properties: {
1095+
Thing: { Ref: 'Res2' },
1096+
},
1097+
},
1098+
Res2: {
1099+
Type: 'Foo',
1100+
DependsOn: ['Res1'],
1101+
},
1102+
},
1103+
});
1104+
}).toThrow(/dependency cycle/);
1105+
});
10871106
});
10881107

10891108
function expectToThrow(fn: () => void, msgs: (RegExp | string)[], done: jest.DoneCallback): void {

0 commit comments

Comments
 (0)