Feature Add Ability to Tag Subnets #458

moofish32 · 2018-07-31T20:40:27Z

This adds the ability to configure tags on subnets.

By default all subnets now will have a Name tag so that they show up nicely in the UI
Tags added to SubnetConfiguration will be applied to all subnets created

Questions:

Are there issues with the default?
Docs still go in the README for now?
Test was a bit painful, is there something we should extract?
~~Do we need to support modification of the tags via the VpcSubnet classes? This would appear to be difficult with constructor based creation flow.~~ Update - we can support this with the cdk.Token function capability. I will modify to add this, unless I'm told this is the wrong path. I will follow a pattern similar to attachToClassicLB in auto-scaling-group.ts

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

`SubnetConfiguration` interfaces; add a default `Name` tag if none are provided

integration tests bump for rds

…id changing VpcSubnetRef because that caused additional breaks in Java code

moofish32

Added the addTags function to VpcSubnet

I also made a few comments about the implementation that I was not positive I did correctly.

moofish32 · 2018-08-01T23:23:46Z

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

+     * The AWS resource tags to associate with the Subnet.
+     *
+     */
+    tags?: cdk.Tag[];


Everywhere I exposed to the customer I tried to use cdk.Tag but internally I need to use cdk.Token to get the lazy evaluation. I'm not sure if this is the right pattern or not?

Wouldn't it make more sense for this to just be { [key: string]: any }?

So should I never use cdk.Tag the { [key: string]: any } was my original thought. Then I found VpcNetworkProps used tags?: cdk.Tag[] so I pivoted. So two questions:

should we not use cdk.Tag in any L2s?

is there any reason this is not { [key: string]: string } I don't have a use case yet, but seems to rigid?

I think we should deprecate cdk.Tag. Not sure what value it brings to this world...

@eladb and the second question here about { [key: string]: string } vs any?

moofish32 · 2018-08-01T23:26:32Z

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

