feat(cfn2ts) Add support for Tags on L1 constructs #1007

moofish32 · 2018-10-24T17:25:20Z

If an L1 Resource has a property Tags then add support for a
TagManager

@eladb - This is an initial pass that is working for ec2 and asg. I did
not make this a generic aspect capability that others could later
leverage. I think that is possible, but didn't know what the time line
on these needs were.

Initial working codegen to support detecting L1s with Tags property
Add support to Stack and App for tagging

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

* If an L1 Resource has a property `Tags` then add support for a TagManager

rix0rrr · 2018-10-25T14:38:01Z

tools/cfn2ts/lib/codegen.ts

    const extendsPostfix = superClasses ? ` extends ${superClasses}` : '';
+    const implementPostfix = interfaces ? ` implements ${interfaces.join(', ')}` : '';


Consider an empty list. That shouldn't show implements (empty string).

In general I hate ambiguity between undefined/null and an empty list.

Agree, but to be sure are you saying this?

// ensure interfaces is a list (default param etc) const implementPostfix = interfaces.length !== 0 ? ` implements ${interfaces.join(', ')}` : '';

eladb · 2018-10-25T21:15:03Z

tools/cfn2ts/lib/codegen.ts

+    //
+    if (isTaggable(spec)) {
+      this.code.line('/**');
+      this.code.line(' * Tag Manager for resoure');


typo "resoure"

eladb · 2018-10-25T21:18:25Z

tools/cfn2ts/lib/codegen.ts

-    this.code.line(`return ${genspec.cfnMapperName(propsType).fqn}(${CORE}.resolve(properties));`);
+    this.code.line(`const resolvedProps = ${CORE}.resolve(properties);`);
+    if (taggable) {
+      this.code.line(`const rawTags: ${CORE}.Tag[] = resolvedProps.tags || [];`);


Can we somehow put this code in cdk.Resource instead of generate it?
Maybe we can create a TaggableResource construct and basically just determine if the resource extends Resource or extends TaggableResource, instead of sprinkling generated code around

Let me try, good point.

moofish32 · 2018-10-26T23:44:56Z

@eladb @rix0rrr -- I think this is cleaner and moves this to a TaggableResource, new feedback?

eladb

The general feedback is that this effectively allows using TagManager at any level of the tree, so there is no need to explicitly mark anything as taggable (besides the very low level resources). Then, if you want to tag something, you just attach a TagManager to it and it will take care of things, no?

We need to add a documentation section about tagging to aws-construct-lib.rst.

eladb · 2018-10-28T07:39:44Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+   */
+  public readonly tags: TagManager;
+
+  constructor(parent: Construct, name: string, props: ResourceProps) {


rename name to id please

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

eladb · 2018-10-28T07:40:49Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+   */
+  public readonly tags: TagManager;
+
+  constructor(parent: Construct, name: string, props: ResourceProps) {


Shouldn't props have tags in them for initial tags?

I'm assuming we discuss this in detail below.

eladb · 2018-10-28T07:43:55Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+    const rawTags: Tag[] = resolve(this.properties.tags) || [];
+    const managedTags: Tag[] = resolve(this.tags) || [];
+    const rawTagKeys: string[] = rawTags.map(t => t.key);
+    // order is important L1 (raw) tags override managed tags


Is this really the behavior we want? Perhaps we want to ensure that raw==managed in case there's a duplicate?

I went back and forth here.

The thought I had in my head was that if you created an L1 yourself you likely were 'overriding' something missing in our L2. However, I also coded the reverse, but did not consider the equality situation. I am handling the dedupe by tag key, so what do you mean in case there's duplication?

eladb · 2018-10-28T07:44:59Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+
+  constructor(parent: Construct, name: string, props: ResourceProps) {
+    super(parent, name, props);
+    // const initialTags = !!props.properties ? props.properties.tags || {} : {};


Why not? And then you don't have to do the merge in toCloudFormation?

funny thing I missed deleting this ...

So I started here, but where it blows up is the props.tags type supports just a cdk.Token so where this broke was when a developer creates a L1 and passes a function or value token to the tags.

const tagsArray: cloudformation.Tag[] = []; new cloudformation.L1(parent, 'Name', { tags: new cdk.Token(() => tagsArray)}); tagsArray.push({key: 'Key', value: someOtherL1.ref});

In that situation the initial tags are lost because TagManager doesn't support the array of tokens or just a plain token itself. I could go back and change that implementation to support it, but I'm not sure how many things are going to unravel. I think now that TagManager is a Token itself it may be do-able.

I see... well... technically, a token can also resolve to a CloudFormation intrinsic, so even resolveing it at synthesis time won't really help you:

new cloudformation.L1(parent, 'Name', { tags: new FnImportValue(...) )});

To be honest, I really don't see much value in being able to pass in a token for tags at the L1 layer. Why don't we special case this in cfn2ts so that Tags will always use concrete types (remember, textual tokens can be stringified and assigned to keys/values, so we are not prohibiting this use case). This way, you won't have that issue and you could process the initial tags in the ctor.

I see the special case for tags in cfn2ts. Are you saying we change from this today:

tags?: Array<cdk.Tag | cdk.Token> | cdk.Token;

to

tags?: Array<cdk.Tag | cdk.Token>;

OR this is what I was thinking

tags?: Array<cdk.Tag>;

Which one were you thinking?

I think cdk.Tag[] is sufficient. Keep it simple. I don't see a strong use case for any of the other options.

The only use case where tokens might be needed for tags is as tag values, which are strings, and we know how to represent tokens as strings :-)

eladb · 2018-10-28T07:46:54Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

 }

 /**
 * A root construct which represents a single CloudFormation stack.
 */
-export class Stack extends Construct {
+export class Stack extends Construct implements ITaggable {


I feel that this is actually not needed both here and in App. Practically, every Construct may be taggable now, no? So perhaps the usage is: if you want to tag a tree of constructs, just create a TagManager yourself. No? Otherwise, we will have to make Construct ITaggable...

Agree, I thought the concept of just having a *.tags object made it easier. However, I agree I'm good to take this out and document the expected behavior.

eladb · 2018-10-28T07:47:40Z

packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts

+          tags: [ {key: 'tagKey', value: 'tagValue'} ],
+        },
+      });
+      tagResource.tags.setTag('tagKey', 'notTagValue');


I have to say that my expectation was that setTag post initialization will override the one provided when I initialized (or crash or allow me to decide if I want to override or now)

going to change this once the ctor has the initial tags.

* If an L1 Resource has a property `Tags` then add support for a TagManager

moofish32 · 2018-10-28T21:41:15Z

@eladb I could also do this in TagManager

  initialTags?: Tags | Array<cloudformation.Tag | Token> | Token;

Then handle all this stuff in in TagManager.resolve(). It's a lot of branching logic but since I have access to TagProps.overwrite I think I could make better decisions based on the intent of the cdk user.

What do you think?

* This adds the ability to define a TagManager at level in the construct tree and L1 constructs that support a known tag type will configure tags as expected (propagated from parents, etc). * changes to codegen to add support for currently known tag types - Standard Tags Key/Value - ASG Tags Standard + PropagateAtLaunch - Serverless and DAX Tags Map String-String * cfnspec updates to identify Tag Property and Taggable Resources * added to resource.ts a TaggableResource object that converts initial tags and address tag formatting on resolution via new TagType and ITagFormatter * TagManager now supports composition instead of inheritance with ITagFormatter interfaces. * If an L1 Resource has a property `Tags` then add support for a TagManager

moofish32

@eladb -- tried to update the last commit with my current summary. I tried a lot of different things and ended up here. I may have been blinded by working solutions and really looking to make sure I didn't lose sight of the desired function of making tagging just work.

moofish32 · 2018-11-05T18:08:38Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

@@ -229,6 +230,129 @@ export class Resource extends Referenceable {
  }
 }

