-
Notifications
You must be signed in to change notification settings - Fork 4k
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(ec2-alpha): readme updates, new unit tests, logic update #33086
Changes from 2 commits
e7125f3
0de3fd3
9f997a6
515c6fb
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,10 +1,4 @@ | ||
const baseConfig = require('@aws-cdk/cdk-build-tools/config/jest.config'); | ||
module.exports = { | ||
...baseConfig, | ||
coverageThreshold: { | ||
global: { | ||
statements: 75, | ||
branches: 63, | ||
}, | ||
}, | ||
};; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -511,7 +511,7 @@ export class Ipam extends Resource { | |
if (props?.ipamName) { | ||
Tags.of(this).add(NAME_TAG, props.ipamName); | ||
} | ||
if (!props?.operatingRegion && !Stack.of(this).region) { | ||
if (props?.operatingRegion && (props.operatingRegion.length === 0)) { | ||
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. nit: The 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. addressed in latest rev but this would be breaking change which should be okay for alpha, added in PR description BREAKING CHANGE: operatingRegion property under IPAM class is now renamed to operatingRegions. |
||
throw new Error('Please provide at least one operating region'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -605,12 +605,8 @@ export class RouteTargetType { | |
readonly endpoint?: IVpcEndpoint; | ||
|
||
constructor(props: RouteTargetProps) { | ||
if ((props.gateway && props.endpoint) || (!props.gateway && !props.endpoint)) { | ||
throw new Error('Exactly one of `gateway` or `endpoint` must be specified.'); | ||
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 see this is moved into the 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. While adding unit tests, stack was not throwing expected error while using addRoute method with both endpoint and target as an input, so moved it from RouteTargetType class to |
||
} else { | ||
this.gateway = props.gateway; | ||
this.endpoint = props.endpoint; | ||
} | ||
this.gateway = props.gateway; | ||
this.endpoint = props.endpoint; | ||
} | ||
} | ||
|
||
|
@@ -729,6 +725,10 @@ export class Route extends Resource implements IRouteV2 { | |
if (this.target.gateway?.routerType === RouterType.EGRESS_ONLY_INTERNET_GATEWAY && isDestinationIpv4) { | ||
throw new Error('Egress only internet gateway does not support IPv4 routing'); | ||
} | ||
|
||
if ((props.target.gateway && props.target.endpoint) || (!props.target.gateway && !props.target.endpoint)) { | ||
throw new Error('Exactly one of `gateway` or `endpoint` must be specified.'); | ||
} | ||
this.targetRouterType = this.target.gateway ? this.target.gateway.routerType : RouterType.VPC_ENDPOINT; | ||
// Gateway generates route automatically via its RouteTable, thus we don't need to generate the resource for it | ||
if (!(this.target.endpoint instanceof GatewayVpcEndpoint)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,13 @@ export interface InternetGatewayOptions{ | |
* @default - provisioned without a resource name | ||
*/ | ||
readonly internetGatewayName?: string; | ||
|
||
/** | ||
* List of subnets where route to IGW will be added | ||
* | ||
* @default - route created for all subnets with Type `SubnetType.Public` | ||
*/ | ||
readonly subnets?: SubnetSelection[]; | ||
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. nit: Naming this 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. actually, tried to keep it consistent per the functions in main lib with the current options in main lib for adding interfaces and endpoints, https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/vpc-endpoint.ts#L110 , where mySubnet is a new subnet defined using SubnetV2. myVpc.addInternetGateway({ |
||
} | ||
|
||
/** | ||
|
@@ -437,9 +444,14 @@ export abstract class VpcV2Base extends Resource implements IVpcV2 { | |
}; | ||
|
||
if (options?.subnets) { | ||
// Use Set to ensure unique subnets | ||
const processedSubnets = new Set<string>(); | ||
const subnets = flatten(options.subnets.map(s => this.selectSubnets(s).subnets)); | ||
subnets.forEach((subnet) => { | ||
this.createEgressRoute(subnet, egw, options.destination); | ||
if (!processedSubnets.has(subnet.node.id)) { | ||
this.createEgressRoute(subnet, egw, options.destination); | ||
processedSubnets.add(subnet.node.id); | ||
} | ||
}); | ||
} | ||
} | ||
|
@@ -476,9 +488,23 @@ export abstract class VpcV2Base extends Resource implements IVpcV2 { | |
this._internetConnectivityEstablished.add(igw); | ||
this._internetGatewayId = igw.routerTargetId; | ||
|
||
// If there are no public subnets defined, no default route will be added | ||
if (this.publicSubnets) { | ||
this.publicSubnets.forEach( (s) => this.addDefaultInternetRoute(s, igw, options)); | ||
// Add routes for subnets defined as an input | ||
if (options?.subnets) { | ||
// Use Set to ensure unique subnets | ||
const processedSubnets = new Set<string>(); | ||
const subnets = flatten(options.subnets.map(s => this.selectSubnets(s).subnets)); | ||
subnets.forEach((subnet) => { | ||
if (!processedSubnets.has(subnet.node.id)) { | ||
if (!this.publicSubnets.includes(subnet)) { | ||
Annotations.of(this).addWarningV2('InternetGatewayWarning', | ||
`Subnet ${subnet.node.id} is not a public subnet. Internet Gateway should be added only to public subnets.`); | ||
} | ||
this.addDefaultInternetRoute(subnet, igw, options); | ||
processedSubnets.add(subnet.node.id); | ||
}; | ||
}); // If there are no input subnets defined, default route will be added to all public subnets | ||
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. My understanding is that if a subnet has a route to the internet, then it becomes a public subnet. So should we allow adding internet gateway to private subnets? If we do, should we warn the users so that they do not accidentally make their subnets, intended to be private, public? 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. Yes, this was my understanding as well but after talking to service team there can be cases with VPNGW type of attachment where customer might be leveraging their on-premises network to route the traffic to internet. 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.
And in such case, it won't be a private subnet? Is it correct to say that "Private Subnet" is a concept to describe a subnet that is configured to not able to access the internet? I am thinking if we should keep the public vs private subnet concept in CDK since, say, a private subnet may become public due to drifting. Anyways, this is not a blocking comment. We can discuss further offline. Thank you for explaining. |
||
} else if (!options?.subnets && this.publicSubnets) { | ||
this.publicSubnets.forEach((publicSubnets) => this.addDefaultInternetRoute(publicSubnets, igw, options)); | ||
} | ||
} | ||
|
||
|
@@ -488,10 +514,6 @@ export abstract class VpcV2Base extends Resource implements IVpcV2 { | |
*/ | ||
private addDefaultInternetRoute(subnet: ISubnetV2, igw: InternetGateway, options?: InternetGatewayOptions): void { | ||
|
||
if (subnet.subnetType !== SubnetType.PUBLIC) { | ||
throw new Error('No public subnets defined to add route for internet gateway'); | ||
} | ||
|
||
// Add default route to IGW for IPv6 | ||
if (subnet.ipv6CidrBlock) { | ||
new Route(this, `${subnet.node.id}-DefaultIPv6Route`, { | ||
|
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.
Is it possible for
ipamName
to be a token?