Enable ASG & SG Tags #766

moofish32 · 2018-09-24T06:51:21Z

This adds tags to ASGs.

The change required that PropagateAtLaunch could be set. I modified the current TagManager to accept a property configuration for ASGs. This forces a change in the constructor, but actually makes it more consistent with CDK patterns. I did call this breaking because of this change, but I'm happy to change that.

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

moofish32 · 2018-09-24T06:58:00Z

This is also required in order to support EKS.

rix0rrr · 2018-09-24T14:52:47Z

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

@@ -60,6 +60,11 @@ export interface RemoveProps {
    blockPropagate?: boolean;
 }

+export interface TagManagerProps {
+    initialTags?: Tags;
+    autoScalingGroup?: boolean;


This property feels awfully specific. What is its purpose, exactly? Is it because Tag rendering is slightly different for ASGs (With the propagateAtLaunch setting)?

If so, what if the next type of resource comes along that has a slightly different tag rendering? Do we add a boolean for every different type of tag rendering?

Can we make this mechanism more generally applicable? A potential easy solution is to make a subclass of TagManager that's specific to AutoScalingGroups that renders tags slightly different. We can even keep it in the aws-autoscaling package.

@rix0rrr 💯 agree. I was going to subclass the TagManager. However, we stuffed a lot of methods in resolve. Most of those would need to either move back out of resolve or be duplicated in the subclasses.

In addition to making this extend-able, should I carve out an interface that looks something like

interface ITagManager { setTag(<args>): void; removeTag(<args>): void; resolve(void): any; }

Two questions:

Are we good with me moving the inline functions out of resolve?

Should I create the interface as well or just leave it?

I would make a new protected method and have resolve call that. You can override that method in subclasses.

Does that help?

To be precise, seems that resolve() seems to end with the pattern:

return Object.keys(tags).map( key => some_function(key) );

Where some_function is different depending on whether it's an AutoScalingGroup or not. How about making that some_function a call to a member function on TagManager?

Ok -- yes I'll create that method and make it protected.

moofish32

@rix0rrr -- I deviated a bit from our discussion, but I think it's necessary. Perhaps some more tricks I don't know yet with Typescript though. Let me know if I can clean this up better.

moofish32 · 2018-09-25T03:22:24Z

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

+/**
+ * This is the interface for arguments to `tagFormatResolve` to enable extensions
+ */
+export interface TagGroups {


@rix0rrr we didn't discuss this, but I needed to export the interface to make the extension work. Is there another pattern I should have used?

I think this is the easiest way to achieve this, in principle.

But do the subclasses really need this detail in tag groups? Can't they just get a list of tags that need to be applied, without caring where they come from?

Oh I see it's to determine propagateOnLaunch

moofish32 · 2018-09-25T03:22:50Z

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

@@ -120,8 +155,10 @@ export class TagManager extends Token {
     */
    private readonly blockedTags: string[] = [];

-    constructor(private readonly parent: Construct, initialTags: Tags  = {}) {
+    constructor(private readonly parent: Construct, props: TagManagerProps  = {}) {


no comments on this so I assume we agree this is a better constructor signature.

Yes, although we don't have teh autoScalingGroup parameter anymore do we?

that's why I dropped a comment it's just a props with a single property, but I liked it better and thought it more extensible, plus in line with the base source code.

Yes, I think it's a good pattern to get into.

moofish32 · 2018-09-25T03:23:25Z

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

@@ -135,6 +172,8 @@ export class TagManager extends Token {
     * Converts the `tags` to a Token for use in lazy evaluation
     */
    public resolve(): any {
+        // need this for scoping
+        const blockedTags = this.blockedTags;


If I don't have this here then I have to pass it to the inline functions below. Is there a better to feed scope into the nested functions?

If it's on this, the override can always access it (as long as it's protected). So that might do as well.

rix0rrr · 2018-09-25T09:47:46Z

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

+    /**
+     * Tags that are overwritten by ancestor tags
+     */
+    nonSitckyTags: Tags;


rix0rrr

Approved modulo typo

moofish32 · 2018-09-25T14:26:17Z

Are you ok if I fix up the Tags for Security Group or do you want me to separate that into another PR?

rix0rrr · 2018-09-25T17:33:59Z

Are you ok if I fix up the Tags for Security Group or do you want me to separate that into another PR?

Yeah, please do it in here as well.

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

…ags (aws#766)

rix0rrr · 2018-09-26T12:29:20Z

Has some conflicts that need resolvin'

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

…ags (aws#766)

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

…ags (aws#766)

moofish32 · 2018-09-26T19:05:05Z

packages/@aws-cdk/aws-rds/test/integ.cluster.ts

@@ -14,6 +15,7 @@ const params = new ClusterParameterGroup(stack, 'Params', {
 });
 params.setParameter('character_set_database', 'utf8mb4');

+const kmsKey = new kms.EncryptionKey(stack, 'DbSecurity');


@rix0rrr I can't complete a build with unencrypted RDS do to security automation. Is this ok to modify because getting an exception to this policy is a bit of a headache.

eladb · 2018-09-27T09:44:12Z

@rix0rrr @moofish32 is this ready to merge?

rix0rrr · 2018-09-27T10:07:56Z

Yes I think so.

* **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **aws-cloudformation:** rename the CodePipeline actions ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the README of the module to reflect the new action names ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-codedeploy:** add auto-scaling groups property to ServerDeploymentGroup ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** add deployment configuration construct ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **toolkit:** stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **cdk**: the constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** `@aws-cdk/util` is no longer available * **aws-elasticloadbalancingv2:** adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

Bug Fixes --------- * **aws-apigateway:** allow + in path parts ([#769](#769)) ([0c50d27](0c50d27)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b0f3298](b0f3298)), closes [#787](#787) * **aws-cloudfront:** check for undefined and determining of the defaultRootObject prop is set or not ([#801](#801)) ([32a74c6](32a74c6)) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([5512f70](5512f70)), closes [#721](#721) * **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([d36ee6d](d36ee6d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([f529c80](f529c80)) * **aws-elbv2:** fix load balancer registration ([#890](#890)) ([8cc9abe](8cc9abe)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([a65060d](a65060d)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c592b7f](c592b7f)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([477c827](477c827)) * **cdk:** init templates were not upgraded to typescript ^3.0.0 ([#904](#904)) ([2cc7475](2cc7475)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([76d8031](76d8031)), closes [#830](#830) * **cfn2ts:** expect Token instead of CloudFormationToken ([#896](#896)) ([6eee1d2](6eee1d2)) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([ffac98c](ffac98c)) * Correct heading level of CHANGELOG.md 0.10.0 ([40d9ef0](40d9ef0)) * Emit valid YAML-1.1 ([#876](#876)) ([ff857ea](ff857ea)), closes [#875](#875) * **toolkit:** improve error message for large templates ([#900](#900)) ([a41f48f](a41f48f)), closes [#34](#34) Code Refactoring ---------------- * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([d46a95b](d46a95b)), closes [#196](#196) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **framework:** remove app boilerplate and improvements to cx protocol ([#868](#868)) ([005beec](005beec)), closes [#216](#216) Features -------- * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([905a95d](905a95d)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([72aa872](72aa872)) * **aws-cloudformation:** rename the CFN CodePipeline Actions. ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the ReadMe of the module to reflect the new Action names. ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([b39bf11](b39bf11)), closes [#795](#795) * **aws-codedeploy:** Add the auto-scaling groups property to ServerDeploymentGroup. ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** Deployment Configuration Construct. ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([e7af9f5](e7af9f5)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([d146c8d](d146c8d)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction. ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([c5a4200](c5a4200)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([3601440](3601440)) * **aws-dynamodb:** tags support ([#814](#814)) ([924c84e](924c84e)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([3175af3](3175af3)) * **aws-ec2:** add support for ICMP protocol's classification Types & Codes to SecurityGroupRule ([#893](#893)) ([85bd3c0](85bd3c0)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([8ec761c](8ec761c)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([b42ef90](b42ef90)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([bdee191](bdee191)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([1f717e1](1f717e1)) * **cdk:** configurable default SSM context provider ([#889](#889)) ([353412b](353412b)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **aws-codepipeline**: Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4c69118](4c69118)) * **toolkit:** Stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **aws-autoscaling, aws-ec2:** Tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) ### BREAKING CHANGES * **framework:** The `cdk.App` constructor doesn't accept any arguments, and `app.run()` does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); * **framework:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. * **aws-iam:** `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes moved from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **aws-codepipeline-api**: `Artifact.subartifact` method of the CodePipeline API was renamed to `Artifact.atPath`. * constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** @aws-cdk/util is no longer available * **aws-elasticloadbalancingv2:** Adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

moofish32 force-pushed the tag-manager-asg branch from bb01ecb to 2dda5d5 Compare September 24, 2018 07:05

rix0rrr requested changes Sep 24, 2018

View reviewed changes

moofish32 force-pushed the tag-manager-asg branch 3 times, most recently from d01a36f to 79ff394 Compare September 25, 2018 03:20

moofish32 commented Sep 25, 2018

View reviewed changes

rix0rrr reviewed Sep 25, 2018

View reviewed changes

rix0rrr approved these changes Sep 25, 2018

View reviewed changes

moofish32 force-pushed the tag-manager-asg branch from a55ab9e to 067da75 Compare September 26, 2018 03:24

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

feat(ec2): Add tag support to security groups (aws#766)

067da75

moofish32 force-pushed the tag-manager-asg branch from 067da75 to e55238e Compare September 26, 2018 03:29

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

feat(cdk): Modify TagManager to support ASG Tags (aws#766)

97b6cfe

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

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

feat(aws-autoscaling): Add the ability to tag ASG and propagate the t…

8628b00

…ags (aws#766)

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

feat(ec2): Add tag support to security groups (aws#766)

e55238e

moofish32 changed the title ~~Enable ASG Tags~~ Enable ASG & SG Tags Sep 26, 2018

moofish32 force-pushed the tag-manager-asg branch from e55238e to 83e042f Compare September 26, 2018 06:26

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

feat(ec2): Add tag support to security groups (aws#766)

83e042f

moofish32 force-pushed the tag-manager-asg branch from 83e042f to ac57507 Compare September 26, 2018 06:29

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

feat(ec2): Add tag support to security groups (aws#766)

ac57507

moofish32 force-pushed the tag-manager-asg branch from ac57507 to 178904f Compare September 26, 2018 17:16

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

feat(cdk): Modify TagManager to support ASG Tags (aws#766)

7f9ee52

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

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

feat(aws-autoscaling): Add the ability to tag ASG and propagate the t…

3a7cd68

…ags (aws#766)

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

feat(ec2): Add tag support to security groups (aws#766)

178904f

moofish32 added 3 commits September 26, 2018 12:03

feat(cdk): Modify TagManager to support ASG Tags (aws#766)

a006f2c

BREAKING CHANGE: cdk.TagManager constructor now accepts `TagManagerProps` as a new argument

feat(aws-autoscaling): Add the ability to tag ASG and propagate the t…

8606f8d

…ags (aws#766)

feat(ec2): Add tag support to security groups (aws#766)

fe9be9f

moofish32 force-pushed the tag-manager-asg branch from 178904f to fe9be9f Compare September 26, 2018 19:03

moofish32 commented Sep 26, 2018

View reviewed changes

rix0rrr merged commit 3d48eb2 into aws:master Sep 27, 2018

moofish32 deleted the tag-manager-asg branch September 27, 2018 15:06

eladb mentioned this pull request Sep 27, 2018

v0.10.0 #800

Merged

rix0rrr mentioned this pull request Oct 1, 2018

Support DynamoDB Tags #814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ASG & SG Tags #766

Enable ASG & SG Tags #766

moofish32 commented Sep 24, 2018

moofish32 commented Sep 24, 2018

rix0rrr Sep 24, 2018

moofish32 Sep 24, 2018

rix0rrr Sep 24, 2018

rix0rrr Sep 24, 2018 •

edited

Loading

moofish32 Sep 24, 2018

moofish32 left a comment

moofish32 Sep 25, 2018

rix0rrr Sep 25, 2018 •

edited

Loading

rix0rrr Sep 25, 2018

moofish32 Sep 25, 2018

rix0rrr Sep 25, 2018

moofish32 Sep 25, 2018

rix0rrr Sep 25, 2018

moofish32 Sep 25, 2018

rix0rrr Sep 25, 2018

rix0rrr Sep 25, 2018

rix0rrr left a comment

moofish32 commented Sep 25, 2018

rix0rrr commented Sep 25, 2018

rix0rrr commented Sep 26, 2018

moofish32 Sep 26, 2018

eladb commented Sep 27, 2018

rix0rrr commented Sep 27, 2018

Enable ASG & SG Tags #766

Enable ASG & SG Tags #766

Conversation

moofish32 commented Sep 24, 2018

moofish32 commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Sep 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

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

moofish32 commented Sep 25, 2018

rix0rrr commented Sep 25, 2018

rix0rrr commented Sep 26, 2018

Choose a reason for hiding this comment

eladb commented Sep 27, 2018

rix0rrr commented Sep 27, 2018

rix0rrr Sep 24, 2018 •

edited

Loading

rix0rrr Sep 25, 2018 •

edited

Loading