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

feat(cloudformation): nested stacks #2821

Merged
merged 105 commits into from
Oct 3, 2019
Merged

feat(cloudformation): nested stacks #2821

merged 105 commits into from
Oct 3, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jun 11, 2019

The NestedStack construct is a special kind of Stack. Any resource defined within its scope will be included in a separate template from the parent stack.

The template for the nested stack is synthesized into the cloud assembly but not treated as a deployable unit but rather as a file asset. This will cause the CLI to upload it to S3 and wire it's coordinates to the parent stack so we can reference its S3 URL.

To support references between the parent stack and the nested stack, we abstracted the concept of preparing cross references by inverting the control of consumeReference from the reference object itself to the Stack object. This allows us to override it at the NestedStack level (through prepareCrossReference) and mutate the token accordingly.

When an outside resource is referenced within the nested stack, it is wired through a synthesized CloudFormation parameter. When a resource inside the nested stack is referenced from outside, it will be wired through a synthesized CloudFormation output. This works for arbitrarily deep nesting.

When an asset is referenced within a nested stack, it will be added to the top-level stack and wired through the asset parameter reference (like any other reference).

Fixes #239
Fixes #395

TODO:

  • Support docker assets
  • Support references from the nested stack to the parent stack (through Outputs and Fn::GetAtt)
  • Determine the behavior for cross stack references between nested stacks
  • Add references to integ test
  • Support multiple nesting
  • Verify unit test coverage
  • Asset support
  • stackName, stackId, environment
  • Assert support (expect(nested).to(...))
  • Make experimental
  • Update README (incl. roadmap)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

WIP
* Nested stacks are synthesized and treated as S3 assets
* Nested stack resources can reference resources from the parent stack
  and they are wired as parameters passed from the parent stack to
  the nested stack.
@eladb eladb force-pushed the benisrae/nested-stacks branch from bfaaecb to 276811d Compare July 17, 2019 20:50
@eladb eladb requested a review from rix0rrr July 17, 2019 21:01
@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 17, 2019

Support multiple nesting

This and references is going to be SOOOO shitty!

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 17, 2019

Thinking we might be able to squash this for all cases with a nice and elegant recursive algorithm.

image

I feel like s omething roughly like this should work (with a bunch of unanswered questions like what are the exact values that are passed around and during what phase and where we exactly store what). But it seems simplifying and at the same time generic enough to handle all cases.

(?)

// Concepts:
// - "RefScope": a scope that if a reference crosses it, something needs to 
//   happen (NestedStack, Stack, App).
//
// Intuition: we transform the value in some way when crossing a refscope boundary,
// both "up to" and "down from" the refscope that two constructs share.

function consumeReference(reference) {
    const producerValue = produceReferenceValue(reference.producer, reference.value);
    const consumerValue = consumeReferenceValue(reference.consumer, producerValue);
    // Not entirely sure yet when/how this should happen. Obviously it
    // can't be during resolve(), so it'll have to be during prepare.
    // 
    // Where do we stick the resolved value?
    reference.rememberResolvedValue(reference.consumer, consumerValue);
}

// Transform the value recursively up the tree until the current containing
// refscope is equal to the refscope that the producer shares with the consumer.
function produceReferenceValue(scope, reference) {
    const refScope = findContainingRefScope(scope);
    if (refScope === reference.findSharedRefScope()) {
        return reference;
    }
    return produceReferenceValue(refScope, refScope.produce(reference));
}

// Transform the value recursively down the tree from the shared
// refscope down to the final consumer.
//
// Similar to produceReferenceValue but the order between recurse/transform
// is reversed.
function consumeReferenceValue(scope, reference) {
    const refScope = findContainingRefScope(scope);
    if (refScope === reference.sharedRefScope()) {
        return refScope.consume(reference);
    }
    return refScope.consume(consumeReferenceValue(refScope, reference));
}

// Illustrative implementation of RefScope for NestedStack
class NestedStack implements IRefScope {
    public produce(value: Reference) {
        // Make the value an Output, return { Fn::GetAtt: OutputName }
        const outputName = this.addOutput(value.resolve()); // Add: { Output: { Value: { Ref: Bucket }}}
        return this.getAtt(outputName);
    }

