feat: take all neighbors into account when computing resource digests (#713)

The current definition of the digest of a resource computes a hash of
its properties plus the hashes of its dependencies.

But if we have a graph like:

        A --> B
        C --> D

where `B` and `D` are identical, the current algorithm will compute the
same digest for them, even if `A` is different from `C` (and therefore
have different digests). This is undesirable, as some cases are
considered ambiguous, even if we could very well tell them apart. Real
world example:

<img width="326" height="182" alt="non-ambiguous"
src="https://github.com/user-attachments/assets/32e442c6-9605-43b4-bdd5-a745f1a0426f"
/>

`Role1` and `Role2` tend to be identical, but they can nevertheless be
told apart because at least the functions will have different
properties. So, even if both roles are renamed, for example, we can
still provide a valid mapping.

This change improves the algorithm by also taking into account all
neighbors of a resource for digest computation. It does this by doing a
two pass over the graph: first navigating in topological order and
computing the digests, then navigating in the opposite order and doing
the same. So each resource has two digests, that are in turn, used to
compute the final digest.


---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

Author: otaviomacedo

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<string, ResourceLocation[]> {
+function resourceDigests(stacks: CloudFormationStack[], direction: GraphDirection): Record<string, ResourceLocation[]> {
   // index stacks by name
   const stacksByName = new Map<string, CloudFormationStack>();
   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]) => {

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,28 +23,38 @@ import { ResourceGraph } from './graph';
  * CloudFormation template form a directed acyclic graph, this function is
  * well-defined.
  */
-export function computeResourceDigests(stacks: CloudFormationStack[]): Record<string, string> {
+export function computeResourceDigests(stacks: CloudFormationStack[], direction: GraphDirection = 'direct'): Record<string, string> {
   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 }],
+        ),
     ),
   );
 
   const resources = Object.fromEntries(
     stacks.flatMap((s) => {
       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]);
     }),
   );
 
-  const graph = new ResourceGraph(stacks);
+  const graph = direction == 'direct'
+    ? ResourceGraph.fromStacks(stacks)
+    : ResourceGraph.fromStacks(stacks).opposite();
+
+  return computeDigestsInTopologicalOrder(graph, resources, exports);
+}
+
+function computeDigestsInTopologicalOrder(
+  graph: ResourceGraph,
+  resources: Record<string, CloudFormationResource>,
+  exports: Record<string, { stackName: string; value: any }>): Record<string, string> {
   const nodes = graph.sortedNodes.filter(n => resources[n] != null);
-  // 4. Compute digests in sorted order
   const result: Record<string, string> = {};
   for (const id of nodes) {
     const resource = resources[id];

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<string, Set<string>> = {};
-  private readonly reverseEdges: Record<string, Set<string>> = {};
-
-  constructor(stacks: Omit<CloudFormationStack, 'environment'>[]) {
+  public static fromStacks(stacks: Omit<CloudFormationStack, 'environment'>[]): 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<string, Set<string>> = {};
+    const reverseEdges: Record<string, Set<string>> = {};
     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<string, Set<string>> = {};
+  private readonly reverseEdges: Record<string, Set<string>> = {};
+
+  private constructor(edges: Record<string, Set<string>>, reverseEdges: Record<string, Set<string>>) {
+    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);
+  }
 }

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,