diff --git a/packages/cdkConstructs/src/changesets/checkDestructiveChanges.ts b/packages/cdkConstructs/src/changesets/checkDestructiveChanges.ts index 84ab85b..cccdb5b 100644 --- a/packages/cdkConstructs/src/changesets/checkDestructiveChanges.ts +++ b/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,15 +77,11 @@ 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 } @@ -88,14 +89,27 @@ export function checkDestructiveChanges( logicalId: resourceChange.LogicalResourceId ?? "", physicalId: resourceChange.PhysicalResourceId ?? "", resourceType: resourceChange.ResourceType ?? "", - reason: replacementNeeded - ? `Replacement: ${String(resourceChange.Replacement)}` - : `Action: ${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 ?? ""}, Action: ${action ?? ""}, Replacement: ${replacement ?? ""}` + ) }) throw new Error(`Change set ${changeSetName} contains destructive changes`) } diff --git a/packages/cdkConstructs/tests/changesets/checkDestructiveChanges.test.ts b/packages/cdkConstructs/tests/changesets/checkDestructiveChanges.test.ts index ab83f78..dbd297f 100644 --- a/packages/cdkConstructs/tests/changesets/checkDestructiveChanges.test.ts +++ b/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 = [ + { + 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:") + }) })