Skip to content

Commit 458b843

Browse files
anthony-nhsanthony-nhs
authored
Fix: [AEA-0000] - refactor checkDestructiveChanges (#544)
## Summary - Routine Change ### Details - refactor checkDestructiveChanges
1 parent 0b4cdd5 commit 458b843

File tree

2 files changed

+165
-40
lines changed

2 files changed

+165
-40
lines changed

‎packages/cdkConstructs/src/changesets/checkDestructiveChanges.ts‎

Lines changed: 47 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,41 +5,46 @@ import {
55
Change as CloudFormationChange
66
} from "@aws-sdk/client-cloudformation"
77

8-
const isConditionalCdkMetadataChange = (resourceChange: CloudFormationChange["ResourceChange"]): boolean => {
9-
if (!resourceChange) {
10-
return false
8+
const normalizeToString = (value: unknown): string | undefined => {
9+
if (value === undefined || value === null) {
10+
return undefined
1111
}
1212

13-
return resourceChange.LogicalResourceId === "CDKMetadata" &&
14-
resourceChange.ResourceType === "AWS::CDK::Metadata" &&
15-
String(resourceChange.Replacement ?? "") === "Conditional"
13+
return String(value)
14+
}
15+
16+
const isDestructiveChange = (
17+
policyAction: string | undefined,
18+
action: string | undefined,
19+
replacement: string | undefined
20+
): boolean => {
21+
return policyAction === "Delete" ||
22+
policyAction === "ReplaceAndDelete" ||
23+
action === "Remove" ||
24+
replacement === "True"
1625
}
1726

1827
export type ChangeRequiringAttention = {
1928
logicalId: string;
2029
physicalId: string;
2130
resourceType: string;
22-
reason: string;
31+
policyAction: string;
32+
action: string;
33+
replacement: string;
2334
}
2435

2536
export type AllowedDestructiveChange = {
2637
LogicalResourceId: string;
2738
PhysicalResourceId: string;
2839
ResourceType: string;
40+
PolicyAction?: string | null;
41+
Action?: string | null;
42+
Replacement?: string | null;
2943
ExpiryDate: string | Date;
3044
StackName: string;
3145
AllowedReason: string;
3246
}
3347

34-
const requiresReplacement = (replacement: unknown): boolean => {
35-
if (replacement === undefined || replacement === null) {
36-
return false
37-
}
38-
39-
const normalized = String(replacement)
40-
return normalized === "True" || normalized === "Conditional"
41-
}
42-
4348
const toDate = (value: Date | string | number | undefined | null): Date | undefined => {
4449
if (value === undefined || value === null) {
4550
return undefined
@@ -72,30 +77,39 @@ export function checkDestructiveChanges(
7277
return undefined
7378
}
7479

75-
const replacementNeeded = requiresReplacement(resourceChange.Replacement)
76-
const action = resourceChange.Action
77-
const isRemoval = action === "Remove"
80+
const policyAction = normalizeToString(resourceChange.PolicyAction)
81+
const action = normalizeToString(resourceChange.Action)
82+
const replacement = normalizeToString(resourceChange.Replacement)
7883

79-
if (replacementNeeded && isConditionalCdkMetadataChange(resourceChange)) {
80-
return undefined
81-
}
82-
83-
if (!replacementNeeded && !isRemoval) {
84+
if (!isDestructiveChange(policyAction, action, replacement)) {
8485
return undefined
8586
}
8687

8788
return {
8889
logicalId: resourceChange.LogicalResourceId ?? "<unknown logical id>",
8990
physicalId: resourceChange.PhysicalResourceId ?? "<unknown physical id>",
9091
resourceType: resourceChange.ResourceType ?? "<unknown type>",
91-
reason: replacementNeeded
92-
? `Replacement: ${String(resourceChange.Replacement)}`
93-
: `Action: ${action ?? "<unknown action>"}`
92+
policyAction,
93+
action,
94+
replacement
9495
}
9596
})
9697
.filter((change): change is ChangeRequiringAttention => Boolean(change))
9798
}
9899

100+
const matchesAllowedChange = (change: ChangeRequiringAttention, allowed: AllowedDestructiveChange): boolean => {
101+
const allowedPolicyAction = normalizeToString(allowed.PolicyAction)
102+
const allowedAction = normalizeToString(allowed.Action)
103+
const allowedReplacement = normalizeToString(allowed.Replacement)
104+
105+
return allowed.LogicalResourceId === change.logicalId &&
106+
allowed.PhysicalResourceId === change.physicalId &&
107+
allowed.ResourceType === change.resourceType &&
108+
allowedPolicyAction === change.policyAction &&
109+
allowedAction === change.action &&
110+
allowedReplacement === change.replacement
111+
}
112+
99113
/**
100114
* Describes a CloudFormation change set, applies waiver logic, and throws if destructive changes remain.
101115
*
@@ -125,11 +139,7 @@ export async function checkDestructiveChangeSet(
125139
const changeSetStackName = response.StackName
126140

127141
const remainingChanges = destructiveChanges.filter(change => {
128-
const waiver = allowedChanges.find(allowed =>
129-
allowed.LogicalResourceId === change.logicalId &&
130-
allowed.PhysicalResourceId === change.physicalId &&
131-
allowed.ResourceType === change.resourceType
132-
)
142+
const waiver = allowedChanges.find(allowed => matchesAllowedChange(change, allowed))
133143

134144
if (!waiver || !creationTime || !changeSetStackName || waiver.StackName !== changeSetStackName) {
135145
return true
@@ -161,8 +171,11 @@ export async function checkDestructiveChangeSet(
161171
}
162172

163173
console.error("Resources that require attention:")
164-
remainingChanges.forEach(({logicalId, physicalId, resourceType, reason}) => {
165-
console.error(`- LogicalId: ${logicalId}, PhysicalId: ${physicalId}, Type: ${resourceType}, Reason: ${reason}`)
174+
remainingChanges.forEach(({logicalId, physicalId, resourceType, policyAction, action, replacement}) => {
175+
console.error(
176+
// eslint-disable-next-line max-len
177+
`- LogicalId: ${logicalId}, PhysicalId: ${physicalId}, Type: ${resourceType}, PolicyAction: ${policyAction ?? "<none>"}, Action: ${action ?? "<unknown>"}, Replacement: ${replacement ?? "<none>"}`
178+
)
166179
})
167180
throw new Error(`Change set ${changeSetName} contains destructive changes`)
168181
}

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

Lines changed: 118 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,9 @@ describe("checkDestructiveChanges", () => {
6565
logicalId: "AlarmsAccountLambdaConcurrencyAlarm8AF49AD8",
6666
physicalId: "monitoring-Account_Lambda_Concurrency",
6767
resourceType: "AWS::CloudWatch::Alarm",
68-
reason: "Replacement: True"
68+
policyAction: "ReplaceAndDelete",
69+
action: "Modify",
70+
replacement: "True"
6971
})
7072
})
7173

@@ -80,6 +82,7 @@ describe("checkDestructiveChanges", () => {
8082
Changes: [
8183
{
8284
ResourceChange: {
85+
PolicyAction: "Delete",
8386
LogicalResourceId: "ResourceToRemove",
8487
PhysicalResourceId: "physical-id",
8588
ResourceType: "AWS::S3::Bucket",
@@ -96,19 +99,82 @@ describe("checkDestructiveChanges", () => {
9699
logicalId: "ResourceToRemove",
97100
physicalId: "physical-id",
98101
resourceType: "AWS::S3::Bucket",
99-
reason: "Action: Remove"
102+
policyAction: "Delete",
103+
action: "Remove",
104+
replacement: "False"
100105
}
101106
])
102107
})
103108

104-
test("ignores conditional CDK metadata replacements", () => {
109+
test("marks changes with Delete policy action as destructive even without removal", () => {
105110
const changeSet = {
106111
Changes: [
107112
{
108113
ResourceChange: {
109-
LogicalResourceId: "CDKMetadata",
110-
PhysicalResourceId: "metadata-id",
111-
ResourceType: "AWS::CDK::Metadata",
114+
PolicyAction: "Delete",
115+
LogicalResourceId: "PolicyOnly",
116+
PhysicalResourceId: "policy-only",
117+
ResourceType: "Custom::Thing",
118+
Action: "Modify",
119+
Replacement: "False"
120+
}
121+
}
122+
]
123+
}
124+
125+
const replacements = checkDestructiveChanges(changeSet)
126+
127+
expect(replacements).toEqual([
128+
{
129+
logicalId: "PolicyOnly",
130+
physicalId: "policy-only",
131+
resourceType: "Custom::Thing",
132+
policyAction: "Delete",
133+
action: "Modify",
134+
replacement: "False"
135+
}
136+
])
137+
})
138+
139+
test("marks changes with ReplaceAndDelete policy action as destructive even when replacement is false", () => {
140+
const changeSet = {
141+
Changes: [
142+
{
143+
ResourceChange: {
144+
PolicyAction: "ReplaceAndDelete",
145+
LogicalResourceId: "PolicyReplace",
146+
PhysicalResourceId: "policy-replace",
147+
ResourceType: "Custom::Thing",
148+
Action: "Modify",
149+
Replacement: "False"
150+
}
151+
}
152+
]
153+
}
154+
155+
const replacements = checkDestructiveChanges(changeSet)
156+
157+
expect(replacements).toEqual([
158+
{
159+
logicalId: "PolicyReplace",
160+
physicalId: "policy-replace",
161+
resourceType: "Custom::Thing",
162+
policyAction: "ReplaceAndDelete",
163+
action: "Modify",
164+
replacement: "False"
165+
}
166+
])
167+
})
168+
169+
test("does not mark conditional replacements as destructive when no other indicator is present", () => {
170+
const changeSet = {
171+
Changes: [
172+
{
173+
ResourceChange: {
174+
LogicalResourceId: "Conditional",
175+
PhysicalResourceId: "conditional",
176+
ResourceType: "Custom::Thing",
177+
Action: "Modify",
112178
Replacement: "Conditional"
113179
}
114180
}
@@ -162,6 +228,7 @@ describe("checkDestructiveChangeSet", () => {
162228
LogicalResourceId: "ResourceToRemove",
163229
PhysicalResourceId: "physical-id",
164230
ResourceType: "AWS::S3::Bucket",
231+
PolicyAction: "Delete",
165232
Action: "Remove"
166233
}
167234
}
@@ -174,6 +241,9 @@ describe("checkDestructiveChangeSet", () => {
174241
LogicalResourceId: "ResourceToRemove",
175242
PhysicalResourceId: "physical-id",
176243
ResourceType: "AWS::S3::Bucket",
244+
PolicyAction: "Delete",
245+
Action: "Remove",
246+
Replacement: null,
177247
ExpiryDate: "2026-03-01T00:00:00Z",
178248
StackName: "stack",
179249
AllowedReason: "Pending migration"
@@ -199,6 +269,7 @@ describe("checkDestructiveChangeSet", () => {
199269
LogicalResourceId: "ResourceToRemove",
200270
PhysicalResourceId: "physical-id",
201271
ResourceType: "AWS::S3::Bucket",
272+
PolicyAction: "Delete",
202273
Action: "Remove"
203274
}
204275
}
@@ -211,6 +282,9 @@ describe("checkDestructiveChangeSet", () => {
211282
LogicalResourceId: "ResourceToRemove",
212283
PhysicalResourceId: "physical-id",
213284
ResourceType: "AWS::S3::Bucket",
285+
PolicyAction: "Delete",
286+
Action: "Remove",
287+
Replacement: null,
214288
ExpiryDate: "2026-02-01T00:00:00Z",
215289
StackName: "stack",
216290
AllowedReason: "Expired waiver"
@@ -224,4 +298,42 @@ describe("checkDestructiveChangeSet", () => {
224298
expect(errorSpy).toHaveBeenCalledWith("Resources that require attention:")
225299
expect(logSpy).not.toHaveBeenCalledWith("Change set cs for stack stack has no destructive changes.")
226300
})
301+
302+
test("does not allow waivers that mismatch policy or action", async () => {
303+
const changeSet = {
304+
CreationTime: "2026-02-20T11:54:17.083Z",
305+
StackName: "stack",
306+
Changes: [
307+
{
308+
ResourceChange: {
309+
LogicalResourceId: "ResourceToRemove",
310+
PhysicalResourceId: "physical-id",
311+
ResourceType: "AWS::S3::Bucket",
312+
PolicyAction: "Delete",
313+
Action: "Remove"
314+
}
315+
}
316+
]
317+
}
318+
mockCloudFormationSend.mockResolvedValueOnce(changeSet)
319+
320+
const allowedChanges: Array<AllowedDestructiveChange> = [
321+
{
322+
LogicalResourceId: "ResourceToRemove",
323+
PhysicalResourceId: "physical-id",
324+
ResourceType: "AWS::S3::Bucket",
325+
PolicyAction: "ReplaceAndDelete",
326+
Action: "Remove",
327+
Replacement: null,
328+
ExpiryDate: "2026-03-01T00:00:00Z",
329+
StackName: "stack",
330+
AllowedReason: "Incorrect policy"
331+
}
332+
]
333+
334+
await expect(checkDestructiveChangeSet("cs", "stack", "eu-west-2", allowedChanges))
335+
.rejects.toThrow("Change set cs contains destructive changes")
336+
337+
expect(errorSpy).toHaveBeenCalledWith("Resources that require attention:")
338+
})
227339
})

0 commit comments

Comments
 (0)