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.

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

CLI: stack name is not checked before delete confirmation #32545

Closed
1 task
mrgum opened this issue Dec 16, 2024 · 6 comments · Fixed by #32636 or ryichk/todolist#19
Closed
1 task

CLI: stack name is not checked before delete confirmation #32545

mrgum opened this issue Dec 16, 2024 · 6 comments · Fixed by #32636 or ryichk/todolist#19
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/medium Medium work item – several days of effort p3 package/tools Related to AWS CDK Tools or CLI

Comments

@mrgum
Copy link

mrgum commented Dec 16, 2024

Describe the bug

if you cdk destroy a stack that does not exist (say you are accidentally in the directory) cdk says

Are you sure you want to delete: (y/n)?
and does nothing when you say y

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

cdk to respond
No stacks match the name(s) whatever/you/asked/for
like cdk synth does

Current Behavior

Are you sure you want to delete: (y/n)?

Reproduction Steps

cdk destroy made/of/a/nonexistent/stack

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.173.1 (build 4eac959)

Framework Version

No response

Node.js Version

v22.11.0

OS

Mac

Language

Python

Language Version

3.13

Other information

I don't think the language is relevant

@mrgum mrgum added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 16, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 16, 2024
@khushail khushail added needs-reproduction This issue needs reproduction. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Dec 16, 2024
@khushail khushail self-assigned this Dec 16, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-reproduction This issue needs reproduction. labels Dec 16, 2024
@khushail
Copy link
Contributor

Hi @mrgum , thanks for reporting this.

Yes, I can confirm that while deleting the deployed stack with command cdk destroy, if you mention some other stack (which does not exist) being in the directory where you just deployed stack, there should be some confirmation check or warning message with the name of the stack .

Current situation -

the below mentioned stack does not exist yet the cdk destroy command simply exits and does not print anything

khushail@a07817b31bf3 testStack % cdk destroy abteststack
Are you sure you want to delete:  (y/n)? y

while deleting the one which exists -

khushail@a07817b31bf3 testStack % cdk destroy TestStackStack
Are you sure you want to delete: TestStackStack (y/n)? y
TestStackStack: destroying... [1/1]

 ✅  TestStackStack: destroyed

Looking at the code for destroy , looks like there is no check for stackname

https://github.com/aws/aws-cdk/blob/fb030d6b2a6ebd1a673b13c830389c4fc65c4dda/packages/aws-cdk/lib/cdk-toolkit.ts#L798C1-L828C8

IMO, it might include a region wide search in case , cdk destroy nonExistentStack will check if the mentioned stack name exists in the region, which might be time taking.

This would be good to have feature. Marking it as P3 to keep it open for community contribution. Requesting community upvotes to get the proper traction.

@khushail khushail added p3 effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Dec 16, 2024
@khushail khushail removed their assignment Dec 16, 2024
@khushail khushail added @aws-cdk/core Related to core CDK functionality and removed p2 labels Dec 16, 2024
@nmussy
Copy link
Contributor

nmussy commented Dec 17, 2024

IMO, it might include a region wide search in case , cdk destroy nonExistentStack will check if the mentioned stack name exists in the region, which might be time taking.

I don't know how I feel about cdk destroy being able to destroy stacks that are "outside its scope", either stacks created in other CDK apps or regular CloudFormation stacks, but I guess that's the current behavior:

$ aws cloudformation create-stack --stack-name TestStack --template-body file://template.json
$ cdk init --language=typescript

$ cdk destroy TestStack
Are you sure you want to delete: TestStack (y/n)? y
TestStack: destroying... [1/1]

 ✅  TestStack: destroyed

More than the ability to lookup valid stacks for a given region, I think it would be better to evaluate the list of stacks in the current app and give a warning that they might not belong to this application before the confirmation. I understand that there might be legitimate cases where you would want to cdk destroy an unknown stack, say where an application had that given stack but was since removed from the code base.

@khushail
Copy link
Contributor

@nmussy , I agree with you , deleting stacks outside the scope might pose a bigger risk, rather than checking stacks created in current directory/app sounds better.

@go-to-k
Copy link
Contributor

go-to-k commented Dec 20, 2024

I submitted a PR for an issue like this before, but there was a regression in other cli-integ tests and it was reverted. I'm posting it here in case it might be of some help.

issue: #27179

PR: #27921

@go-to-k
Copy link
Contributor

go-to-k commented Dec 22, 2024

Apparently, the revert of the above PR was due to a cause unrelated to that PR. That has been corrected in this PR (see: https://github.com/aws/aws-cdk/blob/v2.173.1/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts#L286-L291).
Therefore, I will submit the same PR again. This is a safe way to warn if the specified stacks don't belong to the app without searching the out-of-scope stack.

mergify bot pushed a commit that referenced this issue Jan 7, 2025
### Issue # (if applicable)

Closes #32545.
Fixes #27179.

### Reason for this change



Once this [PR](#27921) was reverted by other cli-integ test regression.

- revert PR: #29577
- the test for regression: https://github.com/aws/aws-cdk/blob/07ce8ecc42782475d099b89944571375341c28d3/packages/%40aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts#L190

But the regression was apparently due to a cause unrelated to that PR. That has been corrected in [this PR](#31107) (see: https://github.com/aws/aws-cdk/blob/v2.173.1/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts#L286-L291).

Therefore, I submit the same PR again.

### Description of changes



This PR for cli is to warn if stacks with wrong cases (=not exist) specified in `cdk destroy`.

\* It does not display the message `Are you sure you want to delete:` if there is no matching stack.
\* Even if the stack does not exist, `cdk destroy` will not fail, it will just print a warning.

Actual Outputs: 

![cdk-destroy-warn](https://github.com/user-attachments/assets/c0d70037-c863-4c78-bc22-8b51264393ac)

### Describe any new or updated permissions being added

<!— What new or updated IAM permissions are needed to support the changes being introduced ? -->

Nothing.

### Description of how you validated changes



Both of unit tests and cli-integ tests.

The changes were already approved in the last PR.

### 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*
@mergify mergify bot closed this as completed in #32636 Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 7, 2025
iankhou pushed a commit that referenced this issue Jan 13, 2025
### Issue # (if applicable)

Closes #32545.
Fixes #27179.

### Reason for this change



Once this [PR](#27921) was reverted by other cli-integ test regression.

- revert PR: #29577
- the test for regression: https://github.com/aws/aws-cdk/blob/07ce8ecc42782475d099b89944571375341c28d3/packages/%40aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts#L190

But the regression was apparently due to a cause unrelated to that PR. That has been corrected in [this PR](#31107) (see: https://github.com/aws/aws-cdk/blob/v2.173.1/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts#L286-L291).

Therefore, I submit the same PR again.

### Description of changes



This PR for cli is to warn if stacks with wrong cases (=not exist) specified in `cdk destroy`.

\* It does not display the message `Are you sure you want to delete:` if there is no matching stack.
\* Even if the stack does not exist, `cdk destroy` will not fail, it will just print a warning.

Actual Outputs: 

![cdk-destroy-warn](https://github.com/user-attachments/assets/c0d70037-c863-4c78-bc22-8b51264393ac)

### Describe any new or updated permissions being added

<!— What new or updated IAM permissions are needed to support the changes being introduced ? -->

Nothing.

### Description of how you validated changes



Both of unit tests and cli-integ tests.

The changes were already approved in the last PR.

### 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*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/medium Medium work item – several days of effort p3 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
4 participants