Skip to content

Commit f511000

Browse files
rix0rrrrix0rrr
authored
chore: detect property type renames to protect against breakage (#24718)
Upstream service teams may rename property types in the resource specification. This is strictly speaking not breaking from the point of view of the spec, because the names of the property types don't appear anywhere in the code a user would normally type (i.e., in a CloudFormation template). However, CDK generates classes for these types, and so the name *does* matter and changing it is breaking. To detect these instances, we check that during an upgrade, all old property type names are still present. If not, the reason is probably that they renamed a type. Note that this is not a 100% guaranteed to catch all scenarios (I'm sure you can think of changes that would be breaking and still pass this check), but it's at least very likely to catch honest mistakes in commonly expected scenarios. For those interested in how it works: * During the spec upgrade, we have both the old and the new spec available. * We iterate over all objects keys in the property types of the old spec, looking like: `{ "PropertyTypes": { "AWS::ElastiCache::ReplicationGroup.LogDeliveryConfigurationRequest": { ... }, ... }` object, and make sure that the keys are also present in the property types of the new spec. Also change the `copy/paste` operation pair of a previous patch into a `move` operation, so that if the type definition changes in the future we won't accidentally keep it at an old definition. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent f6cda61 commit f511000

File tree

3 files changed

+69
-85
lines changed

3 files changed

+69
-85
lines changed

‎packages/@aws-cdk/cfnspec/build-tools/spec-diff.ts

+52
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ async function main() {
2727
oldSpec.ResourceTypes = {};
2828
}
2929

30+
validatePropertyTypeNameConsistency(oldSpec, newSpec);
31+
3032
const out = jsonDiff(oldSpec, newSpec);
3133

3234
// Here's the magic output format of this thing
@@ -278,6 +280,56 @@ async function main() {
278280
}
279281
}
280282