+export enum TagType {


docs will be coming here

moofish32 · 2018-11-05T18:10:14Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+    return super.toCloudFormation();
+  }
+
+  private tagFormatterForType(): ITagFormatter {


Is there a way to make this map indexed by the enum? I was fighting with tsc to make it work and finally just moved forward with this because I have bigger questions.

moofish32 · 2018-11-05T18:14:44Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+  /**
+   * Add any of the supported CloudFormatiom Tag Types to be managed
+   *
+   * @param tags: The tag(s) to add


I don't love this. The challenge is the typescript initialization flow is done

parent init

parent constructor

child init

child constructor

So this.tagType actually isn't set when the constructor runs. I kicked around a lot of ideas, but couldn't find an elegant solution. I considered making all tags only support cdk.TagManager instead of the union type. When I coded with that I didn't like interface of having to create a new object.

const MyResource(parent, 'Name', { prop1: 'value', prop2: 'value, tags: new cdk.TagManager(parent, 'TagforMe', { initialTags: { myKey: 'MyValue', key2: 'Value2', }, });

So I went to this method, but I'm very open to some suggestions for a better solution here. In the end the CFN validators still catch mistakes at synthesis time.

moofish32 · 2018-11-05T18:16:43Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+/**
+ * Handles converting TagManager tags to AutoScalingGroupTags
+ */
+export class AutoScalingGroupTagFormatter implements ITagFormatter {


should these live here or in tag-manager? I kept flip-flopping on this. Also I did prefer the composition pattern versus the extension pattern, but it's easy to go back.

moofish32 · 2018-11-05T18:18:24Z

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

-   * Converts the `tags` to a Token for use in lazy evaluation
+   * Creates the CloudFormation representation of the tags
+   *
+   * This invokes the `tagFormatter.toCloudFormationTags` function
   */
  public resolve(): any {


I went round and round on whether this is a Token or not. My final position is this is a delayed evaluation that transforms types to meet the CloudFormation spec. I felt the cdk pattern for that is Token and it's better this play correctly with that than be a 1-off. I did design it both ways and would change again if the team feels it should be more specialized.

moofish32 · 2018-11-05T18:21:13Z

packages/@aws-cdk/cfnspec/lib/schema/property.ts

@@ -72,6 +73,22 @@ export interface ComplexMapProperty extends MapPropertyBase {
  ItemType: string;
 }

+export interface TagPropertyStandard extends PropertyBase {


so this is a variation of the Typescript discriminator pattern. We don't quite have a complete solution, but that was the concept I was following. If this logic should live somewhere cfn2ts, I can move it. However, it felt right here from my view point.

moofish32 · 2018-11-05T18:22:11Z

packages/@aws-cdk/cfnspec/lib/schema/resource-type.ts

@@ -21,6 +21,13 @@ export interface ResourceType extends Documented {
  RefKind?: string;
 }

+export interface TaggableResource extends ResourceType {


not completely necessary but here to be clear to the next developer what I think a TaggableResource is

moofish32 · 2018-11-05T18:23:10Z

tools/cfn2ts/lib/codegen.ts

+      superProps.push('properties');
+    }
+
+    this.code.line(`super(parent, name, { ${superProps.join(', ')} });`);


a little more code, but I found this one-liner hard to follow and this improved my read-ability. Easy to revert.

moofish32 · 2018-11-05T18:24:08Z

tools/cfn2ts/lib/codegen.ts

@@ -490,7 +513,15 @@ export default class CodeGenerator {
    const javascriptPropertyName = genspec.cloudFormationToScriptName(propName);

    this.docLink(spec.Documentation, additionalDocs);
-    this.code.line(`${javascriptPropertyName}${question}: ${this.findNativeType(context, spec)};`);
+    const nativeTypes = this.findNativeType(context, spec);


I was debating on passing propName into this function or moving this logic here. I chose to move the logic.

moofish32 · 2018-11-05T18:25:05Z

tools/cfn2ts/lib/codegen.ts

        if (union.indexOf('|') !== -1) {
          alternatives.push(`Array<${union}>`);
        } else {
-          alternatives.push(`(${union})[]`);
+          alternatives.push(`${union}[]`);


this broke me when I was designing the solution a little differently, but I think it was a dead code path and broken?

rix0rrr

The general feedback is that this effectively allows using TagManager at any level of the tree, so there is no need to explicitly mark anything as taggable (besides the very low level resources). Then, if you want to tag something, you just attach a TagManager to it and it will take care of things, no?

So is your proposal that you go as follows?

const someConstruct = new SomeConstruct(this, 'Some', { /* ... */ });

const tags = new TagManager(someConstruct);
tags.setTags(...);

This is a big style departure from how the rest of the CDK works. In most other cases, constructs come loaded with properties and methods that describe everything you can do to a construct (advantage: IDE-powered discoverability). But now you need to instantiate some object (where do I know to get it?) to apply these tags.

Are we all aware this is happening, and are we sure it's a justified change? I.e., do the benefits outweigh the cognitive cost of there being multiple things to do some operation? We're already getting complaints that some things are configured via properties and other things are configured via mutating methods, now we're adding yet another mechanism.

Now, I know what you're going to say: "this is a cross-cutting concern", and it applies to all objects in the tree (or more accurately, it applies to all objects it applies to in some subset of the tree), so we need a generic mechanism. Otherwise, we'll be implementing ITaggable on every construct, and that doesn't help us one lick when IMonitorable comes along.

We probably want some visitor-like mechanism to "do things to some constructs in the tree". Coinciding with the fact that I hate the name TagManager (can we be any more vague) and now the type name will be in the user-facing path, can I instead suggest an API like so:

someConstruct.apply(new Tag('name', 'hello'));
someConstruct.apply(new Tag('cheap', 'true', { override: false }));

where apply() takes some object that implements some Visitor-like interface. Of course, names are TBD. But at least we're somewhat consistent with changes being applied via a method, and we have an extension point for future aspects.

(So what I'm thinking is constructs will look like Entity/Component systems. Constructs have an identity, and one more Components which say something about how it behaves, but which don't necessarily tie into the declaration of Construct itself, and we can do some processing on all the components of a construct. In fact, the more I think about this the more correct I think this is, also for things like synthesis.)

Ordering

As an aside... if we decide that only leaf resources should be taggable, and intermediate constructs in the tree know nothing of tags (or any other cross-cutting concerns), that means that at rendering time leafs cannot crawl up the tree to find TagManagers above them in the tree to discover all recursive tags.

That in turn means that we'll have to apply tags eagerly, which means we'll end up having an ordering problem where the moment the TagManager is applied, all then-existing resources will be tagged, but resources added in the future will not. So typical example of what I'd want to write:

class SomeConstruct extends cdk.Construct {
  constructor(parent: cdk.Construct, id: string, props: SomeProps) {
    super(parent, id);

    const tags = new cdk.TagManager(this);
    tags.setTag('ownedby', 'SomeConstruct');

    // How will this child find its tags?
    new ChildConstruct(this, 'Child', { /* ... */ });
  }  
}

A generic system of aspects/components and visitors/rules/applications/configurations (pick your favorite timer) could solve this by retaining a list of visitors and reactively applying them to constructs that are added afterwards.

rix0rrr · 2018-11-09T12:58:59Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -185,7 +185,8 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
    });
    this.connections = new ec2.Connections({ securityGroup: this.securityGroup });
    this.securityGroups.push(this.securityGroup);
-    this.tags = new TagManager(this, {initialTags: props.tags});
+    this.tags = new cdk.TagManager(this, {initialTags: props.tags,


Ooh I don't like the line formatting here now that this thing has two property arguments. Can you indent this to new new line.

rix0rrr · 2018-11-09T13:01:21Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -185,7 +185,8 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
    });
    this.connections = new ec2.Connections({ securityGroup: this.securityGroup });
    this.securityGroups.push(this.securityGroup);
-    this.tags = new TagManager(this, {initialTags: props.tags});
+    this.tags = new cdk.TagManager(this, {initialTags: props.tags,
+      tagFormatter: new cdk.AutoScalingGroupTagFormatter()});


I like the composition/formatter approach as well, but I don't feel like AutoScalingGroupTagFormatter needs to live in the CDK library. Why couldn't it just live in the AutoScaling library, next to the only place where it's being used?

Easy to move this one. The Map formatter is actually in the CFN spec in two different ways and currently used only in DAX and Serverless? Should that one move?

rix0rrr · 2018-11-09T13:01:49Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

@@ -290,6 +290,7 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
      instanceTenancy,
      tags: this.tags,
    });
+    // this.resource.tags = this.tags;


Remove completely or comment back in.

rix0rrr · 2018-11-12T15:06:58Z

Can this wait until #1136 has been finalized and implemented? It might take some time though.

moofish32 · 2018-11-12T17:05:49Z

Right now the only thing that requires tags to work that I am aware of is EKS. We have tags in those constructs today. So from my viewpoint this can wait.

* The first aspects are for tags * TagManager is now deprecated and aspects are preferred * Tags can propagate to a depth by number * Tags can be included or excluded based on Resource Type

rix0rrr · 2019-01-15T15:21:07Z

Closing because I think this is superseded by #1451.

Please reopen if you disagree.

moofish32 force-pushed the f-tags-for-L1s branch from afddb18 to 138975a Compare October 24, 2018 17:25

moofish32 added a commit to moofish32/aws-cdk that referenced this pull request Oct 24, 2018

feat(cfn2ts) Add support for Tags on L1 constructs (aws#1007)

138975a

* If an L1 Resource has a property `Tags` then add support for a TagManager

rix0rrr reviewed Oct 25, 2018

View reviewed changes

eladb suggested changes Oct 25, 2018

View reviewed changes

eladb suggested changes Oct 28, 2018

View reviewed changes

moofish32 force-pushed the f-tags-for-L1s branch from cc4e5c4 to 932bde9 Compare October 28, 2018 20:33

moofish32 added a commit to moofish32/aws-cdk that referenced this pull request Oct 28, 2018

feat(cfn2ts) Add support for Tags on L1 constructs (aws#1007)

d9a0bc5

* If an L1 Resource has a property `Tags` then add support for a TagManager

moofish32 force-pushed the f-tags-for-L1s branch from 932bde9 to 2feda87 Compare November 5, 2018 17:59

moofish32 commented Nov 5, 2018

View reviewed changes

moofish32 mentioned this pull request Nov 5, 2018

Adding tags to the underling lambda of a SingletonFunction #1079

Closed

rix0rrr reviewed Nov 9, 2018

View reviewed changes

rix0rrr mentioned this pull request Nov 9, 2018

An aspect system for the construct tree #1136

Closed

rix0rrr mentioned this pull request Jan 11, 2019

Ability to specify Tags across all resources #1516

Closed

rix0rrr closed this Jan 15, 2019

		const extendsPostfix = superClasses ? ` extends ${superClasses}` : '';
		const implementPostfix = interfaces ? ` implements ${interfaces.join(', ')}` : '';

feat(cfn2ts) Add support for Tags on L1 constructs #1007

feat(cfn2ts) Add support for Tags on L1 constructs #1007

Conversation

moofish32 commented Oct 24, 2018 • edited Loading

rix0rrr Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Oct 26, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Oct 28, 2018

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Ordering

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Nov 12, 2018

moofish32 commented Nov 12, 2018

rix0rrr commented Jan 15, 2019

moofish32 commented Oct 24, 2018 •

edited

Loading

rix0rrr Oct 25, 2018 •

edited

Loading

moofish32 Nov 5, 2018 •

edited

Loading