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(cfn2ts) Add support for Tags on L1 constructs #1007

Closed
wants to merge 1 commit into from
Closed

feat(cfn2ts) Add support for Tags on L1 constructs #1007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 2 additions & 11 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
});
this.connections = new ec2.Connections({ securityGroup: this.securityGroup });
this.securityGroups.push(this.securityGroup);
this.tags = new TagManager(this, {initialTags: props.tags});
this.tags = new cdk.TagManager(this, {initialTags: props.tags,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I don't like the line formatting here now that this thing has two property arguments. Can you indent this to new new line.

tagFormatter: new cdk.AutoScalingGroupTagFormatter()});
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the composition/formatter approach as well, but I don't feel like AutoScalingGroupTagFormatter needs to live in the CDK library. Why couldn't it just live in the AutoScaling library, next to the only place where it's being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy to move this one. The Map formatter is actually in the CFN spec in two different ways and currently used only in DAX and Serverless? Should that one move?

this.tags.setTag(NAME_TAG, this.path, { overwrite: false });

this.role = new iam.Role(this, 'InstanceRole', {
Expand Down Expand Up @@ -489,16 +490,6 @@ function renderRollingUpdateConfig(config: RollingUpdateConfiguration = {}): cdk
};
}

class TagManager extends cdk.TagManager {
protected tagFormatResolve(tagGroups: cdk.TagGroups): any {
const tags = {...tagGroups.nonStickyTags, ...tagGroups.ancestorTags, ...tagGroups.stickyTags};
return Object.keys(tags).map( (key) => {
const propagateAtLaunch = !!tagGroups.propagateTags[key] || !!tagGroups.ancestorTags[key];
return {key, value: tags[key], propagateAtLaunch};
});
}
}

/**
* Render a number of seconds to a PTnX string.
*/
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/aws-ec2/lib/security-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
securityGroupIngress: new Token(() => this.directIngressRules),
securityGroupEgress: new Token(() => this.directEgressRules),
vpcId: props.vpc.vpcId,
tags: this.tags,
});

this.securityGroupId = this.securityGroup.securityGroupId;
Expand Down
8 changes: 2 additions & 6 deletions packages/@aws-cdk/aws-ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
instanceTenancy,
tags: this.tags,
});
// this.resource.tags = this.tags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove completely or comment back in.


