fix(core): tags not working for cognito user pools (aws#4225)

* fix(core): Support tags for Cognito User Pools * moved all knowledge about tag names into the schema package and included UserPoolTags as taggable name * refactored codegen to use new schema package to identify tag properties BREAKING CHANGE: * TagManager constructor now takes a property object instead of individual agruments: new TagManager(props: TagManagerProps) instead of new cdk.TagManager(cdk.TagType.STANDARD, resourceType, initialTags); Fixes aws#3882 * moving back to non-breaking change and simplifying the interface/type checking * fixing logic gap in tag type lookup * Update tag-manager.ts * Update tag-manager.ts * cleaning up final comments * refactor(core): create asCfnProperty on TagManager to support tag property names besides tags * Update packages/@aws-cdk/core/lib/tag-manager.ts Co-Authored-By: Elad Ben-Israel <benisrae@amazon.com> * Update packages/@aws-cdk/core/lib/tag-manager.ts Co-Authored-By: Elad Ben-Israel <benisrae@amazon.com> * refactor(core): remove asCfnProperty from tag-manager and put the logic in cfn-resource * Update cfn-resource.ts * refactor(core): clean up cfnProperties to ensure undefined returns {} Co-authored-by: Elad Ben-Israel <benisrae@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Vboivin · Jan 3, 2020 · a67f0ef · a67f0ef
1 parent 2cc10e0
commit a67f0ef
Show file tree

Hide file tree

Showing 8 changed files with 113 additions and 23 deletions.
diff --git a/packages/@aws-cdk/aws-cognito/test/test.user-pool.ts b/packages/@aws-cdk/aws-cognito/test/test.user-pool.ts
@@ -21,6 +21,26 @@ export = {
 
     test.done();
   },
+  'support tags'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    // WHEN
+    const pool = new cognito.UserPool(stack, 'Pool', {
+      userPoolName: 'myPool',
+    });
+    cdk.Tag.add(pool, "PoolTag", "PoolParty");
+
+    // THEN
+    expect(stack).to(haveResourceLike('AWS::Cognito::UserPool', {
+      UserPoolName: 'myPool',
+      UserPoolTags: {
+        PoolTag: "PoolParty",
+      }
+    }));
+
+    test.done();
+  },
 
   'lambda triggers are defined'(test: Test) {
     // GIVEN

diff --git a/packages/@aws-cdk/cfnspec/lib/schema/property.ts b/packages/@aws-cdk/cfnspec/lib/schema/property.ts
@@ -222,6 +222,19 @@ export function isPropertyScrutinyType(str: string): str is PropertyScrutinyType
   return (PropertyScrutinyType as any)[str] !== undefined;
 }
 
