feat(servicecatalog): Add Product Stack Asset Support #22143

wanjacki · 2022-09-20T14:28:34Z

Currently Assets are not supported in Product Stacks. Service Catalog has an unique use case where assets need to be shared cross account and sharing the entire CDK asset bucket is not ideal. Users can either create their own ProductStackAssetBucket or have one automatically generated for them based on their account Id and region. By using S3 Deployments we able to copy the assets to that bucket and share it when a portfolio is shared in Service Catalog.

More details can be found here: #20690. Closes #20690

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

Co-authored-by: Theron Mansilla[imanolympic]

gitpod-io · 2022-09-20T14:28:45Z

padaszewski · 2022-09-21T10:06:49Z

Hi,
Thanks for the great job.
I was comparing the new API with our implementation and I didn't found any way to incorporate the own ProductStackAssetBucket into the ProductStackHistory construct.
Do You plan to provide this option?

wanjacki · 2022-09-21T14:21:24Z

@padaszewski That should automatically be supported.
ie. if you want to specify your own bucket

    const productStackHistory = new ProductStackHistory(this, 'ProductStackHistory', {
      productStack: new HelloServerlessProduct(this, 'HelloServerlessProduct', {
        assetBucket: myBucket,}),
      currentVersionName: 'v1',
      currentVersionLocked: true
      }
    )

padaszewski · 2022-09-21T14:30:36Z

@wanjacki
Ah right, this is a property of the ProductStack. After a quick read of the README I was convinced that the bucket is passed like this servicecatalog.CloudFormationTemplate.fromProductStack(productStack, bucket). My bad, sorry.

kaizencc

Hi @wanjacki! Given this a quick read through with some initial comments.

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

packages/@aws-cdk/aws-servicecatalog/README.md

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

kaizencc · 2022-09-26T17:46:39Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

+  }
+
+  /**
+   * Fetch the expected S3 location of an asset.


this is doing more than fetch right? adds an asset to this.assets

Updated to be a bit more descriptive.

kaizencc · 2022-09-26T17:47:46Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

+
+    this.assets = [];
+
+    cdk.Aspects.of(this).add({


what is the reasoning behind using cdk.Aspects to run deploy assets? not questioning, just wondering.

It can be done without Aspects I believe, but Aspects fits the use case and allows us to cleanly implement our use case as well as abstract the additional code to the new product-stack-asset-bucket construct rather than modifying and adding additional code to product-stack.

Pull request has been modified.

corymhall

I have a couple of big concerns with this PR.

We need an integration test that tests creating a product with assets and provisioning a
product with assets.
I'm not sure about how you are currently handling the asset buckets. Each individual product will
get it's own asset bucket? What about having a single bucket that the user needs to create
themselves? A bucket per product seems like it will lead to more work on the consumer side (if
you want to provision product ABC then add a policy to your role with access to bucket ABC).

corymhall · 2022-09-29T12:21:13Z

packages/@aws-cdk/aws-servicecatalog/lib/cloudformation-template.ts

+
+  /**
+   * The S3 bucket containing product stack assets.
+   * @default - None


What does None mean? If this is not provided are assets not supported?

This class is the result of binding a Artifact Template into a Product.
In the event that it is not binding to a Product Stack (URL or local asset template instead), there is no asset bucket and no asset support.
In the event that they are binding a Product Stack, if they are not using assets, then the asset bucket will not be generated and will be undefined and there will be no asset bucket.
In any other case there is an asset bucket.

I will update to None - no assets are used in this product as well.

corymhall · 2022-09-29T12:23:43Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack.ts

+  /**
+   * Product stack asset bucket.
+   *
+   * @default - None


I would update the docstring to be more descriptive. As a user I want to know should I pass
something here or not? When would I want to provide my own bucket and what is the behavior if I
don't

corymhall · 2022-09-29T12:25:55Z

packages/@aws-cdk/aws-servicecatalog/lib/product.ts

+  /**
+   * The asset bucket of a product created via product stack.
+   * @atrribute
+   * @default - None


Something like None - no assets are used in this product

corymhall · 2022-09-29T12:28:05Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

+  /**
+   * Name of s3 asset bucket deployed
+   *
+   * @default - None


I think you can be more descriptive here since you know what the generated name will be (or atleast
the format).

corymhall · 2022-09-29T12:29:02Z

packages/@aws-cdk/aws-servicecatalog/lib/product-stack-asset-bucket.ts

+ * A Service Catalog product stack asset bucket, which is similar in form to an Amazon S3 bucket.
+ * You can store multiple product stack assets and collectively deploy them to S3.
+ */
+export class ProductStackAssetBucket extends Construct {


I think this should extends s3.Bucket.

corymhall · 2022-09-29T12:47:07Z

packages/@aws-cdk/aws-servicecatalog/lib/private/product-stack-synthesizer.ts

+    if (!this.assetBucket) {
+      const parentStack = (this.stack as ProductStack)._getParentStack();
+      this.assetBucket = new ProductStackAssetBucket(parentStack, `ProductStackAssetBucket${hashValues(this.stack.stackName)}`);
+    }


I'm not sure how I feel about the way we are creating the asset bucket here. What if we just created
the asset bucket by default as part of the product stack?

Do you mean we create a ProductStackAssetBucket regardless of if the user is using an asset or not.
I don't that is a good idea since we would be creating resources that will not be needed (S3-deployment also deploys a lambda). Furthermore, a user could be unintentionally create a bunch of S3 Buckets (since it is one per product-stack) and fill up their S3 Bucket limit capacity.

corymhall · 2022-09-29T12:49:11Z

packages/@aws-cdk/aws-servicecatalog/lib/private/product-stack-synthesizer.ts

+    }
+
+    (this.stack as ProductStack)._setAssetBucket(this.assetBucket._getBucket());
+    return this.assetBucket._addAsset(_asset);


