Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(cli): change-set diff not required for new stack diffs #29265

Closed
mrgrain opened this issue Feb 26, 2024 · 2 comments · Fixed by #29394 or #29492 · May be fixed by NOUIY/aws-solutions-constructs#98, NOUIY/aws-solutions-constructs#99 or NOUIY/aws-solutions-constructs#101
Closed

(cli): change-set diff not required for new stack diffs #29265

mrgrain opened this issue Feb 26, 2024 · 2 comments · Fixed by #29394 or #29492 · May be fixed by NOUIY/aws-solutions-constructs#98, NOUIY/aws-solutions-constructs#99 or NOUIY/aws-solutions-constructs#101
Labels
cli Issues related to the CDK CLI effort/medium Medium work item – several days of effort p1

Comments

@mrgrain
Copy link
Contributor

mrgrain commented Feb 26, 2024

Describe the bug

The change-set diff approach for new stacks is unnecessary. The whole point of change-set based diff was to fix incorrect resource replacements, however for a new stack we don't have resource replacements.

Further more, the clean-up code for change-set based diff on new stacks includes a delete-stack call. This is required to not leave behind an empty ghost stack. But it is quite spooky to see this code and a lot of delete-stack calls in CloudTrail.

Expected Behavior

Diff for new stacks does use the previous local diff implemenation.
CDK does not make any delete-stack calls when executing cdk diff.

Current Behavior

n/a

Reproduction Steps

  • create a new stack
  • run cdk diff
  • check CloudTrail

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.130.0

Framework Version

No response

Node.js Version

any

OS

any

Language

TypeScript, Python, .NET, Java, Go

Language Version

No response

Other information

No response
@mrgrain mrgrain added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 26, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Feb 26, 2024
@mrgrain mrgrain added p1 cli Issues related to the CDK CLI and removed bug This issue is a bug. package/tools Related to AWS CDK Tools or CLI labels Feb 26, 2024
@sumupitchayan sumupitchayan mentioned this issue Feb 26, 2024
Closed
@pahud pahud added effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 27, 2024
@github-actions github-actions bot mentioned this issue Mar 1, 2024
Closed
@scanlonp scanlonp mentioned this issue Mar 7, 2024
Merged
@mergify mergify bot closed this as completed in #29394 Mar 7, 2024
mergify bot pushed a commit that referenced this issue Mar 7, 2024
@scanlonp
fix(cli): prevent changeset diff for non-deployed stacks (#29394)
d33caff 
### Reason for this change

When a stack does not exist in CloudFormation, creating a changeset makes an empty `REVIEW_IN_PROGRESS` stack. We then call `delete-stack` to clean up the empty stack. However, this can cause a race condition with a deploy call. 

### Description of changes

This change prevents changeset diffs for stacks that do not yet exist in CloudFormation. This overrides the changeset diff flag. This change also adds logic for migrate stacks in the old diff logic to represent resource imports without needing the changeset present.

### Description of how you validated changes

Testing with new stacks only uses changeset diffs once the stack is deployed. Testing with new migrate stacks only uses changeset diffs once deployed. Pre-deployment the resources correctly show as imports.  

Note: the deleted test assumes the diff will be calculated using the mocked changeset. The new logic avoids the changeset, so the test is no longer relevant.

Closes #29265.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 7, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@comcalvi comcalvi reopened this Mar 14, 2024
@comcalvi comcalvi mentioned this issue Mar 14, 2024
Merged
1 task
@mergify mergify bot closed this as completed in #29492 Mar 19, 2024
mergify bot pushed a commit that referenced this issue Mar 19, 2024
@comcalvi
fix(CLI): cdk diff stack deletion causes a race condition (#29492)
067539a 
*Co-authored by*: @scanlonp

### Issue # (if applicable)

Closes #29265.

### Reason for this change

Creating a changeset for a stack that has not been deployed yet causes CFN to create a stack in state `REVIEW_IN_PROGRESS`. Previously we deleted this empty stack, but did not wait for the stack status to be `DELETE_COMPLETE`. This allowed `cdk diff` to exit while the stack status was still `DELETE_IN_PROGRESS`, which can cause subsequent CDK commands to fail, because a stack deletion operation is still in progress. 

### Description of changes

No longer create the changeset if the stack doesn't exist. Only perform the existence check if the changeset parameter is specified, to avoid a permission error when looking up a stack. 

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions GitHub Actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

ahammond pushed a commit to ahammond/aws-cdk that referenced this issue Mar 26, 2024
@comcalvi @ahammond
fix(CLI): cdk diff stack deletion causes a race condition (aws#29492)
2a35b1c 
*Co-authored by*: @scanlonp

### Issue # (if applicable)

Closes aws#29265.

### Reason for this change

Creating a changeset for a stack that has not been deployed yet causes CFN to create a stack in state `REVIEW_IN_PROGRESS`. Previously we deleted this empty stack, but did not wait for the stack status to be `DELETE_COMPLETE`. This allowed `cdk diff` to exit while the stack status was still `DELETE_IN_PROGRESS`, which can cause subsequent CDK commands to fail, because a stack deletion operation is still in progress. 

### Description of changes

No longer create the changeset if the stack doesn't exist. Only perform the existence check if the changeset parameter is specified, to avoid a permission error when looking up a stack. 

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This was referenced May 23, 2024
Open
Open
@NOUIY NOUIY mentioned this issue May 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cli Issues related to the CDK CLI effort/medium Medium work item – several days of effort p1
Projects
None yet
Milestone
No milestone
3 participants
@pahud @mrgrain @comcalvi