+const tagPropertyNames = {
+  Tags: "",
+  UserPoolTags: "",
+};
+
+export type TagPropertyName = keyof typeof tagPropertyNames;
+
+export function isTagPropertyName(name?: string): name is TagPropertyName {
+  if (undefined === name) {
+    return false;
+  }
+  return tagPropertyNames.hasOwnProperty(name);
+}
 /**
  * This function validates that the property **can** be a Tag Property
  *

diff --git a/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts b/packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts
@@ -1,5 +1,5 @@
 import { Documented, PrimitiveType } from './base-types';
-import { isTagProperty, Property, TagProperty } from './property';
+import { isTagProperty, isTagPropertyName, Property, TagProperty } from './property';
 
 export interface ResourceType extends Documented {
   /**
@@ -31,6 +31,7 @@ export interface ResourceType extends Documented {
 export interface TaggableResource extends ResourceType {
   Properties: {
     Tags: TagProperty;
+    UserPoolTags: TagProperty;
     [name: string]: Property;
   }
 }
@@ -61,8 +62,13 @@ export interface ComplexListAttribute {
  * generation of properties will be used.
  */
 export function isTaggableResource(spec: ResourceType): spec is TaggableResource {
-  if (spec.Properties && spec.Properties.Tags) {
-    return isTagProperty(spec.Properties.Tags);
+  if (spec.Properties === undefined) {
+    return false;
+  }
+  for (const key of Object.keys(spec.Properties)) {
+    if (isTagPropertyName(key) && isTagProperty(spec.Properties[key])) {
+      return true;
+    }
   }
   return false;
 }

diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts
@@ -318,8 +318,13 @@ export class CfnResource extends CfnRefElement {
   }
 
   protected get cfnProperties(): { [key: string]: any } {
-    const tags = TagManager.isTaggable(this) ? this.tags.renderTags() : {};
-    return deepMerge(this._cfnProperties || {}, {tags});
+    const props = this._cfnProperties || {};
+    if (TagManager.isTaggable(this)) {
+      const tagsProp: { [key: string]: any } = {};
+      tagsProp[this.tags.tagPropertyName] = this.tags.renderTags();
+      return deepMerge(props, tagsProp);
+    }
+    return props;
   }
 
   protected renderProperties(props: {[key: string]: any}): { [key: string]: any } {

diff --git a/packages/@aws-cdk/core/lib/tag-manager.ts b/packages/@aws-cdk/core/lib/tag-manager.ts
@@ -203,6 +203,20 @@ export interface ITaggable {
   readonly tags: TagManager;
 }
 
+/**
+ * Options to configure TagManager behavior
+ */
+export interface TagManagerOptions {
+  /**
+   * The name of the property in CloudFormation for these tags
+   *
+   * Normally this is `tags`, but Cognito UserPool uses UserPoolTags
+   *
+   * @default "tags"
+   */
+  readonly tagPropertyName?: string;
+}
+
 /**
  * TagManager facilitates a common implementation of tagging for Constructs.
  */
@@ -215,18 +229,27 @@ export class TagManager {
     return (construct as any).tags !== undefined;
   }
 
+  /**
+   * The property name for tag values
+   *
+   * Normally this is `tags` but some resources choose a different name. Cognito
+   * UserPool uses UserPoolTags
+   */
+  public readonly tagPropertyName: string;
+
   private readonly tags = new Map<string, Tag>();
   private readonly priorities = new Map<string, number>();
   private readonly tagFormatter: ITagFormatter;
   private readonly resourceTypeName: string;
   private readonly initialTagPriority = 50;
 
-  constructor(tagType: TagType, resourceTypeName: string, tagStructure?: any) {
+  constructor(tagType: TagType, resourceTypeName: string, tagStructure?: any, options: TagManagerOptions = { }) {
     this.resourceTypeName = resourceTypeName;
     this.tagFormatter = TAG_FORMATTERS[tagType];
     if (tagStructure !== undefined) {
       this._setTag(...this.tagFormatter.parseTags(tagStructure, this.initialTagPriority));
     }
+    this.tagPropertyName = options.tagPropertyName || 'tags';
   }
 
   /**
@@ -259,6 +282,12 @@ export class TagManager {
     return this.tagFormatter.formatTags(Array.from(this.tags.values()));
   }
 
+  /**
+   * Determine if the aspect applies here
+   *
+   * Looks at the include and exclude resourceTypeName arrays to determine if
+   * the aspect applies here
+   */
   public applyTagAspectHere(include?: string[], exclude?: string[]) {
     if (exclude && exclude.length > 0 && exclude.indexOf(this.resourceTypeName) !== -1) {
       return false;

diff --git a/packages/@aws-cdk/core/test/test.tag-manager.ts b/packages/@aws-cdk/core/test/test.tag-manager.ts
@@ -3,6 +3,13 @@ import { TagType } from '../lib/cfn-resource';
 import { TagManager } from '../lib/tag-manager';
 
 export = {
+  'TagManagerOptions can set tagPropertyName'(test: Test) {
+    const tagPropName = 'specialName';
+    const mgr = new TagManager(TagType.MAP, 'Foo', undefined, { tagPropertyName: tagPropName });
+
+    test.deepEqual(mgr.tagPropertyName, tagPropName);
+    test.done();
+  },
   '#setTag() supports setting a tag regardless of Type'(test: Test) {
     const notTaggable = new TagManager(TagType.NOT_TAGGABLE, 'AWS::Resource::Type');
     notTaggable.setTag('key', 'value');

diff --git a/tools/cfn2ts/lib/codegen.ts b/tools/cfn2ts/lib/codegen.ts
@@ -4,6 +4,7 @@ import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as genspec from './genspec';
 import { itemTypeNames, PropertyAttributeName, scalarTypeNames, SpecName } from './spec-utils';
+import { upcaseFirst } from './util';
 
 const CORE = genspec.CORE_NAMESPACE;
 const RESOURCE_BASE_CLASS = `${CORE}.CfnResource`; // base class for all resources
@@ -296,8 +297,8 @@ export default class CodeGenerator {
     if (propsType && propMap) {
       this.code.line();
       for (const prop of Object.values(propMap)) {
-        if (prop === 'tags' && isTaggable(spec)) {
-          this.code.line(`this.tags = new ${TAG_MANAGER}(${tagType(spec)}, ${cfnResourceTypeName}, props.tags);`);
+        if (schema.isTagPropertyName(upcaseFirst(prop)) && schema.isTaggableResource(spec)) {
+          this.code.line(`this.tags = new ${TAG_MANAGER}(${tagType(spec)}, ${cfnResourceTypeName}, props.${prop}, { tagPropertyName: '${prop}' });`);
         } else {
           this.code.line(`this.${prop} = props.${prop};`);
         }
@@ -311,7 +312,7 @@ export default class CodeGenerator {
     // setup render properties
     if (propsType && propMap) {
       this.code.line();
-      this.emitCloudFormationProperties(propsType, propMap, isTaggable(spec));
+      this.emitCloudFormationProperties(propsType, propMap, schema.isTaggableResource(spec));
     }
 
     this.closeClass(resourceName);
@@ -329,7 +330,7 @@ export default class CodeGenerator {
     this.code.indent('return {');
     for (const prop of Object.values(propMap)) {
       // handle tag rendering because of special cases
-      if (prop === 'tags' && taggable) {
+      if (taggable && schema.isTagPropertyName(upcaseFirst(prop))) {
         this.code.line(`${prop}: this.tags.renderTags(),`);
         continue;
       }
@@ -553,7 +554,7 @@ export default class CodeGenerator {
     this.docLink(props.spec.Documentation, props.additionalDocs);
     const question = props.spec.Required ? ';' : ' | undefined;';
     const line = `: ${this.findNativeType(props.context, props.spec, props.propName)}${question}`;
-    if (props.propName === 'Tags' && schema.isTagProperty(props.spec)) {
+    if (schema.isTagPropertyName(props.propName) && schema.isTagProperty(props.spec)) {
       this.code.line(`public readonly tags: ${TAG_MANAGER};`);
     } else {
       this.code.line(`public ${javascriptPropertyName}${line}`);
@@ -638,7 +639,7 @@ export default class CodeGenerator {
       // 'tokenizableType' operates at the level of rendered type names in TypeScript, so stringify
       // the objects.
       const renderedTypes = itemTypes.map(t => this.renderCodeName(resourceContext, t));
-      if (!tokenizableType(renderedTypes) && propName !== 'Tags') {
+      if (!tokenizableType(renderedTypes) && !schema.isTagPropertyName(propName)) {
         // Always accept a token in place of any list element (unless the list elements are tokenizable)
         itemTypes.push(genspec.TOKEN_NAME);
       }
@@ -670,7 +671,7 @@ export default class CodeGenerator {
     // everything to be tokenizable because there are languages that do not
     // support union types (i.e. Java, .NET), so we lose type safety if we have
     // a union.
-    if (!tokenizableType(alternatives) && propName !== 'Tags') {
+    if (!tokenizableType(alternatives) && !schema.isTagPropertyName(propName)) {
       alternatives.push(genspec.TOKEN_NAME.fqn);
     }
     return alternatives.join(' | ');
@@ -737,26 +738,25 @@ function tokenizableType(alternatives: string[]): boolean {
   return false;
 }
 
-function tagType(resource: schema.ResourceType): string {
-  if (schema.isTaggableResource(resource)) {
-    const prop = resource.Properties.Tags;
-    if (schema.isTagPropertyStandard(prop)) {
+function tagType(resource: schema.TaggableResource): string {
+  for (const name of Object.keys(resource.Properties)) {
+    if (!schema.isTagPropertyName(name)) {
+      continue;
+    }
+    if (schema.isTagPropertyStandard(resource.Properties[name])) {
       return `${TAG_TYPE}.STANDARD`;
     }
-    if (schema.isTagPropertyAutoScalingGroup(prop)) {
+    if (schema.isTagPropertyAutoScalingGroup(resource.Properties[name])) {
       return `${TAG_TYPE}.AUTOSCALING_GROUP`;
     }
-    if (schema.isTagPropertyJson(prop) || schema.isTagPropertyStringMap(prop)) {
+    if (schema.isTagPropertyJson(resource.Properties[name]) ||
+      schema.isTagPropertyStringMap(resource.Properties[name])) {
       return `${TAG_TYPE}.MAP`;
     }
   }
   return `${TAG_TYPE}.NOT_TAGGABLE`;
 }
 
-function isTaggable(resource: schema.ResourceType): boolean {
-  return tagType(resource) !== `${TAG_TYPE}.NOT_TAGGABLE`;
-}
-
 enum Container {
   Interface = 'INTERFACE',
   Class = 'CLASS',

diff --git a/tools/cfn2ts/lib/util.ts b/tools/cfn2ts/lib/util.ts
@@ -8,6 +8,16 @@ export function downcaseFirst(str: string): string {
   return `${str[0].toLocaleLowerCase()}${str.slice(1)}`;
 }
 
+/**
+ * Upcase the first character in a string.
+ *
+ * @param str the string to be processed.
+ */
+export function upcaseFirst(str: string): string {
+  if (str === '') { return str; }
+  return `${str[0].toLocaleUpperCase()}${str.slice(1)}`;
+}
+
 /**
  * Join two strings with a separator if they're both present, otherwise return the present one
  */