fix: change-set diff can miss changes to JSON-typed properties - #1785
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Pull request was converted to draft
…ties CloudFormation can drop an entire ResourceChange from a DescribeChangeSet response that is requested with IncludePropertyValues, when it fails property validation while rendering the values. The only trace is a [WARN] in StatusReason; Status stays CREATE_COMPLETE. Because the diff merger treats a resource that is absent from the change set as unchanged, the template diff for that resource was erased and `cdk diff` reported no differences. Describe every change set twice, with and without property values, and merge the two responses so dropped resource changes and dropped per-property details are restored. All pages of Changes are now always fetched as well, so a truncated list can no longer make a caller believe a resource is unchanged. Collect the change set describing logic into a ChangeSetDescriber class in the new api/change-sets module, so that every caller goes through the same code path and the distinction between reading the current state, waiting for the change set to settle, and requiring an executable change set is explicit.
| if (changeSetDescription.Status !== 'CREATE_COMPLETE') { | ||
| const status = changeSetDescription.Status ?? 'UNKNOWN'; | ||
| const reason = changeSetDescription.StatusReason ? `: ${changeSetDescription.StatusReason}` : ''; | ||
| throw new ToolkitError( |
There was a problem hiding this comment.
PR body states:
A change set that is not usable previously raised ToolkitError('ChangeSetNotReady') with "is not ready for execution (status: …)". It now raises DeploymentError
I don't actually see where a DeploymentError would be thrown though. What i'm seeing is that await this.cfn.executeChangeSet will be called and throw a raw SDK error - doesn't seem expected.
There was a problem hiding this comment.
this.cfn.executeChangeSet happened before. I just collapsed the single use private method executeChangeSet into checkAndExecuteChangeSet.
The DeploymentError is now thrown at call sites of checkAndExecuteChangeSet. In order to get a ChangeSetReport this is checked.
There was a problem hiding this comment.
Where is DeploymentError thrown? Prior to this if a changeset was in CREATE_PENDING this function would immediately throw a ToolkitError, now I see it would reach await this.cfn.executeChangeSet:
No?
There was a problem hiding this comment.
Oh you mean inside describeForExecution - got it 👍
3 participants
Fixes #1780
Since 2.1129.0,
cdk diffcan report "There were no differences" for a change you actually made. It happens when:ConfigurationonAWS::QBusiness::DataSourcecdk diffwith the default--method=auto, or with--method=change-setThe change is invisible in the diff, but
cdk deployapplies it, and--method=templateshows it. Anything else changing on the same resource is hidden too.The cause is on the CloudFormation side. To tell you whether a change replaces a resource, we ask CloudFormation to describe the change set including property values. For a JSON blob it cannot validate, CloudFormation drops the whole resource from that response and only mentions it in a warning. We read the empty response as "nothing changed here" and threw away the correct answer we already had.
We now notice that warning and ask CloudFormation a second time, without property values, then combine both answers. You get the replacement information where it is available, and the change is no longer lost where it is not.
Diffs of stacks with many changes were affected by the same blind spot: only the first page of changes was ever read, so changes beyond it were silently dropped. All pages are now read.
Behaviour changes worth mentioning
deploynow asks CloudFormation for property values too.describeCurrentStatealways sendsIncludePropertyValues: true, and the deploy path reaches it throughwaitAndThrowOnProblem. Previously onlydiffrequested property values;deploydescribed change sets without them. Worth knowing because it widens where the parameter is used, beyond the command this fix is about.--method=execute-change-seterrors changed class and wording. A change set that is not usable previously raisedToolkitError('ChangeSetNotReady')with "is not ready for execution (status: …)". It now raisesDeploymentError, and the message depends on why: "is still being created" while it is still being created, "cannot be executed (STATUS)" for a change set that is empty or gone, and the full resource-level diagnosis for one that failed to create. The last of those is the reason for the change.Technical notes
The trigger is a
[WARN] --include-property-values option can return incomplete ChangeSet dataprefix inStatusReason;StatusstaysCREATE_COMPLETE, so this is the only signal available. It appears only when data was actually omitted, so the secondDescribeChangeSetcall is made only when needed.Change set describing moved into a
ChangeSetDescriberclass in the newapi/change-setsmodule, replacing free functions and scattereddescribeChangeSetcalls. Its methods name what the caller gets:describeCurrentState(any state),waitForSettled(done being created),waitForReport(plus diagnosis),waitAndThrowOnProblem, anddescribeForExecution(usable for execution). The last one exists because an empty or deleted change set is a normal no-op fordeploybut cannot be executed, sodiagnoseChangeSetgained arequireExecutableoption and achange-set-not-readyproblem source instead of callers re-checking status themselves.StackDiagnosisvalues are wrapped in an internalDiagnosisclass so callers can writediagnosis.throwOnError(). The publicStackDiagnosisunion andDiagnosedStack.resultare unchanged, butStackProblemSourcegains thechange-set-not-readyvariant, which is worth a look as that type is public.Covered by unit tests per merge scenario, including the exact response CloudFormation returns for #1780, and a
toolkit.diffreproduction that fails without the fix.There is no integration test, because this cannot practically be tested in CI. As detailed in #1780 (comment), only three resource types in the entire registry declare a property with an empty schema and so trigger the behaviour:
AWS::QBusiness::DataSource.Configuration,AWS::ControlTower::LandingZone.Manifest, andAWS::QuickSight::Flow.FlowDefinition. The latter two need a Control Tower landing zone or a QuickSight subscription, and Amazon Q Business closes to new customers on 31 July 2026, so a fixture built on it would stop working in the clean accounts CI uses. A working QBusiness reproduction was used to verify the fix by hand instead.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license