Skip to content

Commit

Permalink
chore: detect property type renames to protect against breakage (#24718)
Browse files Browse the repository at this point in the history 
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*
  • Loading branch information
@rix0rrr
rix0rrr committed Mar 21, 2023
1 parent f6cda61 commit f511000
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 85 deletions.
52 changes: 52 additions & 0 deletions packages/@aws-cdk/cfnspec/build-tools/spec-diff.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ async function main() {
oldSpec.ResourceTypes = {};
}

validatePropertyTypeNameConsistency(oldSpec, newSpec);

const out = jsonDiff(oldSpec, newSpec);

// Here's the magic output format of this thing
Expand Down Expand Up @@ -278,6 +280,56 @@ async function main() {
}
}

/**
* Safeguard check: make sure that all old property type names in the old spec exist in the new spec
*
* If not, it's probably because the service team renamed a type between spec
* version `v(N)` to `v(N+1)`.. In the CloudFormation spec itself, this is not a
* problem. However, CDK will have generated actual classes and interfaces with
* the type names at `v(N)`, which people will have written code against. If the
* classes and interfaces would have a new name at `v(N+1)`, all user code would
* break.
*/
function validatePropertyTypeNameConsistency(oldSpec: any, newSpec: any) {
const newPropsTypes = newSpec.PropertyTypes ?? {};
const disappearedKeys = Object.keys(oldSpec.PropertyTypes ?? {}).filter(k => !(k in newPropsTypes));
if (disappearedKeys.length === 0) {
return;
}

const exampleJsonPatch = {
patch: {
description: 'Undoing upstream property type renames of <SERVICE> because <REASON>',
operations: disappearedKeys.map((key) => ({
op: 'move',
from: `/PropertyTypes/${key.split('.')[0]}.<NEW_TYPE_NAME_HERE>`,
path: `/PropertyTypes/${key}`,
})),
},
};

process.stderr.write([
'┌───────────────────────────────────────────────────────────────────────────────────────┐',
'│ ▐█',
'│ PROPERTY TYPES HAVE DISAPPEARED ▐█',
'│ ▐█',
'│ Some type names have disappeared from the old specification. ▐█',
'│ ▐█',
'│ This probably indicates that the service team renamed one of the types. We have ▐█',
'│ to keep the old type names though: renaming them would constitute a breaking change ▐█',
'│ to consumers of the L1 resources. ▐█',
'│ ▐█',
'└─▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▟█',
'',
'See what the renames were, check out this PR locally and add a JSON patch file for these types:',
'',
'(Example)',
'',
JSON.stringify(exampleJsonPatch, undefined, 2),
].join('\n'));
process.exitCode = 1;
}

main().catch(e => {
process.stderr.write(e.stack);
process.stderr.write('\n');
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/cfnspec/build-tools/update.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ scriptdir=$(cd $(dirname $0) && pwd)

rm -f CHANGELOG.md.new


# update-spec <TITLE> <SOURCE> <TARGETDIR> <IS_GZIPPED> <SHOULD_SPLIT> [<SVC> [...]]
function update-spec() {
local title=$1
local url=$2
Expand Down
100 changes: 15 additions & 85 deletions ...fnspec/spec-source/specification/000_cfn/500_WAFv2_RuleGroup_Rename_Properties_patch.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,99 +3,29 @@
"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",
"operations": [
{
"op": "remove",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.AllowAction"
"op": "move",
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.AllowAction",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Allow"
},
{
"op": "add",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Allow",
"value": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-allowaction.html",
"Properties": {
"CustomRequestHandling": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-allowaction.html#cfn-wafv2-rulegroup-allowaction-customrequesthandling",
"Required": false,
"Type": "CustomRequestHandling",
"UpdateType": "Mutable"
}
}
}
"op": "move",
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.BlockAction",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Block"
},
{
"op": "remove",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.BlockAction"
"op": "move",
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.CaptchaAction",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Captcha"
},
{
"op": "add",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Block",
"value": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-blockaction.html",
"Properties": {
"CustomResponse": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-blockaction.html#cfn-wafv2-rulegroup-blockaction-customresponse",
"Required": false,
"Type": "CustomResponse",
"UpdateType": "Mutable"
}
}
}
"op": "move",
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.ChallengeAction",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Challenge"
},
{
"op": "remove",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.CaptchaAction"
},
{
"op": "add",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Captcha",
"value": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-captchaaction.html",
"Properties": {
"CustomRequestHandling": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-captchaaction.html#cfn-wafv2-rulegroup-captchaaction-customrequesthandling",
"Required": false,
"Type": "CustomRequestHandling",
"UpdateType": "Mutable"
}
}
}
},
{
"op": "remove",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.ChallengeAction"
},
{
"op": "add",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Challenge",
"value": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-challengeaction.html",
"Properties": {
"CustomRequestHandling": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-challengeaction.html#cfn-wafv2-rulegroup-challengeaction-customrequesthandling",
"Required": false,
"Type": "CustomRequestHandling",
"UpdateType": "Mutable"
}
}
}
},
{
"op": "remove",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.CountAction"
},
{
"op": "add",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Count",
"value": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-countaction.html",
"Properties": {
"CustomRequestHandling": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-wafv2-rulegroup-countaction.html#cfn-wafv2-rulegroup-countaction-customrequesthandling",
"Required": false,
"Type": "CustomRequestHandling",
"UpdateType": "Mutable"
}
}
}
"op": "move",
"from": "/PropertyTypes/AWS::WAFv2::RuleGroup.CountAction",
"path": "/PropertyTypes/AWS::WAFv2::RuleGroup.Count"
}
]
}
Expand Down

0 comments on commit f511000

Please sign in to comment.