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

Jump to bottom

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

Merged
merged 14 commits into from
Mar 13, 2019
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9a3fade
fix(cdk): merge cloudFormation tags with aspect tags
moofish32 Feb 13, 2019
65c0596
refactoring tag-manager to avoid JSII errors for complex dictionary t…
moofish32 Feb 18, 2019
18ce625
removing export from classes that do not need it, asg tag interface
moofish32 Feb 18, 2019
4305741
can not resolve early; note - go back and consider changing the defau…
moofish32 Feb 18, 2019
708eff3
refactor tags using Map
moofish32 Feb 21, 2019
455d697
trigger retest'
moofish32 Feb 21, 2019
15aa430
refactoring to enable tag formatters to parse L1 tags, remove merge l…
moofish32 Mar 5, 2019
15da077
Merge branch 'master' into b-child-tag-merge
moofish32 Mar 5, 2019
8eebcc9
fixing codegen.ts missing arg to constructor
moofish32 Mar 5, 2019
87eca86
Remove Tokens from cfntag.
rix0rrr Mar 13, 2019
9d8119b
'removedTags' is unused now
rix0rrr Mar 13, 2019
d0281ee
Default argument to empty object
rix0rrr Mar 13, 2019
e026e52
Refactor a bit so that the TagManager works with tags that are mostly…
rix0rrr Mar 13, 2019
5724be9
Remove the internal 'Tag' type from public exposure
rix0rrr Mar 13, 2019
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
View file
Open in desktop
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
13 changes: 7 additions & 6 deletions packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
View file
Open in desktop
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(this.properties.tags) : 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
153 changes: 110 additions & 43 deletions packages/@aws-cdk/cdk/lib/core/tag-manager.ts
View file
Open in desktop
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 @@ -44,16 +40,31 @@ export interface TagProps {
priority?: number;
}

export interface ITag {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to our naming standards the name of this interface should not start with an I since it doesn't have any behavior.

key: string;
value: string;
props: TagProps;
}

export interface ITagFormatter {
renderTags(tags: ITag[], propertyTags: any): any;
}

/**
* TagManager facilitates a common implementation of tagging for Constructs.
*/
export class TagManager {

private readonly tags: Tags = {};

private readonly tags: ITag[] = [];
private readonly tagSet: {[key: string]: number} = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably thick, but I'm going to need more information on what this member does. It seems to be mapping a tag name to an index in the the tags array, but why? Is it just for fast lookup by name?

Why do we not have something like this instead:

class TagManager {
  private readonly tags = new Map<string, ITag>();

  /**
   * Return all active tags
   */
  public get currentlyActiveTags: ITag[] {
    return Array.from(this.tags.values());
  } 
}

If the goal is to be able to switch tags off on demand (by using RemoveTag) while not completely losing track of them, another data structure in the Map<> to keep that state will do nicely:

interface AppliedTag {
  tagObject: ITag;
  active: boolean;  // true if last 'set', false if last 'remove'd.
  appliedAtPriority: number; // Priority of last change
}

class TagManager {
  private readonly tags = new Map<string, AppliedTag>();

