Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature Add Ability to Tag Subnets #458

Closed
wants to merge 3 commits into from
Closed

Feature Add Ability to Tag Subnets #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e075e6
add the ability to tag subnets via `VpcSubnetProps` and
moofish32 Jul 30, 2018
7008c18
updating r53 integ tests for subnet tagging defaults
moofish32 Jul 31, 2018
c78a00f
refactor to enable addTags to subnet; adding cast to VpcSubnet to avo…
moofish32 Aug 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion packages/@aws-cdk/aws-ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ export interface SubnetConfiguration {
* availability zone.
*/
name: string;

/**
* The AWS resource tags to associate with the Subnet.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant newline

*/
tags?: cdk.Tag[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


}

/**
Expand Down Expand Up @@ -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,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation?


switch (subnetConfig.subnetType) {
case SubnetType.Public:
Expand Down Expand Up @@ -452,6 +460,11 @@ export interface VpcSubnetProps {
* Defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Isolated.
*/
mapPublicIpOnLaunch?: boolean;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand All @@ -468,6 +481,11 @@ export class VpcSubnet extends VpcSubnetRef {
*/
public readonly subnetId: VpcSubnetId;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The routeTableId attached to this subnet.
*/
Expand All @@ -476,11 +494,20 @@ export class VpcSubnet extends VpcSubnetRef {
constructor(parent: cdk.Construct, name: string, props: VpcSubnetProps) {
super(parent, name);
this.availabilityZone = props.availabilityZone;
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}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} else {
this.tags = [new cdk.Token({key: 'Name', value: name})];
}
const subnet = new cloudformation.SubnetResource(this, 'Subnet', {
vpcId: props.vpcId,
cidrBlock: props.cidrBlock,
availabilityZone: props.availabilityZone,
mapPublicIpOnLaunch: props.mapPublicIpOnLaunch,
tags: this.tags,
});
this.subnetId = subnet.ref;
const table = new cloudformation.RouteTableResource(this, 'RouteTable', {
Expand All @@ -497,6 +524,10 @@ export class VpcSubnet extends VpcSubnetRef {
this.dependencyElements.push(subnet, table, routeAssoc);
}

public addTag(tag: cdk.Tag): void {
this.tags.push(new cdk.Token(tag));
}

protected addDefaultRouteToNAT(natGatewayId: cdk.Token) {
new cloudformation.RouteResource(this, `DefaultRoute`, {
routeTableId: this.routeTableId,
Expand Down
48 changes: 42 additions & 6 deletions packages/@aws-cdk/aws-ec2/test/integ.everything.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1a",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet1"
}
]
}
},
"VPCPublicSubnet1RouteTableFEE4B781": {
Expand Down Expand Up @@ -80,7 +86,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1b",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet2"
}
]
}
},
"VPCPublicSubnet2RouteTable6F1A15F1": {
Expand Down Expand Up @@ -142,7 +154,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1c",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet3"
}
]
}
},
"VPCPublicSubnet3RouteTable98AE0E14": {
Expand Down Expand Up @@ -204,7 +222,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1a",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet1"
}
]
}
},
"VPCPrivateSubnet1RouteTableBE8A6027": {
Expand Down Expand Up @@ -246,7 +270,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1b",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet2"
}
]
}
},
"VPCPrivateSubnet2RouteTable0A19E10E": {
Expand Down Expand Up @@ -288,7 +318,13 @@
"Ref": "VPCB9E5F0B4"
},
"AvailabilityZone": "test-region-1c",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet3"
}
]
}
},
"VPCPrivateSubnet3RouteTable192186F8": {
Expand Down
48 changes: 42 additions & 6 deletions packages/@aws-cdk/aws-ec2/test/integ.vpc.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1a",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet1"
}
]
}
},
"MyVpcPublicSubnet1RouteTableC46AB2F4": {
Expand Down Expand Up @@ -80,7 +86,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1b",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet2"
}
]
}
},
"MyVpcPublicSubnet2RouteTable1DF17386": {
Expand Down Expand Up @@ -142,7 +154,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1c",
"MapPublicIpOnLaunch": true
"MapPublicIpOnLaunch": true,
"Tags": [
{
"Key": "Name",
"Value": "PublicSubnet3"
}
]
}
},
"MyVpcPublicSubnet3RouteTable15028F08": {
Expand Down Expand Up @@ -204,7 +222,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1a",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet1"
}
]
}
},
"MyVpcPrivateSubnet1RouteTable8819E6E2": {
Expand Down Expand Up @@ -246,7 +270,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1b",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet2"
}
]
}
},
"MyVpcPrivateSubnet2RouteTableCEDCEECE": {
Expand Down Expand Up @@ -288,7 +318,13 @@
"Ref": "MyVpcF9F0CA6F"
},
"AvailabilityZone": "test-region-1c",
"MapPublicIpOnLaunch": false
"MapPublicIpOnLaunch": false,
"Tags": [
{
"Key": "Name",
"Value": "PrivateSubnet3"
}
]
}
},
"MyVpcPrivateSubnet3RouteTableB790927C": {
Expand Down
76 changes: 73 additions & 3 deletions packages/@aws-cdk/aws-ec2/test/test.vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { countResources, expect, haveResource } from '@aws-cdk/assert';
import { countResources, expect, haveResource, isSuperObject} from '@aws-cdk/assert';
import { AvailabilityZoneProvider, Stack } from '@aws-cdk/cdk';
import { Test } from 'nodeunit';
import { DefaultInstanceTenancy, SubnetType, VpcNetwork } from '../lib';
import { DefaultInstanceTenancy, SubnetType, VpcNetwork, VpcSubnet } from '../lib';

export = {

Expand Down Expand Up @@ -60,6 +60,12 @@ export = {
test.equal(vpc.publicSubnets.length, zones);
test.equal(vpc.privateSubnets.length, zones);
test.deepEqual(vpc.vpcId.resolve(), { Ref: 'TheVPC92636AB0' });
expect(stack).to(haveResource("AWS::EC2::Subnet", {
Tags: [ {Key: "Name", Value: "PrivateSubnet2"} ],
}));
expect(stack).to(haveResource("AWS::EC2::Subnet", {
Tags: [ {Key: "Name", Value: "PublicSubnet1"} ],
}));
test.done();
},

Expand All @@ -78,6 +84,9 @@ export = {
expect(stack).to(haveResource("AWS::EC2::Subnet", {
MapPublicIpOnLaunch: false
}));
expect(stack).to(haveResource("AWS::EC2::Subnet", {
Tags: [ {Key: "Name", Value: "IsolatedSubnet3"} ],
}));
test.done();
},

Expand Down Expand Up @@ -119,23 +128,46 @@ export = {
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.Public,
tags: [{key: 'SubnetType', value: 'Public'}],
},
{
cidrMask: 24,
name: 'application',
subnetType: SubnetType.Private,
tags: [{key: 'Compliance', value: 'PCI'}],
},
{
cidrMask: 28,
name: 'rds',
subnetType: SubnetType.Isolated,
tags: [{key: 'Billing', value: 'DatabaseTeam'}],
}
],
maxAZs: 3
});
expect(stack).to(countResources("AWS::EC2::InternetGateway", 1));
expect(stack).to(countResources("AWS::EC2::NatGateway", zones));
expect(stack).to(countResources("AWS::EC2::Subnet", 9));
for (let i = 1; i < 4; i++) {
expect(stack).to(haveResource('AWS::EC2::Subnet',
hasTags([
{ Key: 'Name', Value: `ingressSubnet${i}` },
{ Key: 'SubnetType', Value: 'Public' },
])
));
expect(stack).to(haveResource('AWS::EC2::Subnet',
hasTags([
{ Key: 'Name', Value: `applicationSubnet${i}` },
{ Key: 'Compliance', Value: 'PCI' },
])
));
expect(stack).to(haveResource('AWS::EC2::Subnet',
hasTags([
{ Key: 'Name', Value: `rdsSubnet${i}` },
{ Key: 'Billing', Value: 'DatabaseTeam' },
])
));
}
for (let i = 0; i < 6; i++) {
expect(stack).to(haveResource("AWS::EC2::Subnet", {
CidrBlock: `10.0.${i}.0/24`
Expand Down Expand Up @@ -216,7 +248,7 @@ export = {
},
"with maxAZs set to 2"(test: Test) {
const stack = getTestStack();
new VpcNetwork(stack, 'VPC', { maxAZs: 2 });
const myVpc = new VpcNetwork(stack, 'VPC', { maxAZs: 2 });
expect(stack).to(countResources("AWS::EC2::Subnet", 4));
expect(stack).to(countResources("AWS::EC2::Route", 4));
for (let i = 0; i < 4; i++) {
Expand All @@ -228,6 +260,20 @@ export = {
DestinationCidrBlock: '0.0.0.0/0',
NatGatewayId: { },
}));
expect(stack).notTo(haveResource("AWS::EC2::Subnet",
hasTags([
{ Key: 'AddedTag', Value: 'NewThing' },
])
));
(myVpc.publicSubnets as VpcSubnet[]).forEach((subnet) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@eladb eladb Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

subnet.addTag({key: "AddedTag", value: "NewThing"});
});
expect(stack).to(haveResource('AWS::EC2::Subnet',
hasTags([
{ Key: 'AddedTag', Value: 'NewThing' },
])
));

test.done();
},
"with natGateway set to 1"(test: Test) {
Expand Down Expand Up @@ -262,3 +308,27 @@ export = {
function getTestStack(): Stack {
return new Stack(undefined, 'TestStack', { env: { account: '123456789012', region: 'us-east-1' } });
}

function hasTags(expectedTags: Array<{Key: string, Value: string}>): (props: any) => boolean {
return (props: any) => {
try {
const tags = props.Tags;
const actualTags = tags.filter( (tag: {Key: string, Value: string}) => {
for (const expectedTag of expectedTags) {
if (isSuperObject(expectedTag, tag)) {
return true;
} else {
continue;
}
}
// no values in array so expecting empty
return false;
});
return actualTags.length === expectedTags.length;
} catch (e) {
// tslint:disable-next-line:no-console
console.error('Invalid Tags array in ', props);
throw e;
}
};
}
Loading