-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cli): improved interactions for NestedStack and NestedStack resources #7805
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
8356bf7
to
a3bd47e
Compare
There seems to be no unit tests for |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's desirable to deeply recurse to all nested stacks and not just children of the top-level stack.
packages/aws-cdk/test/integ/cli/test-cdk-nested-stack-status.sh
Outdated
Show resolved
Hide resolved
packages/aws-cdk/lib/api/util/cloudformation/stack-activity-monitor.ts
Outdated
Show resolved
Hide resolved
a3bd47e
to
eaf88fe
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
eaf88fe
to
7f1fc96
Compare
Done! PS: Will try to cover other related issues in this PR. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
1e72cfc
to
93dd0bc
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@andrestone I've added this PR to my list of things to get through. stay tuned! |
This feature is incredibly important to me: current process requires deploying and manually checking CloudFormation UI for changes as my CDK app is a composition of nested stacks. If I can in any way assist this PR to expedite it's approval, please let me know. |
@damonmcminn I'll start reviewing it tomorrow. You are more than welcome to help and provide feedback! A good starting point might be to just pull this PR down to build it and use it locally. I haven't played enough with Nested Stacks so that'll be an exercise I will be going through as well. It will help with providing feedback on the experience, usability gaps, wishlist, etc. |
@andrestone good call. we recently changed all the CLI tests to leverage jest rather than the scripts that were there previously. |
Ping me if you want to chat or something. The less semantic portion of the PR is what makes it backwards compatible by inferring the deeply nested stacks relation to their parent resource (it uses the S3 version Key reference present in the asset). There are many improvements that can be done, but I guess the PR deservers conceptual / design review first. |
I'll get on this today and see what feedback I can provide: at a minimum I'll be able to provide something on the UX. Thanks @andrestone for doing this! |
This has been my process:
Expected: no changes or metadata changes only I am not at liberty to share the output of running the command, but am happy to share privately. |
I'll merge changes so you can clone the repo, build on master and then use build up from the changed packages only. The deeply nested stack might have their parent resources recreated as the hash might have changed due to the metadata field now present in the template. As mentioned before, I'm not sure this is the best way to represent this relation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrestone I'm not close to being done - but I've started and will continue. review fatigue catches up with me after about an hour. I pinged you and hope we can talk through some of this stuff.
A general thought that occurred to me is whether we can scaffold this feature out in a meaningful way or do you see this entire feature list as being an all or nothing. I think we can work through it a bit faster if we can break up the PR into smaller chunks.
Stack.of(this).templateOptions.metadata = { | ||
[cxapi.PATH_METADATA_KEY]: this.node.uniqueId, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth a comment to explain why it's needed and what it represents
@@ -127,6 +128,50 @@ export class CloudFormationDeployments { | |||
return stack.template(); | |||
} | |||
|
|||
public async readCurrentNestedStackTemplates(stackArtifact: CloudFormationStackArtifact): Promise<Array<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add docs strings
if (e.code === 'ValidationError' && e.message === `Stack with id ${stackArtifact.stackName} does not exist`) { | ||
return new Array<{ parentPath?: string, parentResourceId: string, template: Template }>(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a debug statement here to capture that we've swallowed up the exception.
it should describe what we interpret this state to be. might be a useful marker when we debug.
} | ||
} | ||
|
||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like a long method, what do you think about refactoring some functionality out
@@ -0,0 +1,16 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these will need to be updated now that all our integ tests are run through Jest
try { | ||
return await this.cfn.describeStackEvents({StackName: stackName, NextToken: nextToken}).promise(); | ||
} catch (e) { | ||
if (e.code === 'ValidationError' && e.message === `Stack [${this.stackName}] does not exist`) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're using a similar check in cloudformation-deployments, can we DRY it up a bit?
} | ||
|
||
return events; | ||
} | ||
|
||
private async readCfnEvents(stackName: string, nextToken: string | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private async readCfnEvents(stackName: string, nextToken: string | undefined) { | |
private async readCfnEvents(stackName: string, nextToken?: string) { |
const ret = new Array<{ parentPath?: string, parentResourceId: string, template: any }>(); | ||
const resources = [{resources: stack.template?.Resources, parentStackResources: {} as any, parentStackId: stack.stackName}]; | ||
const nestedStackAssets = stack.assets.filter((ast) => { | ||
return ast.path && ast.path.includes('nested.template.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these being read from the location pointed to by the manifest.json file?
private findS3VersionKeyForNestedStackResource( | ||
running: { resources: any; parentStackId: string; parentStackResources: any }, | ||
key: string, | ||
resources: Array<{ resources: any; parentStackId: string; parentStackResources: any }>) { | ||
const url = JSON.stringify(running.resources[key].Properties?.TemplateURL); | ||
let s3Key = url.match(/\w+S3VersionKey\w+/g)?.[0]; | ||
if (!!s3Key && s3Key.includes('referenceto')) { | ||
if (Object.keys(running.parentStackResources[running.parentStackId].Properties?.Parameters || {}).includes(s3Key)) { | ||
s3Key = running.parentStackResources[running.parentStackId].Properties?.Parameters[s3Key].Ref; | ||
} | ||
if (!!s3Key && s3Key.includes('referenceto')) { | ||
let resUtil = [...resources]; | ||
while (resUtil.length > 0) { | ||
const res = resUtil.pop() as any; | ||
if (Object.keys(res.parentStackResources[res.parentStackId]?.Properties?.Parameters || {}).includes(s3Key!)) { | ||
s3Key = res.parentStackResources[res.parentStackId].Properties?.Parameters[s3Key!].Ref; | ||
if (s3Key!.includes('referenceto')) { | ||
resUtil = [...resources]; | ||
} else { | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
return s3Key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth breaking the nested stack parsing out into it's own class so it's more unit-testable. Relying on integ tests has put us in a place that we would like to get out of by raising the unit test coverage.
@shivlaks Thanks for reviewing. I'll try to get back to this by the weekend. However, I'd like to be sure the concept is understood and accepted, before working on optimisations. Key points here are:
It would be nice to have these settled before refactoring / cleaning. |
@andrestone absolutely, I didn't realize it publishes all the other comments. They're just noise right now and I wouldn't prioritize them as they're mainly style/organization. intended to only publish the summary comment. sorry about that! I'm basically writing up a mini-RFC and starting to play with it locally to go over the design, concepts, approach, and alternatives (haven't gotten to this yet). I do think this particular issue requires this exercise. Absolutely agree that the approach is the #1 thing to review and validate here. |
It's been 29 days since the last activity on this PR and this is something I am very interested in using. Thank you all for the work so far. Is there any way that I can help keep it moving? |
This PR is waiting for a conceptual review from the CDK team (ping @SomayaB). It actually looks more like an RFC, even more so due to the volume of changes on the master branch. |
@andrestone @armilam - sorry, I'd started the RFC and had to divert my attention. Thanks for the reminder, I'll pick this back up this week and get the RFC into a review where we can continue the discussion. |
Any update on this? |
This would be a really nice feature to have soon :) any updates ? |
Any updates? I still don't know if I should use a nested stack only for that reason. |
This PR is obsolete due to new CloudFormation support for nested stack change sets: https://aws.amazon.com/about-aws/whats-new/2020/11/aws-cloudformation-change-sets-now-support-nested-stacks/ |
@andrestone when will we see that feature (CloudFormation support for nested stack change sets) make its way into cdk to resolve this ticket ? |
Commit Message
feat(cli): improved interactions for NestedStack and NestedStack resources
NestedStack and NestedStack resources outputs for various workflows
End Commit Message
This is a WIP
Todo
diff
worksdeploy
anddestroy
workdeploy
security warning workssometimes we miss / don't flush messages for deploy / destroy (debug)(bug existent / not introduced by these changes)Discussions
Immutable nature of Assets and NestedStacks traceability
Since NestedStacks aren't regular, deployable stacks, the toolkit can't make use of the standard
selectStacks
method from the assembly, asNestedStacks
are synthesised as separate template units, to be later consumed as assets. These assets are immutable, which means that there's no traceability for changes in those files per se.Therefore, in order to support CLI routines like
diff
andprintSecurityDiff
, some sort of extrinsic mechanism should be established to bind current NestedStack templates to the ones product of asynth
.This could be done by using the the LogicalId of the NestedStack resource as an identifier (and by coding the "resource -> template" relation inference, see item 2 below).
In addition to the referred method (which is backwards compatible), this PR adds the Stack uniqueId as a Metadata to the NestedStack CFN template, for a stronger binding between the nested stack and the parent stack resource as well as better traceability. It was done by adding this code to the
core.NestedStack
constructor:cxapi.NESTEDSTACK_UNIQUEID_KEY
is currentlycxapi.PATH_METADATA_KEY
in the code. Waiting for some sort of approval before rerunning integration tests and rewritting expected.json files.Still, the feature would be backwards compatible since the methods first try the Metadata field and fall back to the parent LogicalId if not found.
^^^ This modifies the source hash of the NestedStacks, that's why so many changes to test files
NestedStacks verbosity on
cdk synth
RFC: Should these always display NestedStack content or optionally via cli args? Currently always showing.
^^^ This impacts expected.json for many integration tests.
NestedStacks and the done / total info on
cdk deploy
1/3 <--- | 4:45:53 PM | CREATE_IN_PROGRESS | AWS::SNS::Topic | MyWrapped/MyTopic (MyTopicB31DCDED)
RFC: Should the total resources and total resources done be updated based on the discovered NestedStacks?
Details
The goal is to improve the CLI interactions by displaying relevant NestedStack's resource information in various workflows:
cdk deploy
andcdk destroy
CloudFormation status updates (#7699, #5974, #6402)CDK displays status messages based on calls to the
describeStackEvents
method of the CloudFormation API. The method doesn't return relevant information about the resources that belong to a NestedStack, acting as if it was a regular resource.This PR implements recursive calls to the
describeStackEvents
API for every NestedStack.Currently:
PR:
cdk deploy
security-impacting changes (#4490)RFC: Prompt confirmation for each nested stack or just dump all securityDiffs and ask once?
Currently: no warning
PR:
cdk diff
(#5722)Currently:
PR:
cdk synth
Currently:
synth
only prints the main stack.PR:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license