  /**
   * Return all active tags
   */
  public get currentlyActiveTags: ITag[] {
    return Array.from(this.tags.values()).filter(t => t.active).map(t => t.tagObject);
  } 
}

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 whole confusion was me not understanding the JSII error above was telling me that I was leaking a private type. Refactored this to use Map where appropriate.

private readonly removedTags: {[key: string]: number} = {};
private readonly tagType: TagType;
private readonly resourceTypeName: string;

constructor(private readonly tagType: TagType, private readonly resourceTypeName: string) { }
constructor(tagType: TagType, resourceTypeName: string) {
this.tagType = tagType;
this.resourceTypeName = resourceTypeName;
}

/**
* Adds the specified tag to the array of tags
Expand All @@ -70,7 +81,8 @@ export class TagManager {
return;
}
tagProps.applyToLaunchedInstances = tagProps.applyToLaunchedInstances !== false;
this.tags[key] = { value, props: tagProps };
const index = this.tags.push({key, value, props: tagProps});
this.tagSet[key] = index - 1;
// ensure nothing is left in removeTags
delete this.removedTags[key];
}
Expand All @@ -87,51 +99,44 @@ export class TagManager {
// tag is blocked by a remove
return;
}
delete this.tags[key];
delete this.tagSet[key];
this.removedTags[key] = priority;
}

/**
* Renders tags into the proper format based on TagType
*/
public renderTags(): any {
const keys = Object.keys(this.tags);
public renderTags(propertyTags?: any): any {
const formatter = this.tagFormatter();
const tags: ITag[] = [];
for (const key of Object.keys(this.tagSet)) {
tags.push(this.tags[this.tagSet[key]]);
}
return formatter.renderTags(tags, propertyTags);
}

private tagFormatter(): ITagFormatter {
switch (this.tagType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

tagType is only used to pick the right type of formatter. So we could replace the whole enum meber with an instance of ITagFormatter. We could have this instead:

class TagManager {
  constructor(private readonly tagFormatter: ITagFormatter) {
  }

  public renderTags(...) {
    this.formatter.renderTags(...);
  }
}

Muy simpler!

If you want to keep the enum for the caller's convenience, do the lookup in the constructor. You can also use an object to house the lookup map:

class TagManager {
  private readonly tagFormatter: ITagFormatter;

  constructor(tagType: TagType) {
    this.tagFormatter = TAG_FORMATTERS[tagType];
  }
}

const TAG_FORMATTERS = {
  [TagType.ASG]: new AsgFormatter(),
  [TagType.Normal]: new NormalFormatter(),
  ...
}

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 preferred to leave the codegen with just the simple enum and moved to the const TAG_FORMATTERS ... pattern. If you strongly otherwise I'll move it to codegen.

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;
}
case TagType.AutoScalingGroup:
return new AsgFormatter();
break;
case TagType.Map:
return new MapFormatter();
break;
case TagType.Standard:
return new StandardFormatter();
break;
default:
return new NoFormat();
break;
}
}

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,9 +145,10 @@ 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.tagSet[key] >= 0) {
const tag = this.tags[this.tagSet[key]];
if (tag.props.priority !== undefined) {
return priority >= tag.props.priority!;
}
}
if (this.removedTags[key]) {
Expand All @@ -151,3 +157,64 @@ export class TagManager {
return true;
}
}

class StandardFormatter implements ITagFormatter {
public renderTags(tags: ITag[], propertyTags: any): any {
const cfnTags: CfnTag[] = [];
for (const tag of tags) {
cfnTags.push({key: tag.key, value: tag.value});
}
const finalTags = mergeTags(propertyTags || [], cfnTags);
return finalTags.length === 0 ? undefined : finalTags;
}
}

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

class AsgFormatter implements ITagFormatter {
public renderTags(tags: ITag[], propertyTags: any): any {
const cfnTags: AsgTag[] = [];
for (const tag of tags) {
cfnTags.push({key: tag.key,
value: tag.value,
propagateAtLaunch: tag.props.applyToLaunchedInstances !== false});
}
const finalTags = mergeTags(propertyTags || [], cfnTags);
return finalTags.length === 0 ? undefined : finalTags;
}
}

class MapFormatter implements ITagFormatter {
public renderTags(tags: ITag[], propertyTags: any): any {
const cfnTags: {[key: string]: string} = {};
for (const tag of tags) {
cfnTags[tag.key] = tag.value;
}
const finalTags = mergeTags(propertyTags || {}, cfnTags);
return Object.keys(finalTags).length === 0 ? undefined : finalTags;
}
}

class NoFormat implements ITagFormatter {
public renderTags(_tags: ITag[], _propertyTags: any): any {
return undefined;
}
}

function mergeTags(target: any, source: any): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't particularly like that this thing can handle both arrays and objects, because we lose type information this way. It should just handle arrays, and the MapFormatter should just do the object assignment directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair - I did this so I could later flip the precedence, but moved it back as you requested.

