fix(diff): properties from ChangeSet diff were ignored #30268
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| // The SDK is only used to reference `DescribeChangeSetOutput`, so the SDK is added as a devDependency. | ||
| // The SDK should not make network calls here | ||
| import type { DescribeChangeSetOutput as DescribeChangeSet, ResourceChangeDetail } from '@aws-sdk/client-cloudformation'; | ||
| import { diffResource } from '.'; | ||
| import * as types from '../diff/types'; | ||
|
|
||
| export type DescribeChangeSetOutput = DescribeChangeSet; | ||
|
|
||
| /** | ||
| * The purpose of this class is to include differences from the ChangeSet to differences in the TemplateDiff. | ||
| */ | ||
| export class TemplateAndChangeSetDiffMerger { | ||
|
There was a problem hiding this comment. I love this. This is a great refactor! |
||
| // If we somehow cannot find the resourceType, then we'll mark it as UNKNOWN, so that can be seen in the diff. | ||
| private UNKNOWN_RESOURCE_TYPE = 'UNKNOWN'; | ||
|
|
||
| changeSet: DescribeChangeSetOutput; | ||
| currentTemplateResources: {[logicalId: string]: any}; | ||
| changeSetResources: types.ChangeSetResources; | ||
|
|
||
| constructor( | ||
| args: { | ||
| changeSet: DescribeChangeSetOutput; | ||
| currentTemplateResources: {[logicalId: string]: any}; | ||
| }, | ||
| ) { | ||
| this.changeSet = args.changeSet; | ||
| this.currentTemplateResources = args.currentTemplateResources; | ||
| this.changeSetResources = this.inspectChangeSet(this.changeSet); | ||
| } | ||
|
|
||
| inspectChangeSet(changeSet: DescribeChangeSetOutput): types.ChangeSetResources { | ||
| const replacements: types.ChangeSetResources = {}; | ||
| for (const resourceChange of changeSet.Changes ?? []) { | ||
| if (resourceChange.ResourceChange?.LogicalResourceId === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| const propertiesReplaced: types.ChangeSetProperties = {}; | ||
| for (const propertyChange of resourceChange.ResourceChange.Details ?? []) { | ||
| if (propertyChange.Target?.Attribute === 'Properties' && propertyChange.Target.Name) { | ||
| propertiesReplaced[propertyChange.Target.Name] = { | ||
| changeSetReplacementMode: this.determineChangeSetReplacementMode(propertyChange), | ||
| beforeValue: propertyChange.Target.BeforeValue, | ||
| afterValue: propertyChange.Target.AfterValue, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| replacements[resourceChange.ResourceChange.LogicalResourceId] = { | ||
| resourceWasReplaced: resourceChange.ResourceChange.Replacement === 'True', | ||
|
There was a problem hiding this comment. Are we sure There was a problem hiding this comment. I guess line 80 would be our check for this? There was a problem hiding this comment. yeah, I'm trying my best to be super paranoid in this PR about running into anything that's undefined... part of me wants to wrap the entire changeset operation in try-catch and just continue with template diff if the changeset operations fail... but that would probably not be good in that it would mask many issues. |
||
| resourceType: resourceChange.ResourceChange.ResourceType ?? this.UNKNOWN_RESOURCE_TYPE, // DescribeChanegSet doesn't promise to have the ResourceType... | ||
| properties: propertiesReplaced, | ||
| }; | ||
| } | ||
|
|
||
| return replacements; | ||
| } | ||
|
|
||
| determineChangeSetReplacementMode(propertyChange: ResourceChangeDetail): types.ChangeSetReplacementMode { | ||
| if (propertyChange.Target!.RequiresRecreation === 'Always') { | ||
| switch (propertyChange.Evaluation) { | ||
| case 'Static': | ||
| return 'Always'; | ||
| case 'Dynamic': | ||
| // If Evaluation is 'Dynamic', then this may cause replacement, or it may not. | ||
| // see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html | ||
| return 'Conditionally'; | ||
| } | ||
| } | ||
|
|
||
| return propertyChange.Target!.RequiresRecreation as types.ChangeSetReplacementMode; | ||
| } | ||
|
|
||
| /** | ||
| * Finds resource differences that are only visible in the changeset diff from CloudFormation (that is, we can't find this difference in the diff between 2 templates) | ||
| * and adds those missing differences to the templateDiff. | ||
| * | ||
| * - One case when this can happen is when a resource is added to the stack through the changeset. | ||
| * - Another case is when a resource is changed because the resource is defined by an SSM parameter, and the value of that SSM parameter changes. | ||
| */ | ||
| addChangeSetResourcesToDiff(resourceDiffs: types.DifferenceCollection<types.Resource, types.ResourceDifference>) { | ||
| for (const [logicalId, changeSetResource] of Object.entries(this.changeSetResources)) { | ||
| const resourceNotFoundInTemplateDiff = !(resourceDiffs.logicalIds.includes(logicalId)); | ||
| if (resourceNotFoundInTemplateDiff) { | ||
| const resourceDiffFromChangeset = diffResource( | ||
| this.convertResourceFromChangesetToResourceForDiff(changeSetResource, 'OLD_VALUES'), | ||
| this.convertResourceFromChangesetToResourceForDiff(changeSetResource, 'NEW_VALUES'), | ||
| ); | ||
| resourceDiffs.set(logicalId, resourceDiffFromChangeset); | ||
| } | ||
|
|
||
| const propertyChangesFromTemplate = resourceDiffs.get(logicalId).propertyUpdates; | ||
| for (const propertyName of Object.keys(this.changeSetResources[logicalId]?.properties ?? {})) { | ||
| if (propertyName in propertyChangesFromTemplate) { | ||
| // If the property is already marked to be updated, then we don't need to do anything. | ||
| continue; | ||
| } | ||
|
|
||
| // This property diff will be hydrated when enhanceChangeImpacts is called. | ||
| const emptyPropertyDiff = new types.PropertyDifference({}, {}, {}); | ||
| emptyPropertyDiff.isDifferent = true; | ||
| resourceDiffs.get(logicalId).setPropertyChange(propertyName, emptyPropertyDiff); | ||
| } | ||
| } | ||
|
|
||
| this.enhanceChangeImpacts(resourceDiffs); | ||
| } | ||
|
|
||
| convertResourceFromChangesetToResourceForDiff( | ||
| resourceInfoFromChangeset: types.ChangeSetResource, | ||
| parseOldOrNewValues: 'OLD_VALUES' | 'NEW_VALUES', | ||
| ): types.Resource { | ||
| const props: { [logicalId: string]: string | undefined } = {}; | ||
| if (parseOldOrNewValues === 'NEW_VALUES') { | ||
| for (const [propertyName, value] of Object.entries(resourceInfoFromChangeset.properties ?? {})) { | ||
| props[propertyName] = value.afterValue; | ||
| } | ||
| } else { | ||
| for (const [propertyName, value] of Object.entries(resourceInfoFromChangeset.properties ?? {})) { | ||
| props[propertyName] = value.beforeValue; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| Type: resourceInfoFromChangeset.resourceType ?? this.UNKNOWN_RESOURCE_TYPE, | ||
| Properties: props, | ||
| }; | ||
| } | ||
|
|
||
| enhanceChangeImpacts(resourceDiffs: types.DifferenceCollection<types.Resource, types.ResourceDifference>) { | ||
| resourceDiffs.forEachDifference((logicalId: string, change: types.ResourceDifference) => { | ||
| if ((!change.resourceTypeChanged) && change.resourceType?.includes('AWS::Serverless')) { | ||
| // CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources | ||
| return; | ||
| } | ||
| change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => { | ||
| if (type === 'Property') { | ||
| if (!this.changeSetResources[logicalId]) { | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
| (value as types.PropertyDifference<any>).isDifferent = false; | ||
| return; | ||
| } | ||
|
|
||
| const changeSetReplacementMode = (this.changeSetResources[logicalId]?.properties ?? {})[name]?.changeSetReplacementMode; | ||
| switch (changeSetReplacementMode) { | ||
| case 'Always': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE; | ||
| break; | ||
| case 'Never': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE; | ||
| break; | ||
| case 'Conditionally': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE; | ||
| break; | ||
| case undefined: | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
| (value as types.PropertyDifference<any>).isDifferent = false; | ||
| break; | ||
| // otherwise, defer to the changeImpact from `diffTemplate` | ||
| } | ||
| } else if (type === 'Other') { | ||
| switch (name) { | ||
| case 'Metadata': | ||
| change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue)); | ||
| break; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| addImportInformation(resourceDiffs: types.DifferenceCollection<types.Resource, types.ResourceDifference>) { | ||
|
There was a problem hiding this comment.
|
||
| const imports = this.findResourceImports(); | ||
| resourceDiffs.forEachDifference((logicalId: string, change: types.ResourceDifference) => { | ||
| if (imports.includes(logicalId)) { | ||
| change.isImport = true; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| findResourceImports(): string[] { | ||
|
There was a problem hiding this comment.
|
||
| const importedResourceLogicalIds = []; | ||
| for (const resourceChange of this.changeSet.Changes ?? []) { | ||
| if (resourceChange.ResourceChange?.Action === 'Import') { | ||
| importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId!); | ||
| } | ||
| } | ||
|
|
||
| return importedResourceLogicalIds; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was refactored into multiple functions in the new class