    public consume(value: Reference) {
        // Add a parameter to the template, pass the current value into the 
        // parameter, inside the template use a { Ref } to the parameter.
        const param = this.addParameter();
        this.parameters[param.name] = value;
        return new Intrinsic({ Ref: param.name });
    }
}

// Implementation of RefScope for Stack
class Stack implements IRefScope {
    public produce(value: Reference) {
        // Make the value an output, return a symbolic placeholder
        // (Since CloudFormation intrinsics have no meaning outside Stack, can be any value)
        const exportName = this.addExport(value.resolve()); // Add: { Output: { Value: { Ref: Bucket }, Export: 'SomeName' }}
        return new CrossStackReferencePlaceholder(exportName);
    }

    public consume(value: Reference) {
        // Take a placeholder value (*must* be that, otherwise something funky is going on)
        // turn it into a { Fn::ImportValue }
        assert(value instanceof CrossStackReferencePlaceholder);
        return new Intrinsic({ "Fn::ImportValue": value.exportName });
    }
}

class App implements IRefScope {
  // App is a refscope to make sure every pair of constructs always has a shared
  // refscope. Its produce() and consume() will never be called, because no reference can cross it.
}

image

Slight complication in the fact that a Stack that's a child of another Stack is treated as a top-level RefScope (it becomes a top-level stack). Not super hard, just something that findContainingRefScope() and findSharedRefScope() will have to take into account (the containing refscope of a Stack is always the App, never another Stack that's higher up in the tree).

@skinny85
Copy link
Contributor

This is like seeing Michelangelo's sketches before he painted the Sistine Chapel... Rare insight into the brilliant creative process.

@eladb eladb marked this pull request as ready for review July 29, 2019 06:10
packages/@aws-cdk/assert/lib/synth-utils.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/assert/lib/synth-utils.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudformation/lib/nested-stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudformation/lib/nested-stack.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudformation/lib/nested-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 Show resolved Hide resolved
packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved
@eladb eladb assigned rix0rrr and unassigned eladb Aug 5, 2019
@eladb eladb mentioned this pull request Aug 6, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 7, 2019

Shouldn't the API of nested stacks be:

class MyStack extends Stack {
}


new  NestedStack(this, 'Nested', {
  stack: new MyStack()
});

As opposed to subclassing NestedStack?

@eladb
Copy link
Contributor Author

eladb commented Aug 7, 2019

@rix0rrr I thought about it. The problem with this approach is that this means that we will not be able to override the behavior of Stack for constructs that are defined within the nested stack such as how assets and references are treated.

@eladb eladb assigned eladb and unassigned rix0rrr Aug 12, 2019
@eladb eladb added @aws-cdk/aws-cloudformation Related to AWS CloudFormation and removed pr/do-not-merge This PR should not be merged at this time. labels Oct 2, 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

@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

@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

@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

@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

@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

@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

BREAKING CHANGE: `ConstructNode.references`, `ConstructNode.addReference` and `OutgoingReference` were removed since they did not behave as expected prior to synthesis (they were always empty and populated only during prepare). This was confusing and did not offer much value.
@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
Copy link
Contributor

mergify bot commented Oct 3, 2019

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

@eladb eladb merged commit 5225306 into master Oct 3, 2019
@eladb eladb deleted the benisrae/nested-stacks branch October 3, 2019 17:42
@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

@jogold
Copy link
Contributor

jogold commented Oct 8, 2019

@eladb I'm facing issues with my snapshot tests with this PR.

Now that the source hash is included in the parameter name I don't get the same CF template locally and in CI, is this correct? How would you create a snapshot locally and compare it in CI after this PR?

Also doesn't it mean that a local deploy of a Lambda function followed by a CI deploy will redeploy even if the content has not changed?

@eladb
Copy link
Contributor Author

eladb commented Oct 8, 2019

Ideally you asset source hash should remain consistent across builds. If it’s not, it means you have some non determinism which is generally considered operationally risky. Having said that, reality is sometimes tricky and you should be able to specifying custom sourceHash when you define your lambda.Code asset. There is a new option for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudformation Related to AWS CloudFormation contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"cdk diff" should take assets into account Nested stack support