fix(cdk): merge cloudFormation tags with aspect tags

This modifies the behavior of TagManager to enable the merging of tags provided during Cfn* properties with tag aspects. If a collision occurs the aspects tag precedence. fixes aws#1725
moofish32 · Feb 14, 2019 · 9a3fade · 9a3fade
1 parent 726dacd
commit 9a3fade
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 13 deletions.
diff --git a/packages/@aws-cdk/cdk/README.md b/packages/@aws-cdk/cdk/README.md
@@ -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
+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

diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts b/packages/@aws-cdk/cdk/lib/cloudformation/resource.ts
@@ -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) ?
+        {tags: this.tags.renderTags(this.properties.tags)} : {tags: undefined};
+      const properties = this.renderProperties(
+        deepMerge(
+          deepMerge(this.properties || { }, tags || {}),
+          this.untypedPropertyOverrides
+        ));
 
       return {
         Resources: {
@@ -254,14 +256,13 @@ export class Resource extends Referenceable {
   protected renderProperties(properties: any): { [key: string]: any } {
     return properties;
   }
-
 }
 
 export enum TagType {
   Standard = 'StandardTag',
-  AutoScalingGroup = 'AutoScalingGroupTag',
-  Map = 'StringToStringMap',
-  NotTaggable = 'NotTaggable',
+    AutoScalingGroup = 'AutoScalingGroupTag',
+    Map = 'StringToStringMap',
+    NotTaggable = 'NotTaggable',
 }
 
 export interface ResourceOptions {

diff --git a/packages/@aws-cdk/cdk/lib/core/tag-manager.ts b/packages/@aws-cdk/cdk/lib/core/tag-manager.ts
@@ -94,7 +94,8 @@ export class TagManager {
   /**
    * Renders tags into the proper format based on TagType
    */
-  public renderTags(): any {
+  public renderTags(propertyTags?: any): any {
+    this.mergeFrom(propertyTags);
     const keys = Object.keys(this.tags);
     switch (this.tagType) {
       case TagType.Standard: {
@@ -150,4 +151,45 @@ export class TagManager {
     }
     return true;
   }
+
+  private mergeFrom(propertyTags: any): void {
+    if (propertyTags === undefined) {
+      return;
+    }
+    const keys = Object.keys(this.tags);
+    switch (this.tagType) {
+      case TagType.Standard: {
+        if (Array.isArray(propertyTags)) {
+          for (const tag of propertyTags) {
+            if (!keys.includes(tag.key)) {
+              this.setTag(tag.key, tag.value);
+            }
+          }
+        } else {
+          throw new Error(`${this.resourceTypeName} expects a tags array of key value pairs, received [ ${JSON.stringify(propertyTags)} ]`);
+        }
+        break;
+      }
+      case TagType.AutoScalingGroup: {
+        if (Array.isArray(propertyTags)) {
+          for (const tag of propertyTags) {
+            if (!keys.includes(tag.key)) {
+              this.setTag(tag.key, tag.value, {applyToLaunchedInstances: tag.propagateAtLaunch});
+            }
+          }
+        } else {
+          throw new Error(`${this.resourceTypeName} expects a tags array of key value pairs, received [ ${JSON.stringify(propertyTags)} ]`);
+        }
+        break;
+      }
+      case TagType.Map: {
+        for (const key of Object.keys(propertyTags)) {
+          if (!keys.includes(key)) {
+            this.setTag(key, propertyTags[key]);
+          }
+        }
+        break;
+      }
+    }
+  }
 }
diff --git a/packages/@aws-cdk/cdk/test/aspects/test.tag-aspect.ts b/packages/@aws-cdk/cdk/test/aspects/test.tag-aspect.ts
@@ -130,7 +130,7 @@ 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) {
     const root = new Stack();
     const aspectBranch = new TaggableResource(root, 'FakeBranchA', {
       type: 'AWS::Fake::Thing',
@@ -150,7 +150,7 @@ export = {
     });
     aspectBranch.node.apply(new Tag('aspects', 'rule'));
     root.node.prepareTree();
-    test.deepEqual(aspectBranch.tags.renderTags(), [{key: 'aspects', value: 'rule'}]);
+    test.deepEqual(aspectBranch.tags.renderTags(), [{key: 'aspects', value: 'rule'}, {key: 'cfn', value: 'is cool'}]);
     test.deepEqual(cfnBranch.testProperties().tags, [{key: 'cfn', value: 'is cool'}]);
     test.done();
   },