-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
fix(aws-ec2): VPC properly uses complex subnet config #610
Changes from 2 commits
319e0cc
5b8bc68
4a4566c
4e2d510
60bcbb9
de44e5a
a5ffd3c
02aa211
ccd22fb
c01e347
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,41 @@ | ||
import { Construct, IDependable, Output, StringListOutput, Token } from "@aws-cdk/cdk"; | ||
|
||
/** | ||
* The type of Subnet | ||
*/ | ||
export enum SubnetType { | ||
/** | ||
* Isolated Subnets do not route Outbound traffic | ||
* | ||
* This can be good for subnets with RDS or | ||
* Elasticache endpoints | ||
*/ | ||
Isolated = 1, | ||
|
||
/** | ||
* Subnet that routes to the internet, but not vice versa. | ||
* | ||
* Instances in a private subnet can connect to the Internet, but will not | ||
* allow connections to be initiated from the Internet. | ||
* | ||
* Outbound traffic will be routed via a NAT Gateway. Preference being in | ||
* the same AZ, but if not available will use another AZ. This is common for | ||
* experimental cost conscious accounts or accounts where HA outbound | ||
* traffic is not needed. | ||
*/ | ||
Private = 2, | ||
|
||
/** | ||
* Subnet connected to the Internet | ||
* | ||
* Instances in a Public subnet can connect to the Internet and can be | ||
* connected to from the Internet as long as they are launched with public IPs. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a reference on how to launch an instance with a public IP |
||
* | ||
* Public subnets route outbound traffic via an Internet Gateway. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... in the same AZ? |
||
*/ | ||
Public = 3 | ||
} | ||
|
||
/** | ||
* Customize how instances are placed inside a VPC | ||
* | ||
|
@@ -8,13 +44,13 @@ import { Construct, IDependable, Output, StringListOutput, Token } from "@aws-cd | |
*/ | ||
export interface VpcPlacementStrategy { | ||
/** | ||
* Whether to use the VPC's public subnets to start instances | ||
* What subnet type to place the instances in | ||
* | ||
* If false, the instances are started in the private subnets. | ||
* By default, the instances are placed in the private subnets. | ||
* | ||
* @default false | ||
* @default SubnetType.Private | ||
*/ | ||
usePublicSubnets?: boolean; | ||
subnetsToUse?: SubnetType; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our VPCs commonly have an egress Any opposition to adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel I would prefer tags or something similar. It doesn't have to be done all at once, we can add support for tag selection after that PR has landed. In the mean time, we can also allow selecting by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on names or anything that gives us the ability to select to specific subnet when type is not distinct |
||
} | ||
|
||
/** | ||
|
@@ -43,6 +79,16 @@ export abstract class VpcNetworkRef extends Construct implements IDependable { | |
*/ | ||
public abstract readonly privateSubnets: VpcSubnetRef[]; | ||
|
||
/** | ||
* List of isolated subnets in this VPC | ||
*/ | ||
public abstract readonly isolatedSubnets: VpcSubnetRef[]; | ||
|
||
/** | ||
* AZs for this VPC | ||
*/ | ||
public abstract readonly availabilityZones: string[]; | ||
|
||
/** | ||
* Parts of the VPC that constitute full construction | ||
*/ | ||
|
@@ -51,9 +97,13 @@ export abstract class VpcNetworkRef extends Construct implements IDependable { | |
/** | ||
* Return the subnets appropriate for the placement strategy | ||
*/ | ||
public subnets(placement?: VpcPlacementStrategy): VpcSubnetRef[] { | ||
if (!placement) { return this.privateSubnets; } | ||
return placement.usePublicSubnets ? this.publicSubnets : this.privateSubnets; | ||
public subnets(placement: VpcPlacementStrategy = {}): VpcSubnetRef[] { | ||
if (placement.subnetsToUse === undefined) { return this.privateSubnets; } | ||
return { | ||
[SubnetType.Isolated]: this.isolatedSubnets, | ||
[SubnetType.Private]: this.privateSubnets, | ||
[SubnetType.Public]: this.publicSubnets, | ||
}[placement.subnetsToUse]; | ||
} | ||
|
||
/** | ||
|
@@ -62,11 +112,17 @@ export abstract class VpcNetworkRef extends Construct implements IDependable { | |
public export(): VpcNetworkRefProps { | ||
return { | ||
vpcId: new Output(this, 'VpcId', { value: this.vpcId }).makeImportValue(), | ||
availabilityZones: this.publicSubnets.map(s => s.availabilityZone), | ||
publicSubnetIds: new StringListOutput(this, 'PublicSubnetIDs', { values: this.publicSubnets.map(s => s.subnetId) }).makeImportValues(), | ||
privateSubnetIds: new StringListOutput(this, 'PrivateSubnetIDs', { values: this.privateSubnets.map(s => s.subnetId) }).makeImportValues(), | ||
availabilityZones: this.availabilityZones, | ||
publicSubnetIds: this.exportSubnetIds('PublicSubnetIDs', this.publicSubnets), | ||
privateSubnetIds: this.exportSubnetIds('PrivateSubnetIDs', this.privateSubnets), | ||
isolatedSubnetIds: this.exportSubnetIds('IsolatedSubnetIDs', this.isolatedSubnets), | ||
}; | ||
} | ||
|
||
private exportSubnetIds(name: string, subnets: VpcSubnetRef[]): Token[] | undefined { | ||
if (subnets.length === 0) { return undefined; } | ||
return new StringListOutput(this, name, { values: subnets.map(s => s.subnetId) }).makeImportValues(); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -88,27 +144,50 @@ class ImportedVpcNetwork extends VpcNetworkRef { | |
*/ | ||
public readonly privateSubnets: VpcSubnetRef[]; | ||
|
||
/** | ||
* List of isolated subnets in this VPC | ||
*/ | ||
public readonly isolatedSubnets: VpcSubnetRef[]; | ||
|
||
/** | ||
* AZs for this VPC | ||
*/ | ||
public readonly availabilityZones: string[]; | ||
|
||
constructor(parent: Construct, name: string, props: VpcNetworkRefProps) { | ||
super(parent, name); | ||
|
||
this.vpcId = props.vpcId; | ||
this.availabilityZones = props.availabilityZones; | ||
|
||
const privateSubnetIds = props.privateSubnetIds || []; | ||
const publicSubnetIds = props.publicSubnetIds || []; | ||
const isolatedSubnetIds = props.isolatedSubnetIds || []; | ||
|
||
if (props.availabilityZones.length !== props.publicSubnetIds.length) { | ||
throw new Error('Availability zone and public subnet ID arrays must be same length'); | ||
if (publicSubnetIds.length > 0 && this.availabilityZones.length !== publicSubnetIds.length) { | ||
throw new Error('Must have Public subnet for every AZ'); | ||
} | ||
|
||
if (props.availabilityZones.length !== props.privateSubnetIds.length) { | ||
throw new Error('Availability zone and private subnet ID arrays must be same length'); | ||
if (privateSubnetIds.length > 0 && this.availabilityZones.length !== privateSubnetIds.length) { | ||
throw new Error('Must have Private subnet for every AZ'); | ||
} | ||
|
||
if (isolatedSubnetIds.length > 0 && this.availabilityZones.length !== isolatedSubnetIds.length) { | ||
throw new Error('Must have Isolated subnet for every AZ'); | ||
} | ||
|
||
const n = props.availabilityZones.length; | ||
this.publicSubnets = range(n).map(i => VpcSubnetRef.import(this, `PublicSubnet${i}`, { | ||
availabilityZone: props.availabilityZones[i], | ||
subnetId: props.publicSubnetIds[i] | ||
availabilityZone: this.availabilityZones[i], | ||
subnetId: publicSubnetIds[i] | ||
})); | ||
this.privateSubnets = range(n).map(i => VpcSubnetRef.import(this, `PrivateSubnet${i}`, { | ||
availabilityZone: props.availabilityZones[i], | ||
subnetId: props.privateSubnetIds[i] | ||
availabilityZone: this.availabilityZones[i], | ||
subnetId: privateSubnetIds[i] | ||
})); | ||
this.isolatedSubnets = range(n).map(i => VpcSubnetRef.import(this, `IsolatedSubnet${i}`, { | ||
availabilityZone: this.availabilityZones[i], | ||
subnetId: isolatedSubnetIds[i] | ||
})); | ||
} | ||
} | ||
|
@@ -135,14 +214,21 @@ export interface VpcNetworkRefProps { | |
* | ||
* Must match the availability zones and private subnet ids in length and order. | ||
*/ | ||
publicSubnetIds: VpcSubnetId[]; | ||
publicSubnetIds?: VpcSubnetId[]; | ||
|
||
/** | ||
* List of private subnet IDs, one for every subnet | ||
* | ||
* Must match the availability zones and public subnet ids in length and order. | ||
*/ | ||
privateSubnetIds: VpcSubnetId[]; | ||
privateSubnetIds?: VpcSubnetId[]; | ||
|
||
/** | ||
* List of isolated subnet IDs, one for every subnet | ||
* | ||
* Must match the availability zones and public subnet ids in length and order. | ||
*/ | ||
isolatedSubnetIds?: VpcSubnetId[]; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add information in the the last paragraph that explains how to achieve this setup.