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

fix(core): dependencies across stack boundaries of all kinds #5211

Merged
merged 10 commits into from
Nov 29, 2019
Merged

fix(core): dependencies across stack boundaries of all kinds #5211

merged 10 commits into from
Nov 29, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Nov 27, 2019 •
edited
Loading

stack.addDependency can now be called between any two stacks in the app and the dependency will be applied at the right scope, either between top-level stacks at the assembly (app) level or between AWS::CloudFormation::Stack resources that represent nested stacks in a common stack.

Similarly, cfnResource.addDependsOn can now be called for any two CfnResources in the app, and thew dependency will be applied at the right scope (either between the two resources directly, between nested stack resources or at the assembly/app level).

This is implemented generically for both resources and stacks in deps.ts/addDependency through the following algorithm:

  • Lookup the lowest common stack
  • If not found, apply the dependency at the assembly level (between two top-level stacks).
  • If found, identify the two CloudFormation resources which represent this relationship within the scope of the common stack (this could be the actual resource if they are both in the same stack or the AWS::CloudFormation::Stack resource if the source or target are with in a nested stack).
  • Apply the dependency at the CloudFormation level (add DependsOn).

To be able to find the AWS::CloudFormation::Stack resource for nested stacks, the nestedStackResource property was added to core.Stack and assigned by NestedStack

Created a thorough test suite to examine various cases under @aws-cdk/aws cloudformation/test.deps.ts. Note that this suite includes tests for nested stacks and for regular cases, but decided to put them all in a single suite to make it easier to reason about coverage.

This change also introduces a cache for Stack.of(x) by attaching the result to x via a hidden property. This method is widely used and the cache hit rate is quite large (at the time of this writing, @aws-cdk/core unit tests hit this cache 1,112 times, @aws-cdk/aws-cloudformation unit tests hit this 2,435 times).

Fixes #4406
Fixes #4474

Elad Ben-Israel added 2 commits November 27, 2019 12:41
fix(cloudformation): dependencies across nested stack boundaries
2eaff5b 
When adding dependencies of any kind (`node.addDependency`, `stack.addDependency` or `cfnResource.addDependsOn`), we now take nested stacks into account and transfer the dependency to the lowest common scope (either a common stack or the assembly).

This is primarily implemented in `cfnResource.addDependsOn` and `stack.addDependency` through the following algorithm:

- Lookup the lowest common stack
- If not found, apply the dependency at the assembly level (between two top-level stacks).
- If found, identify the two CloudFormation resources which represent this relationship within the scope of the common stack (this could be the actual resource if they are both in the same stack or the `AWS::CloudFormation::Stack` resource if the source or target are with in a nested stack).
- Apply the dependency at the CloudFormation level (add `DependsOn`).

There are a bunch of edge cases as well.

To be able to find the AWS::CloudFormation::Stack resource for nested stacks, the `nestedStackResource` property was added to `core.Stack` and assigned by `NestedStack`. It seems like we should probably move `NestedStack` from the `aws-cloudformation` module and into `core` since there is too much coupling right now.

Created a thorough test suite to examine various cases under `test.deps.ts`. Note that this suite includes tests for nested stacks and for regular cases, but decided to put them all in a single suite to make it easier to reason about coverage.

Fixes #4460
Merge remote-tracking branch 'origin/master' into benisrae/nested-sta…
94a4fe2 
…cks-dependencies
@eladb eladb requested a review from rix0rrr November 27, 2019 10:42
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 27, 2019
@mergify
Copy link
Contributor

mergify bot commented Nov 27, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@eladb eladb mentioned this pull request Nov 27, 2019
Closed
rix0rrr
rix0rrr reviewed Nov 27, 2019
View reviewed changes
packages/@aws-cdk/core/lib/cfn-resource.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/util.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/util.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 27, 2019

For the record, I'm almost positive we should be able to cut down on the amount of ifs even more if we put our mind to it. Feels like an elegant recursive solution is juuuust out of reach.

@SomayaB SomayaB added the pr/work-in-progress This PR is a draft and needs further work. label Nov 27, 2019
combine stack and resource deps into a single algorithm
3b53e3b 
- Rewrote the `addDependency` algorithm generically to support defining dependencies between two resources or two stacks.
- Added a cache for `Stack.of` which is expected to have some nice gains since this method is widely used, especially around these algorithms.
@eladb eladb changed the title fix(cloudformation): dependencies across nested stack boundaries fix(core): dependencies across nested stack boundaries Nov 28, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@eladb eladb requested a review from RomainMuller as a code owner November 28, 2019 19:01
@eladb eladb changed the title fix(core): dependencies across nested stack boundaries fix(core): dependencies across stack boundaries of all kinds Nov 28, 2019
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Nov 28, 2019
rix0rrr
rix0rrr approved these changes Nov 28, 2019
View reviewed changes
packages/@aws-cdk/core/lib/stack.ts Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Show resolved Hide resolved
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@eladb eladb removed the pr/do-not-merge This PR should not be merged at this time. label Nov 28, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit d1f0dd5 into master Nov 29, 2019
@mergify mergify bot deleted the benisrae/nested-stacks-dependencies branch November 29, 2019 06:14
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr approved these changes

@RomainMuller RomainMuller Awaiting requested review from RomainMuller

Assignees

@eladb eladb

Labels
contribution/core This is a PR that came from AWS. pr/work-in-progress This PR is a draft and needs further work.
Projects
None yet
Milestone
No milestone
4 participants
@eladb @aws-cdk-automation @rix0rrr @SomayaB