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.

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

fix(cdk): merge cloudFormation tags with aspect tags #1762

Merged
merged 14 commits into from
Mar 13, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
10 changes: 9 additions & 1 deletion packages/@aws-cdk/cdk/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,19 @@ has a few features that are covered later to explain how this works.

### API

In order to enable additional controls a Tags can specifically include or
In order to enable additional controls a Tag can specifically include or
exclude a CloudFormation Resource Type, propagate tags for an autoscaling group,
and use priority to override the default precedence. See the `TagProps`
interface for more details.

Tags can be configured by using the properties for the AWS CloudFormation layer
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
resources or by using the tag aspects described here. The aspects will always
take precedence over the AWS CloudFormation layer in the event of a name
collision. The tags will be merged otherwise. For the aspect based tags, the
tags applied closest to the resource will take precedence, given an equal
priority. A higher priority tag will always take precedence over a lower
priority tag.

#### applyToLaunchedInstances

This property is a boolean that defaults to `true`. When `true` and the aspect
Expand Down
8 changes: 6 additions & 2 deletions packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ export class Tag extends TagBase {
*/
public readonly value: string;

private readonly defaultPriority = 100;

constructor(key: string, value: string, props: TagProps = {}) {
super(key, props);
this.props.applyToLaunchedInstances = props.applyToLaunchedInstances !== false;
this.props.priority = props.priority === undefined ? 0 : props.priority;
this.props.priority = props.priority === undefined ? this.defaultPriority : props.priority;
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
if (value === undefined) {
throw new Error('Tag must have a value');
}
Expand All @@ -63,9 +65,11 @@ export class Tag extends TagBase {
*/
export class RemoveTag extends TagBase {

private readonly defaultPriority = 200;

constructor(key: string, props: TagProps = {}) {
super(key, props);
this.props.priority = props.priority === undefined ? 1 : props.priority;
this.props.priority = props.priority === undefined ? this.defaultPriority : props.priority;
}

protected applyTag(resource: ITaggable): void {
Expand Down
13 changes: 7 additions & 6 deletions packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,14 @@ export class Resource extends Referenceable {
*/
public toCloudFormation(): object {
try {
if (Resource.isTaggable(this)) {
const tags = this.tags.renderTags();
this.properties.tags = tags === undefined ? this.properties.tags : tags;
}
// merge property overrides onto properties and then render (and validate).
const properties = this.renderProperties(deepMerge(this.properties || { }, this.untypedPropertyOverrides));
const tags = Resource.isTaggable(this) ?
this.tags.renderTags() : undefined;
const properties = this.renderProperties(
deepMerge(
deepMerge(this.properties || { }, {tags}),
this.untypedPropertyOverrides
));

return {
Resources: {
Expand Down Expand Up @@ -254,7 +256,6 @@ export class Resource extends Referenceable {
protected renderProperties(properties: any): { [key: string]: any } {
return properties;
}

}

export enum TagType {
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/cdk/lib/cloudformation/tag.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { Token } from '../core/tokens/token';
/**
* @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html
*/
export interface CfnTag {
/**
* @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html#cfn-resource-tags-key
*/
key: string;
key: string | Token;
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 did this here because we talked about it in the PR. However, the code generation for ASG does not add tokens here any longer. Do we still want this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we don't.


/**
* @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html#cfn-resource-tags-value
*/
value: string;
value: string | Token;
}
205 changes: 147 additions & 58 deletions packages/@aws-cdk/cdk/lib/core/tag-manager.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import { TagType } from '../cloudformation/resource';

/**
* Properties Tags is a dictionary of tags as strings
*/
type Tags = { [key: string]: {value: string, props: TagProps }};
import { CfnTag } from '../cloudformation/tag';

/**
* Properties for a tag
Expand Down Expand Up @@ -39,21 +35,144 @@ export interface TagProps {
* Setting priority will enable the user to control tags when they need to not
* follow the default precedence pattern of last applied and closest to the
* construct in the tree.
* @default 0 for Tag 1 for RemoveTag
* Defaults are set in Tag and RemoveTag for priority.
* @see Tag
* @see RemoveTag
* @default 0 when this class is used without aspects mentioned above.
*/
priority?: number;
}

interface Tag {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
key: string;
value: string;
props: TagProps;
}

interface CfnAsgTag {
key: string;
value: string;
propagateAtLaunch: boolean;
}

interface ITagFormatter {
formatTags(tags: Tag[]): any;
parseTags(cfnPropertyTags: any): Tag[];
}

class StandardFormatter implements ITagFormatter {
public parseTags(cfnPropertyTags: any): Tag[] {
const tags: Tag[] = [];
if (Array.isArray(cfnPropertyTags)) {
for (const tag of cfnPropertyTags) {
if (tag.key === undefined || tag.value === undefined) {
throw new Error(`Invalid tag input expected {key, value} have ${JSON.stringify(tag)}`);
}
// using interp to ensure Token is now string
tags.push({key: `${tag.key}`, value: `${tag.value}`, props: {}});
}
return tags;
}
throw new Error(`Invalid tag input expected array of {key, value} have ${JSON.stringify(cfnPropertyTags)}`);
}

public formatTags(tags: Tag[]): any {
const cfnTags: CfnTag[] = [];
for (const tag of tags) {
cfnTags.push({key: tag.key, value: tag.value});
}
return cfnTags.length === 0 ? undefined : cfnTags;
}
}

class AsgFormatter implements ITagFormatter {
public parseTags(cfnPropertyTags: any): Tag[] {
const tags: Tag[] = [];
if (Array.isArray(cfnPropertyTags)) {
for (const tag of cfnPropertyTags) {
if (tag.key === undefined ||
tag.value === undefined ||
tag.propagateAtLaunch === undefined) {
throw new Error(`Invalid tag input expected {key, value, propagateAtLaunch} have ${JSON.stringify(tag)}`);
}
// using interp to ensure Token is now string
tags.push({key: `${tag.key}`, value: `${tag.value}`, props: {
applyToLaunchedInstances: tag.propagateAtLaunch}});
}
return tags;
}
throw new Error(`Invalid tag input expected array of {key, value, propagateAtLaunch} have ${JSON.stringify(cfnPropertyTags)}`);
}
public formatTags(tags: Tag[]): any {
const cfnTags: CfnAsgTag[] = [];
for (const tag of tags) {
cfnTags.push({key: tag.key,
value: tag.value,
propagateAtLaunch: tag.props.applyToLaunchedInstances !== false});
}
return cfnTags.length === 0 ? undefined : cfnTags;
}
}

class MapFormatter implements ITagFormatter {
public parseTags(cfnPropertyTags: any): Tag[] {
const tags: Tag[] = [];
if (Array.isArray(cfnPropertyTags) || typeof(cfnPropertyTags) !== 'object') {
throw new Error(`Invalid tag input expected map of {key: value} have ${JSON.stringify(cfnPropertyTags)}`);
}

for (const key of Object.keys(cfnPropertyTags)) {
tags.push({key, value: cfnPropertyTags[key], props: {}});
}
return tags;
}

public formatTags(tags: Tag[]): any {
const cfnTags: {[key: string]: string} = {};
for (const tag of tags) {
cfnTags[`${tag.key}`] = `${tag.value}`;
}
return Object.keys(cfnTags).length === 0 ? undefined : cfnTags;
}
}

class NoFormat implements ITagFormatter {
public parseTags(_cfnPropertyTags: any): Tag[] {
return [];
}
public formatTags(_tags: Tag[]): any {
return undefined;
}
}

const TAG_FORMATTERS: {[key: string]: ITagFormatter} = {
[TagType.AutoScalingGroup]: new AsgFormatter(),
[TagType.Standard]: new StandardFormatter(),
[TagType.Map]: new MapFormatter(),
[TagType.NotTaggable]: new NoFormat(),
};
/**
* TagManager facilitates a common implementation of tagging for Constructs.
*/
export class TagManager {

private readonly tags: Tags = {};

private readonly removedTags: {[key: string]: number} = {};
private readonly tags = new Map<string, {value: string, props: TagProps}>();
private readonly removedTags = new Map<string, number>();
private readonly priorities = new Map<string, number>();
private readonly tagFormatter: ITagFormatter;
private readonly resourceTypeName: string;
private readonly initialTagPriority = 50;

constructor(private readonly tagType: TagType, private readonly resourceTypeName: string) { }
constructor(tagType: TagType, resourceTypeName: string, tags?: any) {
this.resourceTypeName = resourceTypeName;
this.tagFormatter = TAG_FORMATTERS[tagType];
if (tags !== undefined) {
const initialTags = this.tagFormatter.parseTags(tags);
for (const tag of initialTags) {
this.setTag(tag.key, tag.value, {priority: this.initialTagPriority});
}
}
}

/**
* Adds the specified tag to the array of tags
Expand All @@ -64,15 +183,17 @@ export class TagManager {
*/
public setTag(key: string, value: string, props?: TagProps): void {
const tagProps: TagProps = props || {};
tagProps.applyToLaunchedInstances = tagProps.applyToLaunchedInstances !== false;
tagProps.priority = tagProps.priority === undefined ? 0 : tagProps.priority;

if (!this.canApplyTag(key, tagProps)) {
// tag is blocked by a remove
// can't apply tag so return doing nothing
return;
}
tagProps.applyToLaunchedInstances = tagProps.applyToLaunchedInstances !== false;
this.tags[key] = { value, props: tagProps };
this.tags.set(key, {value, props: tagProps});
this.priorities.set(key, tagProps.priority);
// ensure nothing is left in removeTags
delete this.removedTags[key];
this.removedTags.delete(key);
}

/**
Expand All @@ -82,56 +203,32 @@ export class TagManager {
*/
public removeTag(key: string, props?: TagProps): void {
const tagProps = props || {};
const priority = tagProps.priority === undefined ? 0 : tagProps.priority;
tagProps.priority = tagProps.priority === undefined ? 0 : tagProps.priority;
if (!this.canApplyTag(key, tagProps)) {
// tag is blocked by a remove
// can't apply tag so return doing nothing
return;
}
delete this.tags[key];
this.removedTags[key] = priority;
this.tags.delete(key);
this.priorities.set(key, tagProps.priority);
this.removedTags.set(key, tagProps.priority);
}

/**
* Renders tags into the proper format based on TagType
*/
public renderTags(): any {
const keys = Object.keys(this.tags);
switch (this.tagType) {
case TagType.Standard: {
const tags: Array<{key: string, value: string}> = [];
for (const key of keys) {
tags.push({key, value: this.tags[key].value});
}
return tags.length === 0 ? undefined : tags;
}
case TagType.AutoScalingGroup: {
const tags: Array<{key: string, value: string, propagateAtLaunch: boolean}> = [];
for (const key of keys) {
tags.push({
key,
value: this.tags[key].value,
propagateAtLaunch: this.tags[key].props.applyToLaunchedInstances !== false}
);
}
return tags.length === 0 ? undefined : tags;
}
case TagType.Map: {
const tags: {[key: string]: string} = {};
for (const key of keys) {
tags[key] = this.tags[key].value;
}
return Object.keys(tags).length === 0 ? undefined : tags;
}
case TagType.NotTaggable: {
return undefined;
}
}
const tags: Tag[] = [];
this.tags.forEach( (tag, key) => {
tags.push({key, value: tag.value, props: tag.props});
});
return this.tagFormatter.formatTags(tags);
}

private canApplyTag(key: string, props: TagProps): boolean {
const include = props.includeResourceTypes || [];
const exclude = props.excludeResourceTypes || [];
const priority = props.priority === undefined ? 0 : props.priority;

if (exclude.length !== 0 &&
exclude.indexOf(this.resourceTypeName) !== -1) {
return false;
Expand All @@ -140,14 +237,6 @@ export class TagManager {
include.indexOf(this.resourceTypeName) === -1) {
return false;
}
if (this.tags[key]) {
if (this.tags[key].props.priority !== undefined) {
return priority >= this.tags[key].props.priority!;
}
}
if (this.removedTags[key]) {
return priority >= this.removedTags[key];
}
return true;
return priority >= (this.priorities.get(key) || 0);
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 this simplification enough? or did you want me to look at removing the include/exclude checks above as well.

}
}
Loading