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 2 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
11 changes: 5 additions & 6 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,20 @@ 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.
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', {
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', {
environment: ['dev', 'alpha', 'prod'],
});
product.associateTagOptions(tagOptionsForProduct);
```
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export interface IPortfolio extends cdk.IResource {
addProduct(product: IProduct): void;

/**
* [disable-awslint:ref-via-interface]
* Associate Tag Options.
* A TagOption is a key-value pair 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.
Expand Down Expand Up @@ -278,6 +279,7 @@ export interface PortfolioProps {
* TagOptions associated directly to a portfolio.
*
* @default - No tagOptions provided
* [disable-awslint:ref-via-interface]
*/
readonly tagOptions?: TagOptions
}
Expand Down
45 changes: 14 additions & 31 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,36 +139,7 @@ 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,
});
}
});
};
}

private static setLaunchRoleConstraint(
public static setLaunchRoleConstraint(
arcrank marked this conversation as resolved.
Show resolved Hide resolved
portfolio: IPortfolio, product: IProduct, options: CommonConstraintOptions,
roleOptions: LaunchRoleConstraintRoleOptions,
): void {
Expand Down Expand Up @@ -196,6 +167,18 @@ export class AssociationManager {
}
}

public static associateTagOptions(resource: cdk.IResource, resourceId: string, tagOptions: TagOptions): void {
arcrank marked this conversation as resolved.
Show resolved Hide resolved
for (const [tagOptionIdentifier, cfnTagOption] of Object.entries(tagOptions._tagOptionsMap)) {
const tagAssocationConstructId = `TagOptionAssociation${hashValues(resource.node.addr, tagOptionIdentifier)}`;
if (!resource.node.tryFindChild(tagAssocationConstructId)) {
new CfnTagOptionAssociation(resource as cdk.Resource, tagAssocationConstructId, {
resourceId: resourceId,
tagOptionId: cfnTagOption.ref,
});
}
}
}

private static stackSetConstraintLogicalId(associationKey: string): string {
return `StackSetConstraint${associationKey}`;
}
Expand Down
6 changes: 4 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 All @@ -24,6 +24,7 @@ export interface IProduct extends IResource {
readonly productId: string;

/**
* [disable-awslint:ref-via-interface]
* Associate Tag Options.
* A TagOption is a key-value pair 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.
Expand Down Expand Up @@ -133,11 +134,12 @@ export interface CloudFormationProductProps {
readonly supportUrl?: string;

/**
* [disable-awslint:ref-via-interface]
arcrank marked this conversation as resolved.
Show resolved Hide resolved
* TagOptions associated directly to a product.
*
* @default - No tagOptions provided
*/
readonly tagOptions?: TagOptions
readonly tagOptions?: TagOptions;
}

/**
Expand Down
48 changes: 40 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,46 @@
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.
* 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
* @resource AWS::ServiceCatalog::TagOption
arcrank marked this conversation as resolved.
Show resolved Hide resolved
*/
export class TagOptions {
export class TagOptions extends cdk.Resource {
/**
* List of CfnTagOption
*/
public readonly tagOptionsMap: { [key: string]: string[] };
* Map of underlying TagOption resources.
*
* @internal
*/
public readonly _tagOptionsMap: { [key: string]: CfnTagOption };

constructor(scope: Construct, id: string, tagOptions: {[key: string]: string[] }) {
Copy link
Contributor Author

@arcrank arcrank Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The cdk linting doesn't like this setup (no interface and no props struct) but I don't particularly see value of having classes that are just pass throughs. I don't know if making it props with a tagOptions field is better since it seems redundant to do

const tagO = new servicecatalog.TagOptions(this, 'TagOptions', {
  tagOptions : {
    key1: ['v1', 'v2', 'v3'],
  }
});

Only if its a little bit of future proofing in case we want to add another field to props then it would be easier to make change.
I will defer to you though on this.

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 think linting rules have become more stringent? Build is failing since you apparently need to disable interface ref in js docs ( I had hoped I could just do it in package.json).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just introduce a TagOptionsProps interface as the third argument, like you suggested in #18314 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that works, leaves more wiggle room to not break things in future should changes happen and we want to modify props.

super(scope, id);
arcrank marked this conversation as resolved.
Show resolved Hide resolved
this._tagOptionsMap = this.createUnderlyingTagOptions(tagOptions);
}

constructor(tagOptionsMap: { [key: string]: string[]} ) {
this.tagOptionsMap = { ...tagOptionsMap };
private createUnderlyingTagOptions(tagOptions: { [key: string]: string[] }): { [key: string]: CfnTagOption } {
var tagOptionMap: { [key: string]: CfnTagOption } = {};
for (const [key, tagOptionsList] of Object.entries(tagOptions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

key is also a bad name. This is actually a tagName, right?

Copy link
Contributor Author

@arcrank arcrank Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

sure,
the raw values -> tagKey: tagValue and then the hashed value in map tagOptionIdentifier

InputValidator.validateLength(this.node.addr, 'TagOption key', 1, 128, key);
tagOptionsList.forEach((value: string) => {
arcrank marked this conversation as resolved.
Show resolved Hide resolved
InputValidator.validateLength(this.node.addr, 'TagOption value', 1, 256, value);
const tagOptionIdentifier = `$TagOption${hashValues(key, value)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's kill the TagOptions prefix from here - it makes the logical ID duplicated, like "TagOptionsTagOption5f31c54ba705C49A7414".

Just do:

Suggested change
const tagOptionIdentifier = `$TagOption${hashValues(key, value)}`;
const tagOptionIdentifier = hashValues(key, value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I wasn't sure before because someone might have used a contextual name in naming the resource like 'TagOptionsForCatalog2' that might make it easier to understand the enumerated resource names in template.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would you say if, instead of hashing, we use just number indexes for these, counting from 1? (Not that each TagOptions is a construct, these don't have to be unique, unlike when we created these under the Stack scope).

This would make the resulting template much cleaner.

So, this would be something like:

Suggested change
const tagOptionIdentifier = `$TagOption${hashValues(key, value)}`;
const tagOptionIdentifier = `${i + 1}_${j + 1}`;

Where i and j are indexes in a loop - i is the index of the tag, and j is the index of the allowed values of the tag.

Thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about in the case of updating and modifying the values? e.g. we might have

key: ['value1', 'value2', 'value3', value4']

and then I update this to just be

key: ['value1', 'value2', 'value4']

I wouldn't expect to see 2 updated resources in template (1 deleted, 1 modified), only the one that was removed.

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: key,
value: value,
});
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;
}
}
7 changes: 6 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,12 @@
"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",
"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",
"construct-ctor:@aws-cdk/aws-servicecatalog.TagOptions.<initializer>.params[2]"
]
},
"maturity": "experimental",
Expand Down
39 changes: 18 additions & 21 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": {
"TestPortfolioTagOptionAssociation588b5adc10dd7F84BFE3": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOptionc0d88a3c4b8b"
"Ref": "TagOptionsTagOption5f31c54ba705C49A7414"
}
}
},
"TestPortfolioTagOptionAssociationb38e9aae7f1bD3708991": {
"TestPortfolioTagOptionAssociationf2f58e7500fcE8FB28E4": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOption9b16df08f83d"
"Ref": "TagOptionsTagOption8d263919cebbA227EEAD"
}
}
},
"TestPortfolioTagOptionAssociationeeabbf0db0e3ADBF0A6D": {
"TestPortfolioTagOptionAssociation3529ad9bdc559519AFC8": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestPortfolio4AC794EB"
},
"TagOptionId": {
"Ref": "TagOptiondf34c1c83580"
"Ref": "TagOptionsTagOptiona260cbbd99c48E537D5E"
}
}
},
Expand Down Expand Up @@ -217,28 +217,25 @@
"TestPortfolioPortfolioProductAssociationa0185761d231B0D998A7"
]
},
"TagOptionc0d88a3c4b8b": {
"TagOptionsTagOption5f31c54ba705C49A7414": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value1",
"Active": true
arcrank marked this conversation as resolved.
Show resolved Hide resolved
"Value": "value1"
}
},
"TagOption9b16df08f83d": {
"TagOptionsTagOption8d263919cebbA227EEAD": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key1",
"Value": "value2",
"Active": true
"Value": "value2"
}
},
"TagOptiondf34c1c83580": {
"TagOptionsTagOptiona260cbbd99c48E537D5E": {
"Type": "AWS::ServiceCatalog::TagOption",
"Properties": {
"Key": "key2",
"Value": "value1",
"Active": true
"Value": "value1"
}
},
"TestProduct7606930B": {
Expand All @@ -256,36 +253,36 @@
]
}
},
"TestProductTagOptionAssociation667d45e6d8a1F30303D6": {
"TestProductTagOptionAssociationac2ca35a25e686F7BF73": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptionc0d88a3c4b8b"
"Ref": "TagOptionsTagOption5f31c54ba705C49A7414"
}
}
},
"TestProductTagOptionAssociationec68fcd0154fF6DAD979": {
"TestProductTagOptionAssociatione355ed94b1c70354F0BD": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOption9b16df08f83d"
"Ref": "TagOptionsTagOption8d263919cebbA227EEAD"
}
}
},
"TestProductTagOptionAssociation259ba31b62cc63D068F9": {
"TestProductTagOptionAssociation25ff027af76d99BF7AF0": {
"Type": "AWS::ServiceCatalog::TagOptionAssociation",
"Properties": {
"ResourceId": {
"Ref": "TestProduct7606930B"
},
"TagOptionId": {
"Ref": "TagOptiondf34c1c83580"
"Ref": "TagOptionsTagOptiona260cbbd99c48E537D5E"
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion 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,7 +22,7 @@ const portfolio = new servicecatalog.Portfolio(stack, 'TestPortfolio', {
portfolio.giveAccessToRole(role);
portfolio.giveAccessToGroup(group);

const tagOptions = new servicecatalog.TagOptions({
const tagOptions = new servicecatalog.TagOptions(stack, 'TagOptions', {
key1: ['value1', 'value2'],
key2: ['value1'],
});
Expand Down
Loading