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.

Sign up for GitHub

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): Create TagOptions Construct #18314

Merged
merged 19 commits into from
Jan 25, 2022
Merged

feat(servicecatalog): Create TagOptions Construct #18314

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d0e30d3
feat(servicecatalog): Create standalone TagOptions Construct
Jan 7, 2022
c3d930c
addressing some comments
Jan 14, 2022
023e440
quick fix
Jan 14, 2022
d519888
put modified tests back
Jan 14, 2022
e6cc8cb
Merge branch 'master' into tagOptionsConstruct
arcrank Jan 14, 2022
fb125c1
removing linting lines
Jan 14, 2022
f67a303
Merging in changes from mainline
Jan 14, 2022
d1cfcd4
fixing lint in package.json
Jan 14, 2022
58e9391
make props struct
Jan 14, 2022
f33d001
addressing comments
Jan 19, 2022
8c0f465
removing unintentional change
Jan 19, 2022
e434806
another unintentional typo
Jan 19, 2022
35e831f
one last doc cleanup
Jan 20, 2022
03d733a
add validation
Jan 20, 2022
e5bfdef
Merge branch 'master' into tagOptionsConstruct
arcrank Jan 21, 2022
e934d89
simplify iterating over unique tag values
Jan 21, 2022
ca9ec79
maintain current behavior for association resource naming
Jan 24, 2022
84f9410
merging from master
Jan 24, 2022
5f1f4a6
Merge branch 'master' into tagOptionsConstruct
mergify[bot] Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions packages/@aws-cdk/aws-servicecatalog/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,21 +201,24 @@ portfolio.addProduct(product);
## Tag Options

TagOptions allow administrators to easily manage tags on provisioned products by creating a selection of tags for end users to choose from.
For example, an end user can choose an `ec2` for the instance type size.
arcrank marked this conversation as resolved.
Show resolved Hide resolved
TagOptions are created by specifying a key with a selection of values and can be associated with both portfolios and products.
TagOptions are created by specifying a tag key with a selection of allowed values and can be associated with both portfolios and products.
When launching a product, both the TagOptions associated with the product and the containing portfolio are made available.

At the moment, TagOptions can only be disabled in the console.

```ts fixture=portfolio-product
const tagOptionsForPortfolio = new servicecatalog.TagOptions({
costCenter: ['Data Insights', 'Marketing'],
const tagOptionsForPortfolio = new servicecatalog.TagOptions(this, 'OrgTagOptions', {
allowedValuesForTags: {
Group: ['finance', 'engineering', 'marketing', 'research'],
CostCenter: ['01', '02','03'],
},
});
portfolio.associateTagOptions(tagOptionsForPortfolio);

const tagOptionsForProduct = new servicecatalog.TagOptions({
ec2InstanceType: ['A1', 'M4'],
ec2InstanceSize: ['medium', 'large'],
const tagOptionsForProduct = new servicecatalog.TagOptions(this, 'ProductTagOptions', {
allowedValuesForTags: {
Environment: ['dev', 'alpha', 'prod'],
},
});
product.associateTagOptions(tagOptionsForProduct);
```
Expand Down
37 changes: 10 additions & 27 deletions packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { IPortfolio } from '../portfolio';
import { IProduct } from '../product';
import {
CfnLaunchNotificationConstraint, CfnLaunchRoleConstraint, CfnLaunchTemplateConstraint, CfnPortfolioProductAssociation,
CfnResourceUpdateConstraint, CfnStackSetConstraint, CfnTagOption, CfnTagOptionAssociation,
CfnResourceUpdateConstraint, CfnStackSetConstraint, CfnTagOptionAssociation,
} from '../servicecatalog.generated';
import { TagOptions } from '../tag-options';
import { hashValues } from './util';
Expand Down Expand Up @@ -139,33 +139,16 @@ export class AssociationManager {
}
}


