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

wanjacki · 2022-11-09T22:19:11Z

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

RFC: aws/aws-cdk-rfcs#458

All Submissions:

[X ] 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?
- [X ] 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-11-09T22:19:17Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

wanjacki · 2022-11-11T20:43:49Z

@corymhall Hey Cory, sorry can you reopen this PR. Was trying to fix the the integ test build failing (passing locally) and this closed when I tried to rebase

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

corymhall

@wanjacki sorry for the delay in reviewing this! I wanted to make sure I was
able to test it out myself. Overall this is looking really good! I only have a
couple of minor comments.

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

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

corymhall · 2022-11-29T18:58:02Z

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

+  }
+
+  private physicalNameOfBucket(bucket: IBucket) {
+    const resolvedName = cdk.Stack.of(bucket).resolve((bucket.node.defaultChild as CfnBucket).bucketName);


I think this should just be bucket.bucketName right?

const bucketName = bucket.bucketName; // if the bucketName is undefined or is a token if (!bucketName || Token.isUnresolved(bucketName)) { throw new Error(...); }

The bucketName is always a token regardless of if we pass a bucketName or not.
I asked Rico about a solution and he suggested his comment here: #21487 (comment).
Implementing his suggestion we access the underlying CfnBucket and retrieve the bucketName.

ahh ok. In that case I think you will just have to handle the case where someone passes an IBucket and there is no default child. Maybe

if (Resource.isOwnedResource(bucket)) { cdk.Stack.of(bucket).resolve((bucket.node.defaultChild as CfnBucket).bucketName); } else { bucket.bucketName; }

corymhall · 2022-11-29T19:00:43Z

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

+
+    const physicalName = this.physicalNameOfBucket(this.assetBucket);
+
+    (this.boundStack as ProductStack)._addAsset(asset);


It feels like we should manage the logic for _addAsset and _deployAssets
here instead of within ProductStack.

I moved them both over as suggested.

corymhall · 2022-11-29T19:03:17Z

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

+});
+
+const testAssetBucket = new s3.Bucket(stack, 'TestAssetBucket', {
+  bucketName: 'product-stack-asset-bucket-12345678-test-region',


Suggested change

bucketName: 'product-stack-asset-bucket-12345678-test-region',

bucketName: `product-stack-asset-bucket-${stack.account}-${stack.region}`,

corymhall · 2022-11-29T19:05:19Z

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

+ ```
+ $ cdk destroy --app "node integ.product.js" integ-servicecatalog-product
+ ```
+ */

 const app = new cdk.App();
 const stack = new cdk.Stack(app, 'integ-servicecatalog-product');


Suggested change

const stack = new cdk.Stack(app, 'integ-servicecatalog-product');

const stack = new cdk.Stack(app, 'integ-servicecatalog-product', {

env: {

account: process.env.CDK_INTEG_ACCOUNT ?? process.env.CDK_DEFAULT_ACCOUNT,

region: process.env.CDK_INTEG_REGION ?? process.env.CDK_DEFAULT_REGION,

},

});

This allows you to use the actual account+region in the bucket name. The
snapshot will contain the dummy values.

This results in an error that I'm not fully clear on. Are we missing something else?

Error: Resolution error: Resolution error: Resolution error: Resolution error: Cannot use resource 'integ-servicecatalog-product/S3AssetProduct/Custom::CDKBucketDeployment8693BB64968944B69AAFB0CC9EB8756C/ServiceRole' in a cross-environment fashion, the resource's physical name must be explicit set or use `PhysicalName.GENERATE_IF_NEEDED`.

nvm i fixed it somehow.

corymhall · 2022-11-29T19:07:09Z

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


-product.associateTagOptions(tagOptions);
+new cdk.CfnOutput(stack, 'PortfolioId', { value: portfolio.portfolioId });


Can you use the IntegTest for this test?

new IntegTest(app, 'integ-product', { testCases: [stack], enableLookups: true, });

corymhall · 2022-11-29T19:07:48Z

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

+import { ProductStackHistory, ProductStackProps } from '../lib';
+
+/**
+ * Follow these instructions to manually test provisioning a Product with an Asset with the resources provisioned in this stack:


Thanks for the instructions!

corymhall · 2022-11-29T19:26:49Z

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

+Furthermore, in order for a spoke account to provision a product with an asset, the role launching 
+the product needs permissions to read from the asset bucket.
+We recommend you utilize a launch role with permissions to read from the asset bucket.
+For example your launch role would need to include at least the following policy:


Could we make the permissions more specific if they are using a launch role?
For example if they provide a launch role via portfolio.setLocalLaunchRoleName() then we could specify that as part of the bucket policy. From my testing it looks like the most specific we can get is something like this:

new iam.PolicyStatement({ actions: [ 's3:GetObject*', 's3:GetBucket*', 's3:List*', ], effect: iam.Effect.ALLOW, resources: [ bucket.bucketArn, bucket.arnForObjects('*'), ], principals: [ ...this.sharedAccounts.map(account => new iam.ArnPrincipal(cdk.Stack.of(this).formatArn({ service: 'iam', region: '', account, resource: 'role', resourceName: launchRoleName, }))), ], conditions: { 'ForAnyValue:StringEquals': { 'aws:CalledVia': ['cloudformation.amazonaws.com'], }, 'Bool': { 'aws:ViaAWSService': true, }, }, });

Do you mean actually adding that PolicyStatement to their launch role. We would have to save the LaunchRole in portfolio when it is set and add it our aspect, so we can propagate the sharedAccounts regardless of the order it is called. There can also be multiple bucket and multiple accounts and other possible complications trying to generate that PolicyStatement. I'm not if it worth the effort or if the customer wants us adding permissions to their launchRole for them.

Or do you mean adding this to the documentation as a recommended policy statement to use for launch role when deploying assets? I think this a good alternative as they can craft a PolicyStatement using that more specific template you provided and easily add it to their LaunchRole if needed.

No not to the launch role. This would be the policy that we add to the bucket. Rather than allowing read from any principal in the shared accounts, this only allows read from the launch role (if one is specified).

If this isn't going to cover all use cases though then we can leave the bucket policy as is and just add this as an example policy to the docs if users want to scope down the access more.

Pull request has been modified.

wanjacki · 2022-11-30T21:41:47Z

@corymhall Hey Cory thanks for the feedback. I submitted a new commit with all suggested changes. I also updated the integ test to deploy an actual lambda with an asset. Let me know what you think.

PR #22857 is introducing a use case where we need to be able to add additional sources after the `BucketDeployment` resource is created. This PR adds an `addSource` method and changes all the sources evaluation within the construct to be lazy. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

…t_support

Pull request has been modified.

corymhall

@wanjacki great work on this one! Thanks for being patient with us! 🚀

mergify · 2022-12-16T13:19:36Z

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).

wanjacki · 2022-12-20T19:14:29Z

Hey @corymhall
Is that build failure something I need to fix on our side?

corymhall · 2022-12-22T14:21:01Z

Is that build failure something I need to fix on our side?

@wanjacki we fixed a bug in the addSource functionality which is changing the snapshot diff. Can you just rerun the integ test? You can just run it with --dry-run.

Pull request has been modified.

mergify · 2022-12-28T19:45:43Z

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 · 2022-12-28T21:38:01Z

AWS CodeBuild CI Report

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

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

mergify · 2022-12-28T21:38:17Z

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).

PR aws#22857 is introducing a use case where we need to be able to add additional sources after the `BucketDeployment` resource is created. This PR adds an `addSource` method and changes all the sources evaluation within the construct to be lazy. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

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: aws#20690. Closes aws#20690 RFC: aws/aws-cdk-rfcs#458 ---- ### All Submissions: * [X ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [X] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X ] 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](https://github.com/imanolympic)]

PR aws#22857 is introducing a use case where we need to be able to add additional sources after the `BucketDeployment` resource is created. This PR adds an `addSource` method and changes all the sources evaluation within the construct to be lazy. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

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: aws#20690. Closes aws#20690 RFC: aws/aws-cdk-rfcs#458 ---- ### All Submissions: * [X ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [X] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X ] 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](https://github.com/imanolympic)]

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Nov 9, 2022

aws-cdk-automation requested a review from a team November 9, 2022 22:19

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Nov 9, 2022

wanjacki mentioned this pull request Nov 9, 2022

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

Closed

11 tasks

wanjacki force-pushed the asset_support branch 5 times, most recently from 7c69300 to ffeaf9a Compare November 11, 2022 20:01

corymhall marked this pull request as draft November 11, 2022 20:27

feat(servicecatalog): Add Product Stack Asset Support

b1405cb

wanjacki closed this Nov 11, 2022

wanjacki force-pushed the asset_support branch from ffeaf9a to 0e97c15 Compare November 11, 2022 20:40

aws-cdk-automation previously requested changes Nov 11, 2022

View reviewed changes

corymhall reopened this Nov 11, 2022

wanjacki force-pushed the asset_support branch from 773c275 to b61b4a5 Compare November 14, 2022 19:51

Updated Integ Test with manual steps for provisioning asset product

1afdca3

wanjacki force-pushed the asset_support branch from b61b4a5 to 1afdca3 Compare November 14, 2022 21:05

corymhall previously requested changes Nov 29, 2022

View reviewed changes

corymhall marked this pull request as ready for review November 29, 2022 19:33

wanjacki force-pushed the asset_support branch from 1109c1f to e6f49a0 Compare December 1, 2022 16:30

Addressed comments

ef9285c

wanjacki force-pushed the asset_support branch from e6f49a0 to ef9285c Compare December 1, 2022 16:54

wanjacki mentioned this pull request Dec 13, 2022

(servicecatalog): ProductStack does not support Assets #20690

Closed

2 tasks

wanjacki and others added 7 commits December 14, 2022 13:36

feat(servicecatalog): Add Product Stack Asset Support

1a2bacf

Updated Integ Test with manual steps for provisioning asset product

47742ba

Addressed comments

96700ea

Removed instanceof and moved lambda to dev dependency

6e2d226

Merge branch 'asset_support' of github.com:wanjacki/aws-cdk into asse…

4c0b57c

…t_support

Use addSource for BucketDeployment

85b3cd3

Merge branch 'aws:main' into asset_support

08f4947

corymhall previously approved these changes Dec 16, 2022

View reviewed changes

Merge branch 'main' into asset_support

b1cce24

Merge branch 'aws:main' into asset_support

c857bf2

fix integ test

400d4bd

corymhall approved these changes Dec 28, 2022

View reviewed changes

Merge branch 'main' into asset_support

6c76ac3

mergify bot merged commit ceaac3a into aws:main Dec 28, 2022

pgarbe mentioned this pull request Jan 4, 2023

StackSets Support aws/aws-cdk-rfcs#66

Closed

11 tasks

padaszewski mentioned this pull request Jan 12, 2023

fix(servicecatalog): incorrect objectkey produced from asset relative… #23580

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wanjacki commented Nov 9, 2022

gitpod-io bot commented Nov 9, 2022

aws-cdk-automation left a comment

wanjacki commented Nov 11, 2022

corymhall left a comment

corymhall Nov 29, 2022

wanjacki Nov 29, 2022

corymhall Nov 30, 2022

corymhall Nov 29, 2022

wanjacki Nov 30, 2022

corymhall Nov 29, 2022

corymhall Nov 29, 2022

wanjacki Nov 29, 2022

wanjacki Nov 30, 2022

corymhall Nov 29, 2022

corymhall Nov 29, 2022

corymhall Nov 29, 2022

wanjacki Nov 29, 2022

corymhall Nov 30, 2022

wanjacki commented Nov 30, 2022

corymhall left a comment

mergify bot commented Dec 16, 2022

wanjacki commented Dec 20, 2022

corymhall commented Dec 22, 2022

mergify bot commented Dec 28, 2022

aws-cdk-automation commented Dec 28, 2022

mergify bot commented Dec 28, 2022


		const physicalName = this.physicalNameOfBucket(this.assetBucket);

		(this.boundStack as ProductStack)._addAsset(asset);

	bucketName: 'product-stack-asset-bucket-12345678-test-region',
	bucketName: `product-stack-asset-bucket-${stack.account}-${stack.region}`,

-const stack = new cdk.Stack(app, 'integ-servicecatalog-product');
+const stack = new cdk.Stack(app, 'integ-servicecatalog-product', {
+  env: {
+    account: process.env.CDK_INTEG_ACCOUNT ?? process.env.CDK_DEFAULT_ACCOUNT,
+    region: process.env.CDK_INTEG_REGION ?? process.env.CDK_DEFAULT_REGION,
+  },
+});


		product.associateTagOptions(tagOptions);
		new cdk.CfnOutput(stack, 'PortfolioId', { value: portfolio.portfolioId });

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

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

Conversation

wanjacki commented Nov 9, 2022

RFC: aws/aws-cdk-rfcs#458

All Submissions:

Adding new Unconventional Dependencies:

New Features

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

gitpod-io bot commented Nov 9, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

wanjacki commented Nov 11, 2022

corymhall 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

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 Nov 30, 2022

corymhall left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 16, 2022

wanjacki commented Dec 20, 2022

corymhall commented Dec 22, 2022

mergify bot commented Dec 28, 2022

aws-cdk-automation commented Dec 28, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 28, 2022