283+
/**
284+
* Safeguard check: make sure that all old property type names in the old spec exist in the new spec
285+
*
286+
* If not, it's probably because the service team renamed a type between spec
287+
* version `v(N)` to `v(N+1)`.. In the CloudFormation spec itself, this is not a
288+
* problem. However, CDK will have generated actual classes and interfaces with
289+
* the type names at `v(N)`, which people will have written code against. If the
290+
* classes and interfaces would have a new name at `v(N+1)`, all user code would
291+
* break.
292+
*/
293+
function validatePropertyTypeNameConsistency(oldSpec: any, newSpec: any) {
294+
const newPropsTypes = newSpec.PropertyTypes ?? {};
295+
const disappearedKeys = Object.keys(oldSpec.PropertyTypes ?? {}).filter(k => !(k in newPropsTypes));
296+
if (disappearedKeys.length === 0) {
297+
return;
298+
}
299+
300+
const exampleJsonPatch = {
301+
patch: {
302+
description: 'Undoing upstream property type renames of <SERVICE> because <REASON>',
303+
operations: disappearedKeys.map((key) => ({
304+
op: 'move',
305+
from: `/PropertyTypes/${key.split('.')[0]}.<NEW_TYPE_NAME_HERE>`,
306+
path: `/PropertyTypes/${key}`,
307+
})),
308+
},
309+
};
310+
311+
process.stderr.write([
312+
'┌───────────────────────────────────────────────────────────────────────────────────────┐',
313+
'│ ▐█',
314+
'│ PROPERTY TYPES HAVE DISAPPEARED ▐█',
315+
'│ ▐█',
316+
'│ Some type names have disappeared from the old specification. ▐█',
317+
'│ ▐█',
318+
'│ This probably indicates that the service team renamed one of the types. We have ▐█',
319+
'│ to keep the old type names though: renaming them would constitute a breaking change ▐█',
320+
'│ to consumers of the L1 resources. ▐█',
321+
'│ ▐█',
322+
'└─▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▟█',
323+
'',
324+
'See what the renames were, check out this PR locally and add a JSON patch file for these types:',
325+
'',
326+
'(Example)',
327+
'',
328+
JSON.stringify(exampleJsonPatch, undefined, 2),
329+
].join('\n'));
330+
process.exitCode = 1;
331+
}
332+
281333
main().catch(e => {
282334
process.stderr.write(e.stack);
283335
process.stderr.write('\n');

‎packages/@aws-cdk/cfnspec/build-tools/update.sh

+2
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ scriptdir=$(cd $(dirname $0) && pwd)
1010

1111
rm -f CHANGELOG.md.new
1212

13+
14+
# update-spec <TITLE> <SOURCE> <TARGETDIR> <IS_GZIPPED> <SHOULD_SPLIT> [<SVC> [...]]
1315
function update-spec() {
1416
local title=$1
1517
local url=$2

‎packages/@aws-cdk/cfnspec/spec-source/specification/000_cfn/500_WAFv2_RuleGroup_Rename_Properties_patch.json

+15-85
Original file line numberDiff line numberDiff line change
@@ -3,99 +3,29 @@
33
"description": "Reverting property type names from FooAction to Foo, which were introduced as part of this PR: https://github.com/aws/aws-cdk/pull/23984",
44
"operations": [
55
{
6-
"op": "remove",
7-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.AllowAction"
6+
"op": "move",
7+
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.AllowAction",
8+
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Allow"
89
},
910
{
10-
"op": "add",
11-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Allow",
12-
"value": {
13-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-allowaction.html",
14-
"Properties": {
15-
"CustomRequestHandling": {
16-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-allowaction.html#cfn-wafv2-rulegroup-allowaction-customrequesthandling",
17-
"Required": false,
18-
"Type": "CustomRequestHandling",
19-
"UpdateType": "Mutable"
20-
}
21-
}
22-
}
11+
"op": "move",
12+
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.BlockAction",
13+
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Block"
2314
},
2415
{
25-
"op": "remove",
26-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.BlockAction"
16+
"op": "move",
17+
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.CaptchaAction",
18+
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Captcha"
2719
},
2820
{
29-
"op": "add",
30-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Block",
31-
"value": {
32-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-blockaction.html",
33-
"Properties": {
34-
"CustomResponse": {
35-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-blockaction.html#cfn-wafv2-rulegroup-blockaction-customresponse",
36-
"Required": false,
37-
"Type": "CustomResponse",
38-
"UpdateType": "Mutable"
39-
}
40-
}
41-
}
21+
"op": "move",
22+
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.ChallengeAction",
23+
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Challenge"
4224
},
4325
{
44-
"op": "remove",
45-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.CaptchaAction"
46-
},
47-
{
48-
"op": "add",
49-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Captcha",
50-
"value": {
51-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-captchaaction.html",
52-
"Properties": {
53-
"CustomRequestHandling": {
54-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-captchaaction.html#cfn-wafv2-rulegroup-captchaaction-customrequesthandling",
55-
"Required": false,
56-
"Type": "CustomRequestHandling",
57-
"UpdateType": "Mutable"
58-
}
59-
}
60-
}
61-
},
62-
{
63-
"op": "remove",
64-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.ChallengeAction"
65-
},
66-
{
67-
"op": "add",
68-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Challenge",
69-
"value": {
70-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-challengeaction.html",
71-
"Properties": {
72-
"CustomRequestHandling": {
73-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-challengeaction.html#cfn-wafv2-rulegroup-challengeaction-customrequesthandling",
74-
"Required": false,
75-
"Type": "CustomRequestHandling",
76-
"UpdateType": "Mutable"
77-
}
78-
}
79-
}
80-
},
81-
{
82-
"op": "remove",
83-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.CountAction"
84-
},
85-
{
86-
"op": "add",
87-
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Count",
88-
"value": {
89-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-countaction.html",
90-
"Properties": {
91-
"CustomRequestHandling": {
92-
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-countaction.html#cfn-wafv2-rulegroup-countaction-customrequesthandling",
93-
"Required": false,
94-
"Type": "CustomRequestHandling",
95-
"UpdateType": "Mutable"
96-
}
97-
}
98-
}
26+
"op": "move",
27+
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.CountAction",
28+
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Count"
9929
}
10030
]
10131
}

0 commit comments

Comments
 (0)