I think this is where we should handle the asset deployment, rather than in the bucket construct.

corymhall · 2022-09-29T12:54:53Z

packages/@aws-cdk/aws-servicecatalog/lib/private/product-stack-synthesizer.ts

@@ -16,7 +25,17 @@ export class ProductStackSynthesizer extends cdk.StackSynthesizer {
  }

  public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation {


Suggested change

public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation {

public addFileAsset(asset: cdk.FileAssetSource): cdk.FileAssetLocation {

corymhall · 2022-09-29T13:02:05Z

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

+  protected addBucketPermissionsToSharedAccounts() {
+    if (this.sharedAccounts.length > 0) {
+      for (const bucket of this.assetBuckets) {
+        bucket.grantRead(new iam.CompositePrincipal(...this.sharedAccounts.map(account => new iam.AccountPrincipal(account))),


How are you granting access on the other side? The IAM role that gets from these buckets needs
access to be granted access to the bucket in its princial policy.

Addressed in my other comment, but there is not much more we can do. We made it as easy as possible but we don't have access to the accounts it is being shared with, it is up to the Admin to configure these spoke accounts.

corymhall · 2022-09-29T13:05:13Z

packages/@aws-cdk/aws-servicecatalog/test/integ.product.ts

@@ -1,8 +1,9 @@
 import * as path from 'path';


This integration test only tests half of the scenario. It confirms that we can publish to the
bucket, but how do we know that we can then deploy one of these products?

The same can be said about any of the other existing integration test. There exist validation in Service Catalog to validate the template before it can be created but it can't check everything. We can't provision the product in CDK code and even then trying to provision it would probably fall in the scope of a provision-product construct.
I'm not sure if is the products job to check if a Product is provisionable, Service Catalog allows us to create a Product regardless of if its deployable or not. We also do our best in unit test to check the generated template is as expected.

This is different though than existing functionality. In this PR we are adding functionality outside of service catalog. This PR is completely useless if the consumer cannot access the assets in the bucket and we have no test to assert that.

Hey Cory, I refactored ProductStackAssetBucket to extend S3.Bucket as suggested as well as implemented the other comments.
I also added additional unit test to check Bucket Name and Bucket Policy Permissions are correct in template when shared.
If we can catch that the bucket is correct and the permissions are correct then the product should be deployable and the bucket should be acessable. If this is still not enough, please advise what what other test we can add (I don't think provisioning it is an option).

wanjacki · 2022-09-29T15:14:06Z

I have a couple of big concerns with this PR.

1. We need an integration test that tests creating a product with assets _and_ provisioning a
   product with assets.

2. I'm not sure about how you are currently handling the asset buckets. Each individual product will
   get it's own asset bucket? What about having a single bucket that the user needs to create
   themselves? A bucket per product seems like it will lead to more work on the consumer side (if
   you want to provision product ABC then add a policy to your role with access to bucket ABC).

We can't provision a product directly in CDK and there has been no precedent for having to provision a product in any of the other integration test.
Yes we are limited to one asset bucket Product Stack if we are generating for the user. It is not trivial to implement it on a portfolio level and the asset itself are tied to Products not Portfolios. As a workaround we allow users to create their own asset bucket, this asset bucket can be used for multiple Product Stacks and bypasses this restriction. We grant users that the admin shares their portfolio/product with permissions to read the Bucket. Any further policy that might be needed would have to be setup and managed by the Admin (Hub) to Shared Accounts (Spoke) anyways.

Pull request has been modified.

corymhall · 2022-09-30T13:36:56Z

@wanjacki I would recommend going through the RFC process for this feature. There are a lot of design considerations that we are not yet agreed on. The biggest thing for me is that we have to consider the entire experience (publishing & consuming).

corymhall

Putting back in request changes.

…nal unit test for bucket name and permission sharing.

Pull request has been modified.

aws-cdk-automation · 2022-09-30T17:27:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0b84503
Result: SUCCEEDED
Build Logs (available for 30 days)

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

TheRealAmazonKendra · 2022-10-02T05:52:22Z

@wanjacki I would recommend going through the RFC process for this feature. There are a lot of design considerations that we are not yet agreed on. The biggest thing for me is that we have to consider the entire experience (publishing & consuming).

+1 on this comment. I'm going to convert this into a draft until we have an approved RFC for this.

TheRealAmazonKendra

Putting in changes requested to accurately reflect status.

aws-cdk-automation · 2022-10-24T00:15:09Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-10-31T18:48:03Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions bot added the p2 label Sep 20, 2022

aws-cdk-automation requested a review from a team September 20, 2022 14:28

wanjacki force-pushed the asset_support branch 2 times, most recently from 4d764ea to 593cec0 Compare September 20, 2022 15:08

wanjacki force-pushed the asset_support branch from 593cec0 to 6079ef4 Compare September 21, 2022 14:58

feat(servicecatalog): Add Product Stack Asset Support

86b14f6

wanjacki force-pushed the asset_support branch from 6079ef4 to 86b14f6 Compare September 22, 2022 19:37

kaizencc previously requested changes Sep 26, 2022

View reviewed changes

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Sep 26, 2022

Address Initial Comments

e245852

corymhall previously requested changes Sep 29, 2022

View reviewed changes

Merge branch 'aws:main' into asset_support

37d12c3

corymhall previously requested changes Sep 30, 2022

View reviewed changes

Refactored ProductStackAssetBucket to extend bucket and added additio…

0b84503

…nal unit test for bucket name and permission sharing.

TheRealAmazonKendra marked this pull request as draft October 2, 2022 05:52

TheRealAmazonKendra requested changes Oct 2, 2022

View reviewed changes

wanjacki mentioned this pull request Oct 6, 2022

Service Catalog ProductStack Asset Support aws/aws-cdk-rfcs#458

Closed

11 tasks

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Oct 31, 2022

aws-cdk-automation closed this Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalog): Add Product Stack Asset Support #22143

feat(servicecatalog): Add Product Stack Asset Support #22143

wanjacki commented Sep 20, 2022 •

edited by kaizencc

Loading

gitpod-io bot commented Sep 20, 2022

padaszewski commented Sep 21, 2022

wanjacki commented Sep 21, 2022

padaszewski commented Sep 21, 2022

kaizencc left a comment

kaizencc Sep 26, 2022

wanjacki Sep 27, 2022

kaizencc Sep 26, 2022

wanjacki Sep 27, 2022 •

edited

Loading

corymhall left a comment

corymhall Sep 29, 2022

wanjacki Sep 29, 2022 •

edited

Loading

corymhall Sep 29, 2022

corymhall Sep 29, 2022

corymhall Sep 29, 2022

corymhall Sep 29, 2022

corymhall Sep 29, 2022

wanjacki Sep 29, 2022

corymhall Sep 29, 2022

corymhall Sep 29, 2022

corymhall Sep 29, 2022

wanjacki Sep 29, 2022

corymhall Sep 29, 2022

wanjacki Sep 29, 2022

corymhall Sep 30, 2022

wanjacki Sep 30, 2022

wanjacki commented Sep 29, 2022

corymhall commented Sep 30, 2022

corymhall left a comment

aws-cdk-automation commented Sep 30, 2022

TheRealAmazonKendra commented Oct 2, 2022

TheRealAmazonKendra left a comment

aws-cdk-automation commented Oct 24, 2022

aws-cdk-automation commented Oct 31, 2022

		@@ -16,7 +25,17 @@ export class ProductStackSynthesizer extends cdk.StackSynthesizer {
		}

		public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation {

	public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation {
	public addFileAsset(asset: cdk.FileAssetSource): cdk.FileAssetLocation {

feat(servicecatalog): Add Product Stack Asset Support #22143

feat(servicecatalog): Add Product Stack Asset Support #22143

Conversation

wanjacki commented Sep 20, 2022 • edited by kaizencc Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 20, 2022

padaszewski commented Sep 21, 2022

wanjacki commented Sep 21, 2022

padaszewski commented Sep 21, 2022

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanjacki Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanjacki Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanjacki commented Sep 29, 2022

corymhall commented Sep 30, 2022

corymhall left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 30, 2022

AWS CodeBuild CI Report

TheRealAmazonKendra commented Oct 2, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 24, 2022

aws-cdk-automation commented Oct 31, 2022

wanjacki commented Sep 20, 2022 •

edited by kaizencc

Loading

wanjacki Sep 27, 2022 •

edited

Loading

wanjacki Sep 29, 2022 •

edited

Loading