this.availabilityZones = new cdk.AvailabilityZoneProvider(this).availabilityZones;
this.availabilityZones.sort();
Expand All @@ -309,9 +310,7 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {

// Create an Internet Gateway and attach it if necessary
if (allowOutbound) {
const igw = new cloudformation.InternetGatewayResource(this, 'IGW', {
tags: new cdk.TagManager(this),
});
const igw = new cloudformation.InternetGatewayResource(this, 'IGW');
const att = new cloudformation.VPCGatewayAttachmentResource(this, 'VPCGW', {
internetGatewayId: igw.ref,
vpcId: this.resource.ref
Expand Down Expand Up @@ -494,12 +493,10 @@ export class VpcSubnet extends VpcSubnetRef implements cdk.ITaggable {
cidrBlock: props.cidrBlock,
availabilityZone: props.availabilityZone,
mapPublicIpOnLaunch: props.mapPublicIpOnLaunch,
tags: this.tags,
});
this.subnetId = subnet.subnetId;
const table = new cloudformation.RouteTableResource(this, 'RouteTable', {
vpcId: props.vpcId,
tags: new cdk.TagManager(this),
});
this.routeTableId = table.ref;

Expand Down Expand Up @@ -556,7 +553,6 @@ export class VpcPublicSubnet extends VpcSubnet {
allocationId: new cloudformation.EIPResource(this, `EIP`, {
domain: 'vpc'
}).eipAllocationId,
tags: new cdk.TagManager(this),
});
return ngw.natGatewayId;
}
Expand Down
46 changes: 24 additions & 22 deletions packages/@aws-cdk/aws-ec2/test/test.vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ export = {
'the Name tag is defaulted to path'(test: Test) {
const stack = getTestStack();
new VpcNetwork(stack, 'TheVPC');
expect(stack).to(haveResource('AWS::EC2::VPC',
hasTags( [ {Key: 'Name', Value: 'TheVPC'} ])));
expect(stack).to(
haveResource('AWS::EC2::VPC',
hasTags( [ {Key: 'Name', Value: 'TheVPC'} ]))
);
test.done();
},

Expand Down Expand Up @@ -124,25 +126,25 @@ export = {
new VpcNetwork(stack, 'TheVPC', {
cidr: '10.0.0.0/21',
subnetConfiguration: [
{
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.Public,
tags: {
type: 'Public',
init: 'No',
{
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.Public,
tags: {
type: 'Public',
init: 'No',
},
},
},
{
cidrMask: 24,
name: 'application',
subnetType: SubnetType.Private,
},
{
cidrMask: 28,
name: 'rds',
subnetType: SubnetType.Isolated,
}
{
cidrMask: 24,
name: 'application',
subnetType: SubnetType.Private,
},
{
cidrMask: 28,
name: 'rds',
subnetType: SubnetType.Isolated,
}
],
maxAZs: 3
});
Expand All @@ -151,12 +153,12 @@ export = {
expect(stack).to(countResources("AWS::EC2::Subnet", 9));
for (let i = 0; i < 6; i++) {
expect(stack).to(haveResource("AWS::EC2::Subnet", {
CidrBlock: `10.0.${i}.0/24`
CidrBlock: `10.0.${i}.0/24`
}));
}
for (let i = 0; i < 3; i++) {
expect(stack).to(haveResource("AWS::EC2::Subnet", {
CidrBlock: `10.0.6.${i * 16}/28`
CidrBlock: `10.0.6.${i * 16}/28`
}));
}
expect(stack).to(haveResource("AWS::EC2::Subnet", hasTags(
Expand Down
124 changes: 124 additions & 0 deletions packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Construct } from '../core/construct';
import { ITagFormatter, ITaggable, TagGroups, TagManager } from '../core/tag-manager';
import { capitalizePropertyNames, ignoreEmpty } from '../core/util';
import { CloudFormationToken } from './cloudformation-token';
import { Condition } from './condition';
Expand Down Expand Up @@ -229,6 +230,129 @@ export class Resource extends Referenceable {
}
}

export enum TagType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs will be coming here

Standard = "StandardTag",
AutoScalingGroup = "AutoScalingGroupTag",
Map = "StringToStringMap",
}

/**
* Represents a CloudFormation resource which supports Tags
*
* The resource exposes `tags` property as the `TagManager` for this resource. The
* developer can set and remove tags from this location in the construct tree using
* the tags object. For example:
*
* ```
* const myResource = new MyTaggableResource(parent, id, props: {});
* myResource.setTag('Mykey, 'MyValue');
* // you can also configure behavior with `TagProps`
* myResource.setTag('MyKey', 'MyValue', { propagate: false });
* ```
*/
export class TaggableResource extends Resource implements ITaggable {
moofish32 marked this conversation as resolved.
Show resolved Hide resolved
/**
* TagManager to manage the propagation and assignment of tags
*/
public readonly tags: TagManager;

protected readonly tagType: TagType = TagType.Standard;

constructor(parent: Construct, id: string, props: ResourceProps) {
super(parent, id, props);
if (props.properties && props.properties.tags instanceof TagManager) {
this.tags = props.properties.tags;
} else {
this.tags = new TagManager(this);
if (!!props.properties && props.properties.tags) {
this.addCloudFormationTags(props.properties.tags);
}
}
}

/**
* Emits CloudFormation for this resource.
*
* This method calls super after resolving the cloudformation tags.
*/
public toCloudFormation(): object {
// typescript initializes child second so this can't be dont in the constructor
this.tags.tagFormatter = this.tagFormatterForType();
this.properties.tags = this.tags;
return super.toCloudFormation();
}

private tagFormatterForType(): ITagFormatter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to make this map indexed by the enum? I was fighting with tsc to make it work and finally just moved forward with this because I have bigger questions.

switch (this.tagType) {
case TagType.Standard: {
return new StandardTagFormatter();
}
case TagType.AutoScalingGroup: {
return new AutoScalingGroupTagFormatter();
}
case TagType.Map: {
return new MapTagFormatter();
}
}
}

/**
* Add any of the supported CloudFormatiom Tag Types to be managed
*
* @param tags: The tag(s) to add
Copy link
Contributor Author

@moofish32 moofish32 Nov 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't love this. The challenge is the typescript initialization flow is done

  1. parent init
  2. parent constructor
  3. child init
  4. child constructor

So this.tagType actually isn't set when the constructor runs. I kicked around a lot of ideas, but couldn't find an elegant solution. I considered making all tags only support cdk.TagManager instead of the union type. When I coded with that I didn't like interface of having to create a new object.

const MyResource(parent, 'Name', {
  prop1: 'value',
  prop2: 'value,
  tags: new cdk.TagManager(parent, 'TagforMe', {
    initialTags: {
       myKey: 'MyValue',
       key2: 'Value2',
    },
  });

So I went to this method, but I'm very open to some suggestions for a better solution here. In the end the CFN validators still catch mistakes at synthesis time.

*/
private addCloudFormationTags(tags: any) {
if (tags === undefined) { return; }
if (tags.map) {
for (const tag of tags) {
const propagate = tag.propagateAtLaunch !== false;
this.tags.setTag(tag.key, tag.value, {propagate});
}
} else {
for (const key of Object.keys(tags)) {
this.tags.setTag(key, tags[key]);
}
}
}
}

/**
* Handles converting TagManager tags to AutoScalingGroupTags
*/
export class AutoScalingGroupTagFormatter implements ITagFormatter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should these live here or in tag-manager? I kept flip-flopping on this. Also I did prefer the composition pattern versus the extension pattern, but it's easy to go back.

public toCloudFormationTags(tagGroups: TagGroups): any {
const tags = {...tagGroups.nonStickyTags, ...tagGroups.ancestorTags, ...tagGroups.stickyTags};
return Object.keys(tags).map( (key) => {
const propagateAtLaunch = !!tagGroups.propagateTags[key] || !!tagGroups.ancestorTags[key];
return {key, value: tags[key], propagateAtLaunch};
});
}
}

/**
* Handles converting TagManager tags to a map of string to string tags
*/
export class MapTagFormatter implements ITagFormatter {
public toCloudFormationTags(tagGroups: TagGroups): any {
const tags = {...tagGroups.nonStickyTags, ...tagGroups.ancestorTags, ...tagGroups.stickyTags};
return Object.keys(tags).length === 0 ? undefined : tags;
}
}

/**
* Handles converting TagManager tags to a standard array of key value pairs
*/
export class StandardTagFormatter implements ITagFormatter {
public toCloudFormationTags(tagGroups: TagGroups): any {
const tags = {...tagGroups.nonStickyTags, ...tagGroups.ancestorTags, ...tagGroups.stickyTags};
const cfnTags = Object.keys(tags).map( key => ({key, value: tags[key]}));
if (cfnTags.length === 0) {
return undefined;
}
return cfnTags;
}
}

export interface ResourceOptions {
/**
* A condition to associate with this resource. This means that only if the condition evaluates to 'true' when the stack
Expand Down
Loading