+                    { Key: 'AddedTag', Value: 'NewThing' },
+                ])
+            ));
+            (myVpc.publicSubnets as VpcSubnet[]).forEach((subnet) => {


I wanted to change vpc.publicSubnets to be type VpcSubnet instead of VpcSubnetRef because that made a lot of sense based on both file location and the change I am making. However, this breaks part of the java test cases. Is that an indication of an agreed to convention that is for now required or is that open for debate?

publicSubnets is part of the API of VpcNetworkRef which can represent either a VPC defined in your stack or an imported VPC, which is a very important use case (see #506). Therefore, we also want the subnets to use the XxxRef version, so they represent a more abstract notion.

There's a little bit of documentation about imports under the AWS Construct Library topic in our docs. We should probably improve it...

RE: Imports

I was under the impression (from reading the Importing section of https://awslabs.github.io/aws-cdk/refs/_aws-cdk_aws-certificatemanager.html) that you had to export a construct before you could import it

XxxRef.import() static methods accept a set of props. If you can satisfy them, you can instantiate an XxxRef. For example, BucketRef.import asks for two (optional) attributes: bucketName and bucketArn. If you provide one of them, the other can be deduced, so:

const importedBucket = Bucket.import(this, 'MyImportedBucket', { bucketName: 'my_bucket' });

Would return a BucketRef instance associated with an existing bucket named my_bucket.

Other resources may have different heuristics for imports (for example, VPCs need much more information in order to be usable).

When you export something, it will produce a set of Outputs and return a set of Fn::ImportValue tokens that match the same set of props needed for import, so you can export/import across stack boundaries:

const exportedFoo = fooBucket.export(); // then in another stack: const importedFoo = Bucket.import(this, 'ImportedFoo', exportedFoo);

Should we expand on the this topic?

I'll take a stab at updating the topic. Watch for a new PR soon.

PaulMaddox · 2018-08-02T09:39:01Z

Thank you for adding this. I was just creating a CDK project to deploy some K8s/EKS stuff and realised I was blocked as EKS requires specific tags on subnets to indicate to K8s their purpose.

+1 for the default Name tag being created if not overridden by the user. I wonder if this is something we could introduce elsewhere / make more generic in the future.

eladb · 2018-08-02T12:46:34Z

@moofish32 sorry we haven't been super responsive. We are heads down with the launch this week. We'll get back to you with feedback next week. In the meantime, see #91 for some initial discussion on cross-cutting tag support. I am not saying we need to implement this right now, but maybe worth chiming in on the discussion if you have some insights

moofish32 · 2018-08-02T15:26:37Z

@PaulMaddox -- I have some interest in EKS, ping me in gitter chat if you want to discuss. In particular I'm curious on your Service Broker thoughts and potentially replacing the ansible aspect with CDK.

moofish32 · 2018-08-03T22:53:12Z

@eladb @PaulMaddox @rix0rrr -- coming back to this issue from the comment. I'm open to requesting merge here. I think I can pick up #91 now that @eladb enlightened me. I lean towards merge because I think #91 will take more review time and the team is quite busy; with this @PaulMaddox can move forward with some EKS use cases. I'll of course clean this up and I don't see any reason it would be a breaking change when we close #91.

eladb · 2018-08-06T06:19:53Z

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

+
+    /**
+     * The AWS resource tags to associate with the Subnet.
+     *


Redundant newline

eladb · 2018-08-06T06:20:11Z

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

+     * The AWS resource tags to associate with the Subnet.
+     *
+     */
+    tags?: cdk.Tag[];


Wouldn't it make more sense for this to just be { [key: string]: any }?

eladb · 2018-08-06T06:20:36Z

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

@@ -400,7 +407,8 @@ export class VpcNetwork extends VpcNetworkRef {
                vpcId: this.vpcId,
                cidrBlock: this.networkBuilder.addSubnet(cidrMask),
                mapPublicIpOnLaunch: (subnetConfig.subnetType === SubnetType.Public),
-            };
+                tags: subnetConfig.tags,
+                };


Indentation?

eladb · 2018-08-06T06:20:48Z

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

+    /**
+     * The AWS resource tags to associate with the Subnet.
+     */
+    tags?: cdk.Tag[];


map == { [key: string]: any}?

eladb · 2018-08-06T06:23:37Z

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

+    /**
+     * The tags for this subnet
+     */
+    private readonly tags: cdk.Token[] = [];


There's no need to store these as Tokens internally. Just store a hash and initialize it like this:

this.tags = { Name: this.path, ...(this.props.tags || {}) // if props.tags contains "Name" it will be overridden by the splat };

Then use a token to "lazy render" to CloudFormation:

const subnet = new cloudformation.SubnetResource(this, 'Subnet', { // ... tags: new Token(() => Object.keys(this.tags).map(key => ({ key, value: this.tags[key] }))) };

(you can also create a helper function if you think it's more readable)

I have this in my work for #91 - thinking it might be better just to MVP that solution and only enable VPC first. I need to pivot away from cdk.Tag as mentioned above.

eladb · 2018-08-06T06:26:34Z

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

+        if (props.tags !== undefined) {
+            this.tags = props.tags.map( tag => new cdk.Token(tag) );
+            if (props.tags.filter( tag => tag.key === 'Name' ).length !== 1) {
+                this.tags.push(new cdk.Token({key: 'Name', value: name}));


Use this.path instead of name as the default. name is a local logical identity and path is globally unique within the stack, and provides much more context.

moofish32

Just a couple of questions.

moofish32 · 2018-08-07T18:44:52Z

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

+     * The AWS resource tags to associate with the Subnet.
+     *
+     */
+    tags?: cdk.Tag[];


So should I never use cdk.Tag the { [key: string]: any } was my original thought. Then I found VpcNetworkProps used tags?: cdk.Tag[] so I pivoted. So two questions:

should we not use cdk.Tag in any L2s?

is there any reason this is not { [key: string]: string } I don't have a use case yet, but seems to rigid?

moofish32 · 2018-08-07T18:45:47Z

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

+    /**
+     * The AWS resource tags to associate with the Subnet.
+     */
+    tags?: cdk.Tag[];


map == { [key: string]: any}?

moofish32 · 2018-08-07T18:46:32Z

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

+    /**
+     * The tags for this subnet
+     */
+    private readonly tags: cdk.Token[] = [];


I have this in my work for #91 - thinking it might be better just to MVP that solution and only enable VPC first. I need to pivot away from cdk.Tag as mentioned above.

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-27T19:19:41Z

Closing this PR in favor #538

Construct authors can use `TagManager` 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. Fixes #91, Closes #458 (as obsolete)

moofish32 added 3 commits July 31, 2018 13:27

add the ability to tag subnets via VpcSubnetProps and

6e075e6

`SubnetConfiguration` interfaces; add a default `Name` tag if none are provided

updating r53 integ tests for subnet tagging defaults

7008c18

integration tests bump for rds

refactor to enable addTags to subnet; adding cast to VpcSubnet to avo…

c78a00f

…id changing VpcSubnetRef because that caused additional breaks in Java code

moofish32 commented Aug 1, 2018

View reviewed changes

PaulMaddox approved these changes Aug 2, 2018

View reviewed changes

moofish32 mentioned this pull request Aug 2, 2018

AWS Tags Support #91

Closed

eladb suggested changes Aug 6, 2018

View reviewed changes

moofish32 commented Aug 7, 2018

View reviewed changes

moofish32 mentioned this pull request Aug 9, 2018

Create A Tag Management for Constructs #538

Merged

moofish32 closed this Aug 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Add Ability to Tag Subnets #458

Feature Add Ability to Tag Subnets #458

moofish32 commented Jul 31, 2018 •

edited

Loading

moofish32 left a comment

moofish32 Aug 1, 2018

eladb Aug 6, 2018

moofish32 Aug 7, 2018

eladb Aug 7, 2018

moofish32 Aug 7, 2018

moofish32 Aug 1, 2018

eladb Aug 7, 2018

Doug-AWS Aug 7, 2018

eladb Aug 8, 2018 •

edited

Loading

Doug-AWS Aug 9, 2018

PaulMaddox commented Aug 2, 2018

eladb commented Aug 2, 2018

moofish32 commented Aug 2, 2018

moofish32 commented Aug 3, 2018

eladb Aug 6, 2018

eladb Aug 6, 2018

eladb Aug 6, 2018

eladb Aug 6, 2018

moofish32 Aug 7, 2018

eladb Aug 6, 2018

moofish32 Aug 7, 2018

eladb Aug 6, 2018

moofish32 left a comment

moofish32 Aug 7, 2018

moofish32 Aug 7, 2018

moofish32 Aug 7, 2018

moofish32 commented Aug 27, 2018

Feature Add Ability to Tag Subnets #458

Feature Add Ability to Tag Subnets #458

Conversation

moofish32 commented Jul 31, 2018 • edited Loading

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

eladb Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulMaddox commented Aug 2, 2018

eladb commented Aug 2, 2018

moofish32 commented Aug 2, 2018

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

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

moofish32 commented Aug 27, 2018

moofish32 commented Jul 31, 2018 •

edited

Loading

eladb Aug 8, 2018 •

edited

Loading