public static associateTagOptions(resource: cdk.IResource, resourceId: string, tagOptions: TagOptions): void {
const resourceStack = cdk.Stack.of(resource);
for (const [key, tagOptionsList] of Object.entries(tagOptions.tagOptionsMap)) {
InputValidator.validateLength(resource.node.addr, 'TagOption key', 1, 128, key);
tagOptionsList.forEach((value: string) => {
InputValidator.validateLength(resource.node.addr, 'TagOption value', 1, 256, value);
const tagOptionKey = hashValues(key, value, resourceStack.node.addr);
const tagOptionConstructId = `TagOption${tagOptionKey}`;
let cfnTagOption = resourceStack.node.tryFindChild(tagOptionConstructId) as CfnTagOption;
if (!cfnTagOption) {
cfnTagOption = new CfnTagOption(resourceStack, tagOptionConstructId, {
key: key,
value: value,
active: true,
});
}
const tagAssocationKey = hashValues(key, value, resource.node.addr);
const tagAssocationConstructId = `TagOptionAssociation${tagAssocationKey}`;
if (!resource.node.tryFindChild(tagAssocationConstructId)) {
new CfnTagOptionAssociation(resource as cdk.Resource, tagAssocationConstructId, {
resourceId: resourceId,
tagOptionId: cfnTagOption.ref,
});
}
});
};
for (const [tagOptionIdentifier, cfnTagOption] of Object.entries(tagOptions._tagOptionsMap)) {
const tagAssocationConstructId = `TagOptionAssociation${hashValues(resource.node.addr, tagOptionIdentifier)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is interesting - we are actually changing the algorithm used for generating the identifier of the CfnTagOptionAssociation resource. Previously, it was `TagOptionAssociation${hashValues(key, value, resource.node.addr)}` - now, it's `TagOptionAssociation${hashValues(resource.node.addr, tagOptionIdentifier)}`.

I wonder why this change? Wouldn't keeping the same algorithm reduce the number of changes in the template for customers upgrading to a new CDK version that will include these changes?

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 have been able to maintain current behavior here. I didn't try to maintain this before because from my OOP life it felt wrong to add a more coupling between the two resources, and I more or less tracked the impact of changes that would happen as all or nothing - since we could expect to see the TagOption resources changing I thought it might just go hand in hand to expect associations to change as well.

If this new version is fine, my understanding is now better for it.

if (!resource.node.tryFindChild(tagAssocationConstructId)) {
new CfnTagOptionAssociation(resource as cdk.Resource, tagAssocationConstructId, {
resourceId: resourceId,
tagOptionId: cfnTagOption.ref,
});
}
}
}

private static setLaunchRoleConstraint(
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-servicecatalog/lib/product.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { ArnFormat, IResource, Resource, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { TagOptions } from '.';
import { CloudFormationTemplate } from './cloudformation-template';
import { MessageLanguage } from './common';
import { AssociationManager } from './private/association-manager';
import { InputValidator } from './private/validation';
import { CfnCloudFormationProduct } from './servicecatalog.generated';
import { TagOptions } from './tag-options';

/**
* A Service Catalog product, currently only supports type CloudFormationProduct
Expand Down Expand Up @@ -137,7 +137,7 @@ export interface CloudFormationProductProps {
*
* @default - No tagOptions provided
*/
readonly tagOptions?: TagOptions
readonly tagOptions?: TagOptions;
}

/**
Expand Down
69 changes: 61 additions & 8 deletions packages/@aws-cdk/aws-servicecatalog/lib/tag-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,67 @@
import * as cdk from '@aws-cdk/core';
import { hashValues } from './private/util';
import { InputValidator } from './private/validation';
import { CfnTagOption } from './servicecatalog.generated';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from 'constructs';

/**
* Defines a Tag Option, which are similar to tags
* but have multiple values per key.
* Properties for TagOptions.
*/
export class TagOptions {
export interface TagOptionsProps {
/**
* List of CfnTagOption
*/
public readonly tagOptionsMap: { [key: string]: string[] };
* The values that are allowed to be set for specific tags.
* The keys of the map represent the tag keys,
* and the values of the map are a list of allowed values for that particular tag key.
*/
readonly allowedValuesForTags: { [tagKey: string]: string[] };
}

/**
* Defines a set of TagOptions, which are a list of key-value pairs managed in AWS Service Catalog.
* It is not an AWS tag, but serves as a template for creating an AWS tag based on the TagOption.
arcrank marked this conversation as resolved.
Show resolved Hide resolved
* See https://docs.aws.amazon.com/servicecatalog/latest/adminguide/tagoptions.html
*
* @resource AWS::ServiceCatalog::TagOption
arcrank marked this conversation as resolved.
Show resolved Hide resolved
*/
export class TagOptions extends cdk.Resource {
/**
* Map of underlying TagOption resources.
*
* @internal
*/
public readonly _tagOptionsMap: { [tagOptionIdentifier: string]: CfnTagOption };

constructor(scope: Construct, id: string, props: TagOptionsProps) {
super(scope, id);
arcrank marked this conversation as resolved.
Show resolved Hide resolved
this._tagOptionsMap = this.createUnderlyingTagOptions(props.allowedValuesForTags);
}

constructor(tagOptionsMap: { [key: string]: string[]} ) {
this.tagOptionsMap = { ...tagOptionsMap };
private createUnderlyingTagOptions(allowedValuesForTags: { [tagKey: string]: string[] }): { [tagOptionIdentifier: string]: CfnTagOption } {
if (Object.keys(allowedValuesForTags).length === 0) {
throw new Error(`No tag option keys or values were provided for resource ${this.node.path}`);
}
var tagOptionMap: { [tagOptionIdentifier: string]: CfnTagOption } = {};
for (const [tagKey, tagValues] of Object.entries(allowedValuesForTags)) {
InputValidator.validateLength(this.node.addr, 'TagOption key', 1, 128, tagKey);
if (tagValues.length === 0) {
throw new Error(`No tag option values were provided for tag option key ${tagKey} for resource ${this.node.path}`);
}
tagValues.forEach((tagValue: string) => {
InputValidator.validateLength(this.node.addr, 'TagOption value', 1, 256, tagValue);
const tagOptionIdentifier = hashValues(tagKey, tagValue);
if (!this.node.tryFindChild(tagOptionIdentifier)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if doesn't make sense anymore, right? It can never be true anymore.

Pretty sure we should kill it.

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 is carryover from how we originally did things so that if you for example had duplicate values it would be idempotent and ignore. I would prefer it if we could modify it so that creation is very strict, but that would change behavior again.

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 changed this now to just get check for unique values, and we don't need to examine construct tree, which to some degree might have minor latency if construct tree is big.

const tagOption = new CfnTagOption(this, tagOptionIdentifier, {
key: tagKey,
value: tagValue,
active: true,
});
tagOptionMap[tagOptionIdentifier] = tagOption;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since tagOptionIdentifier is also the identifier of the CfnTagOption resource, is there actually any reason for this map? I think we can just have an array of CfnTagOption, and then retrieve its identifier with cfnTagOption.node.id (or possibly Node.of(cfnTagOption).id if it uses the new construct API).

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 can just make this a list, the map part was a relic of an earlier point where we considered adding lookup access to an underlying tagOption(s), and a map felt more efficient than searching list.

}
});
}
return tagOptionMap;
}
}
8 changes: 7 additions & 1 deletion packages/@aws-cdk/aws-servicecatalog/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,13 @@
"props-physical-name:@aws-cdk/aws-servicecatalog.CloudFormationProductProps",
"resource-attribute:@aws-cdk/aws-servicecatalog.Portfolio.portfolioName",
"props-physical-name:@aws-cdk/aws-servicecatalog.PortfolioProps",
"props-physical-name:@aws-cdk/aws-servicecatalog.ProductStack"
"props-physical-name:@aws-cdk/aws-servicecatalog.ProductStack",
"props-struct-name:@aws-cdk/aws-servicecatalog.ITagOptions",
"props-physical-name:@aws-cdk/aws-servicecatalog.TagOptionsProps",
"ref-via-interface:@aws-cdk/aws-servicecatalog.CloudFormationProductProps.tagOptions",
"ref-via-interface:@aws-cdk/aws-servicecatalog.IProduct.associateTagOptions.tagOptions",
"ref-via-interface:@aws-cdk/aws-servicecatalog.IPortfolio.associateTagOptions.tagOptions",
"ref-via-interface:@aws-cdk/aws-servicecatalog.PortfolioProps.tagOptions"
]
},
"maturity": "experimental",
Expand Down
30 changes: 15 additions & 15 deletions packages/@aws-cdk/aws-servicecatalog/test/integ.portfolio.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,36 +74,36 @@
"PrincipalType": "IAM"
}
},
"TestPortfolioTagOptionAssociation517ba9dbaf19EA8252F0": {
"TestPortfolioTagOptionAssociation0dc7162cf16e2040A630": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOptionc0d88a3c4b8b"
"Ref": "TagOptions5f31c54ba705F110F743"
}
}
},
"TestPortfolioTagOptionAssociationb38e9aae7f1bD3708991": {
"TestPortfolioTagOptionAssociation7bee6a69204e42AD5E51": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOption9b16df08f83d"
"Ref": "TagOptions8d263919cebb6764AC10"
}
}
},
"TestPortfolioTagOptionAssociationeeabbf0db0e3ADBF0A6D": {
"TestPortfolioTagOptionAssociation936d6047c6d1807D4E89": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOptiondf34c1c83580"
"Ref": "TagOptionsa260cbbd99c416C40F73"
}
}
},
Expand Down Expand Up @@ -217,23 +217,23 @@
"TestPortfolioPortfolioProductAssociationa0185761d231B0D998A7"
]
},
"TagOptionc0d88a3c4b8b": {
"TagOptions5f31c54ba705F110F743": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value1",
"Active": true
arcrank marked this conversation as resolved.
Show resolved Hide resolved
}
},
"TagOption9b16df08f83d": {
"TagOptions8d263919cebb6764AC10": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value2",
"Active": true
}
},
"TagOptiondf34c1c83580": {
"TagOptionsa260cbbd99c416C40F73": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key2",
Expand All @@ -256,36 +256,36 @@
]
}
},
"TestProductTagOptionAssociation667d45e6d8a1F30303D6": {
"TestProductTagOptionAssociationd168a0728cec938F3943": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptionc0d88a3c4b8b"
"Ref": "TagOptions5f31c54ba705F110F743"
}
}
},
"TestProductTagOptionAssociationec68fcd0154fF6DAD979": {
"TestProductTagOptionAssociation5c417c784ec745E5B016": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOption9b16df08f83d"
"Ref": "TagOptions8d263919cebb6764AC10"
}
}
},
"TestProductTagOptionAssociation259ba31b62cc63D068F9": {
"TestProductTagOptionAssociation804e41acdcbfC22D2E68": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptiondf34c1c83580"
"Ref": "TagOptionsa260cbbd99c416C40F73"
}
}
},
Expand Down
8 changes: 5 additions & 3 deletions packages/@aws-cdk/aws-servicecatalog/test/integ.portfolio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ const portfolio = new servicecatalog.Portfolio(stack, 'TestPortfolio', {
portfolio.giveAccessToRole(role);
portfolio.giveAccessToGroup(group);

const tagOptions = new servicecatalog.TagOptions({
key1: ['value1', 'value2'],
key2: ['value1'],
const tagOptions = new servicecatalog.TagOptions(stack, 'TagOptions', {
allowedValuesForTags: {
key1: ['value1', 'value2'],
key2: ['value1'],
},
});
portfolio.associateTagOptions(tagOptions);

Expand Down
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-servicecatalog/test/integ.product.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,56 +219,56 @@
]
}
},
"TestProductTagOptionAssociation0d813eebb333DA3E2F21": {
"TestProductTagOptionAssociation6980d25bd022DE24DD5B": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptionab501c9aef99"
"Ref": "TagOptions5f31c54ba705F110F743"
}
}
},
"TestProductTagOptionAssociation5d93a5c977b4B664DD87": {
"TestProductTagOptionAssociation5e4145fb974a24712424": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptiona453ac93ee6f"
"Ref": "TagOptions8d263919cebb6764AC10"
}
}
},
"TestProductTagOptionAssociationcfaf40b186a3E5FDECDC": {
"TestProductTagOptionAssociation5a46c2a214678FC25723": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptiona006431604cb"
"Ref": "TagOptionsa260cbbd99c416C40F73"
}
}
},
"TagOptionab501c9aef99": {
"TagOptions5f31c54ba705F110F743": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value1",
"Active": true
}
},
"TagOptiona453ac93ee6f": {
"TagOptions8d263919cebb6764AC10": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value2",
"Active": true
}
},
"TagOptiona006431604cb": {
"TagOptionsa260cbbd99c416C40F73": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key2",
Expand Down
Loading