feat(project): tear down the stack on a deploy of an empty project, behind --yes - #2089
Conversation
a1b6cbf to
c255cdd
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2089 +/- ##
============================================
+ Coverage 97.19% 97.24% +0.04%
============================================
Files 471 471
Lines 28731 28911 +180
============================================
+ Hits 27925 28114 +189
+ Misses 806 797 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1a13ff1 to
dfb7780
Compare
378208a to
f20c060
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| ): void { | ||
| // An artifact with no environment is environment-agnostic: it deploys into | ||
| // whatever the credentials resolve to, which the account preflight checked. | ||
| if (environment === undefined) return; |
There was a problem hiding this comment.
For my own understanding: wouldn't CDK write unknown-account and unknown-region if env-agnostic? The comment on L9 says thats the case, but this comment says no env = env-agnostic.
I kind of think we want the opposite. If I clear all of my resources, then deploy, I'd expect that to delete the stack. Without this functionality, how do I destroy the AWS resources I've created with the CLI? I could see an alternative where we offer an explicit |
1763df3 to
6263477
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
Hweinstock
left a comment
There was a problem hiding this comment.
mostly nits or questions, but two things I'm hesitant about.
- the cloudformation client deviates from the existing aws client pattern and isn't injected. This means we don't cache it and make use of persistent connections, and can't leverage the existing fixtures for testing.
- we have a lot of testing at a lot of different levels, which is great, but I'm wondering if we can rely on handlers for most of this. I remember having a discussion with alex about how these tests can become friction when making changes, rather than giving us confidence we didn't break anything.
| .passthrough(); | ||
|
|
||
| /** The synthesized stack a deploy selected, and where its template lives. */ | ||
| export interface StackArtifact { |
There was a problem hiding this comment.
nit: should this be a type since its not expecting extensions or implementations?
There was a problem hiding this comment.
yep, changing to type
| }; | ||
| } | ||
|
|
||
| async function promptForTeardown( |
There was a problem hiding this comment.
very simple! I like the injection + how we re-use UserCancellationError too.
| */ | ||
| confirmTeardown: boolean; | ||
| /** Requests approval after the backend discovers that this deploy is a teardown. */ | ||
| requestTeardownConfirmation?: TeardownConfirmationHandler; |
There was a problem hiding this comment.
Would this be simpler if we inject a teardown confirmer that returns true if --y was passed, and otherwise prompts?
| export type TeardownConfirmationRequest = { | ||
| projectName: string; | ||
| targetName: string; | ||
| stackName: string; |
There was a problem hiding this comment.
is this leaking CDK specifics into this interface? If we do support non-IaC or terraform, what would we fill here?
There was a problem hiding this comment.
you are right good catch let me keep it generic here
|
|
||
| // The backend decides whether a deploy of nothing is a teardown; the manager's | ||
| // job is only to carry the user's confirmation through to it. | ||
| test("passes the teardown confirmation through to the backend", async () => { |
There was a problem hiding this comment.
is this property already covered by the handler tests where we inject the fake backend? If it didn't route, those tests wouldn't work.
There was a problem hiding this comment.
yes it is let me remove this test
| const describeStack: StackReader = async (stackName, region, credentials) => { | ||
| const { CloudFormationClient, DescribeStacksCommand } = | ||
| await import("@aws-sdk/client-cloudformation"); | ||
| const client = new CloudFormationClient({ credentials, region }); |
There was a problem hiding this comment.
I think we'll want to inject this client to follow the existing DI pattern.
There was a problem hiding this comment.
ahh good point, I’ll follow the existing injected and cached AWS client pattern here
Two review findings from #2058, both cases of deploy trusting something it had not checked. A synthesized template with no resources makes the CDK Toolkit *delete* an existing stack of that name and return as though it deployed. #2058 caught that after the fact, by which point the stack was already gone. Check the resource count before handing the assembly to the Toolkit instead. Stack selection matched on the target-name tag alone, never on the account and region the artifact was synthesized for. Those derive from the same target today and so cannot disagree, but nothing enforced it, and the Toolkit deploys where the artifact's environment points rather than where the tag says. Both fields were also being stripped on read, since the manifest schema declared neither. stackArtifactIdForTarget becomes stackArtifactForTarget, returning the template path alongside the id so the resource check needs no second read of the manifest.
…ith --yes The resource check added in the previous commit never fired. CDK writes an AWS::CDK::Metadata resource into every stack unless version reporting is disabled, so a project whose spec declares nothing still synthesizes a template with one resource in it: Object.keys(Resources).length === 0 is unreachable through the CLI. Verified against eight synthesized assemblies. Count only the resources the project asked for, and the check becomes real. Making it real needs somewhere for that deploy to go. main handles it -- an empty project plus deploy destroys the stack -- and refactor dropped that along the way, so refusing outright would trade a silent delete for a regression. Route it to an explicit toolkit.destroy() instead, gated on --yes, and report it as "Removed" rather than "Deployed" since the stack no longer exists. Destroying explicitly rather than deploying the empty template is what makes the outcome reportable: destroy fails loudly when the stack cannot be removed, where a deploy of an empty template succeeds either way. The existing guard in performCdkOperation stays as the backstop for reaching that state some other way. Two states are distinguished before anything is destroyed, both by probing CloudFormation for the stack: nothing to deploy and no stack to remove is a project that needs a resource added, not a teardown. Detection reads the synthesized template rather than counting spec collections the way main does, so a resource type added to the spec later is covered without anyone remembering to extend a list. Also corrects the comment nico flagged: an artifact with no environment is not the env-agnostic case -- CDK spells that out as aws://unknown-account/unknown-region -- it is a manifest field the cloud assembly schema leaves optional.
df52c92 to
552717b
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Hweinstock
left a comment
There was a problem hiding this comment.
lgtm, thanks for addressing earlier comments.
| ) => Promise<boolean>; | ||
|
|
||
| /** Shares CloudFormation connections for calls using the same credentials and region. */ | ||
| export function createCloudFormationStackReader( |
There was a problem hiding this comment.
ah, for my own understanding, is this to handle deploying to multiple targets?
There was a problem hiding this comment.
a single deploy is scoped to one target/region, so this isn't a multi-target thing.
it's just memoizing one CloudFormationClient per (credentials, region) so the CloudFormation reads we make (the CDKToolkit bootstrap probe and the empty-project stack probe) reuse a client instead of building a new one each time. Keying by (credentials, region) means it'd also stay correct if a caller ever spanned regions/targets
Summary
Deploying a project that declares no resources tore down its stack silently. This makes that outcome explicit, gated, and reported — and fixes the stack-selection gap found in the same review.
A deploy of nothing was a silent delete. The CDK Toolkit reads a template with no resources as an instruction to delete an existing stack of that name, and reports the run as an ordinary successful deploy.
#2058caught it after the fact, by which point the stack was gone.The first attempt at a guard here was dead code. CDK writes an
AWS::CDK::Metadataresource into every stack unless version reporting is disabled, so a project whose spec declares nothing still synthesizes a template with one resource in it —Object.keys(Resources).length === 0is unreachable through the CLI. Verified against eight real synthesized assemblies. Counting only the resources the project asked for makes the check fire.Making it fire needs somewhere for that deploy to go.
mainsupports this: empty the project, deploy, and the stack is destroyed. The refactor dropped it, so refusing outright would trade a silent delete for a regression. Instead the deploy routes to an explicittoolkit.destroy(), gated on--yes:Three states are distinguished, all before anything is destroyed:
--yesproject add runtimeDestroying explicitly rather than deploying the empty template is what makes the outcome reportable:
destroyfails loudly when the stack cannot be removed, where a deploy of an empty template succeeds either way. The empty-outputs guard inperformCdkOperationstays as the backstop for reaching that state some other way.Detection reads the synthesized template rather than counting spec collections the way
maindoes, so a resource type added to the spec later is covered without anyone remembering to extend a list.Stack selection also matched on the target-name tag alone, never on the account and region the artifact was synthesized for. Those derive from the same target today and so cannot disagree, but nothing enforced it, and the Toolkit deploys where the artifact's environment points rather than where the tag says. Both fields were being stripped on read, since the manifest schema declared neither.
stackArtifactIdForTargetbecomesstackArtifactForTarget, returning the template path and the deployed stack name alongside the id. The stack name is derived asproperties.stackName ?? artifactId, which is CDK's own derivation — so the name handed to CloudFormation is the one the Toolkit would have used.Testing
bun test src/— 1985 pass, 0 fail against these files. Unit coverage for each state in the table, for the metadata-only template that made the original guard dead, and for the ordering that makes the guards meaningful: every check runs before the Toolkit is called, so a bad target or an unconfirmed teardown fails without touching AWS.probeStackis covered for a present stack, a stack mid-rollback, an absent stack, an empty response, and a permissions failure — the last matters because reporting "no stack" on anAccessDeniedExceptionwould turn a confirmed teardown into an unexplained "add a resource" error.Notes
maindeletes orphaned harnesses and config bundles before destroying the stack, because it creates some of them imperatively. This branch creates everything through CloudFormation, so the stack deletion covers them; nothing is intentionally left behind, but flagging it in case a later imperative resource needs the same treatment.