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(servicecatalog): allow creating a CFN Product Version with CDK code #17144

Merged
merged 24 commits into from
Nov 2, 2021

Conversation

arcrank
Copy link
Contributor

@arcrank arcrank commented Oct 25, 2021

Add ability to define a product version entirely within CDK as opposed to referencing templates or local assets.
The service catalog ProductStack is similar to NestedStacks that do not deploy themselves but rather are referenced
by the parent stacks. The resources defined in your product are added to the product stack like any other cdk app.


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

Co-authored-by: Dillon Ponzo [email protected]

Aidan Crank and others added 2 commits October 25, 2021 12:40
…te a product version from cdk code.

Add ability to define a product version entirely within CDK as opposed to referencing templates or local assets.
The service catalog `ProductStack` is similar to `NestedStacks` that do not deploy themselves but rather are referenced
by the parent stacks.  The resources defined in your product are added to the product stack like any other cdk app.

Co-authored-by: Dillon Ponzo <[email protected]>
@gitpod-io
Copy link

gitpod-io bot commented Oct 25, 2021

@github-actions github-actions bot added the @aws-cdk/aws-servicecatalog Related to AWS Service Catalog label Oct 25, 2021
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @arcrank! A few comments before we merge this in.

packages/@aws-cdk/aws-servicecatalog/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-servicecatalog/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-servicecatalog/test/product.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-servicecatalog/test/product.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Mis-clicked on "Comment" instead of "Request changes".

@mergify mergify bot dismissed skinny85’s stale review October 26, 2021 16:22

Pull request has been modified.

@arcrank
Copy link
Contributor Author

arcrank commented Oct 26, 2021

So there are two things that stick out.

  1. Regarding us calling the preparing template call ourselves rather than letting it be handled, we call it and resolve the path ourselves in constructor, the tests fail if we don't, so either those need to be fixed. Is there a specific example for where this would fail that I can use to build off of?
  2. Regarding how to verify the multiple stacks and finding the other template file as I mentioned above. I'm still working on this.

@arcrank arcrank requested a review from skinny85 October 26, 2021 17:09
@skinny85
Copy link
Contributor

  1. Regarding us calling the preparing template call ourselves rather than letting it be handled, we call it and resolve the path ourselves in constructor, the tests fail if we don't, so either those need to be fixed. Is there a specific example for where this would fail that I can use to build off of?

Fail how?

Considering the NestedStacks work the same, and their tests work fine, I think there's something wrong with the current implementation.

@skinny85
Copy link
Contributor

I think _prepareTemplateAsset() might not be the correct function to override here. Look when it is invoked.

I think you might need to hook up into the synthesis step in a different way (prepare() maybe?).

@arcrank
Copy link
Contributor Author

arcrank commented Oct 29, 2021

@skinny85 this was the simplest form I could get this into, hopefully there isn't some JSII compatibility issue with this sort of interface check.

Wasn't sure if the renaming appropriate, let me know if there is better terminology we can use.

@arcrank arcrank requested a review from skinny85 October 29, 2021 19:20
arcrank and others added 4 commits October 29, 2021 15:24
erge branch 'prod_stack' of github.com:arcrank/aws-cdk into prod_stack
@arcrank arcrank requested a review from skinny85 November 1, 2021 20:30
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @arcrank! A few small comments, but we should be very, very close to merging this in.

@mergify mergify bot dismissed skinny85’s stale review November 2, 2021 02:43

Pull request has been modified.

@arcrank arcrank requested a review from skinny85 November 2, 2021 02:44
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

This is an amazing feature, and I think will be a game changer for ServiceCatalog usability from the CDK. Awesome work @arcrank!

@skinny85 skinny85 changed the title feat(servicecatalog): Add ProductStack resource and ability to create a product version from cdk code. feat(servicecatalog): allow creating a CFN Product Version with CDK code Nov 2, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2021

Thank you for contributing! Your pull request will be updated from master 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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 96e9689
  • 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 f8d0ef5 into aws:master Nov 2, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2021

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

@shahbhavik01
Copy link

Can someone point me to the documentation or examples of how we can use this product stack to create Service Catalog product? We're itching to use this functionality but struggling

@skinny85
Copy link
Contributor

@shahbhavik01 does this help? https://docs.aws.amazon.com/cdk/api/latest/docs/aws-servicecatalog-readme.html#creating-a-product-from-a-stack

@shahbhavik01
Copy link

@shahbhavik01 does this help? https://docs.aws.amazon.com/cdk/api/latest/docs/aws-servicecatalog-readme.html#creating-a-product-from-a-stack

Yes! Thank you @skinny85

@shahbhavik01
Copy link

shahbhavik01 commented Dec 22, 2021

@skinny85 @arcrank What's the correct way to reference parameters as value within the productStack? I'm trying to do something like:

        const natGateways = new cdk.CfnParameter(this, "natGateways", {
          type: "Number",
          description: "The number of Nat Gateways to attach",
        });

And then:

        const vpc = new ec2.Vpc(this, "threeTierVPC", {
          cidr: "10.0.0.0/16", 
          natGateways: natGateways.valueAsNumber,
        });

Both within the product stack.

This however doesn't create a Nat Gateway even if I pass two when deploying the product.
Can we not use any higher level constructs within the product stack? Are there other more complex examples you know of besides the s3 example that use the productStack to create a service catalog product?

@skinny85
Copy link
Contributor

@shahbhavik01 can you show your entire CDK code, and the resulting template that's generated? (You will find it in the cdk.out directory)

@shahbhavik01
Copy link

@skinny85 Yes. See attached.

service_catalog_files.zip

The goal we're working on is to create a set of products under one portfolio.

Attached is the code containing the product. I have added my bin/ lib/ and cdk.out folders.

@arcrank
Copy link
Contributor Author

arcrank commented Dec 22, 2021

Thanks, I am able to replicate. I think this partly an issue with passing in tokens to the vpc construct, as it does conditional generation of underlying resources based on that, but need to confirm this behavior.

@shahbhavik01
Copy link

@arcrank Thank you for looking into it. If I get rid of the conditional VPC resources and then create each resource individually, would this work? Does the code look ok otherwise? Is this how you intended the use of service catalog product on CDK?

@arcrank
Copy link
Contributor Author

arcrank commented Dec 23, 2021

@shahbhavik01, putting CfnParameters in the ProductStack should and does work when it's mapping to a field in the template. The crux of issue is that it's not really a CfnParameter for the template, but rather a functional part of cdk code, that abstracts away generating part of a template. In this case I actually think the VPC construct should be checking if Token is resolvable and throwing an error. While this is not ideal, it should at least fail fast. So yes, if you for example have all of the resources added and are using a CfnParameter for a cidr mask value it will work.

You bring up a good point that we definitely want to consider going forward, as more powerful abstractions for resources will not be available to product stack, when a construct or resources represents a cdk function that can map to n underlying resources depending on the configuration. We will definitely be looking into this going forward.

@shahbhavik01
Copy link

Thank you for the detailed explanation. I will keep that in mind and develop accordingly.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ode (aws#17144)

Add ability to define a product version entirely within CDK as opposed to referencing templates or local assets.
The service catalog `ProductStack` is similar to `NestedStacks` that do not deploy themselves but rather are referenced
by the parent stacks.  The resources defined in your product are added to the product stack like any other cdk app.


----

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



Co-authored-by: Dillon Ponzo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-servicecatalog Related to AWS Service Catalog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants