Fix: [AEA-0000] - refactor checkDestructiveChanges

packages/cdkConstructs/src/changesets/checkDestructiveChanges.ts

@@ -5,41 +5,46 @@ import {
   Change as CloudFormationChange
 } from "@aws-sdk/client-cloudformation"
 
-const isConditionalCdkMetadataChange = (resourceChange: CloudFormationChange["ResourceChange"]): boolean => {
-  if (!resourceChange) {
-    return false
+const normalizeToString = (value: unknown): string | undefined => {
+  if (value === undefined || value === null) {
+    return undefined
   }
 
-  return resourceChange.LogicalResourceId === "CDKMetadata" &&
-    resourceChange.ResourceType === "AWS::CDK::Metadata" &&
-    String(resourceChange.Replacement ?? "") === "Conditional"
+  return String(value)
+}
+
+const isDestructiveChange = (
+  policyAction: string | undefined,
+  action: string | undefined,
+  replacement: string | undefined
+): boolean => {
+  return policyAction === "Delete" ||
+    policyAction === "ReplaceAndDelete" ||
+    action === "Remove" ||
+    replacement === "True"
 }
 
 export type ChangeRequiringAttention = {
   logicalId: string;
   physicalId: string;
   resourceType: string;
-  reason: string;
+  policyAction: string;
+  action: string;
+  replacement: string;
 }
 
 export type AllowedDestructiveChange = {
   LogicalResourceId: string;
   PhysicalResourceId: string;
   ResourceType: string;
+  PolicyAction?: string | null;
+  Action?: string | null;
+  Replacement?: string | null;
   ExpiryDate: string | Date;
   StackName: string;
   AllowedReason: string;
 }
 
-const requiresReplacement = (replacement: unknown): boolean => {
-  if (replacement === undefined || replacement === null) {
-    return false
-  }
-
-  const normalized = String(replacement)
-  return normalized === "True" || normalized === "Conditional"
-}
-
 const toDate = (value: Date | string | number | undefined | null): Date | undefined => {
   if (value === undefined || value === null) {
     return undefined
@@ -72,30 +77,39 @@ export function checkDestructiveChanges(
         return undefined
       }
 
-      const replacementNeeded = requiresReplacement(resourceChange.Replacement)
-      const action = resourceChange.Action
-      const isRemoval = action === "Remove"
+      const policyAction = normalizeToString(resourceChange.PolicyAction)
+      const action = normalizeToString(resourceChange.Action)
+      const replacement = normalizeToString(resourceChange.Replacement)
 
-      if (replacementNeeded && isConditionalCdkMetadataChange(resourceChange)) {
-        return undefined
-      }
-
-      if (!replacementNeeded && !isRemoval) {
+      if (!isDestructiveChange(policyAction, action, replacement)) {
         return undefined
       }
 
       return {
         logicalId: resourceChange.LogicalResourceId ?? "<unknown logical id>",
         physicalId: resourceChange.PhysicalResourceId ?? "<unknown physical id>",
         resourceType: resourceChange.ResourceType ?? "<unknown type>",
-        reason: replacementNeeded
-          ? `Replacement: ${String(resourceChange.Replacement)}`
-          : `Action: ${action ?? "<unknown action>"}`
+        policyAction,
+        action,
+        replacement
       }
     })
     .filter((change): change is ChangeRequiringAttention => Boolean(change))
 }
 
+const matchesAllowedChange = (change: ChangeRequiringAttention, allowed: AllowedDestructiveChange): boolean => {
+  const allowedPolicyAction = normalizeToString(allowed.PolicyAction)
+  const allowedAction = normalizeToString(allowed.Action)
+  const allowedReplacement = normalizeToString(allowed.Replacement)
+
+  return allowed.LogicalResourceId === change.logicalId &&
+    allowed.PhysicalResourceId === change.physicalId &&
+    allowed.ResourceType === change.resourceType &&
+    allowedPolicyAction === change.policyAction &&
+    allowedAction === change.action &&
+    allowedReplacement === change.replacement
+}
+
 /**
  * Describes a CloudFormation change set, applies waiver logic, and throws if destructive changes remain.
  *
@@ -125,11 +139,7 @@ export async function checkDestructiveChangeSet(
   const changeSetStackName = response.StackName
 
   const remainingChanges = destructiveChanges.filter(change => {
-    const waiver = allowedChanges.find(allowed =>
-      allowed.LogicalResourceId === change.logicalId &&
-      allowed.PhysicalResourceId === change.physicalId &&
-      allowed.ResourceType === change.resourceType
-    )
+    const waiver = allowedChanges.find(allowed => matchesAllowedChange(change, allowed))
 
     if (!waiver || !creationTime || !changeSetStackName || waiver.StackName !== changeSetStackName) {
       return true
@@ -161,8 +171,11 @@ export async function checkDestructiveChangeSet(
   }
 
   console.error("Resources that require attention:")
-  remainingChanges.forEach(({logicalId, physicalId, resourceType, reason}) => {
-    console.error(`- LogicalId: ${logicalId}, PhysicalId: ${physicalId}, Type: ${resourceType}, Reason: ${reason}`)
+  remainingChanges.forEach(({logicalId, physicalId, resourceType, policyAction, action, replacement}) => {
+    console.error(
+      // eslint-disable-next-line max-len
+      `- LogicalId: ${logicalId}, PhysicalId: ${physicalId}, Type: ${resourceType}, PolicyAction: ${policyAction ?? "<none>"}, Action: ${action ?? "<unknown>"}, Replacement: ${replacement ?? "<none>"}`
+    )
   })
   throw new Error(`Change set ${changeSetName} contains destructive changes`)
 }

packages/cdkConstructs/tests/changesets/checkDestructiveChanges.test.ts

@@ -65,7 +65,9 @@ describe("checkDestructiveChanges", () => {
       logicalId: "AlarmsAccountLambdaConcurrencyAlarm8AF49AD8",
       physicalId: "monitoring-Account_Lambda_Concurrency",
       resourceType: "AWS::CloudWatch::Alarm",
-      reason: "Replacement: True"
+      policyAction: "ReplaceAndDelete",
+      action: "Modify",
+      replacement: "True"
     })
   })
 
@@ -80,6 +82,7 @@ describe("checkDestructiveChanges", () => {
       Changes: [
         {
           ResourceChange: {
+            PolicyAction: "Delete",
             LogicalResourceId: "ResourceToRemove",
             PhysicalResourceId: "physical-id",
             ResourceType: "AWS::S3::Bucket",
@@ -96,19 +99,82 @@ describe("checkDestructiveChanges", () => {
         logicalId: "ResourceToRemove",
         physicalId: "physical-id",
         resourceType: "AWS::S3::Bucket",
-        reason: "Action: Remove"
+        policyAction: "Delete",
+        action: "Remove",
+        replacement: "False"
       }
     ])
   })
 
-  test("ignores conditional CDK metadata replacements", () => {
+  test("marks changes with Delete policy action as destructive even without removal", () => {
     const changeSet = {
       Changes: [
         {
           ResourceChange: {
-            LogicalResourceId: "CDKMetadata",
-            PhysicalResourceId: "metadata-id",
-            ResourceType: "AWS::CDK::Metadata",
+            PolicyAction: "Delete",
+            LogicalResourceId: "PolicyOnly",
+            PhysicalResourceId: "policy-only",
+            ResourceType: "Custom::Thing",
+            Action: "Modify",
+            Replacement: "False"
+          }
+        }
+      ]
+    }
+
+    const replacements = checkDestructiveChanges(changeSet)
+
+    expect(replacements).toEqual([
+      {
+        logicalId: "PolicyOnly",
+        physicalId: "policy-only",
+        resourceType: "Custom::Thing",
+        policyAction: "Delete",
+        action: "Modify",
+        replacement: "False"
+      }
+    ])
+  })
+
+  test("marks changes with ReplaceAndDelete policy action as destructive even when replacement is false", () => {
+    const changeSet = {
+      Changes: [
+        {
+          ResourceChange: {
+            PolicyAction: "ReplaceAndDelete",
+            LogicalResourceId: "PolicyReplace",
+            PhysicalResourceId: "policy-replace",
+            ResourceType: "Custom::Thing",
+            Action: "Modify",
+            Replacement: "False"
+          }
+        }
+      ]
+    }
+
+    const replacements = checkDestructiveChanges(changeSet)
+
+    expect(replacements).toEqual([
+      {
+        logicalId: "PolicyReplace",
+        physicalId: "policy-replace",
+        resourceType: "Custom::Thing",
+        policyAction: "ReplaceAndDelete",
+        action: "Modify",
+        replacement: "False"
+      }
+    ])
+  })
+
+  test("does not mark conditional replacements as destructive when no other indicator is present", () => {
+    const changeSet = {
+      Changes: [
+        {
+          ResourceChange: {
+            LogicalResourceId: "Conditional",
+            PhysicalResourceId: "conditional",
+            ResourceType: "Custom::Thing",
+            Action: "Modify",
             Replacement: "Conditional"
           }
         }
@@ -162,6 +228,7 @@ describe("checkDestructiveChangeSet", () => {
             LogicalResourceId: "ResourceToRemove",
             PhysicalResourceId: "physical-id",
             ResourceType: "AWS::S3::Bucket",
+            PolicyAction: "Delete",
             Action: "Remove"
           }
         }
@@ -174,6 +241,9 @@ describe("checkDestructiveChangeSet", () => {
         LogicalResourceId: "ResourceToRemove",
         PhysicalResourceId: "physical-id",
         ResourceType: "AWS::S3::Bucket",
+        PolicyAction: "Delete",
+        Action: "Remove",
+        Replacement: null,
         ExpiryDate: "2026-03-01T00:00:00Z",
         StackName: "stack",
         AllowedReason: "Pending migration"
@@ -199,6 +269,7 @@ describe("checkDestructiveChangeSet", () => {
             LogicalResourceId: "ResourceToRemove",
             PhysicalResourceId: "physical-id",
             ResourceType: "AWS::S3::Bucket",
+            PolicyAction: "Delete",
             Action: "Remove"
           }
         }
@@ -211,6 +282,9 @@ describe("checkDestructiveChangeSet", () => {
         LogicalResourceId: "ResourceToRemove",
         PhysicalResourceId: "physical-id",
         ResourceType: "AWS::S3::Bucket",
+        PolicyAction: "Delete",
+        Action: "Remove",
+        Replacement: null,
         ExpiryDate: "2026-02-01T00:00:00Z",
         StackName: "stack",
         AllowedReason: "Expired waiver"
@@ -224,4 +298,42 @@ describe("checkDestructiveChangeSet", () => {
     expect(errorSpy).toHaveBeenCalledWith("Resources that require attention:")
     expect(logSpy).not.toHaveBeenCalledWith("Change set cs for stack stack has no destructive changes.")
   })
+
+  test("does not allow waivers that mismatch policy or action", async () => {
+    const changeSet = {
+      CreationTime: "2026-02-20T11:54:17.083Z",
+      StackName: "stack",
+      Changes: [
+        {
+          ResourceChange: {
+            LogicalResourceId: "ResourceToRemove",
+            PhysicalResourceId: "physical-id",
+            ResourceType: "AWS::S3::Bucket",
+            PolicyAction: "Delete",
+            Action: "Remove"
+          }
+        }
+      ]
+    }
+    mockCloudFormationSend.mockResolvedValueOnce(changeSet)
+
+    const allowedChanges: Array<AllowedDestructiveChange> = [
+      {
+        LogicalResourceId: "ResourceToRemove",
+        PhysicalResourceId: "physical-id",
+        ResourceType: "AWS::S3::Bucket",
+        PolicyAction: "ReplaceAndDelete",
+        Action: "Remove",
+        Replacement: null,
+        ExpiryDate: "2026-03-01T00:00:00Z",
+        StackName: "stack",
+        AllowedReason: "Incorrect policy"
+      }
+    ]
+
+    await expect(checkDestructiveChangeSet("cs", "stack", "eu-west-2", allowedChanges))
+      .rejects.toThrow("Change set cs contains destructive changes")
+
+    expect(errorSpy).toHaveBeenCalledWith("Resources that require attention:")
+  })
 })