chore(core): ITaggableV2 is safe to implement for L1s (#25610)

`ITaggable` defines a property: `tags: TagManager`.

This definition conflicts with many L1 resources: they have `<cfnProperty>: <PropType>` properties on them, and if a property is named `tags` they conflict.

The old behavior is to have `tags: TagManager` for a select set of resources we recognize, and `tags: CfnResource.TagProperty[]` for all other resources.

There is occlusion whichever way we choose these properties.

Introduce a new interface, `ITaggable2 { tagManager: TagManager }`. I'm going to state there is *very little chance* of upstream properties being called `TagManager`, and so we can have both the `tags: CnfResource.TagProperty[]` property as well as the `tagManager: TagManager` property.

The plan is to generate one of the following L1 patterns:

```ts
// Old
class CfnSomething implements ITaggable {
  readonly tags: TagManager;  // Backwards compatible
  tagsRaw: CfnSomething.TagProperty[]; // Also allow direct access
}

// New
class CfnSomething implements ITaggable2 {
  readonly tags: CfnSomething.TagProperty[];  // Backwards compatible
  readonly cdkTagManager: TagManager;  // Allow access to the TagManager
}
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: rix0rrr

packages/aws-cdk-lib/core/lib/cfn-resource.ts

@@ -482,9 +482,12 @@ export class CfnResource extends CfnRefElement {
 
   protected get cfnProperties(): { [key: string]: any } {
     const props = this._cfnProperties || {};
-    if (TagManager.isTaggable(this)) {
+    const tagMgr = TagManager.of(this);
+    if (tagMgr) {
       const tagsProp: { [key: string]: any } = {};
-      tagsProp[this.tags.tagPropertyName] = this.tags.renderTags();
+      // If this object has a TagManager, then render it out into the correct field. We assume there
+      // is no shadow tags object, so we don't pass anything to renderTags().
+      tagsProp[tagMgr.tagPropertyName] = tagMgr.renderTags();
       return deepMerge(props, tagsProp);
     }
     return props;

packages/aws-cdk-lib/core/lib/tag-aspect.ts

@@ -1,7 +1,7 @@
 import { Construct, IConstruct } from 'constructs';
 import { Annotations } from './annotations';
 import { IAspect, Aspects } from './aspect';
-import { ITaggable, TagManager } from './tag-manager';
+import { ITaggable, ITaggableV2, TagManager } from './tag-manager';
 
 /**
  * Properties for a tag
@@ -72,12 +72,15 @@ abstract class TagBase implements IAspect {
   }
 
   public visit(construct: IConstruct): void {
-    if (TagManager.isTaggable(construct)) {
+    if (TagManager.isTaggableV2(construct)) {
+      this.applyTagV2(construct);
+    } else if (TagManager.isTaggable(construct)) {
       this.applyTag(construct);
     }
   }
 
   protected abstract applyTag(resource: ITaggable): void;
+  protected abstract applyTagV2(resource: ITaggableV2): void;
 }
 
 /**
@@ -121,8 +124,16 @@ export class Tag extends TagBase {
   }
 
   protected applyTag(resource: ITaggable) {
-    if (resource.tags.applyTagAspectHere(this.props.includeResourceTypes, this.props.excludeResourceTypes)) {
-      resource.tags.setTag(
+    this.applyManager(resource.tags);
+  }
+
+  protected applyTagV2(resource: ITaggableV2) {
+    this.applyManager(resource.cdkTagManager);
+  }
+
+  private applyManager(mgr: TagManager) {
+    if (mgr.applyTagAspectHere(this.props.includeResourceTypes, this.props.excludeResourceTypes)) {
+      mgr.setTag(
         this.key,
         this.value,
         this.props.priority ?? this.defaultPriority,
@@ -173,8 +184,16 @@ export class RemoveTag extends TagBase {
   }
 
   protected applyTag(resource: ITaggable): void {
-    if (resource.tags.applyTagAspectHere(this.props.includeResourceTypes, this.props.excludeResourceTypes)) {
-      resource.tags.removeTag(this.key, this.props.priority ?? this.defaultPriority);
+    this.applyManager(resource.tags);
+  }
+
+  protected applyTagV2(resource: ITaggableV2): void {
+    this.applyManager(resource.cdkTagManager);
+  }
+
+  private applyManager(mgr: TagManager) {
+    if (mgr.applyTagAspectHere(this.props.includeResourceTypes, this.props.excludeResourceTypes)) {
+      mgr.removeTag(this.key, this.props.priority ?? this.defaultPriority);
     }
   }
 }

packages/aws-cdk-lib/core/lib/tag-manager.ts

@@ -238,6 +238,23 @@ export interface ITaggable {
   readonly tags: TagManager;
 }
 
+/**
+ * Modernized version of ITaggable
+ *
+ * `ITaggable` has a problem: for a number of L1 resources, we failed to generate
+ * `tags: TagManager`, and generated `tags: CfnSomeResource.TagProperty[]` instead.
+ *
+ * To mark these resources as taggable, we need to put the `TagManager` in a new property
+ * whose name is unlikely to conflict with any existing properties. Hence, a new interface
+ * for that purpose. All future resources will implement `ITaggableV2`.
+ */
+export interface ITaggableV2 {
+  /**
+   * TagManager to set, remove and format tags
+   */
+  readonly cdkTagManager: TagManager;
+}
+
 /**
  * Options to configure TagManager behavior
  */
@@ -283,7 +300,22 @@ export class TagManager {
    * Check whether the given construct is Taggable
    */
   public static isTaggable(construct: any): construct is ITaggable {
-    return (construct as any).tags !== undefined;
+    const tags = (construct as any).tags;
+    return tags && typeof tags === 'object' && tags.constructor.name === 'TagManager';
+  }
+
+  /**
+   * Check whether the given construct is ITaggableV2
+   */
+  public static isTaggableV2(construct: any): construct is ITaggableV2 {
+    return (construct as any).cdkTagManager !== undefined;
+  }
+
+  /**
+   * Return the TagManager associated with the given construct, if any
+   */
+  public static of(construct: any): TagManager | undefined {
+    return TagManager.isTaggableV2(construct) ? construct.cdkTagManager : TagManager.isTaggable(construct) ? construct.tags : undefined;
   }
 
   /**
@@ -303,21 +335,19 @@ export class TagManager {
   public readonly renderedTags: IResolvable;
 
   private readonly tags = new Map<string, Tag>();
-  private readonly dynamicTags: any;
+  private dynamicTags?: any;
   private readonly priorities = new Map<string, number>();
   private readonly tagFormatter: ITagFormatter;
   private readonly resourceTypeName: string;
-  private readonly initialTagPriority = 50;
+  private readonly externalTagPriority = 50;
+  private readonly didHaveInitialTags: boolean;
 
-  constructor(tagType: TagType, resourceTypeName: string, tagStructure?: any, options: TagManagerOptions = { }) {
+  constructor(tagType: TagType, resourceTypeName: string, initialTags?: any, options: TagManagerOptions = { }) {
     this.resourceTypeName = resourceTypeName;
     this.tagFormatter = TAG_FORMATTERS()[tagType];
-    if (tagStructure !== undefined) {
-      const parseTagsResult = this.tagFormatter.parseTags(tagStructure, this.initialTagPriority);
-      this.dynamicTags = parseTagsResult.dynamicTags;
-      this._setTag(...parseTagsResult.tags);
-    }
     this.tagPropertyName = options.tagPropertyName || 'tags';
+    this.parseExternalTags(initialTags);
+    this.didHaveInitialTags = initialTags !== undefined;
 
     this.renderedTags = Lazy.any({ produce: () => this.renderTags() });
   }
@@ -353,8 +383,13 @@ export class TagManager {
    * which will return a `Lazy` value that will resolve to the correct
    * tags at synthesis time.
    */
-  public renderTags(): any {
+  public renderTags(combineWithTags?: any): any {
+    if (combineWithTags !== undefined && this.didHaveInitialTags) {
+      throw new Error('Specify external tags either during the creation of TagManager, or as a parameter to renderTags(), but not both');
+    }
+    this.parseExternalTags(combineWithTags);
     const formattedTags = this.tagFormatter.formatTags(this.sortedTags);
+
     if (Array.isArray(formattedTags) || Array.isArray(this.dynamicTags)) {
       const ret = [...formattedTags ?? [], ...this.dynamicTags ?? []];
       return ret.length > 0 ? ret : undefined;
@@ -412,4 +447,17 @@ export class TagManager {
     return Array.from(this.tags.values())
       .sort((a, b) => a.key.localeCompare(b.key));
   }
+
+  /**
+   * Parse external tags.
+   *
+   * Set the parseable ones into this tag manager. Save the rest (tokens, lazies) in `this.dynamicTags`.
+   */
+  private parseExternalTags(initialTags: any) {
+    if (initialTags !== undefined) {
+      const parseTagsResult = this.tagFormatter.parseTags(initialTags, this.externalTagPriority);
+      this.dynamicTags = parseTagsResult.dynamicTags;
+      this._setTag(...parseTagsResult.tags);
+    }
+  }
 }

packages/aws-cdk-lib/core/test/tag-aspect.test.ts

@@ -1,8 +1,9 @@
 import { Construct } from 'constructs';
-import { CfnResource, CfnResourceProps, RemoveTag, Stack, Tag, TagManager, TagType, Aspects, Tags } from '../lib';
+import { toCloudFormation } from './util';
+import { CfnResource, CfnResourceProps, RemoveTag, Stack, Tag, TagManager, TagType, Aspects, Tags, ITaggable, ITaggableV2 } from '../lib';
 import { synthesize } from '../lib/private/synthesis';
 
-class TaggableResource extends CfnResource {
+class TaggableResource extends CfnResource implements ITaggable {
   public readonly tags: TagManager;
   constructor(scope: Construct, id: string, props: CfnResourceProps) {
     super(scope, id, props);
@@ -14,7 +15,19 @@ class TaggableResource extends CfnResource {
   }
 }
 
-class AsgTaggableResource extends CfnResource {
+class TaggableResource2 extends CfnResource implements ITaggableV2 {
+  public readonly cdkTagManager: TagManager;
+  constructor(scope: Construct, id: string, props: CfnResourceProps) {
+    super(scope, id, props);
+    const tags = props.properties?.tags;
+    this.cdkTagManager = new TagManager(TagType.STANDARD, 'AWS::Fake::Resource', tags);
+  }
+  public testProperties() {
+    return this.cfnProperties;
+  }
+}
+
+class AsgTaggableResource extends CfnResource implements ITaggable {
   public readonly tags: TagManager;
   constructor(scope: Construct, id: string, props: CfnResourceProps) {
     super(scope, id, props);
@@ -26,7 +39,7 @@ class AsgTaggableResource extends CfnResource {
   }
 }
 
-class MapTaggableResource extends CfnResource {
+class MapTaggableResource extends CfnResource implements ITaggable {
   public readonly tags: TagManager;
   constructor(scope: Construct, id: string, props: CfnResourceProps) {
     super(scope, id, props);
@@ -39,12 +52,14 @@ class MapTaggableResource extends CfnResource {
 }
 
 describe('tag aspect', () => {
-  test('Tag visit all children of the applied node', () => {
+  test.each([
+    ['TaggableResource', TaggableResource], ['TaggableResource2', TaggableResource2],
+  ])('Tag visit all children of the applied node, using class %s', (_, taggableClass) => {
     const root = new Stack();
-    const res = new TaggableResource(root, 'FakeResource', {
+    const res = new taggableClass(root, 'FakeResource', {
       type: 'AWS::Fake::Thing',
     });
-    const res2 = new TaggableResource(res, 'FakeResource', {
+    const res2 = new taggableClass(res, 'FakeResource', {
       type: 'AWS::Fake::Thing',
     });
     const asg = new AsgTaggableResource(res, 'AsgFakeResource', {
@@ -58,10 +73,18 @@ describe('tag aspect', () => {
 
     synthesize(root);
 
-    expect(res.tags.renderTags()).toEqual([{ key: 'foo', value: 'bar' }]);
-    expect(res2.tags.renderTags()).toEqual([{ key: 'foo', value: 'bar' }]);
+    expect(TagManager.of(res)?.renderTags()).toEqual([{ key: 'foo', value: 'bar' }]);
+    expect(TagManager.of(res2)?.renderTags()).toEqual([{ key: 'foo', value: 'bar' }]);
     expect(map.tags.renderTags()).toEqual({ foo: 'bar' });
     expect(asg.tags.renderTags()).toEqual([{ key: 'foo', value: 'bar', propagateAtLaunch: true }]);
+
+    const template = toCloudFormation(root);
+    expect(template.Resources.FakeResource).toEqual({
+      Type: 'AWS::Fake::Thing',
+      Properties: {
+        tags: [{ key: 'foo', value: 'bar' }],
+      },
+    });
   });
 
   test('The last aspect applied takes precedence', () => {

packages/aws-cdk-lib/core/test/tag-manager.test.ts

@@ -9,6 +9,34 @@ describe('tag manager', () => {
     expect(mgr.tagPropertyName).toEqual(tagPropName);
   });
 
+  test.each(['early' as const, 'late' as const])('supplying tags %s works for MAP tags', (when) => {
+    const externalTags = { someTag: 'someValue' };
+    const mgr = new TagManager(TagType.MAP, 'Foo', when === 'early' ? externalTags : undefined);
+    mgr.setTag('givenTag', 'givenValue');
+
+    expect(mgr.renderTags(when === 'late' ? externalTags : undefined)).toEqual({
+      givenTag: 'givenValue',
+      someTag: 'someValue',
+    });
+  });
+
+  test.each(['early' as const, 'late' as const])('supplying tags %s works for STANDARD tags', (when) => {
+    const externalTags = [{ key: 'someTag', value: 'someValue' }];
+    const mgr = new TagManager(TagType.STANDARD, 'Foo', when === 'early' ? externalTags : undefined);
+    mgr.setTag('givenTag', 'givenValue');
+
+    expect(mgr.renderTags(when === 'late' ? externalTags : undefined)).toEqual([
+      {
+        key: 'givenTag',
+        value: 'givenValue',
+      },
+      {
+        key: 'someTag',
+        value: 'someValue',
+      },
+    ]);
+  });
+
   test('#setTag() supports setting a tag regardless of Type', () => {
     const notTaggable = new TagManager(TagType.NOT_TAGGABLE, 'AWS::Resource::Type');
     notTaggable.setTag('key', 'value');
@@ -129,6 +157,61 @@ describe('tag manager', () => {
     ]);
   });
 
+  test('can add direct tags: STANDARD', () => {
+    // GIVEN
+    const mgr = new TagManager(TagType.STANDARD, 'AWS::Resource::Type');
+
+    // WHEN
+    mgr.setTag('key', 'value');
+    const rendered = mgr.renderTags([
+      { key: 'key2', value: 'value2' },
+    ]);
+
+    // THEN
+    expect(rendered).toEqual([
+      { key: 'key', value: 'value' },
+      { key: 'key2', value: 'value2' },
+    ]);
+  });
+
+  test('can add direct tags: MAP', () => {
+    // GIVEN
+    const mgr = new TagManager(TagType.MAP, 'AWS::Resource::Type');
+
+    // WHEN
+    mgr.setTag('key', 'value');
+    const rendered = mgr.renderTags({
+      key2: 'value2',
+    });
+
+    // THEN
+    expect(rendered).toEqual({
+      key: 'value',
+      key2: 'value2',
+    });
+  });
+
+  test('may not specify external tags both at TagManager creation AND into renderTags', () => {
+    // GIVEN
+    const mgr = new TagManager(TagType.MAP, 'AWS::Resource::Type', { initial: 'tag' });
+
+    // WHEN
+    expect(() => mgr.renderTags({
+      external: 'tag',
+    })).toThrow(/not both/);
+  });
+
+  test('it is safe to call renderTags multiple times with external tags', () => {
+    // GIVEN
+    const mgr = new TagManager(TagType.STANDARD, 'AWS::Resource::Type');
+    mgr.setTag('tagOne', 'one');
+    mgr.setTag('tagTwo', 'two');
+
+    // WHEN
+    const renders = [1, 2].map(() => mgr.renderTags([{ key: 'external', value: 'tag' }]));
+    expect(renders[0]).toEqual(renders[1]);
+  });
+
   test('excludeResourceTypes only tags resources that do not match', () => {
     const mgr = new TagManager(TagType.STANDARD, 'AWS::Fake::Resource');
 

tools/@aws-cdk/cfn2ts/lib/codegen.ts

@@ -17,6 +17,8 @@ enum TreeAttributes {
   CFN_PROPS = 'aws:cdk:cloudformation:props'
 }
 
+export const LEGACY_TAGGING: Record<string, string> = {};
+
 interface Dictionary<T> { [key: string]: T; }
 
 export interface CodeGeneratorOptions {
@@ -351,6 +353,7 @@ export default class CodeGenerator {
       this.code.line();
       for (const prop of Object.values(propMap)) {
         if (schema.isTagPropertyName(upcaseFirst(prop)) && schema.isTaggableResource(spec)) {
+          LEGACY_TAGGING[cfnName] = upcaseFirst(prop);
           this.code.line(`this.tags = new ${TAG_MANAGER}(${tagType(spec)}, ${cfnResourceTypeName}, props.${prop}, { tagPropertyName: '${prop}' });`);
         } else {
           this.code.line(`this.${prop} = props.${prop};`);