if (Array.isArray(target) && Array.isArray(source)) {
const result = [...source];
const keys = result.map( (tag) => (tag.key) );
result.push(...target.filter( (tag) => (!keys.includes(tag.key))));
return result;
}
if (typeof(target) === 'object' && typeof(source) === 'object' &&
!Array.isArray(target) && !Array.isArray(source)) {
return Object.assign(target, source);
}
throw new Error(`Invalid usage. Both source (${JSON.stringify(source)}) and target (${JSON.stringify(target)}) must both be string maps or arrays`);
}
61 changes: 59 additions & 2 deletions packages/@aws-cdk/cdk/test/aspects/test.tag-aspect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,16 +130,35 @@ export = {
test.deepEqual(res2.tags.renderTags(), [{key: 'key', value: 'value'}]);
test.done();
},
'Aspects are mutually exclusive with tags created by L1 Constructor'(test: Test) {
'Aspects are merged with tags created by L1 Constructor'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha

const root = new Stack();
const aspectBranch = new TaggableResource(root, 'FakeBranchA', {
type: 'AWS::Fake::Thing',
properties: {
tags: [
{key: 'cfn', value: 'is cool'},
{key: 'aspects', value: 'overwrite'},
],
},
});
const asgResource = new AsgTaggableResource(aspectBranch, 'FakeAsg', {
type: 'AWS::Fake::Thing',
properties: {
tags: [
{key: 'cfn', value: 'is cool', propagateAtLaunch: true},
{key: 'aspects', value: 'overwrite'},
],
},
});
const mapTaggable = new MapTaggableResource(aspectBranch, 'FakeSam', {
type: 'AWS::Fake::Thing',
properties: {
tags: {
cfn: 'is cool',
aspects: 'overwrite',
},
},
});
const cfnBranch = new TaggableResource(root, 'FakeBranchB', {
type: 'AWS::Fake::Thing',
properties: {
Expand All @@ -150,8 +169,46 @@ export = {
});
aspectBranch.node.apply(new Tag('aspects', 'rule'));
root.node.prepareTree();
test.deepEqual(aspectBranch.tags.renderTags(), [{key: 'aspects', value: 'rule'}]);
test.deepEqual(aspectBranch.testProperties().tags, [{key: 'aspects', value: 'rule'}, {key: 'cfn', value: 'is cool'}]);
test.deepEqual(asgResource.testProperties().tags, [
{key: 'aspects', value: 'rule', propagateAtLaunch: true},
{key: 'cfn', value: 'is cool', propagateAtLaunch: true}
]);
test.deepEqual(mapTaggable.testProperties().tags, {
aspects: 'rule',
cfn: 'is cool',
});
test.deepEqual(cfnBranch.testProperties().tags, [{key: 'cfn', value: 'is cool'}]);
test.done();
},
'when invalid tag properties are passed from L1s': {
'map passed instead of array it raises'(test: Test) {
const root = new Stack();
new TaggableResource(root, 'FakeBranchA', {
type: 'AWS::Fake::Thing',
properties: {
tags: {
cfn: 'is cool',
aspects: 'overwrite',
},
},
});
test.throws(() => root.node.prepareTree());
test.done();
},
'if array is passed instead of map it raises'(test: Test) {
const root = new Stack();
new MapTaggableResource(root, 'FakeSam', {
type: 'AWS::Fake::Thing',
properties: {
tags: [
{key: 'cfn', value: 'is cool', propagateAtLaunch: true},
{key: 'aspects', value: 'overwrite'},
],
},
});
test.throws(() => root.node.prepareTree());
test.done();
},
},
};
3 changes: 3 additions & 0 deletions packages/@aws-cdk/cdk/test/core/test.tag-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ export = {
mgr.setTag('key', 'newVal', {
priority: 1,
});
test.deepEqual(mgr.renderTags(), [
{key: 'key', value: 'myVal'},
]);
mgr.removeTag('key', {priority: 1});
test.deepEqual(mgr.renderTags(), [
{key: 'key', value: 'myVal'},
Expand Down