-
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
feat(vpcV2): support custom route and subnet #30538
Conversation
… into vpc_full_control
… into vpc_full_control
8f4dd9b
to
1d0e247
Compare
aaccea2
to
c9e0251
Compare
/** | ||
* The operating Regions for an IPAM. | ||
* Operating Regions are AWS Regions where the IPAM is allowed to manage IP address CIDRs | ||
* For more information about operating Regions, see [Create an IPAM](https://docs.aws.amazon.com//vpc/latest/ipam/create-ipam.html) in the *Amazon VPC IPAM User Guide* . |
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.
we need to check how this will look like in Docs
public provisionCidr(id: string, options: IpamPoolCidrProvisioningOptions): CfnIPAMPoolCidr { | ||
const cidr = new CfnIPAMPoolCidr(this, id, { | ||
...options, | ||
ipamPoolId: this.ipamPoolId, | ||
}); | ||
this.ipamCidrs.push(cidr); | ||
return cidr; | ||
} |
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.
[non blocking] should we add validation to the input CIDR ?
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.
This can be used for both ipv4 and ipv6 but mainly used to provision CIDRs with netmasklength as mentioned in the documentation
Can be used for provisioning Amazon-provided IPv6 CIDRs to top-level pools and for provisioning CIDRs to pools with source pools. Cannot be used to provision BYOIP CIDRs to top-level pools. "NetmaskLength" or "Cidr" is required.
So I believe the validation should be around netmask length, added a point to think about all the validations in IPAM class.
*/ | ||
public readonly operatingRegions: string[]; | ||
|
||
constructor(scope: Construct, id: string, props?: IpamProps) { |
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.
i believe the node.defaultChild
of this construct is the CfnIpam resource.
*/ | ||
public readonly operatingRegions: string[]; | ||
|
||
constructor(scope: Construct, id: string, props?: IpamProps) { |
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.
[to think about] shall we add a validation that number of Ipam created per stack is only one ?
* gateway and cannot be specified with a private NAT gateway. | ||
* @default none | ||
*/ | ||
readonly allocationId?: string; |
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.
so I think we should add a constructor for it.
if (this.targetRouterType == RouterType.GATEWAY) { | ||
if (this.target.gateway instanceof InternetGateway) { | ||
new CfnVPCGatewayAttachment(this, 'GWAttachment', { | ||
vpcId: this.target.gateway.vpcId, | ||
internetGatewayId: this.target.gateway.routerTargetId, | ||
}); | ||
} else if (this.target.gateway instanceof VPNGateway) { | ||
new CfnVPCGatewayAttachment(this, 'GWAttachment', { | ||
vpcId: this.target.gateway.vpcId, | ||
vpnGatewayId: this.target.gateway.routerTargetId, | ||
}); | ||
} |
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.
I know this will be done in next phases, but I want to keep track of the comments that will be done later to facilitate my review next time :)
The attachment should be moved to the place where we create the Gateways.
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.
Thanks @moelasmar , that's why i'm keeping this comment as unresolved so we could revisit this in next implementation along with adding in feature plan
9ff92f8
to
24d2c15
Compare
a3d77e6
to
0b7a56f
Compare
6bcf8b7
to
daa15c7
Compare
*/ | ||
class IpamIpv4 implements IIpAddresses { | ||
|
||
constructor(private readonly props: IpamOptions, private readonly ipProps?: SecondaryAddressProps) { |
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.
why do you make a new object for the name property ?
… into vpc_full_control
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes RFC#507.
Issue#5927
Tracking Ticket #30762
Reason for this change
This PR implements below RFC for Full Control VPC configuration
Implementing RFC Full Control VPC
Description of changes
A new Experimental API for VPC called VPCv2
Lifecycle Doc: https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md
Next Steps:
Iterate on the API with the feedback from community and team to make it ergonomic.
Close on the features listed in tracking ticket
Will follow the exit criteria for this experimental API as outlined in below doc:
https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md
Description of how you validated changes
Added unit tests with current coverage ~70%
Added integration tests for subnet, vpc and routing features.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license