From 134b4bfadb0abe2d70159c62a5b3b8520c2e4683 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Sat, 12 Jul 2025 06:38:18 +0100 Subject: [PATCH 1/2] feat: take all neighbors into account when computing resource digests --- .../toolkit-lib/lib/api/refactoring/digest.ts | 32 +++++++++++++++- .../toolkit-lib/lib/api/refactoring/graph.ts | 32 ++++++++++++---- .../test/api/refactoring/refactoring.test.ts | 37 +++++++++++++++++++ 3 files changed, 91 insertions(+), 10 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts index 966596b18..9c98abe3a 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts @@ -38,9 +38,37 @@ export function computeResourceDigests(stacks: CloudFormationStack[]): Record B + C --> D + + As long as A and C are different, the digests of B and D will also be different, even if + they are structurally identical and have no dependencies. + */ + const graph = ResourceGraph.fromStacks(stacks); + const directDigests = computeDigestsInTopologicalOrder(graph, resources, exports); + const oppositeDigests = computeDigestsInTopologicalOrder(graph.opposite(), resources, exports); + const result: Record = {}; + + for (const id in resources) { + result[id] = crypto.createHash('sha256') + .update(directDigests[id]) + .update(oppositeDigests[id]) + .digest('hex'); + } + + return result; +} + +function computeDigestsInTopologicalOrder( + graph: ResourceGraph, + resources: Record, + exports: Record): Record { const nodes = graph.sortedNodes.filter(n => resources[n] != null); - // 4. Compute digests in sorted order const result: Record = {}; for (const id of nodes) { const resource = resources[id]; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/graph.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/graph.ts index 049699f19..ea18cf8c7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/graph.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/graph.ts @@ -5,10 +5,7 @@ import { ToolkitError } from '../../toolkit/toolkit-error'; * An immutable directed graph of resources from multiple CloudFormation stacks. */ export class ResourceGraph { - private readonly edges: Record> = {}; - private readonly reverseEdges: Record> = {}; - - constructor(stacks: Omit[]) { + public static fromStacks(stacks: Omit[]): ResourceGraph { const exports: { [p: string]: { stackName: string; value: any } } = Object.fromEntries( stacks.flatMap((s) => Object.values(s.template.Outputs ?? {}) @@ -35,9 +32,11 @@ export class ResourceGraph { ); // 1. Build adjacency lists + const edges: Record> = {}; + const reverseEdges: Record> = {}; for (const id of Object.keys(resources)) { - this.edges[id] = new Set(); - this.reverseEdges[id] = new Set(); + edges[id] = new Set(); + reverseEdges[id] = new Set(); } // 2. Detect dependencies by searching for Ref/Fn::GetAtt @@ -84,11 +83,21 @@ export class ResourceGraph { const deps = findDependencies(stackName, res || {}); for (const dep of deps) { if (dep in resources && dep !== id) { - this.edges[id].add(dep); - this.reverseEdges[dep].add(id); + edges[id].add(dep); + reverseEdges[dep].add(id); } } } + + return new ResourceGraph(edges, reverseEdges); + } + + private readonly edges: Record> = {}; + private readonly reverseEdges: Record> = {}; + + private constructor(edges: Record>, reverseEdges: Record>) { + this.edges = edges; + this.reverseEdges = reverseEdges; } /** @@ -129,4 +138,11 @@ export class ResourceGraph { } return Array.from(this.edges[node] || []); } + + /** + * Returns another graph with the same nodes, but with the edges inverted + */ + public opposite(): ResourceGraph { + return new ResourceGraph(this.reverseEdges, this.edges); + } } diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts index 367f2f593..cac12a327 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts @@ -530,6 +530,43 @@ describe(computeResourceDigests, () => { expect(result['Stack2.AnotherBucket']).toBeDefined(); expect(result['Stack1.Bucket']).not.toEqual(result['Stack2.AnotherBucket']); }); + + test('identical resources up to dependers', () => { + { + const template = { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Bucket2: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Foo: { + Type: 'AWS::Foo::Foo', + Properties: { + SomeProp: { Ref: 'Bucket1' }, + }, + }, + Bar: { + Type: 'AWS::Foo::Bar', + Properties: { + SomeProp: { Ref: 'Bucket2' }, + }, + }, + }, + }; + + const stacks = makeStacks([template]); + const result = computeResourceDigests(stacks); + expect(result['Stack1.Bucket1']).toBeDefined(); + expect(result['Stack1.Bucket2']).toBeDefined(); + // Not the same digest even though the properties are the same + // The structure of the graph tells us that these are different resources + expect(result['Stack1.Bucket1']).not.toEqual(result['Stack1.Bucket2']); + } + }); }); describe(usePrescribedMappings, () => { From 2e739efb9d6eba80efb4d2c3ce3648dd281df202 Mon Sep 17 00:00:00 2001 From: Otavio Macedo <288203+otaviomacedo@users.noreply.github.com> Date: Tue, 15 Jul 2025 12:55:57 +0100 Subject: [PATCH 2/2] Using opposite graph only to resolve ambiguities --- .../lib/api/refactoring/context.ts | 41 +++++-- .../toolkit-lib/lib/api/refactoring/digest.ts | 42 +++---- .../test/api/refactoring/context.test.ts | 111 ++++++++++++++++++ .../test/api/refactoring/refactoring.test.ts | 37 ------ 4 files changed, 159 insertions(+), 72 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts index 76418bdfe..9cdfeab26 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/context.ts @@ -1,6 +1,7 @@ import type { Environment } from '@aws-cdk/cx-api'; import type { CloudFormationStack } from './cloudformation'; import { ResourceLocation, ResourceMapping } from './cloudformation'; +import type { GraphDirection } from './digest'; import { computeResourceDigests } from './digest'; import { ToolkitError } from '../../toolkit/toolkit-error'; import { equalSets } from '../../util/sets'; @@ -29,9 +30,12 @@ export class RefactoringContext { constructor(props: RefactorManagerOptions) { this.environment = props.environment; - const moves = resourceMoves(props.deployedStacks, props.localStacks); - const [nonAmbiguousMoves, ambiguousMoves] = partitionByAmbiguity(props.overrides ?? [], moves); + const moves = resourceMoves(props.deployedStacks, props.localStacks, 'direct'); + const additionalOverrides = structuralOverrides(props.deployedStacks, props.localStacks); + const overrides = (props.overrides ?? []).concat(additionalOverrides); + const [nonAmbiguousMoves, ambiguousMoves] = partitionByAmbiguity(overrides, moves); this.ambiguousMoves = ambiguousMoves; + this._mappings = resourceMappings(nonAmbiguousMoves); } @@ -48,9 +52,32 @@ export class RefactoringContext { } } -function resourceMoves(before: CloudFormationStack[], after: CloudFormationStack[]): ResourceMove[] { - const digestsBefore = resourceDigests(before); - const digestsAfter = resourceDigests(after); +/** + * Generates an automatic list of overrides that can be deduced from the structure of the opposite resource graph. + * Suppose we have the following resource graph: + * + * A --> B + * C --> D + * + * such that B and D are identical, but A is different from C. Then digest(B) = digest(D). If both resources are moved, + * we have an ambiguity. But if we reverse the arrows: + * + * A <-- B + * C <-- D + * + * then digest(B) ≠ digest(D), because they now have different dependencies. If we compute the mappings from this + * opposite graph, we can use them as a set of overrides to disambiguate the original moves. + * + */ +function structuralOverrides(deployedStacks: CloudFormationStack[], localStacks: CloudFormationStack[]): ResourceMapping[] { + const moves = resourceMoves(deployedStacks, localStacks, 'opposite'); + const [nonAmbiguousMoves] = partitionByAmbiguity([], moves); + return resourceMappings(nonAmbiguousMoves); +} + +function resourceMoves(before: CloudFormationStack[], after: CloudFormationStack[], direction: GraphDirection): ResourceMove[] { + const digestsBefore = resourceDigests(before, direction); + const digestsAfter = resourceDigests(after, direction); const stackNames = (stacks: CloudFormationStack[]) => stacks.map((s) => s.stackName).sort().join(', '); if (!isomorphic(digestsBefore, digestsAfter)) { @@ -119,14 +146,14 @@ function zip( /** * Computes a list of pairs [digest, location] for each resource in the stack. */ -function resourceDigests(stacks: CloudFormationStack[]): Record { +function resourceDigests(stacks: CloudFormationStack[], direction: GraphDirection): Record { // index stacks by name const stacksByName = new Map(); for (const stack of stacks) { stacksByName.set(stack.stackName, stack); } - const digests = computeResourceDigests(stacks); + const digests = computeResourceDigests(stacks, direction); return groupByKey( Object.entries(digests).map(([loc, digest]) => { diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts index 9c98abe3a..cf1d0e4de 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/refactoring/digest.ts @@ -2,6 +2,10 @@ import * as crypto from 'node:crypto'; import type { CloudFormationResource, CloudFormationStack } from './cloudformation'; import { ResourceGraph } from './graph'; +export type GraphDirection = + 'direct' // Edge A -> B mean that A depends on B + | 'opposite'; // Edge A -> B mean that B depends on A + /** * Computes the digest for each resource in the template. * @@ -19,12 +23,15 @@ import { ResourceGraph } from './graph'; * CloudFormation template form a directed acyclic graph, this function is * well-defined. */ -export function computeResourceDigests(stacks: CloudFormationStack[]): Record { +export function computeResourceDigests(stacks: CloudFormationStack[], direction: GraphDirection = 'direct'): Record { const exports: { [p: string]: { stackName: string; value: any } } = Object.fromEntries( stacks.flatMap((s) => Object.values(s.template.Outputs ?? {}) .filter((o) => o.Export != null && typeof o.Export.Name === 'string') - .map((o) => [o.Export.Name, { stackName: s.stackName, value: o.Value }] as [string, { stackName: string; value: any }]), + .map( + (o) => + [o.Export.Name, { stackName: s.stackName, value: o.Value }] as [string, { stackName: string; value: any }], + ), ), ); @@ -32,36 +39,15 @@ export function computeResourceDigests(stacks: CloudFormationStack[]): Record { return Object.entries(s.template.Resources ?? {}) .filter(([_, res]) => res.Type !== 'AWS::CDK::Metadata') - .map( - ([id, res]) => [`${s.stackName}.${id}`, res] as [string, CloudFormationResource], - ); + .map(([id, res]) => [`${s.stackName}.${id}`, res] as [string, CloudFormationResource]); }), ); - /* - Compute the final digest for each resource by combining the direct and opposite digests. - This is to make sure that the digest of a resource takes into account both its in-neighbors - and out-neighbors. For example, if we have the following graph: + const graph = direction == 'direct' + ? ResourceGraph.fromStacks(stacks) + : ResourceGraph.fromStacks(stacks).opposite(); - A --> B - C --> D - - As long as A and C are different, the digests of B and D will also be different, even if - they are structurally identical and have no dependencies. - */ - const graph = ResourceGraph.fromStacks(stacks); - const directDigests = computeDigestsInTopologicalOrder(graph, resources, exports); - const oppositeDigests = computeDigestsInTopologicalOrder(graph.opposite(), resources, exports); - const result: Record = {}; - - for (const id in resources) { - result[id] = crypto.createHash('sha256') - .update(directDigests[id]) - .update(oppositeDigests[id]) - .digest('hex'); - } - - return result; + return computeDigestsInTopologicalOrder(graph, resources, exports); } function computeDigestsInTopologicalOrder( diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts index 17051183f..9a14ff51a 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/context.test.ts @@ -402,6 +402,117 @@ describe('typed mappings', () => { ]); }); + test('ambiguous pairs that can be disambiguated by the structure', () => { + const stack1 = { + environment, + stackName: 'Foo', + template: { + Resources: { + Bucket1: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Bucket2: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Depender1: { + Type: 'AWS::Foo::Foo', + Properties: { + SomeProp: { Ref: 'Bucket1' }, + }, + }, + Depender2: { + Type: 'AWS::Bar::Bar', + Properties: { + SomeProp: { Ref: 'Bucket2' }, + }, + }, + }, + }, + }; + + const stack2 = { + environment, + stackName: 'Bar', + template: { + Resources: { + Bucket3: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Bucket4: { + Type: 'AWS::S3::Bucket', + Properties: {}, + }, + Depender1: { + Type: 'AWS::Foo::Foo', + Properties: { + SomeProp: { Ref: 'Bucket3' }, + }, + }, + Depender2: { + Type: 'AWS::Bar::Bar', + Properties: { + SomeProp: { Ref: 'Bucket4' }, + }, + }, + }, + }, + }; + + const context = new RefactoringContext({ + environment, + deployedStacks: [stack1], + localStacks: [stack2], + }); + expect(context.ambiguousPaths.length).toEqual(0); + expect(context.mappings.map(toCfnMapping)).toEqual([ + // Despite Bucket1 and Bucket2 being identical, we could still disambiguate + // them based on the resources that depend on them. + { + Destination: { + LogicalResourceId: 'Bucket3', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket1', + StackName: 'Foo', + }, + }, + { + Destination: { + LogicalResourceId: 'Bucket4', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Bucket2', + StackName: 'Foo', + }, + }, + { + Destination: { + LogicalResourceId: 'Depender1', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Depender1', + StackName: 'Foo', + }, + }, + { + Destination: { + LogicalResourceId: 'Depender2', + StackName: 'Bar', + }, + Source: { + LogicalResourceId: 'Depender2', + StackName: 'Foo', + }, + }, + ]); + }); + test('combines addition, deletion, update, and rename', () => { const stack1 = { environment, diff --git a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts index cac12a327..367f2f593 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/refactoring/refactoring.test.ts @@ -530,43 +530,6 @@ describe(computeResourceDigests, () => { expect(result['Stack2.AnotherBucket']).toBeDefined(); expect(result['Stack1.Bucket']).not.toEqual(result['Stack2.AnotherBucket']); }); - - test('identical resources up to dependers', () => { - { - const template = { - Resources: { - Bucket1: { - Type: 'AWS::S3::Bucket', - Properties: {}, - }, - Bucket2: { - Type: 'AWS::S3::Bucket', - Properties: {}, - }, - Foo: { - Type: 'AWS::Foo::Foo', - Properties: { - SomeProp: { Ref: 'Bucket1' }, - }, - }, - Bar: { - Type: 'AWS::Foo::Bar', - Properties: { - SomeProp: { Ref: 'Bucket2' }, - }, - }, - }, - }; - - const stacks = makeStacks([template]); - const result = computeResourceDigests(stacks); - expect(result['Stack1.Bucket1']).toBeDefined(); - expect(result['Stack1.Bucket2']).toBeDefined(); - // Not the same digest even though the properties are the same - // The structure of the graph tells us that these are different resources - expect(result['Stack1.Bucket1']).not.toEqual(result['Stack1.Bucket2']); - } - }); }); describe(usePrescribedMappings, () => {