fix: stack definitions for refactor are incorrect when matching resources are different #555
Conversation
…different In most cases, when a resource exists both in the deployed stacks and the local stacks, they will be identical (up to references). But if a resource has a physical ID defined, we take use it to identify the resource, instead of computing a hash from the resource's contents. This allows the toolkit to detect correspondence between these resources even if users changed some properties while refactoring. When it comes to generating the stacks to send to the refactor API, we need to take this into account. This means taking the deployed resource as the source of truth, and only updating its references, taking them from the local stacks. To do this, we traverse the deployed resource, and every time we find a reference (either within the same stack or cross-stack), transform it to the corresponding local one (also either within- or cross-stack), using the set of resource mappings we already have.
otaviomacedo
temporarily deployed
to
integ-approval
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #555 +/- ##
==========================================
- Coverage 79.09% 79.01% -0.08%
==========================================
Files 47 47
Lines 7074 7086 +12
Branches 790 790
==========================================
+ Hits 5595 5599 +4
- Misses 1460 1468 +8
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
otaviomacedo
temporarily deployed
to
integ-approval
GitHub Actions
Inactive
otaviomacedo
temporarily deployed
to
integ-approval
GitHub Actions
Inactive
| import type * as cxapi from '@aws-cdk/cx-api'; | ||
| import { ToolkitError } from '../../toolkit/toolkit-error'; | ||
|
|
||
| export interface CloudFormationResource { |
There was a problem hiding this comment.
This is not intended to be complete, right?
There was a problem hiding this comment.
No. Just enough for what we need here.
| return new Ref(stackName, value.Ref); | ||
| } | ||
|
|
||
| constructor(public readonly stackName: string, public readonly logicalResourceId: string) { |
There was a problem hiding this comment.
You might also consider modeling it as a reference to an object that has a logicalID as an attribute.
Then the rename operation is as simple as changing the logicalID field of the resource, and re-rendering. You then do not need the map and replace operations anymore.
| } | ||
| const att = value['Fn::GetAtt']; | ||
| if (typeof att === 'string') { | ||
| const [id, attributeName] = att.split('.'); |
There was a problem hiding this comment.
I think attributeName can contain . as well, so you need to make sure to max split once.
| constructor(private readonly stackName: string, private readonly logicalIds: string[]) { | ||
| } | ||
|
|
||
| public equals(other: ResourceReference): boolean { |
There was a problem hiding this comment.
Is this what we need to say? That the entire array is the same?
| other instanceof DependsOn && | ||
| this.stackName === other.stackName && | ||
| other.logicalIds.length === this.logicalIds.length && | ||
| this.logicalIds.every((id) => other.logicalIds.includes(id)) |
There was a problem hiding this comment.
But we do allow reordering?
|
|
||
| const newInputString = this.inputString.replace(/\${([a-zA-Z0-9_.]+)}/g, (_: any, varName: string) => { | ||
| if (varName.includes('.')) { | ||
| const [logicalId, attr] = varName.split('.'); |
| } | ||
| } | ||
|
|
||
| function replaceFirst(mappings: ResourceMapping[], stackName: string, logicalId: string): string { |
There was a problem hiding this comment.
This name makes me think there can be more than one replacement; but that seems like it would be illegal anyway?
| return resolveLocalReference(fnSub); | ||
| } | ||
|
|
||
| if ('DependsOn' in value) { |
There was a problem hiding this comment.
DependsOn only has a special meaning if it is at the top-level of a resource definition, not anywhere in the template. It might not be happening (yet), but I still wouldn't want to rely on it never happening.
There was a problem hiding this comment.
Also, apparently UpdatePolicy can contain references as well. But since you're processing the entire resource's JSON object that should all automatically work already.
| } else { | ||
| // We've got a deployed resource matching a local resource. | ||
| // The final template should contain the deployed resource, but with local references. | ||
| localTemplates[stackName].Resources![logicalResourceId] = updateReferences( |
There was a problem hiding this comment.
Don't forget references in the Output block.
| .Resources?.[location.logicalResourceId]; | ||
| const deployedResource = deployedStacks.find((s) => s.stackName === location.stackName)?.template.Resources?.[ | ||
| location.logicalResourceId | ||
| ]; | ||
|
|
||
| if (deployedResource == null) { | ||
| delete localTemplates[stackName].Resources?.[logicalResourceId]; |
There was a problem hiding this comment.
I suppose we just assume/assert there will be no references to this resource when we are done with the loop, eh? Worth a comment somewhere?
BTW, thinking about this some more:
- We now take the "new" template, removing all "new" resources. So effectively, we will be left with the "old" template + modifications.
- But, any modifications aren't allowed and will lead to a deployment error.
- So if the deployment were to succeed, we would have produced the "old" template, given the new template.
If that's true, why don't we take the "old" template and apply the renames there?
|
|
||
| // After copying (an updated version of) the deployed resource, | ||
| // we may end up with dangling dependencies. Sanitize them. | ||
| sanitizeDependencies(localTemplates[stackName]); |
There was a problem hiding this comment.
How is it possible that we have problems with dangling DependsOns, but we don't have any problems with dangling Refs?
There was a problem hiding this comment.
If a resource that was pointed at via a Ref gets moved to another stack, that Ref will be converted into a Fn::ImportValue, which doesn't happen for DependsOn.
Pull request was closed
In most cases, when a resource exists both in the deployed stacks and the local stacks, they will be identical (up to references). But if a resource has a physical ID defined, we take use it to identify the resource, instead of computing a hash from the resource's contents. This allows the toolkit to detect correspondence between these resources even if users changed some properties while refactoring.
When it comes to generating the stacks to send to the refactor API, we need to take this into account. This means taking the deployed resource as the source of truth, and only updating its references, taking them from the local stacks. To do this, we traverse the deployed resource, and every time we find a reference (either within the same stack or cross-stack), transform it to the corresponding local one (also either within- or cross-stack), using the set of resource mappings we already have.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license