Create A Tag Management for Constructs #538

moofish32 · 2018-08-09T17:50:00Z

Created a tag-manager in cdk/core as a consistent means to manage tags for constructs. Construct authors are responsible for assigning tags to Cloudformation Resources. Implemented tags for Vpc and VpcSubnet for an initial example.

I will do a self review to highlight some areas that did not end up
where I wanted and I think may need to change.

Focused on #91 and likely impacted by #518, closes #458 (as obsolete)

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

moofish32

I struggled to balance the simplicity of the interface with user experience a little (in my mind). I used my comments to ask a few questions to help get to a better solution. Once settled I'll fix integ tests and implement for current L2s.

moofish32 · 2018-08-09T17:52:48Z

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

@@ -42,7 +42,7 @@ export interface VpcNetworkProps {
 /**
 * The AWS resource tags to associate with the VPC.
 */
- tags?: cdk.Tag[];
+ tags?: cdk.ITags;


I am open to opinions on what the user should see as interface here. I have landed on an object {[key: string]: {value: string | Token, propagate: boolean}. The need to expose propagate to the user was an oversight early and I'm curious if we think it's the right exposure now with default to true?

@eladb I originally started with value: any, and I can go back to that. It just appeared like Token was going to be only possible value besides string. Easy fix to go back, just make the comment.

I think string should do since we will soon allow tokens to be represented as strings (#518). I don't think we really need to expose propagate in props. The 80% case (which we always try to optimize for) would just want to set key/value and propagate: true. We should obvsiouly document this behavior and explain how users can customize via addTag

I've got an issue, #360, to track the docs for this feature.

moofish32 · 2018-08-09T17:56:17Z

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

 /**
 * The routeTableId attached to this subnet.
 */
 private readonly routeTableId: cdk.Token;

 constructor(parent: cdk.Construct, name: string, props: VpcSubnetProps) {
 super(parent, name);
+ this.tags = new cdk.TagManager(this, props.tags);
+ if (!this.tags.hasTag('Name')) {
+ this.tags.addTag({key: 'Name', value: this.path, propagate: false});


Let's take a look at the integ test and I'll provide some screen shots. However, I think I might actually want the shorter this.name here because this.path has more context, but it's a bit large in the UI.

Some screenshots for comparison.

This is still an open question for me after the refactor

moofish32 · 2018-08-09T18:11:06Z

packages/@aws-cdk/aws-ec2/test/integ.everything.expected.json

@@ -119,7 +134,8 @@
 },
 "SubnetId": {
 "Ref": "VPCPublicSubnet2Subnet74179F39"
- }
+ },
+ "Tags": []


Do we care about the empty Tags here? I can remove this I think by simply returning undefined if the list is empty, but is that the right path?

moofish32 · 2018-08-09T18:12:13Z

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

+/**
+ * A single tag property values
+ */
+interface ITagProps {


Should I separate this into it's own interface? The compiler seemed to fight me with the inline object definition.

I don't see why this interface is needed.

moofish32 · 2018-08-09T18:15:06Z

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

+/**
+ * A single tag
+ */
+export interface ITag {


I created this because I had to export an array of objects and this is the array. However, for addTag I use this interface which we could switch to ITags and we could support multiple tags (rename to addTags). If we remove this from the addTag method then we can also drop propagate, and this really is CloudFormationTag if we capitalize Key and Value. I kept flip-flopping here and could use another opinion.

I changed this design and use ITags with the renamed addTags and I eliminated the propagate member.

I don't think this is needed

I did use the cloudformation.Tag interface, I think that is a better fit. If you meant I should not have a reference to this data structure at all, then let me know.

moofish32 · 2018-08-09T18:15:23Z

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

+ * interface and properly pass tags to the `Resources` (Cloudformation) elements
+ * the `Construct` creates.
+ */
+export class TagManager {


TagManager vs Tags?

This question is still applicable after

moofish32 · 2018-08-09T18:18:08Z

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

+ /**
+ * Converts the `tags` to a Token for use in lazy evaluation
+ */
+ public toToken(propgateOnly = false): Token {


propagateOnly wasn't initially needed, but as I implemented the VpcNetwork I discovered that I needed somewhere to support passing just the tags that should propagate to Resources that were not directly implementing ITaggable. If I didn't do this I needed to addTag at a specific time to ensure propagation worked as expected. I preferred this over the latter.

I changed the way propagation works by having children go back up to parents. However, this function is still needed and I'm open to improvements if this is confusing.

moofish32 · 2018-08-09T18:22:38Z

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

+ * from a parent and combine them with user provided tag list before
+ * creating a new resource.
+ */
+ public merge(tags: ITags = {}): ITags {


This was also discovered as I needed to combine tags coming in for the subnet configuration and propagating. I just realized I think I also need to support propagateOnly here and there is a bug in VpcSubnet flow. I'll fix that.

This has been eliminated in the new design

moofish32 · 2018-08-09T18:25:31Z

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

+ * Merges tags and returns a new ITags object.
+ *
+ * If tag collision occurs the passed in tags are taken. The propagate value
+ * is defaulted to true if it is not provided. This is useful to tag tags


change This is useful to tag tags ... to This is useful to combine tags from a parent with user provided tag list before creating a new resource

moofish32 · 2018-08-09T18:26:52Z

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

 });

 expect(stack).to(haveResource('AWS::EC2::VPC', {
 CidrBlock: '192.168.0.0/16',
 EnableDnsHostnames: false,
 EnableDnsSupport: false,
 InstanceTenancy: DefaultInstanceTenancy.Dedicated,
- Tags: [{ Key: tag.key, Value: tag.value }]
+ Tags: vpc.tags.toArray().map( tag => ({Key: tag.key, Value: tag.value}) ),


I should use tags for this not vpc.tags bad testing, I'll fix.

moofish32 · 2018-08-11T07:45:37Z

Tests are still broken (that's expected right now for integ tests).

I inverted the initial approach after fighting the need for a childAdded(child) callback. Instead of pushing tags down, I enabled the children to walk back up to the parents and pull in propagated tags. This actually works pretty well with our Token lazy evaluation. I also changed the interface for ITag to match the CFN tag, and changed addTag(ITag) to addTags(ITags) because that enabled the removal of propagate from ITag which felt a little cleaner. In this design I used object merging and the spread operator much more, I don't know if those are antipatterns in CDK, if they are point towards the right pattern/convention. I tried to go back mark my prior comments to indicate whether they were still valid.

eladb · 2018-08-12T06:14:17Z

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

@@ -42,7 +42,7 @@ export interface VpcNetworkProps {
 /**
 * The AWS resource tags to associate with the VPC.
 */
- tags?: cdk.Tag[];
+ tags?: cdk.ITags;


I think string should do since we will soon allow tokens to be represented as strings (#518). I don't think we really need to expose propagate in props. The 80% case (which we always try to optimize for) would just want to set key/value and propagate: true. We should obvsiouly document this behavior and explain how users can customize via addTag

eladb · 2018-08-12T06:15:46Z

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


 // Define a VPC using the provided CIDR range
 this.resource = new cloudformation.VPCResource(this, 'Resource', {
 cidrBlock,
 enableDnsHostnames,
 enableDnsSupport,
 instanceTenancy,
- tags
+ tags: this.tags.toToken(),


I think toCloudFormation

eladb · 2018-08-12T06:17:51Z

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

@@ -336,7 +346,9 @@ export class VpcNetwork extends VpcNetworkRef {

 // Create an Internet Gateway and attach it if necessary
 if (allowOutbound) {
- const igw = new cloudformation.InternetGatewayResource(this, 'IGW');
+ const igw = new cloudformation.InternetGatewayResource(this, 'IGW', {
+ tags: this.tags.toToken(true),


Use a named argument instead to make this more readable. I wonder if a policy-based name for this option such as subResource: true instead of a mechanism-based name (propagateOnly) will make it clear in what use cases this needs to be used.

In general, this is a result of a short-cut we are taking in the VPC, where we didn't implement L2s for those sub-resources. Ideally, we should have L2s for InternetGateway, InternetGatewayAttachment, etc, and they should have a tag manager of their own.

I wonder if the right approach is to create a TagManager for each one of those:

const att = new cloudformation.VPCGatewayAttachmentResource(this, 'VPCGW', { // ... tags: new TagManager(/* parent */ this).toToken() });

Then, propagation will Just Work™️, I think...

eladb · 2018-08-12T06:19:00Z

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

 /**
 * The routeTableId attached to this subnet.
 */
 private readonly routeTableId: cdk.Token;

 constructor(parent: cdk.Construct, name: string, props: VpcSubnetProps) {
 super(parent, name);
+ this.tags = new cdk.TagManager(this, props.tags);
+ if (!this.tags.hasTag('Name')) {
+ this.tags.addTags({Name: {value: this.path, propagate: false}});


I wonder if this is what we really want here? Why won't the various sub-resources have a Name based on their parent?

Perhaps you want something like: addTag(key, value, { propagate: true, overwrite: false })

I am trying to understand the overwrite aspect here. Are you saying that if I have a tag I want to default (e.g. Name) and I find a propagated Name with overwrite: false I would set the value to my default? If I find overwrite: true I would not set the value with my default?

My first thought on this particular use case is that propagating Name for subnet actually does make sense and we should change to propagate: true. Instead of overwrite becoming a property, couldn't I just checked the passed in tags (props.tags) instead of trying to figure out propagated and overwrite? If Name isn't passed in add it, otherwise the user wanted a specific name.

I agree that propagating Name for subnets make sense in this case, so let's just do that and think about overwrite later (the question is whether propagation is "forced" or "optional", but if we don't have a clear use case right not, let's punt).

I ended up adding a method hasLocalTag(key) that I think allows us to punt on this.

eladb · 2018-08-12T06:22:03Z

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

+
+ private readonly parentTagManager: TagManager | undefined;
+
+ constructor(partner: Construct, initialTags: ITags = {}) {


Doesn't look like partner is used at all. Why not just pass parentConstruct here? This will also allow you to create TagManagers for the child-resources of the VPC without needing L2s for them (see comment above).

great insight -- this helped clean some stuff up.

eladb · 2018-08-12T06:25:55Z

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

+ * @param tag The tag to add
+ * @param propagate Whether to add this tag to all children
+ */
+ public addTags(tags: ITags): void {


Ergonomics: the common use case would be to just set a propagating tag. And key, value would be the intuition of most users when they set a tag:

foo.setTag(key, value);

Then, we can include options: TagOptions = {}:

foo.setTag('key', 'value', { propagate: false, overwrite: true })

Also transitioned to setTag because addTag implies an always adding and this will overwrite. I'm a little nervous because JSII seems to prevent getXXX for Java name collision protection, but not setXXX. If I need to change this let me know.

eladb · 2018-08-12T06:26:46Z

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

+ /**
+ * Retrieve all propagated tags including tags from parents
+ */
+ private propagatedTags(): ITags {


Since parent tags are mutable, this won't really work... Can this be lazy? Instead of fetching all tags from parent upon init, can we do this only during synthesis (i.e. toToken).

This is used in the toCloudformation() call and is lazy there? I am not sure if you are implying I should always wrap this in a Token or if I should add warnings to the consumer in the doc string?

eladb · 2018-08-12T06:27:02Z

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

+ /**
+ * Propagate defaults to true
+ */
+ private defaultPropagate(tags: ITags): ITags {


Can't this be determined just based on whether propagate is set?

eladb · 2018-08-12T06:27:40Z

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

+import { Construct, Root } from '../../lib/core/construct';
+import { ITaggable, ITags, TagManager } from '../../lib/core/tag-manager';
+
+class ChildTagger extends Construct implements ITaggable {


Add a use case where only a grand-grand(-grand?)-parent is taggable. I still expect the grand-grand-children to get propagated tags.

eladb · 2018-08-12T06:28:13Z

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

+
+ constructor(partner: Construct, initialTags: ITags = {}) {
+ if (TagManager.isTaggable(partner.parent)) {
+ this.parentTagManager = partner.parent.tags;


What happens if the parent is not taggable, but the grand-parent is (etc). Should probably recurse until you find someone who is taggable...

Yep this was by design, but you are right it should not stop at the first non-taggable parent. Fixed by changing the logic with ancestors and test cases added.

eladb · 2018-08-12T07:33:52Z

We are planning soon to move to automatic CHANGELOG generation, please fix your commit title and message to adhere to conventional commits

moofish32

@eladb -- I have refactored based on your comments and replied to your comments if I thought additional clarification was needed.

If we are good with these changes, do you want me to rebase and squash to single commit following the convention or provide you a commit message for you to squash and merge?

I do still need to run integration tests for the VPC dependent integrations. I wonder if there is a good way to track those dependencies going forward? Right now I just "know" most and build and fail to find the rest.

moofish32 · 2018-08-15T08:10:41Z

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

 /**
 * The routeTableId attached to this subnet.
 */
 private readonly routeTableId: cdk.Token;

 constructor(parent: cdk.Construct, name: string, props: VpcSubnetProps) {
 super(parent, name);
+ this.tags = new cdk.TagManager(this, props.tags);
+ if (!this.tags.hasTag('Name')) {
+ this.tags.addTags({Name: {value: this.path, propagate: false}});


I ended up adding a method hasLocalTag(key) that I think allows us to punt on this.

moofish32 · 2018-08-15T08:11:57Z

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

+ * A an object of tags
+ */
+export interface ITags {
+ [key: string]: ITagProps;


I modified this to be concrete struct to ensure that properties were set internal to the class. I removed the export.

moofish32 · 2018-08-15T08:12:56Z

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

+/**
+ * A single tag
+ */
+export interface ITag {


I did use the cloudformation.Tag interface, I think that is a better fit. If you meant I should not have a reference to this data structure at all, then let me know.

moofish32 · 2018-08-15T08:13:22Z

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

+
+ private readonly parentTagManager: TagManager | undefined;
+
+ constructor(partner: Construct, initialTags: ITags = {}) {


great insight -- this helped clean some stuff up.

moofish32 · 2018-08-15T08:14:11Z

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

+
+ constructor(partner: Construct, initialTags: ITags = {}) {
+ if (TagManager.isTaggable(partner.parent)) {
+ this.parentTagManager = partner.parent.tags;


Yep this was by design, but you are right it should not stop at the first non-taggable parent. Fixed by changing the logic with ancestors and test cases added.

moofish32 · 2018-08-15T08:15:40Z

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

+ * @param tag The tag to add
+ * @param propagate Whether to add this tag to all children
+ */
+ public addTags(tags: ITags): void {


Also transitioned to setTag because addTag implies an always adding and this will overwrite. I'm a little nervous because JSII seems to prevent getXXX for Java name collision protection, but not setXXX. If I need to change this let me know.

moofish32 · 2018-08-15T08:17:20Z

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

+ /**
+ * Retrieve all propagated tags including tags from parents
+ */
+ private propagatedTags(): ITags {


This is used in the toCloudformation() call and is lazy there? I am not sure if you are implying I should always wrap this in a Token or if I should add warnings to the consumer in the doc string?

moofish32 · 2018-08-15T08:17:30Z

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

+ /**
+ * Propagate defaults to true
+ */
+ private defaultPropagate(tags: ITags): ITags {


moofish32 · 2018-08-15T08:52:29Z

Github FAIL on my part. I started the review as I double checked I addressed your comments and they now map to the wrong commit sha. The tests are still failing becauseI need to update the remaining integs. I'll get to work on that, but just need to know if I'm close to code complete on TagManager or if you want some more changes.

eladb

I still feel that the top-level props of VpcNetwork and VpcSubnet don't need to support non-propagating tags. { [key: string]: string } would go a long way in terms of ergonomics there.

Also, try to see if you can reduce the surface area of TagManager (I am okay with this name) to be as concise as possible. On that note, I am not in love with hasLocalTag. Would rather model this as an option to setTag if possible. jsii disallows getXxx() and setXxx(y) but if setXxx accepts more than a single argument (like setTag(x,y)) then, it's fine.

eladb · 2018-08-15T10:51:54Z

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

+}
+
+/*
+ * An internal Tags representation with concrete types


Why is this needed? To avoid the null-check?

Yes. Internally I always set the value so checking for null etc was a nuisance. Is this a bad pattern?

It just feels heavy handed and potentially more expensive to evolve. For example, it means that if new options are added they would need to be added in both interfaces (and nothing will enforce that)

eladb · 2018-08-15T10:53:25Z

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

+ /**
+ * Returns a deep copy of the tags object and current propagated tags
+ *
+ * Deep copies of all primiatve `ITagProps` are returned. If a `value` is a


primiatve => primitive

I'll clean this up

eladb · 2018-08-15T10:53:44Z

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

+ * the consumer should use `addTags` with the corresponding key to change
+ * the value or override a propagated parent tag.
+ */
+ public get tags(): Tags {


Why is this API needed?

eladb · 2018-08-15T10:54:03Z

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

+ /**
+ * Returns a deep copy of the tags as `Array<{key: string, value: any}>`
+ */
+ public toArray(): Tag[] {


Why is this API needed?

eladb · 2018-08-15T10:56:42Z

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

+ *
+ * @param tagType The `TagType` to filter the local tag list for
+ */
+ public localTagsByType(tagType: TagType): Tags {


Why is this API needed? Always prefer the minimum surface area. If we need to expose this in the future, we always can. But closing is much harder.

moofish32 · 2018-08-20T05:26:59Z

@rix0rrr @RomainMuller

Your comments regarding CSS in chat I think are consistent with where I was heading. The concept of selectors is missing. I think that could be a potentially awesome idea. However, I didn't try to implement that yet, I'd like to get a better feel for what we think the selectors would be.

What I've enabled so far is the following use cases:

Tag the highest level (likely Stack once implemented) and propagate (cascade) those tags to any child constructs beneath it.
Enable child constructs to allow parent tags to override them.
Enable overwrite control for setTag for construct local tags via properties.
Enable removeTag to prevent future propagation of tags from parents.

Based on prior feedback I've gone back and tried to minimize the public API and simplify the tags interface to [key: string]: string.

Yes tests are still broken, because of the VPC changes, but I'd like to get aligned before fixing all those.

rix0rrr · 2018-08-20T09:43:24Z

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

+/**
+ * Properties Tags is a dictionary of tags as strings
+ */
+export interface Tags {


Does this interface translate cleanly across jsii? What will it turn into?

(It's okay if you don't know the answer to this, @moofish32, I'm expecing @RomainMuller or @eladb to weigh in here)

(For example, it might just be an export type = {[key: string]: string} ?)

I would be interested in learning where the JSII experts look to determine this. I have not made the time to dig in there yet, but likely I should.

That particular syntax element will be ignored by jsii I believe... so no porting to other languages... we could conceivably support it though.

Right now the best "proxy" would be to just make get and set methods... it's not very pretty in TypeScript, but... at least it'll be portable right now.

On second thought... places it's used may actually render it as a Map<string, whatever>. Probably want to inspect the assembly to know for sure :)

Just tested this out; the interface definition as written is interpreted by jsii as an empty interface (issue aws/jsii#194).

Better to just write it as:

export type Tags = {[key: string]: string};

rix0rrr · 2018-08-20T09:47:59Z

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

+ propagateOverride?: boolean;
+
+ /**
+ * If set this tag will overwrite existing tags


Also for propagated tags?

If you set this overwrite: true and propagateOverride: true, the tag will be written locally but if a propagating tag is set it will take precedence.

If you set this overwrite: true and propagateOverride: false (default), the tag will be written locally and will take precedence over the same tag propagated from a parent. I had the classic situation where a single binary value was not enough to address the three use cases:

overwrite the local tag

do not overwrite the local tag

if set now or in the future take the parent tag

never take the parent tag

If you combine these possibilities some are illogical but the table is larger than 0 & 1.

Okay, I may have misunderstood the purpose of propagateOverride. Here was my assumption:

propagateOverride will control whether we overwrite existing tags while propagating this tag down.

Upon reading your comment and looking at the test, does it actually mean this?

propagateOverride will control whether this tag can be overwritten by tags that are being propagated from parents.

If it's the latter, I would suggest renaming, because this name breaks my brain. May I suggest calling it sticky and reversing the meaning (i.e., sticky=true <=> propagateOverride=false and vice versa).

Also, if that's the meaning of this property, that takes care of my other comment as well, because I thought that functionality was lacking (but turns out it isn't).

rix0rrr · 2018-08-20T09:50:43Z

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

+ */
+ private readonly blockedTags: string[] = [];
+
+ constructor(private readonly parent: Construct, initialTags: Tags = {}) {


parent was confusing to me, since this class is not actually a Construct itself, so does not participate in the construct tree.

Can you call it something like construct or taggableConstruct or something?

@rix0rrr -- the property literally is constructParent (maybe taggable). I'll avoid tagging Elad while he is out. Originally I called this partner and set it to the Construct the tags were for, but then the only thing I used if for was parent. So we moved it back to parent. The other complication is the parent does not have to be taggable. Does constructParent make more sense?

Ah, I see, Elad suggested this name? I still would prefer something like associatedConstruct or something, but I can also live with parent. Leave it.

rix0rrr · 2018-08-20T09:53:33Z

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

+ /**
+ * Converts the `tags` to a Token for use in lazy evaluation
+ */
+ public toCloudformation(): Token {


Typo toCloudFormation(), but again I would find this name slightly misleading since it's not actually a StackElement that's participating in the toCloudFormation() rendering as usual, and in fact the returned object does not have the same shape.

Maybe call it something like renderTags() or asTagObject(), toTagJson() or similar instead.

You might also make TagManager extend Token and implement resolve() to return this.toArray(). Then you can use the object everywhere you would pass tags directly to CloudFormation objects.

There is precedent for this: we do the same for PolicyDocument, for example.

Agree on the typo (🤦‍♂️ ). I think I like renderTags better. Let me look at policy document because I was considering the extends Token. Is #518 going to change anything here?

No, #518 is only about embedding Tokens representing string values into actual string types (which becomes possible once you extends Token, but we're not going to use that in any case).

I think I want to take a stab at extends Token I'll separate this into a separate commit after fixing some of the naming that we can revert if we have disagreements.

rix0rrr · 2018-08-20T10:03:19Z

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

+ *
+ * @default false
+ */
+ propagateOverride?: boolean;


I have a hard time thinking of use cases where I would want this value to be false (as I'm feeling like the "construct assembler" has the most information, so tags applied higher in the tree should basically always propagate and override). Can you enlighten me with some situations where it makes sense for children to set a tag that should NOT be overridden?

The only ones I can think of are tags like "name" or "path". But in those cases, does it not make more sense declare that tag value to be "sticky" or "important" or whatever on the CHILD marking the tag as nonoverridable, rather than require all ancestors in perpetuity to set propagateOverride=false when setting tags?

@rix0rrr -- so let's first be clear that my viewpoint is heavily influenced by my experience and company and by no means do I consider myself to have full breadth. Tagging is such a customized space that I've actually struggled a lot with the design for 80 % principal.

Cost allocation is the use case in mind for me when designing this feature. A VPC can be shared by multiple teams and commonly cost allocation is done by tags (companies are all over the board with how much to put in 1 account or 1 VPC. My experience is many factors influence here). The VPC itself may be in a top level cost center that amortizes cost across the subsidiaries. While the Dynamo Tables might be divided into specific cost centers. Depending on your team (SRE, AppOps, etc) the VPC and Dynamo may be deployed and operated by a common team. Thus when they create a stack they might default the cost allocation tag to the roll up center, but override each element in the path with a more specific cost center. Another example from my experience is VPC Flow Logs. The cost for flow log and any associated cost of centralization (kinesis streams - normally these are in a central account, but again each company may be different) is actually assigned to a central security business.

My thinking was at the top level you would put a cost allocation tag, and then at various levels of the tree you would override that tag. If you did nothing the propagation will occur correctly.

The next line of thinking was that as teams start to implement custom Constructs they might want to default these tag values. For example, perhaps you have a requirement that all components are tagged with a ComplianceLevel tag that indicates PCI, NIST, SOX, None, etc. The construct developer would default the tag to None, but if the parent was PCI then they would want that propagation to override their value. In this case the assembler would not have to do anything special, at the top level they would set the ComplianceLevel and it would propagate everywhere correctly assuming the sub-construct authors configured it correctly.

I am trying to provide some specific thoughts I have had on the way here, but literally this can be What if ... forever. I'm open to suggestions for the best way evaluate these.

rix0rrr · 2018-08-20T10:07:33Z

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

@@ -296,21 +312,23 @@ export class VpcNetwork extends VpcNetworkRef {
 throw new Error('To use DNS Hostnames, DNS Support must be enabled, however, it was explicitly disabled.');
 }

+ this.tags = new cdk.TagManager(this, props.tags);


I guess the overwrite=false is to allow consumers to set the "name" property? This seems like it's going to be a common case, so I wonder if it makes sense to add another constructor parameter to TagManager like initialTags, which would do this internally.

I considered adding things like standard defaults (Name = this.path) was the one that came to mind. Then perhaps we just remove the overwrite property?

Well, but you probably still want users to be able to pass in a custom name if they so desire, right? So the overwrite still needs to be there so we don't overwrite the consumer's customization.

I was just suggesting to make the pattern of "initial tags that can be overwritten" explit in the TagManager class.

rix0rrr · 2018-08-20T10:12:10Z

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

+ * interface and properly pass tags to the `Resources` (Cloudformation) elements
+ * the `Construct` creates using `toCloudformation()` for lazy evaluations
+ */
+export class TagManager {


I feel this class wants to be named Tags, feels cleaner to me. I understand there is a conflict with the current type Tags which also naturally is to be called that, but that could also be named TagDict or TagValues for example.

I am on the fence here. It does do more than just hold tags, but logically it was the first name in mind until I hit the conflicts. I can start renaming if we have consensus, but I don't really want to plumb this change to find a stronger opinion reverting it. :)

You are right, I have a feeling my opinion will be the minority one :). Don't change it.

rix0rrr · 2018-08-20T10:12:38Z

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

+ *
+ * @param filter The tag props to filter tags
+ */
+ public filterTags(filter: TagProps = {}): Tags {


Don't think this needs to be public?

@rix0rrr -- this is called on ancestors in propagatedTags(). I think if I went back to a more pure recursive pattern I could eliminate.

moofish32 · 2018-08-20T18:25:07Z

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

@@ -162,7 +162,7 @@ export class TagManager {
 *
 * @param filter The tag props to filter tags
 */
- public filterTags(filter: TagProps = {}): Tags {
+ private filterTags(filter: TagProps = {}): Tags {


@rix0rrr -- this passes unit tests, but If I was in Java wouldn't this break?

Apart from it doesn't matter (because private functions are never exposed or called over jsii), the same code would also work if it was all written in Java. This is a common misconception about the meaning of private:

private does not mean internal to the OBJECT; it means internal to the CLASS. If you have two instances (objects) a and b, that are both instances of the same class Thing, they can access each other's private parts.

This is not a leak: private is not a security mechanism, it's about hiding implementation details: class designers are free to change the implementation details of a class, while having assurances that other code cannot break because it COULD NOT have depended on those details. If we were to change the implementation of the hypothetical class Thing, that change would affect both a and b at the same time (they would both become aware of all new private parts to the class Thing), but the change would still be isolated from any other code.

So in this case, as long as this method is only being called from this class, it can be private.

@rix0rrr - thanks for explanation, I definitely didn't understand this correctly!

rix0rrr · 2018-08-21T09:30:53Z

Apart from the name of propagateOverride, and potentially a mechanism for setting initial (but overridable) tags, I'm ready to ship this.

moofish32 · 2018-08-22T05:40:46Z

@rix0rrr -- I think I like the Token pattern better because there is no renderTagValues or toCloudFormation function at all. I also made the change to sticky. I left our other more debatable name changes omitted for now.

I'll have to resolve some conflicts and update other integration tests before this will go green.

Fixes aws#91, Closes aws#458 (as obsolete) The TagManager is a class Construct authors can use to implement tagging consistently. The manager provides the ability to propagate tags from parents to child, override parent tags in the child, set default tags on the child that can be overwritten by parents, and block tags from parents. Adding tagging support for the Vpc and Subnet constructs.

moofish32 · 2018-08-23T20:53:02Z

@rix0rrr -- not sure I can debug the Codebuild issue?

rix0rrr · 2018-08-24T08:19:07Z

The issue is in the Java build (which we can't run on Travis because we don't have the required build images available):

[ERROR] /tmp/jsii-pacmak-codexaibW6/src/main/java/software/amazon/awscdk/services/ec2/VpcSubnet.java:[6,8] software.amazon.awscdk.services.ec2.VpcSubnet is not abstract and does not override abstract method setTags(software.amazon.awscdk.TagManager) in software.amazon.awscdk.ITaggable

rix0rrr · 2018-08-24T08:19:27Z

Honestly, TypeScript should have complained here and I'm not sure why it doesn't, but there you go.

moofish32 · 2018-08-24T08:32:57Z

I’ll get this fixed

…

On Fri, Aug 24, 2018 at 1:19 AM Rico Huijbers ***@***.***> wrote: Honestly, TypeScript should have complained here and I'm not sure why it doesn't, but there you go. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#538 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoi2OL8IV0LyN1PnBcVDtooSv7-sfnDks5uT7cRgaJpZM4V2Iux> .

Fixes aws#91, Closes aws#458 (as obsolete) The TagManager is a class Construct authors can use to implement tagging consistently. The manager provides the ability to propagate tags from parents to child, override parent tags in the child, set default tags on the child that can be overwritten by parents, and block tags from parents. Adding tagging support for the Vpc and Subnet constructs.

moofish32 · 2018-08-24T22:37:51Z

@RomainMuller -- readonly was the problem. Can you mention me in the fix for JSII so I can see where this maps?

RomainMuller · 2018-08-25T01:20:42Z

@moofish32 sure will!

eladb · 2018-08-28T06:05:52Z

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

+ */
+export class TagManager extends Token {
+
+ public static readonly DEFAULT_TAG_PROPS: TagProps = {


Why public?

eladb · 2018-08-28T06:07:04Z

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

+ */
+ private readonly blockedTags: string[] = [];
+
+ constructor(private readonly parent: Construct, initialTags: Tags = {}) {


eladb · 2018-08-28T06:07:44Z

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

+ constructor(private readonly parent: Construct, initialTags: Tags = {}) {
+ super();
+ for (const key of Object.keys(initialTags)) {
+ const tag = {value: initialTags[key],


Please fix indentation:

const tag = { value: initialTags[key], props: { propagate: true, sticky: true } }

eladb · 2018-08-28T06:07:59Z

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

+ super();
+ for (const key of Object.keys(initialTags)) {
+ const tag = {value: initialTags[key],
+ props: {propagate: true, sticky: true}};


Don't you want to use the defaults you have above?

eladb · 2018-08-28T06:08:41Z

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

+ * Converts the `tags` to a Token for use in lazy evaluation
+ */
+ public resolve(): any {
+ return this.toArray();


Why do we need toArray as a separate method? Doesn't seem like it's used anywhere besides here.

eladb · 2018-08-28T06:13:55Z

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

+ */
+ public setTag(key: string, value: string, tagProps: TagProps = {} ): void {
+ const props = {...TagManager.DEFAULT_TAG_PROPS, ...tagProps};
+ if (props.overwrite === false) {


(!props.overwrite)

I must have been lacking coffee or sleep here ...

eladb · 2018-08-28T06:15:01Z

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

+ *
+ * @param key The key of the tag to check existence
+ */
+ public hasTag(key: string): boolean {


Do we really need/want this method due to the fact that the result could be misleading?

nope it's dead code now, will delete

eladb · 2018-08-28T06:15:24Z

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

+ * Returns tags that meet the criteria of the filter
+ *
+ * The function operates only tags local to this contsturct. The function
+ * provides the ability to filter tags baseed on the `TagProps`.


"baseed" => "based"

eladb · 2018-08-28T06:17:05Z

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

+ *
+ * This takes into account blockedTags and propagationOverride tags.
+ */
+ private get tags(): Tags {


Why do we need the tags? Seems like it's only used in toArray? I would merge all these methods into resolve (and make filterTags and propagatedTags local functions in that scope). This will make it much easier to read.

eladb · 2018-08-28T06:20:18Z

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

+/**
+ * Properties for a tag
+ */
+export interface TagProps {


Not sure I understand the difference and interplay between sticky and overwrite and also blockPropagate. The API still feels a bit convoluted and I can't seem to find examples in the VPC tagging that justify this.

Perhaps elaborate in the documentation of TagManager and provide some examples?

moofish32 · 2018-08-28T17:19:50Z

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

+ * Overwrite: Construct authors have the need to set a tag, but only if one was
+ * not provided by the conumer. The most common example is the `Name` tag.
+ * Overwrite is for this purpose and is controlled by `TagProps.overwrite`. The
+ * default is `true`.


@eladb -- this is my next attempt at describing the what, why, and how for TagManager use. @Doug-AWS this might help you as well. If not then I still need help with naming and I'll take another stab or solicit via survey while reading https://www.thesaurus.com/.

moofish32 · 2018-08-28T18:58:42Z

@eladb -- I think I've addressed your comments and I'm ready for some feedback.

eladb · 2018-08-28T17:22:53Z

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

+ * defaults to true to reflect this.
+ *
+ * Overwrite: Construct authors have the need to set a tag, but only if one was
+ * not provided by the conumer. The most common example is the `Name` tag.


conumer => consumer

@mpiroc

The headliners of this release are __.NET support__, and a wealth of commits by external contributors who are stepping up to fix the CDK for their use cases! Thanks all for the effort put into this release! * Add strongly-named .NET targets, and a `cdk init` template for C# projects ([@mpiroc] in [#617](#617), [#643](#643)). * __@aws-cdk/aws-autoscaling__: Allow attaching additional security groups to Launch Configuration ([@moofish32] in [#636](#636)). * __@aws-cdk/aws-autoscaling__: Support update and creation policies on AutoScalingGroups ([@rix0rrr] in [#595](#595)). * __@aws-cdk/aws-codebuild__: Add support for running script from an asset ([@rix0rrr] in [#677](#677)). * __@aws-cdk/aws-codebuild__: New method `addBuildToPipeline` on Project ([@skinny85] in [783dcb3](783dcb3)). * __@aws-cdk/aws-codecommit__: New method `addToPipeline` on Repository ([@skinny85] in [#616](#616)). * __@aws-cdk/aws-codedeploy__: Add initial support for CodeDeploy ([@skinny85] in [#593](#593), [#641](#641)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB autoscaling ([@SeekerWing] in [#637](#637)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB streams ([@rhboyd] in [#633](#633)). * __@aws-cdk/aws-dynamodb__: Add support for server-side encryption ([@jungseoklee] in [#684](#864)). * __@aws-cdk/aws-ec2__ (_**BREAKING**_): SecurityGroup can now be used as a Connectable [#582](#582)). * __@aws-cdk/aws-ec2__: Add VPC tagging ([@moofish] in [#538](#538)). * __@aws-cdk/aws-ec2__: Add support for `InstanceSize.Nano` ([@rix0rrr] in [#581](#581)) * __@aws-cdk/aws-lambda__: Add support for dead letter queues ([@SeekerWing] in [#663](#663)). * __@aws-cdk/aws-lambda__: Add support for placing a Lambda in a VPC ([@rix0rrr] in [#598](#598)). * __@aws-cdk/aws-logs__: Add `extractMetric()` helper function ([@rix0rrr] in [#676](#676)). * __@aws-cdk/aws-rds__: Add support for Aurora PostreSQL/MySQL engines ([@cookejames] in [#586](#586)) * __@aws-cdk/aws-s3__: Additional grant methods for Buckets ([@eladb] in [#591](#591)) * __@aws-cdk/aws-s3__: New method `addToPipeline` on Bucket ([@skinny85] in [c8b7a49](c8b7a49)). * __aws-cdk__: Add support for HTTP proxies ([@rix0rrr] in [#666](#666)). * __aws-cdk__: Toolkit now shows failure reason if stack update fails ([@rix0rrr] in [#609](#609)). * __cdk-build-tools__: Add support for running experiment JSII versions ([@RomainMuller] in [#649](#649)). * _**BREAKING**_: Generate classes and types for the CloudFormation resource `.ref` attributes ([@rix0rrr] in [#627](#627)). * _**BREAKING**_: Make types accepted in Policy-related classes narrower (from `any` to `Arn`, for example) to reduce typing mistakes ([@rix0rrr] in [#629](#629)). * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): Align the CodePipeline APIs ([@skinny85] in [#492](#492), [#568](#568)) * __@aws-cdk/aws-ec2__ (_**BREAKING**_): Move Fleet/AutoScalingGroup to its own package ([@rix0rrr] in [#608](#608)). * __aws-cdk__: Simplify plugin protocol ([@RomainMuller] in [#646](#646)). * __@aws-cdk/aws-cloudfront__: Fix CloudFront behavior for ViewerProtocolPolicy ([@mindstorms6] in [#615](#615)). * __@aws-cdk/aws-ec2__: VPC Placement now supports picking Isolated subnets ([@rix0rrr] in [#610](#610)). * __@aws-cdk/aws-logs__: Add `export()/import()` capabilities ([@rix0rrr] in [#630](#630)). * __@aws-cdk/aws-rds__: Fix a bug where a cluster with 1 instance could not be created ([@cookejames] in [#578](#578)) * __@aws-cdk/aws-s3__: Bucket notifications can now add dependencies, fixing creation order ([@eladb] in [#584](#584)). * __@aws-cdk/aws-s3__: Remove useless bucket name validation ([@rix0rrr] in [#628](#628)). * __@aws-cdk/aws-sqs__: Make `QueueRef.encryptionMasterKey` readonly ([@RomainMuller] in [#650](#650)). * __assets__: S3 read permissions are granted on a prefix to fix lost permissions during asset update ([@rix0rrr] in [#510](#510)). * __aws-cdk__: Remove bootstrapping error if multiple stacks are in the same environment ([@RomainMuller] in [#625](#625)). * __aws-cdk__: Report and continue if git throws errors during `cdk init` ([@rix0rrr] in [#587](#587)). * __@aws-cdk/cfnspec__: Updated [CloudFormation resource specification] to `v2.6.0` ([@RomainMuller] in [#594](#594)) + **New AWS Construct Library** - `@aws-cdk/aws-sagemaker` supports AWS::SageMaker resources + **New Resource Types** - AWS::AmazonMQ::Broker - AWS::AmazonMQ::Configuration - AWS::CodePipeline::Webhook - AWS::Config::AggregationAuthorization - AWS::Config::ConfigurationAggregator - AWS::EC2::VPCEndpointConnectionNotification - AWS::EC2::VPCEndpointServicePermissions - AWS::IAM::ServiceLinkedRole - AWS::SSM::ResourceDataSync - AWS::SageMaker::Endpoint - AWS::SageMaker::EndpointConfig - AWS::SageMaker::Model - AWS::SageMaker::NotebookInstance - AWS::SageMaker::NotebookInstanceLifecycleConfig + **Attribute Changes** - AWS::CodePipeline::Pipeline Version (__added__) + **Property Changes** - AWS::AppSync::DataSource HttpConfig (__added__) - AWS::DAX::Cluster SSESpecification (__added__) - AWS::DynamoDB::Table Stream (__added__) - AWS::DynamoDB::Table AutoScalingSupport (__added__) - AWS::EC2::VPCEndpoint IsPrivateDnsEnabled (__added__) - AWS::EC2::VPCEndpoint SecurityGroupIds (__added__) - AWS::EC2::VPCEndpoint SubnetIds (__added__) - AWS::EC2::VPCEndpoint VPCEndpointType (__added__) - AWS::EC2::VPCEndpoint RouteTableIds.DuplicatesAllowed (__deleted__) - AWS::EC2::VPCPeeringConnection PeerRegion (__added__) - AWS::EFS::FileSystem ProvisionedThroughputInMibps (__added__) - AWS::EFS::FileSystem ThroughputMode (__added__) - AWS::EMR::Cluster KerberosAttributes (__added__) - AWS::Glue::Classifier JsonClassifier (__added__) - AWS::Glue::Classifier XMLClassifier (__added__) - AWS::Glue::Crawler Configuration (__added__) - AWS::Lambda::Lambda DLQConfigurationSupport (__added__) - AWS::Neptune::DBInstance DBSubnetGroupName.UpdateType (__changed__) - Old: Mutable - New: Immutable - AWS::SNS::Subscription DeliveryPolicy (__added__) - AWS::SNS::Subscription FilterPolicy (__added__) - AWS::SNS::Subscription RawMessageDelivery (__added__) - AWS::SNS::Subscription Region (__added__) - AWS::SQS::Queue Tags (__added__) - AWS::ServiceDiscovery::Service HealthCheckCustomConfig (__added__) + **Property Type Changes** - AWS::AppSync::DataSource.HttpConfig (__added__) - AWS::DAX::Cluster.SSESpecification (__added__) - AWS::EMR::Cluster.KerberosAttributes (__added__) - AWS::Glue::Classifier.JsonClassifier (__added__) - AWS::Glue::Classifier.XMLClassifier (__added__) - AWS::ServiceDiscovery::Service.HealthCheckCustomConfig (__added__) - AWS::CloudFront::Distribution.CacheBehavior FieldLevelEncryptionId (__added__) - AWS::CloudFront::Distribution.DefaultCacheBehavior FieldLevelEncryptionId (__added__) - AWS::CodeBuild::Project.Artifacts EncryptionDisabled (__added__) - AWS::CodeBuild::Project.Artifacts OverrideArtifactName (__added__) - AWS::CodeBuild::Project.Environment Certificate (__added__) - AWS::CodeBuild::Project.Source ReportBuildStatus (__added__) - AWS::ServiceDiscovery::Service.DnsConfig RoutingPolicy (__added__) - AWS::WAF::WebACL.ActivatedRule Action.Required (__changed__) - Old: true - New: false * __@aws-cdk/cfnspec__: Updated Serverless Application Model (SAM) Resource Specification ([@RomainMuller] in [#594](#594)) + **Property Changes** - AWS::Serverless::Api MethodSettings (__added__) + **Property Type Changes** - AWS::Serverless::Function.SQSEvent (__added__) - AWS::Serverless::Function.EventSource Properties.Types (__changed__) - Added SQSEvent

@mpiroc

The headliners of this release are __.NET support__, and a wealth of commits by external contributors who are stepping up to fix the CDK for their use cases! Thanks all for the effort put into this release! * Add strongly-named .NET targets, and a `cdk init` template for C# projects ([@mpiroc] in [#617](#617), [#643](#643)). * __@aws-cdk/aws-autoscaling__: Allow attaching additional security groups to Launch Configuration ([@moofish32] in [#636](#636)). * __@aws-cdk/aws-autoscaling__: Support update and creation policies on AutoScalingGroups ([@rix0rrr] in [#595](#595)). * __@aws-cdk/aws-codebuild__: Add support for running script from an asset ([@rix0rrr] in [#677](#677)). * __@aws-cdk/aws-codebuild__: New method `addBuildToPipeline` on Project ([@skinny85] in [783dcb3](783dcb3)). * __@aws-cdk/aws-codecommit__: New method `addToPipeline` on Repository ([@skinny85] in [#616](#616)). * __@aws-cdk/aws-codedeploy__: Add initial support for CodeDeploy ([@skinny85] in [#593](#593), [#641](#641)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB autoscaling ([@SeekerWing] in [#637](#637)). * __@aws-cdk/aws-dynamodb__: Add support for DynamoDB streams ([@rhboyd] in [#633](#633)). * __@aws-cdk/aws-dynamodb__: Add support for server-side encryption ([@jungseoklee] in [#684](#864)). * __@aws-cdk/aws-ec2__ (_**BREAKING**_): SecurityGroup can now be used as a Connectable [#582](#582)). * __@aws-cdk/aws-ec2__: Add VPC tagging ([@moofish] in [#538](#538)). * __@aws-cdk/aws-ec2__: Add support for `InstanceSize.Nano` ([@rix0rrr] in [#581](#581)) * __@aws-cdk/aws-lambda__: Add support for dead letter queues ([@SeekerWing] in [#663](#663)). * __@aws-cdk/aws-lambda__: Add support for placing a Lambda in a VPC ([@rix0rrr] in [#598](#598)). * __@aws-cdk/aws-logs__: Add `extractMetric()` helper function ([@rix0rrr] in [#676](#676)). * __@aws-cdk/aws-rds__: Add support for Aurora PostreSQL/MySQL engines ([@cookejames] in [#586](#586)) * __@aws-cdk/aws-s3__: Additional grant methods for Buckets ([@eladb] in [#591](#591)) * __@aws-cdk/aws-s3__: New method `addToPipeline` on Bucket ([@skinny85] in [c8b7a49](c8b7a49)). * __aws-cdk__: Add support for HTTP proxies ([@rix0rrr] in [#666](#666)). * __aws-cdk__: Toolkit now shows failure reason if stack update fails ([@rix0rrr] in [#609](#609)). * __cdk-build-tools__: Add support for running experiment JSII versions ([@RomainMuller] in [#649](#649)). * _**BREAKING**_: Generate classes and types for the CloudFormation resource `.ref` attributes ([@rix0rrr] in [#627](#627)). * _**BREAKING**_: Make types accepted in Policy-related classes narrower (from `any` to `Arn`, for example) to reduce typing mistakes ([@rix0rrr] in [#629](#629)). * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): Align the CodePipeline APIs ([@skinny85] in [#492](#492), [#568](#568)) * __@aws-cdk/aws-ec2__ (_**BREAKING**_): Move Fleet/AutoScalingGroup to its own package ([@rix0rrr] in [#608](#608)). * __aws-cdk__: Simplify plugin protocol ([@RomainMuller] in [#646](#646)). * __@aws-cdk/aws-cloudfront__: Fix CloudFront behavior for ViewerProtocolPolicy ([@mindstorms6] in [#615](#615)). * __@aws-cdk/aws-ec2__: VPC Placement now supports picking Isolated subnets ([@rix0rrr] in [#610](#610)). * __@aws-cdk/aws-logs__: Add `export()/import()` capabilities ([@rix0rrr] in [#630](#630)). * __@aws-cdk/aws-rds__: Fix a bug where a cluster with 1 instance could not be created ([@cookejames] in [#578](#578)) * __@aws-cdk/aws-s3__: Bucket notifications can now add dependencies, fixing creation order ([@eladb] in [#584](#584)). * __@aws-cdk/aws-s3__: Remove useless bucket name validation ([@rix0rrr] in [#628](#628)). * __@aws-cdk/aws-sqs__: Make `QueueRef.encryptionMasterKey` readonly ([@RomainMuller] in [#650](#650)). * __assets__: S3 read permissions are granted on a prefix to fix lost permissions during asset update ([@rix0rrr] in [#510](#510)). * __aws-cdk__: Remove bootstrapping error if multiple stacks are in the same environment ([@RomainMuller] in [#625](#625)). * __aws-cdk__: Report and continue if git throws errors during `cdk init` ([@rix0rrr] in [#587](#587)). * __@aws-cdk/cfnspec__: Updated [CloudFormation resource specification] to `v2.6.0` ([@RomainMuller] in [#594](#594)) + **New AWS Construct Library** - `@aws-cdk/aws-sagemaker` supports AWS::SageMaker resources + **New Resource Types** - AWS::AmazonMQ::Broker - AWS::AmazonMQ::Configuration - AWS::CodePipeline::Webhook - AWS::Config::AggregationAuthorization - AWS::Config::ConfigurationAggregator - AWS::EC2::VPCEndpointConnectionNotification - AWS::EC2::VPCEndpointServicePermissions - AWS::IAM::ServiceLinkedRole - AWS::SSM::ResourceDataSync - AWS::SageMaker::Endpoint - AWS::SageMaker::EndpointConfig - AWS::SageMaker::Model - AWS::SageMaker::NotebookInstance - AWS::SageMaker::NotebookInstanceLifecycleConfig + **Attribute Changes** - AWS::CodePipeline::Pipeline Version (__added__) + **Property Changes** - AWS::AppSync::DataSource HttpConfig (__added__) - AWS::DAX::Cluster SSESpecification (__added__) - AWS::DynamoDB::Table Stream (__added__) - AWS::DynamoDB::Table AutoScalingSupport (__added__) - AWS::EC2::VPCEndpoint IsPrivateDnsEnabled (__added__) - AWS::EC2::VPCEndpoint SecurityGroupIds (__added__) - AWS::EC2::VPCEndpoint SubnetIds (__added__) - AWS::EC2::VPCEndpoint VPCEndpointType (__added__) - AWS::EC2::VPCEndpoint RouteTableIds.DuplicatesAllowed (__deleted__) - AWS::EC2::VPCPeeringConnection PeerRegion (__added__) - AWS::EFS::FileSystem ProvisionedThroughputInMibps (__added__) - AWS::EFS::FileSystem ThroughputMode (__added__) - AWS::EMR::Cluster KerberosAttributes (__added__) - AWS::Glue::Classifier JsonClassifier (__added__) - AWS::Glue::Classifier XMLClassifier (__added__) - AWS::Glue::Crawler Configuration (__added__) - AWS::Lambda::Lambda DLQConfigurationSupport (__added__) - AWS::Neptune::DBInstance DBSubnetGroupName.UpdateType (__changed__) - Old: Mutable - New: Immutable - AWS::SNS::Subscription DeliveryPolicy (__added__) - AWS::SNS::Subscription FilterPolicy (__added__) - AWS::SNS::Subscription RawMessageDelivery (__added__) - AWS::SNS::Subscription Region (__added__) - AWS::SQS::Queue Tags (__added__) - AWS::ServiceDiscovery::Service HealthCheckCustomConfig (__added__) + **Property Type Changes** - AWS::AppSync::DataSource.HttpConfig (__added__) - AWS::DAX::Cluster.SSESpecification (__added__) - AWS::EMR::Cluster.KerberosAttributes (__added__) - AWS::Glue::Classifier.JsonClassifier (__added__) - AWS::Glue::Classifier.XMLClassifier (__added__) - AWS::ServiceDiscovery::Service.HealthCheckCustomConfig (__added__) - AWS::CloudFront::Distribution.CacheBehavior FieldLevelEncryptionId (__added__) - AWS::CloudFront::Distribution.DefaultCacheBehavior FieldLevelEncryptionId (__added__) - AWS::CodeBuild::Project.Artifacts EncryptionDisabled (__added__) - AWS::CodeBuild::Project.Artifacts OverrideArtifactName (__added__) - AWS::CodeBuild::Project.Environment Certificate (__added__) - AWS::CodeBuild::Project.Source ReportBuildStatus (__added__) - AWS::ServiceDiscovery::Service.DnsConfig RoutingPolicy (__added__) - AWS::WAF::WebACL.ActivatedRule Action.Required (__changed__) - Old: true - New: false * __@aws-cdk/cfnspec__: Updated Serverless Application Model (SAM) Resource Specification ([@RomainMuller] in [#594](#594)) + **Property Changes** - AWS::Serverless::Api MethodSettings (__added__) + **Property Type Changes** - AWS::Serverless::Function.SQSEvent (__added__) - AWS::Serverless::Function.EventSource Properties.Types (__changed__) - Added SQSEvent

Doug-AWS · 2018-10-11T13:50:14Z

IMHO the most-common use case is setting a tag that propagates to all resources. What does that code look like? My initial stab is something like the following in the main stack's constructor:

// Tag all constructs with "HelloCDK"
const tm = new cdk.TagManager(this);
tm.setTag('Stack', 'HelloCDK', {
  propagate: true
});

But I don't see any tags in the resulting CFN template.

moofish32 · 2018-10-13T17:26:10Z

@Doug-AWS -- this topic is a bit hard to follow, but let me try. Originally we wanted something like what you have to work. However, that would mean on addChild we would have needed some sort of callback function. @eladb or @rix0rrr (sorry can't remember who) pointed out to me that this would likely be better implemented with the node event emitter. However, that refactor is much larger and we would like a solution in the near term.

So, what I implemented in the TagManager is actually a class for AWS Construct authors to use in their classes. This means that we did not magically just add tags everywhere we actually have to implement tags for each construct that supports them. Right now I've focused on getting tags where EKS requires it. This VPC, Subnets, ASGs, and Security Groups. Along the way I knocked off all the children in VPC (NATs etc). So a quick example of where and how those might work:

class MyApp extends cdk.App {
  constructor() {
    super();
    new ec2.VpcNetwork(this, 'TaggedVpc', {
        subnetConfiguration: [ {
          subnetType: ec2.SubnetType.Public,
          name: 'EksPublic'
        } ],
      tags: {
        'kubernetes.io/cluster/MyEksClusterName': 'shared',
        BusinessUnit: 'Marketing',
        Compliance: 'None',
      }
    });
   // you could also vpc.tags.setTag( ... ) if you returned the const
}

new MyApp().run();

You should see the tag propagate to subnets and other components taggable within the VPC. Autoscaling group and security groups have similar behavior, but that's because the CDK developers have implemented the ITaggable interface and properly connected the children. There is another story here about inheritance tree and specifically the difference between L2 and cdk.Resource, but I don't think that will help to much in the short term.

Now what recently came to may attention is CloudFormation Stack tags are not supported. I think we should open an issue to track that. Stack Tags are special in that they are not exposed in CloudFormation Templates only in the CloudFormation API calls. So we will need move those tags from the stack to the API call. Those tags will propagate to all resources and are independent from the *.cloudformation.*Resource tags, but we can likely use the same core TagManager to pass them across to the cloudformation API call.

Does this help explain where we are with the feature? @eladb or @rix0rrr, please correct any mistakes if you see them.

eladb · 2018-10-14T10:30:40Z

@moofish32 you are right that some core functionality might be needed to support something like @Doug-AWS describes, but I don't see a reason for us not to have that support without needing to use CloudFormation's out-of-template APIs.

I think aspects (#282) should be designed to support use cases exactly like this. Perhaps TagManager will be our first aspect. We just haven't had the time to dive into this design yet.

moofish32 · 2018-10-15T02:28:43Z

@eladb -- You are right. We could implement tags for Stack and App.

class MyApp extends cdk.App implements cdk.ITaggable {

  public readonly tags: cdk.TagManager;

  constructor() {
    super();

    this.tags = new cdk.TagManager(this);
    this.tags.setTag('appName', 'MyAppTags');
    // ... my stacks in here 
  }
}

At this point the tags would flow just like the CloudFormation Stack Level tags, if we use the default setTag properties.

Should I open a PR to add these to cdk.App and cdk.Stack? In addition I can start adding the tag support for an L2 we have in this list: https://docs.aws.amazon.com/awsconsolehelpdocs/latest/gsg/supported-resources.html?

This doesn't fully flesh out the design of #282, but it brings the functionality for tags closer to where I think you were talking about?

eladb · 2018-10-15T06:45:29Z

@moofish32 would love the contribution (as usual 👍), here are a few thoughts:

cfn2ts should be able to identify any L1 resources which support tags by using a heuristic (has a "Tags" property).
We can add some low-level tagging support in cdk.Resource (e.g. add a TagManager to all resources that support tagging).

moofish32 · 2018-10-22T16:59:45Z

@eladb -- I think we may want to connect these: #932

moofish32 commented Aug 9, 2018

View reviewed changes

eladb suggested changes Aug 12, 2018

View reviewed changes

Doug-AWS mentioned this pull request Aug 13, 2018

docs: Create a "How-To Tag my Resources" section #360

Closed

moofish32 commented Aug 15, 2018

View reviewed changes

eladb reviewed Aug 15, 2018

View reviewed changes

rix0rrr reviewed Aug 20, 2018

View reviewed changes

moofish32 commented Aug 20, 2018

View reviewed changes

moofish32 mentioned this pull request Aug 21, 2018

fix(aws-ec2): VPC properly uses complex subnet config #610

Merged

rix0rrr approved these changes Aug 23, 2018

View reviewed changes

moofish32 force-pushed the f-tag-manager branch from d7ba40f to fdfe875 Compare August 23, 2018 19:32

moofish32 force-pushed the f-tag-manager branch from fdfe875 to e0cef90 Compare August 24, 2018 22:34

RomainMuller mentioned this pull request Aug 25, 2018

JSII seems to not prevent a read-write property from being downgraded to read-only aws/jsii#199

Closed

moofish32 mentioned this pull request Aug 27, 2018

Feature Add Ability to Tag Subnets #458

Closed

eladb reviewed Aug 28, 2018

View reviewed changes

moofish32 added 6 commits August 28, 2018 01:58

cleaning up code via comments

e4a0fb6

cleaning up tests

e7a17cb

code cleanup

24da7ac

moving public DEFAULT_TAG_PROPS to private, fixing indentation

6658fc9

adding TagManager doc

8e79a7e

Merge branch 'master' into f-tag-manager

12d1d02

moofish32 commented Aug 28, 2018

View reviewed changes

adding details about tag removal

6ec468e

eladb approved these changes Sep 3, 2018

View reviewed changes

eladb merged commit 45d0aa2 into aws:master Sep 3, 2018

rix0rrr mentioned this pull request Sep 11, 2018

v0.9.0 #688

Merged

sumodgeorge mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities sumodgeorge/aws-cdk#1

Open

SNYKabbott mentioned this pull request Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities SNYKabbott/aws-cdk#27

Open


		private readonly parentTagManager: TagManager \| undefined;

		constructor(partner: Construct, initialTags: ITags = {}) {

Create A Tag Management for Constructs #538

Create A Tag Management for Constructs #538

Conversation

moofish32 commented Aug 9, 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

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

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 Aug 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Aug 12, 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

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 Aug 15, 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

moofish32 commented Aug 20, 2018

rix0rrr Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 Aug 20, 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

rix0rrr Aug 20, 2018 •

edited

Loading

moofish32 Aug 20, 2018 •

edited

Loading

rix0rrr Aug 21, 2018 •

edited

Loading