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

fix(servicecatalog): support nested stacks in product stacks #26311

Merged
merged 30 commits into from
Jul 12, 2023

Conversation

colifran
Copy link
Contributor

This PR fixes a bug that prevented a nested stack from being used within a product stack. The logic in the addFileAsset method as part of the ProductStackSynthesizer was updated to forward all assets to the parent stack and uses the FileAssetLocation returned from the parent stack synthesizer to create a bucket Source from the bucketName and objectKey.

Closes #24317


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

Signed-off-by: Francis <colifran@amazon.com>
…is property to true when nested stack adds its template file to the parent stack

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…te correct FileAssetLocation

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…f not provided and will be required for imported buckets

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…ests

Signed-off-by: Francis <colifran@amazon.com>
@gitpod-io
Copy link

gitpod-io bot commented Jul 10, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team July 10, 2023 19:55
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Jul 10, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 10, 2023
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@colifran colifran marked this pull request as ready for review July 10, 2023 22:53
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 10, 2023

constructor(assetBucket?: IBucket) {
constructor(private readonly parentDeployment: cdk.IStackSynthesizer, private readonly assetBucket?: IBucket) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This okay in principle, but we tend to prefer props objects.
Because I have just approved an other PR that also adds stuff to this: https://github.com/aws/aws-cdk/pull/26303/files#diff-26862b8f0be4955057aed48a0d3dfabefaa27d96e95d3c5c99a3a85cc10ce770R18

Can you rebase your PR and then turn this whole thing into a prop object please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been updated. I created an interface named ProductStackSynthesizerProps and I moved all constructor properties inside of that.


constructor(assetBucket?: IBucket) {
constructor(private readonly parentDeployment: cdk.IStackSynthesizer, private readonly assetBucket?: IBucket) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constructor(private readonly parentDeployment: cdk.IStackSynthesizer, private readonly assetBucket?: IBucket) {
constructor(private readonly parentStack: cdk.Stack, private readonly assetBucket?: IBucket) {

I know this is only an internal class, but I wonder if passing the parent stack is a bit cleaner as an API. Passing a synthesizer feels like we are implying that any synthesizer could be passed in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to pass the parentStack as part of the newly added ProductStackSynthesizerProps

Comment on lines +380 to +382
SourceBucketNames: cdk.Lazy.uncachedList({ produce: () => this.sources.map(source => source.bucket.bucketName) }),
SourceObjectKeys: cdk.Lazy.uncachedList({ produce: () => this.sources.map(source => source.zipObjectKey) }),
SourceMarkers: cdk.Lazy.uncachedAny({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this is needed.... but why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

TL;DR - this is needed to ensure the nested stack template file is added as a Source to the BucketDeployment.

Take this code block as an example:

class SampleNestedStack extends cdk.NestedStack {
      constructor(scope: Construct, id: string) {
        super(scope, id);

        new lambda.Function(this, 'HelloHandler1', {
          runtime: lambda.Runtime.PYTHON_3_9,
          code: lambda.Code.fromAsset(path.join(__dirname, 'assets')),
          handler: 'index.handler',
        });
        
        new lambda.Function(this, 'HelloHandler2', {
          runtime: lambda.Runtime.PYTHON_3_9,
          code: lambda.Code.fromAsset(path.join(__dirname, 'assetsv2')),
          handler: 'index.handler',
        });
      }
    }

Note: I'm not a Lazy expert, but the process is something like this. I had some help from @rix0rrr to figure this part out.

First, the Code assets from the two lambda Functions will be added as Sources to the BucketDeployment. Since these are evaluated with Lazy they are added to a cache before evaluation is carried out. At the point when the NestedStack is ready to add its own template, the Lazy logic has already assumed all assets have been added to the cache. This results in the template file for NestedStack being left out when SourceBucketNames, SourceObjectKeys, and SourceMarkers is evaluated. Lazy.uncachedList is basically saying don't only rely on the cache, there may be more items to evaluate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, probably not necessary for the markers. Doesn’t really harm, but maybe put it back? And add a comment saying that its uncached because the contents may change for nestedstacks in a productstacj

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think so either, but it actually is. An error is thrown if SourceObjectKeys length isn't equal to SourceMarkers length and it'll be off by 1 if List.any is used.

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@colifran colifran force-pushed the colifran/nested-stack-in-product-stack branch from 7035135 to b1a4ef3 Compare July 11, 2023 16:51
colifran and others added 7 commits July 11, 2023 10:12
Signed-off-by: Francis <colifran@amazon.com>
…s interface

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 11, 2023
@colifran colifran requested a review from mrgrain July 12, 2023 13:16
@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 1107ef2
  • 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 cad0635 into main Jul 12, 2023
@mergify mergify bot deleted the colifran/nested-stack-in-product-stack branch July 12, 2023 14:19
@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this pull request Jul 29, 2023
This PR fixes a bug that prevented a nested stack from being used within a product stack. The logic in the `addFileAsset` method as part of the `ProductStackSynthesizer` was updated to forward all assets to the parent stack and uses the `FileAssetLocation` returned from the parent stack synthesizer to create a bucket `Source` from the  `bucketName` and `objectKey`.

Closes aws#24317

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws_servicecatalog): nested stack in product stack failed in cdk synth
4 participants