-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(pipelines): undeployable due to dependency cycle #18686
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7f53b7d
fix: dependency cycles
rix0rrr be378f3
Properly remove free dependencies from depsets
rix0rrr 7e27ddc
Fix identitypools
rix0rrr 7f8ffa2
Fix pipelines
rix0rrr 8d97eee
Fixes to Sub
rix0rrr 5f8c62b
Typo
rix0rrr 9539347
Two more fixes to sub parsing
rix0rrr ec167b5
Merge branch 'master' into huijbers/dep-cycle
rix0rrr 3903361
flatMap!
rix0rrr e9dee61
Merge branch 'huijbers/dep-cycle' of github.com:aws/aws-cdk into huij…
rix0rrr 6dd81c7
Merge branch 'master' into huijbers/dep-cycle
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import { Resource, Template } from './template'; | ||
|
|
||
| /** | ||
| * Check a template for cyclic dependencies | ||
| * | ||
| * This will make sure that we don't happily validate templates | ||
| * in unit tests that wouldn't deploy to CloudFormation anyway. | ||
| */ | ||
| export function checkTemplateForCyclicDependencies(template: Template): void { | ||
| const logicalIds = new Set(Object.keys(template.Resources ?? {})); | ||
|
|
||
| const dependencies = new Map<string, Set<string>>(); | ||
| for (const [logicalId, resource] of Object.entries(template.Resources ?? {})) { | ||
| dependencies.set(logicalId, intersect(findResourceDependencies(resource), logicalIds)); | ||
| } | ||
|
|
||
| // We will now progressively remove entries from the map of 'dependencies' that have | ||
| // 0 elements in them. If we can't do that anymore and the map isn't empty, we | ||
| // have a cyclic dependency. | ||
| while (dependencies.size > 0) { | ||
| const free = Array.from(dependencies.entries()).filter(([_, deps]) => deps.size === 0); | ||
| if (free.length === 0) { | ||
| // Oops! | ||
| const cycle = findCycle(dependencies); | ||
|
|
||
| const cycleResources: any = {}; | ||
| for (const logicalId of cycle) { | ||
| cycleResources[logicalId] = template.Resources?.[logicalId]; | ||
| } | ||
|
|
||
| throw new Error(`Template is undeployable, these resources have a dependency cycle: ${cycle.join(' -> ')}:\n\n${JSON.stringify(cycleResources, undefined, 2)}`); | ||
| } | ||
|
|
||
| for (const [logicalId, _] of free) { | ||
| for (const deps of dependencies.values()) { | ||
| deps.delete(logicalId); | ||
| } | ||
| dependencies.delete(logicalId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function findResourceDependencies(res: Resource): Set<string> { | ||
| return new Set([ | ||
| ...toArray(res.DependsOn ?? []), | ||
| ...findExpressionDependencies(res.Properties), | ||
| ]); | ||
| } | ||
|
|
||
| function toArray<A>(x: A | A[]): A[] { | ||
| return Array.isArray(x) ? x : [x]; | ||
| } | ||
|
|
||
| function findExpressionDependencies(obj: any): Set<string> { | ||
| const ret = new Set<string>(); | ||
| recurse(obj); | ||
| return ret; | ||
|
|
||
| function recurse(x: any): void { | ||
| if (!x) { return; } | ||
| if (Array.isArray(x)) { | ||
| x.forEach(recurse); | ||
| } | ||
| if (typeof x === 'object') { | ||
| const keys = Object.keys(x); | ||
| if (keys.length === 1 && keys[0] === 'Ref') { | ||
| ret.add(x[keys[0]]); | ||
| } else if (keys.length === 1 && keys[0] === 'Fn::GetAtt') { | ||
| ret.add(x[keys[0]][0]); | ||
| } else if (keys.length === 1 && keys[0] === 'Fn::Sub') { | ||
| const argument = x[keys[0]]; | ||
| const pattern = Array.isArray(argument) ? argument[0] : argument; | ||
| for (const logId of logicalIdsInSubString(pattern)) { | ||
| ret.add(logId); | ||
| } | ||
| const contextDict = Array.isArray(argument) ? argument[1] : undefined; | ||
| if (contextDict) { | ||
| Object.values(contextDict).forEach(recurse); | ||
| } | ||
| } else { | ||
| Object.values(x).forEach(recurse); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Return the logical IDs found in a {Fn::Sub} format string | ||
| */ | ||
| function logicalIdsInSubString(x: string): string[] { | ||
| return analyzeSubPattern(x).flatMap((fragment) => { | ||
| switch (fragment.type) { | ||
| case 'getatt': | ||
| case 'ref': | ||
| return [fragment.logicalId]; | ||
| case 'literal': | ||
| return []; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| function analyzeSubPattern(pattern: string): SubFragment[] { | ||
| const ret: SubFragment[] = []; | ||
| let start = 0; | ||
|
|
||
| let ph0 = pattern.indexOf('${', start); | ||
| while (ph0 > -1) { | ||
| if (pattern[ph0 + 2] === '!') { | ||
| // "${!" means "don't actually substitute" | ||
| start = ph0 + 3; | ||
| ph0 = pattern.indexOf('${', start); | ||
| continue; | ||
| } | ||
|
|
||
| const ph1 = pattern.indexOf('}', ph0 + 2); | ||
| if (ph1 === -1) { | ||
| break; | ||
| } | ||
| const placeholder = pattern.substring(ph0 + 2, ph1); | ||
|
|
||
| if (ph0 > start) { | ||
| ret.push({ type: 'literal', content: pattern.substring(start, ph0) }); | ||
| } | ||
| if (placeholder.includes('.')) { | ||
| const [logicalId, attr] = placeholder.split('.'); | ||
| ret.push({ type: 'getatt', logicalId: logicalId!, attr: attr! }); | ||
| } else { | ||
| ret.push({ type: 'ref', logicalId: placeholder }); | ||
| } | ||
|
|
||
| start = ph1 + 1; | ||
| ph0 = pattern.indexOf('${', start); | ||
| } | ||
|
|
||
| if (start < pattern.length - 1) { | ||
| ret.push({ type: 'literal', content: pattern.substr(start) }); | ||
| } | ||
|
|
||
| return ret; | ||
| } | ||
|
|
||
| type SubFragment = | ||
| | { readonly type: 'literal'; readonly content: string } | ||
| | { readonly type: 'ref'; readonly logicalId: string } | ||
| | { readonly type: 'getatt'; readonly logicalId: string; readonly attr: string }; | ||
|
|
||
|
|
||
| function intersect<A>(xs: Set<A>, ys: Set<A>): Set<A> { | ||
| return new Set<A>(Array.from(xs).filter(x => ys.has(x))); | ||
| } | ||
|
|
||
| /** | ||
| * Find cycles in a graph | ||
| * | ||
| * Not the fastest, but effective and should be rare | ||
| */ | ||
| function findCycle(deps: ReadonlyMap<string, ReadonlySet<string>>): string[] { | ||
| for (const node of deps.keys()) { | ||
| const cycle = recurse(node, [node]); | ||
| if (cycle) { return cycle; } | ||
| } | ||
| throw new Error('No cycle found. Assertion failure!'); | ||
|
|
||
| function recurse(node: string, path: string[]): string[] | undefined { | ||
| for (const dep of deps.get(node) ?? []) { | ||
| if (dep === path[0]) { return [...path, dep]; } | ||
|
|
||
| const cycle = recurse(dep, [...path, dep]); | ||
| if (cycle) { return cycle; } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So cool! 🎉