feat(cli): add cdk orphan command to detach resources from a stack#1324
feat(cli): add cdk orphan command to detach resources from a stack#1324
cdk orphan command to detach resources from a stack#1324Conversation
|
This is actually @LeeroyHannigan's change, I'm just turning it into a PR so I can leave comments more conveniently. |
rix0rrr
left a comment
There was a problem hiding this comment.
Great start!
Initial round of comments on this.
This also needs an integ test (and to be frank probably more than 1 😉 ).
| * Get the CloudFormation SDK client for a stack's environment. | ||
| * Used by the orphaner to call DescribeStackResources. | ||
| */ | ||
| public async stackSdk(stackArtifact: cxapi.CloudFormationStackArtifact) { |
There was a problem hiding this comment.
Do we need this method? We probably should just inline this at the call site.
There was a problem hiding this comment.
Done. Removed stackSdk() from Deployments. The orphaner accesses the SDK via deployments.envs.accessStackForReadOnlyStackOperations(stack) directly.
| */ | ||
| public async loadResourceIdentifiers(available: ImportableResource[], filename: string): Promise<ImportMap> { | ||
| const contents = await fs.readJson(filename); | ||
| public async loadResourceIdentifiers(available: ImportableResource[], filenameOrJson: string): Promise<ImportMap> { |
There was a problem hiding this comment.
Not a fan of implicitly overloading an argument like this. I'd rather be explicit, like this:
type ResourceIdentifiersSource =
| { type: 'file'; fileName: string }
| { type: 'direct'; resourceIdentifiers: Record<string, ResourceIdentifierProperties> };
public async loadResourceIdentifiers(available: ImportableResource[], source: ResourceIdentifiersSource): Promise<ImportMap> {Or honestly, even simpler
public async loadResourceIdentifiersFromFile(available: ImportableResource[], fileName: string): Promise<ImportMap> {
const contents = /* load file */;
return this.loadResourceIdentifiers(available, contents);
}
public async loadResourceIdentifiers(available: ImportableResource[], identifiers: Record<string, ResourceIdentifierProperties>): Promise<ImportMap> {There was a problem hiding this comment.
Done. Split into loadResourceIdentifiersFromFile(available, fileName) which loads the file and calls loadResourceIdentifiers(available, identifiers). The CLI layer decides which to call. Also added --resource-mapping-inline as a separate CLI option for passing JSON directly (used by cdk orphan output).
| } | ||
|
|
||
| public async orphan(options: OrphanOptions) { | ||
| const stacks = await this.selectStacksForDeploy(options.selector, true, true, false); |
There was a problem hiding this comment.
If the argument is one or more construct paths (which it should be) then the stack selection is implicit. No need for the user to pick the stack.
| } | ||
|
|
||
| const stack = stacks.stackArtifacts[0]; | ||
| await this.ioHost.asIoHelper().defaults.info(chalk.bold(`Orphaning construct '${options.constructPath}' from ${stack.displayName}`)); |
There was a problem hiding this comment.
We should split this into "plan" and "execute" stages:
- First determine actually which constructs are going to be orphaned, by evaluating the construct path. And include what resources are going to have their references replaced with literals. It should probably be an error if there are 0. Then we print a proper report of what we're going to do to the user (not a guess at the result of their instruction). We should probably give them a chance to confirm as well.
- Only after they confirm do we do the actual work.
I would like to see that in the API in some way. For example:
class Orphaner {
constructor(...) { }
public async makePlan(...): Promise<OrphanPlan> {
// ...
}
}
class OrphanPlan {
// The properties below here are purely hypothetical to show the idea! I have not done enough
// mental design to think about whether these are the best to expose.
public readonly orphanedResoures: OrpanPlanResource[];
public readonly affectedResources: OrpanPlanResource[];
public readonly stacks: OrphanPlanStack[];
public async execute() {
// ...
}
}
class Toolkit {
public async orphan(...) {
// And then something like this
const orphaner = new Orphaner(...):
const plan = await orphaner.makePlan(...);
await showPlanToUser(plan, this.ioHost);
const yes = await this.ioHost.confirm(...);
if (yes) {
await plan.execute();
}
}
}There was a problem hiding this comment.
Done. Implemented exactly as described:
makePlan(stack, constructPaths)→ returnsOrphanPlanwithstackName,orphanedResources(each withlogicalId,resourceType,cdkPath), and anexecute()method. Read-only, no deployments.Toolkit.orphan()shows the plan, then confirms viarequestResponse(skippable with--force).plan.execute()runs the 3 CloudFormation deployments, returnsOrphanResultwithresourceMapping.
| roleArn: options.roleArn, | ||
| toolkitStackName: this.toolkitStackName, |
There was a problem hiding this comment.
Feels like both of these should be constructor arguments.
There was a problem hiding this comment.
Done. roleArn and toolkitStackName are on ResourceOrphanerProps (the constructor), not on the orphan() method options.
| const cfn = sdk.cloudFormation(); | ||
|
|
||
| // Get physical resource IDs (Ref values) from CloudFormation | ||
| const describeResult = await cfn.describeStackResources({ StackName: stack.stackName }); |
There was a problem hiding this comment.
nice catch, its not (truncates at 100). I used listStackResources which is paginated, instead. This meant I needed permissions on the CLI role, to run integ tests. LMK if thats not correct.
| } | ||
| } | ||
|
|
||
| private replaceInObject(obj: any, logicalId: string, values: { ref: string; attrs: Record<string, string> }): any { |
There was a problem hiding this comment.
Doesn't use this so doesn't need to be a method. Could be a helper function.
And in fact should be since it operates on a CFN template.
There was a problem hiding this comment.
Done, walkObject, replaceInObject, replaceReferences, removeDependsOn, findResourcesByPath, findBlockingResources, hasAnyCdkPathMetadata, and assertSafeDeployResult are all standalone functions in actions/orphan/private/helpers.ts. None of them are methods on the class.
There was a problem hiding this comment.
Kept them scoped to orphan for now since no other code needs them yet, happy to move to a shared location if you'd prefer, or as a follow-up when other commands need similar utilities.
| return result; | ||
| } | ||
|
|
||
| private removeDependsOn(template: any, logicalId: string): void { |
There was a problem hiding this comment.
Doesn't use this so doesn't need to be a method. Could be a helper function.
And in fact should be since it operates on a CFN template.
There was a problem hiding this comment.
Done, walkObject, replaceInObject, replaceReferences, removeDependsOn, findResourcesByPath, findBlockingResources, hasAnyCdkPathMetadata, and assertSafeDeployResult are all standalone functions in actions/orphan/private/helpers.ts. None of them are methods on the class.
| } | ||
| } | ||
|
|
||
| private walkObject(obj: any, visitor: (value: any) => void): void { |
There was a problem hiding this comment.
Doesn't use this so doesn't need to be a method. Could be a helper function.
And in fact should be since it operates on a CFN template.
There was a problem hiding this comment.
Done, walkObject, replaceInObject, replaceReferences, removeDependsOn, findResourcesByPath, findBlockingResources, hasAnyCdkPathMetadata, and assertSafeDeployResult are all standalone functions in actions/orphan/private/helpers.ts. None of them are methods on the class.
Head branch was pushed to by a user without write access
82e0371 to
8ad5411
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1324 +/- ##
==========================================
- Coverage 88.29% 88.14% -0.15%
==========================================
Files 73 74 +1
Lines 10386 10494 +108
Branches 1413 1428 +15
==========================================
+ Hits 9170 9250 +80
- Misses 1189 1216 +27
- Partials 27 28 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Put an item in the table before orphan | ||
| const itemFile = path.join(fixture.integTestDir, 'test-item.json'); | ||
| await fs.writeFile(itemFile, JSON.stringify({ PK: { S: 'before-orphan' } })); | ||
| await fixture.shell([ |
There was a problem hiding this comment.
We are in a programming language, and we have an AWS SDK available to us (or we could without a lot of effort). I would prefer us using @aws-sdk/client-dynamodb directly.
There was a problem hiding this comment.
Wasn't sure how to easily import the SDK, but its done now, thanks.
| new GetTemplateCommand({ StackName: stackName }), | ||
| ); | ||
| const templateBody = yaml.parse(templateAfter.TemplateBody!); | ||
| expect(templateBody.Resources).not.toHaveProperty('MyTable794EDED1'); |
There was a problem hiding this comment.
This would also silently succeed if the logical ID changed, for some reason. We should also check that the expected resource is present before we start orphaning.
There was a problem hiding this comment.
Added expect(templateBodyBefore.Resources).toHaveProperty('MyTable794EDED1') before orphan
|
|
||
| // Verify the Lambda still exists and its env vars have been replaced with literals | ||
| // (Ref -> physical table name, GetAtt -> physical ARN) | ||
| const lambdaResource = Object.values(templateBody.Resources).find( |
There was a problem hiding this comment.
This is very hard to read, and if any of these steps fail the output will not be helpful to diagnose what the problem is.
I'd rather use a single structural assertion like:
expect(templateBody).toMatchObject({
Resources: {
MyFunction1234: {
Properties: {
...
}
}
}
});etc.
There was a problem hiding this comment.
Single toMatchObject with expect.objectContaining and expect.stringContaining
| '--region', fixture.aws.region, | ||
| '--output', 'json', | ||
| ]); | ||
| expect(getItemOutput).toContain('before-orphan'); |
There was a problem hiding this comment.
We need to clean up that table otherwise it will leak.
Stick it in a try/finally, I really don't want to leak it even if the test fails.
There was a problem hiding this comment.
Test body in try, DeleteTableCommand in finally
| * Construct path prefix(es) to orphan. Each path must be in the format | ||
| * `StackName/ConstructPath`, e.g. `MyStack/MyTable`. | ||
| * | ||
| * The stack is derived from the path — all paths must reference the same stack. |
There was a problem hiding this comment.
Ah yeah? 😅
Bummer. But acceptable for now I guess.
|
|
||
| for (const output of stackDesc.Stacks?.[0]?.Outputs ?? []) { | ||
| if (!output.OutputKey || !output.OutputValue) continue; | ||
| const ref = getAttRefs.find(r => r.outputKey === output.OutputKey); |
There was a problem hiding this comment.
If getAttRefs was a map or an object, we wouldn't need to search a list here.
Doubly-indexing maps is a bit brain breaking but perfectly legit and more concise.
| // Only include the primary resource for each construct path | ||
| // e.g. for path "MyTable", match "StackName/MyTable/Resource" exactly | ||
| const cdkPath = resource.Metadata?.[PATH_METADATA_KEY] ?? ''; | ||
| const primaryPaths = constructPaths.map(p => `${stack.stackName}/${p}/Resource`); | ||
| if (!primaryPaths.includes(cdkPath)) continue; |
There was a problem hiding this comment.
This smells to me. I can't be bothered to try to understand what this is doing exactly, but it doesn't feel right.
Why the interpolation with stackName? Why only exactly this L1 reosurce? Why includes(cdkPath) instead of startsWith(cdkPath)?
What are we trying to do here?
There was a problem hiding this comment.
Removed the path filter entirely. Will need to discuss.
| } catch { | ||
| await this.ioHelper.defaults.debug('Could not retrieve resource identifier summaries'); |
There was a problem hiding this comment.
Wait what?
There was an exception, we're not stopping, we're not even logging the except, we're just chugging along as if nothing happened? And whispering that "something might be off" at debug level?
There was a problem hiding this comment.
Now warn with the actual error message instead of debug with a generic string. Added unit test.
| const thisStack = p.substring(0, slashIdx); | ||
| const constructPath = p.substring(slashIdx + 1); |
There was a problem hiding this comment.
Unfortunately a construct ID !== a stack name.
"Construct ID" is a CDK concept, "Stack Name" is a CloudFormation concept.
Yes, by default the stack name of a Stack is the same as its Construct ID, but not necessarily.
new Stack(this, 'A', { // <-- construct ID, in the path
stackName: 'B' // <-- Stack Name in CFN
});The translation of construct path to stack name needs to go through the assembly. To be frank, I don't know the best way offhand (it should probably be somewhere in the manifest), but perhaps the magic black box can come up with an idea.
There was a problem hiding this comment.
Done, now matches stack.hierarchicalId instead of using the path segment as a CloudFormation stack name pattern
| await this.ioHost.asIoHelper().defaults.error((e as Error).message); | ||
| throw e; |
There was a problem hiding this comment.
Logging and rethrowing is usually an antipattern, leading to duplicated error messages in the output. Is this what we do in other places?
There was a problem hiding this comment.
I was following the refactor implementation here:
But I'll remove the try catch
| try { | ||
| await this.toolkit.orphan(this.props.cloudExecutable, { | ||
| constructPaths: options.constructPath, | ||
| roleArn: options.roleArn, | ||
| toolkitStackName: options.toolkitStackName, | ||
| force: options.force, | ||
| }); | ||
| } catch (e) { | ||
| await this.ioHost.asIoHelper().defaults.error((e as Error).message); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
| try { | |
| await this.toolkit.orphan(this.props.cloudExecutable, { | |
| constructPaths: options.constructPath, | |
| roleArn: options.roleArn, | |
| toolkitStackName: options.toolkitStackName, | |
| force: options.force, | |
| }); | |
| } catch (e) { | |
| await this.ioHost.asIoHelper().defaults.error((e as Error).message); | |
| throw e; | |
| } | |
| await this.toolkit.orphan(this.props.cloudExecutable, { | |
| constructPaths: options.constructPath, | |
| roleArn: options.roleArn, | |
| toolkitStackName: options.toolkitStackName, | |
| force: options.force, | |
| }); |
Adds a new CLI command that safely removes resources from a CloudFormation
stack without deleting them, enabling resource type migrations (e.g.
DynamoDB Table to GlobalTable).
`cdk orphan --path <ConstructPath>` will:
- Find all resources under the construct path via aws:cdk:path metadata
- Resolve {Ref} values via DescribeStackResources
- Resolve {Fn::GetAtt} values by injecting temporary stack Outputs
- Set DeletionPolicy: Retain on all matched resources
- Remove the resources from the stack
- Output an inline `cdk import` command with the resource mapping
Also adds inline JSON support for `--resource-mapping` in `cdk import`,
and exposes `stackSdk()` on Deployments for read-only SDK access.
1036138 to
211b87e
Compare
deea40c to
06a93bd
Compare
06a93bd to
616d59a
Compare
| # `cdk import` | ||
| - cloudformation:GetTemplateSummary | ||
| # `cdk orphan` | ||
| - cloudformation:ListStackResources |
There was a problem hiding this comment.
This needs an update to the bootstrap stack version as well (in 2 places)
Adds a new CLI command that safely removes resources from a CloudFormation stack without deleting them, enabling resource type migrations (e.g. DynamoDB Table to GlobalTable).
cdk orphan --path <ConstructPath>will:cdk importcommand with the resource mappingAlso adds inline JSON support for
--resource-mappingincdk import, and exposesstackSdk()on Deployments for read-only SDK access.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license