Skip to content

Commit

Permalink
chore(core): ITaggableV2 is safe to implement for L1s (#25610)
Browse files Browse the repository at this point in the history 
`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*
  • Loading branch information
@rix0rrr
rix0rrr authored May 25, 2023
1 parent 21fc1d1 commit 334008f
Show file tree
Hide file tree
Showing 7 changed files with 209 additions and 28 deletions.
7 changes: 5 additions & 2 deletions packages/aws-cdk-lib/core/lib/cfn-resource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
31 changes: 25 additions & 6 deletions packages/aws-cdk-lib/core/lib/tag-aspect.ts
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}
}
68 changes: 58 additions & 10 deletions packages/aws-cdk-lib/core/lib/tag-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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() });
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
41 changes: 32 additions & 9 deletions packages/aws-cdk-lib/core/test/tag-aspect.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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', {
Expand All @@ -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', () => {
Expand Down
83 changes: 83 additions & 0 deletions packages/aws-cdk-lib/core/test/tag-manager.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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');

Expand Down
3 changes: 3 additions & 0 deletions tools/@aws-cdk/cfn2ts/lib/codegen.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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};`);
Expand Down
Loading

0 comments on commit 334008f

Please sign in to comment.