-
Notifications
You must be signed in to change notification settings - Fork 4k
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): Add Product Stack Asset Support #22143
Changes from 3 commits
86b14f6
e245852
37d12c3
0b84503
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import * as iam from '@aws-cdk/aws-iam'; | ||
import { IBucket } from '@aws-cdk/aws-s3'; | ||
import * as sns from '@aws-cdk/aws-sns'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import { Construct } from 'constructs'; | ||
import { Construct, IConstruct } from 'constructs'; | ||
import { MessageLanguage } from './common'; | ||
import { | ||
CloudFormationRuleConstraintOptions, CommonConstraintOptions, | ||
|
@@ -105,7 +106,7 @@ export interface IPortfolio extends cdk.IResource { | |
* @param product A service catalog product. | ||
* @param options options for the constraint. | ||
*/ | ||
constrainCloudFormationParameters(product:IProduct, options: CloudFormationRuleConstraintOptions): void; | ||
constrainCloudFormationParameters(product: IProduct, options: CloudFormationRuleConstraintOptions): void; | ||
|
||
/** | ||
* Force users to assume a certain role when launching a product. | ||
|
@@ -155,6 +156,8 @@ abstract class PortfolioBase extends cdk.Resource implements IPortfolio { | |
public abstract readonly portfolioArn: string; | ||
public abstract readonly portfolioId: string; | ||
private readonly associatedPrincipals: Set<string> = new Set(); | ||
private readonly assetBuckets: Set<IBucket> = new Set<IBucket>(); | ||
private readonly sharedAccounts: string[] = []; | ||
|
||
public giveAccessToRole(role: iam.IRole): void { | ||
this.associatePrincipal(role.roleArn, role.node.addr); | ||
|
@@ -169,11 +172,15 @@ abstract class PortfolioBase extends cdk.Resource implements IPortfolio { | |
} | ||
|
||
public addProduct(product: IProduct): void { | ||
if (product.assetBucket) { | ||
this.assetBuckets.add(product.assetBucket); | ||
} | ||
AssociationManager.associateProductWithPortfolio(this, product, undefined); | ||
} | ||
|
||
public shareWithAccount(accountId: string, options: PortfolioShareOptions = {}): void { | ||
const hashId = this.generateUniqueHash(accountId); | ||
this.sharedAccounts.push(accountId); | ||
new CfnPortfolioShare(this, `PortfolioShare${hashId}`, { | ||
portfolioId: this.portfolioId, | ||
accountId: accountId, | ||
|
@@ -236,6 +243,19 @@ abstract class PortfolioBase extends cdk.Resource implements IPortfolio { | |
} | ||
} | ||
|
||
/** | ||
* Gives access to Asset Buckets to Shared Accounts. | ||
* | ||
*/ | ||
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))), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are you granting access on the other side? The IAM role that gets from these buckets needs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Create a unique id based off the L1 CfnPortfolio or the arn of an imported portfolio. | ||
*/ | ||
|
@@ -336,6 +356,14 @@ export class Portfolio extends PortfolioBase { | |
if (props.tagOptions !== undefined) { | ||
this.associateTagOptions(props.tagOptions); | ||
} | ||
|
||
cdk.Aspects.of(this).add({ | ||
visit(c: IConstruct) { | ||
if (c instanceof Portfolio) { | ||
c.addBucketPermissionsToSharedAccounts(); | ||
}; | ||
}, | ||
}); | ||
} | ||
|
||
protected generateUniqueHash(value: string): string { | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,7 @@ | ||||||
import * as cdk from '@aws-cdk/core'; | ||||||
import { ProductStack } from '../product-stack'; | ||||||
import { ProductStackAssetBucket } from '../product-stack-asset-bucket'; | ||||||
import { hashValues } from './util'; | ||||||
|
||||||
/** | ||||||
* Deployment environment for an AWS Service Catalog product stack. | ||||||
|
@@ -7,6 +10,12 @@ import * as cdk from '@aws-cdk/core'; | |||||
*/ | ||||||
export class ProductStackSynthesizer extends cdk.StackSynthesizer { | ||||||
private stack?: cdk.Stack; | ||||||
private assetBucket?: ProductStackAssetBucket; | ||||||
|
||||||
constructor(assetBucket?: ProductStackAssetBucket) { | ||||||
super(); | ||||||
this.assetBucket = assetBucket; | ||||||
} | ||||||
|
||||||
public bind(stack: cdk.Stack): void { | ||||||
if (this.stack !== undefined) { | ||||||
|
@@ -16,7 +25,17 @@ export class ProductStackSynthesizer extends cdk.StackSynthesizer { | |||||
} | ||||||
|
||||||
public addFileAsset(_asset: cdk.FileAssetSource): cdk.FileAssetLocation { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
throw new Error('Service Catalog Product Stacks cannot use Assets'); | ||||||
if (!this.stack) { | ||||||
throw new Error('You must call bindStack() first'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this message will reach the user. Do you mean to tell your user to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the error we throw in our |
||||||
} | ||||||
|
||||||
if (!this.assetBucket) { | ||||||
const parentStack = (this.stack as ProductStack)._getParentStack(); | ||||||
this.assetBucket = new ProductStackAssetBucket(parentStack, `ProductStackAssetBucket${hashValues(this.stack.stackName)}`); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how I feel about the way we are creating the asset bucket here. What if we just created There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean we create a ProductStackAssetBucket regardless of if the user is using an asset or not. |
||||||
|
||||||
(this.stack as ProductStack)._setAssetBucket(this.assetBucket._getBucket()); | ||||||
return this.assetBucket._addAsset(_asset); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is where we should handle the asset deployment, rather than in the bucket construct. |
||||||
} | ||||||
|
||||||
public addDockerImageAsset(_asset: cdk.DockerImageAssetSource): cdk.DockerImageAssetLocation { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
import { IBucket, BlockPublicAccess, Bucket, BucketEncryption } from '@aws-cdk/aws-s3'; | ||
import { BucketDeployment, ISource, Source } from '@aws-cdk/aws-s3-deployment'; | ||
import * as cdk from '@aws-cdk/core'; | ||
import { Construct, IConstruct } from 'constructs'; | ||
import { hashValues } from './private/util'; | ||
/** | ||
* Product stack asset bucket props. | ||
*/ | ||
export interface ProductStackAssetBucketProps { | ||
/** | ||
* Name of s3 asset bucket deployed | ||
* | ||
* @default - generated | ||
*/ | ||
readonly assetBucketName?: string; | ||
} | ||
|
||
/** | ||
* 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should extends |
||
private readonly bucketName: string; | ||
private readonly bucket: IBucket; | ||
private readonly assets: ISource[]; | ||
|
||
constructor(scope: Construct, id: string, props: ProductStackAssetBucketProps = {}) { | ||
super(scope, id); | ||
|
||
if (props.assetBucketName) { | ||
this.bucketName = props.assetBucketName; | ||
} else { | ||
this.bucketName = this.generateBucketName(id); | ||
} | ||
|
||
this.bucket = new Bucket(scope, `${id}S3Bucket`, { | ||
bucketName: this.bucketName, | ||
blockPublicAccess: BlockPublicAccess.BLOCK_ALL, | ||
encryption: BucketEncryption.KMS, | ||
removalPolicy: cdk.RemovalPolicy.DESTROY, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is |
||
autoDeleteObjects: true, | ||
kaizencc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
this.assets = []; | ||
|
||
cdk.Aspects.of(this).add({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the reasoning behind using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
visit(c: IConstruct) { | ||
if (c instanceof ProductStackAssetBucket) { | ||
c.deployAssets(); | ||
}; | ||
}, | ||
}); | ||
} | ||
|
||
/** | ||
* Fetch the S3 bucket. | ||
* | ||
* @internal | ||
*/ | ||
public _getBucket(): IBucket { | ||
return this.bucket; | ||
} | ||
|
||
/** | ||
* Generate unique name for S3 bucket. | ||
*/ | ||
private generateBucketName(id: string): string { | ||
wanjacki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const accountId = cdk.Stack.of(this).account; | ||
const region = cdk.Stack.of(this).region; | ||
if (cdk.Token.isUnresolved(accountId)) { | ||
throw new Error('CDK Account ID must be defined in the application environment'); | ||
} | ||
if (cdk.Token.isUnresolved(region)) { | ||
throw new Error('CDK Region must be defined in the application environment'); | ||
} | ||
return `product-stack-asset-bucket-${accountId}-${region}-${hashValues(id)}`; | ||
} | ||
|
||
/** | ||
* Fetch the expected S3 location of an asset. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is doing more than fetch right? adds an asset to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to be a bit more descriptive. |
||
* Assets are also prepared for bulk deployment to S3. | ||
* | ||
* @internal | ||
*/ | ||
public _addAsset(asset: cdk.FileAssetSource): cdk.FileAssetLocation { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the asset management should be done in this bucket construct. These assets should be |
||
const assetPath = './cdk.out/' + asset.fileName; | ||
this.assets.push(Source.asset(assetPath)); | ||
|
||
const bucketName = this.bucketName; | ||
const s3Filename = asset.fileName?.split('.')[1] + '.zip'; | ||
const objectKey = `${s3Filename}`; | ||
const s3ObjectUrl = `s3://${bucketName}/${objectKey}`; | ||
const httpUrl = `https://s3.${bucketName}/${objectKey}`; | ||
|
||
return { bucketName, objectKey, httpUrl, s3ObjectUrl, s3Url: httpUrl }; | ||
} | ||
|
||
/** | ||
* Deploy all assets to S3. | ||
*/ | ||
private deployAssets() { | ||
if (this.assets.length > 0) { | ||
new BucketDeployment(this, 'AssetsBucketDeployment', { | ||
sources: this.assets, | ||
destinationBucket: this.bucket, | ||
extract: false, | ||
prune: false, | ||
}); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
None
mean? If this is not provided are assets not supported?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.