Repository navigation
feat(ec2): support IPAM CIDR allocation for Subnet - #38810
gauranshahuja wants to merge 8 commits into
Conversation
|
Exemption Request: I could not deploy the integration test (no AWS account available), so the snapshot for integ.subnet-ipam.ts is not included. The test is written per INTEGRATION_TESTS.md; could a maintainer run it to generate the snapshot? Happy to adjust the test if the pool setup needs changing. |
# Conflicts: # CHANGELOG.v2.alpha.md # CHANGELOG.v2.md # packages/@aws-cdk/metrics-facade-alpha/package.json # packages/@aws-cdk/mixins-preview/package.json # packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts # packages/aws-cdk-lib/package.json # tools/@aws-cdk/spec2cdk/package.json # version.v2.json # yarn.lock
|
Automated review A maintainer will still review this — treat the notes below as a starting point. This PR adds IPAM CIDR allocation to the L2 🔴 0 blocking · 🟡 1 recommended · ⚪ 2 optional Files with findings (2)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| import * as cdk from 'aws-cdk-lib'; | ||
| import { ExpectedResult, IntegTest } from '@aws-cdk/integ-tests-alpha'; | ||
| import { CfnIPAM, CfnIPAMPool, IpAddresses, Subnet, Vpc } from 'aws-cdk-lib/aws-ec2'; | ||
| import { EC2_RESTRICT_DEFAULT_SECURITY_GROUP } from 'aws-cdk-lib/cx-api'; | ||
|
|
||
| /* | ||
| * Stack verification steps: | ||
| * * The subnet is created without a CidrBlock property; IPAM allocates its CIDR at deploy time | ||
| * * The assertion checks that the allocated CIDR is the first /24 of the pool's provisioned range | ||
| * | ||
| * ### MANUAL CLEAN UP REQUIRED ### | ||
| * | ||
| * As in integ.vpc-ipam.ts, the IPAM and the pool are retained after the test run and must be | ||
| * deleted manually. | ||
| */ | ||
|
|
||
| const app = new cdk.App(); | ||
| const stack = new cdk.Stack(app, 'aws-cdk-ec2-ipam-subnet'); | ||
| stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false); | ||
|
|
||
| const ipam = new CfnIPAM(stack, 'IPAM', { | ||
| operatingRegions: [ | ||
| { regionName: stack.region }, | ||
| ], | ||
| tags: [{ | ||
| key: 'stack', | ||
| value: stack.stackId, | ||
| }], | ||
| }); | ||
| ipam.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); | ||
|
|
||
| // A VPC with a concrete CIDR and no subnets of its own | ||
| const vpc = new Vpc(stack, 'Vpc', { | ||
| ipAddresses: IpAddresses.cidr('10.0.0.0/16'), | ||
| subnetConfiguration: [], | ||
| }); | ||
|
|
||
| // A pool that plans the VPC's address space for subnets: it provisions the VPC CIDR | ||
| const pool = new CfnIPAMPool(stack, 'Pool', { | ||
| description: 'Subnet pool for the VPC', | ||
| addressFamily: 'ipv4', | ||
| autoImport: false, | ||
| locale: stack.region, | ||
| ipamScopeId: ipam.attrPrivateDefaultScopeId, | ||
| provisionedCidrs: [{ | ||
| cidr: '10.0.0.0/16', | ||
| }], | ||
| }); | ||
| pool.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); | ||
|
|
||
| const subnet = new Subnet(stack, 'IpamSubnet', { | ||
| vpcId: vpc.vpcId, | ||
| availabilityZone: vpc.availabilityZones[0], | ||
| ipv4IpamAllocation: { | ||
| ipamPool: pool, | ||
| netmaskLength: 24, | ||
| }, | ||
| }); | ||
|
|
||
| const integ = new IntegTest(app, 'SubnetIpam', { | ||
| testCases: [stack], | ||
| allowDestroy: ['EC2::IPAM'], | ||
| }); | ||
|
|
||
| // The first allocation from a fresh pool is the lowest /24 of the provisioned range | ||
| integ.assertions.awsApiCall('EC2', 'describeSubnets', { | ||
| SubnetIds: [subnet.subnetId], | ||
| }).expect(ExpectedResult.objectLike({ | ||
| Subnets: [ | ||
| { | ||
| CidrBlock: '10.0.0.0/24', | ||
| }, | ||
| ], | ||
| })); |
There was a problem hiding this comment.
🟡 Recommended — This integration test's source is added, but no committed *.snapshot/ directory ships with it (the change contains only the four source and doc files). An integration test's core value is running cdk synth and diffing against the stored Cloud Assembly snapshot (INTEGRATION_TESTS.md); without a snapshot the test does no regression detection and the change is not verified to deploy. That matters especially here because the feature exercises previously-unused CFN properties (Ipv4IpamPoolId/Ipv4NetmaskLength on AWS::EC2::Subnet) and the assertion hard-codes CidrBlock: '10.0.0.0/24', an outcome only a real deploy can confirm.
Suggested change: Deploy and record the snapshot rather than granting a no-snapshot exemption — a maintainer can run yarn integ integ.subnet-ipam.js --update-on-failed to deploy the stack and commit the resulting *.snapshot/ directory, which also confirms the hard-coded 10.0.0.0/24 allocation assumption holds.
There was a problem hiding this comment.
I'm not able to run the real deployment for this one: the test needs an IPAM in the Advanced Tier, and I don't have an account where I can run that. Following CONTRIBUTING.md ("What if you cannot run integration tests"), could a maintainer run yarn integ integ.subnet-ipam.js --update-on-failed for this PR? I haven't used --dry-run or written the snapshot by hand.
The IPAM and the pool are retained, so the cleanup is aws ec2 delete-ipam --ipam-id <ipam-id> --cascade afterwards (also in the test's header comment).
There was a problem hiding this comment.
Thanks, that's right. The changes look good. I'll run the deploy and push the snapshot here.
| // A pool that plans the VPC's address space for subnets: it provisions the VPC CIDR | ||
| const pool = new CfnIPAMPool(stack, 'Pool', { | ||
| description: 'Subnet pool for the VPC', | ||
| addressFamily: 'ipv4', | ||
| autoImport: false, | ||
| locale: stack.region, | ||
| ipamScopeId: ipam.attrPrivateDefaultScopeId, | ||
| provisionedCidrs: [{ | ||
| cidr: '10.0.0.0/16', | ||
| }], | ||
| }); | ||
| pool.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); | ||
|
|
There was a problem hiding this comment.
For subnets, IPAM allocates from a resource planning pool: a pool whose sourceResource points at the VPC (tutorial, SourceResource). Without it, the subnet is expected to fail at deploy:
const pool = new CfnIPAMPool(stack, 'Pool', {
description: 'Resource planning pool for the VPC',
addressFamily: 'ipv4',
autoImport: false,
locale: stack.region,
ipamScopeId: ipam.attrPrivateDefaultScopeId,
sourceResource: {
resourceId: vpc.vpcId,
resourceOwner: stack.account,
resourceRegion: stack.region,
resourceType: 'vpc',
},
provisionedCidrs: [{ cidr: '10.0.0.0/16' }],
});The provisioned CIDR has to match the VPC's CIDR, and 10.0.0.0/16 already does.
There was a problem hiding this comment.
Done: the pool in the integ test is now a resource planning pool for the VPC (sourceResource with resourceType: 'vpc' and the VPC's id, account and Region). I also set tier: 'advanced' on the CfnIPAM, since the pool is in the private scope.
| }); | ||
| ``` | ||
|
|
||
| The CIDR block allocated from the pool must lie within the CIDR of the VPC. To use a pool that is not defined in your CDK app (for example one shared with your account through AWS RAM), reference it by ID with `ec2.CfnIPAMPool.fromIpamPoolId(this, 'Pool', 'ipam-pool-0123456789abcdef0')`. If the pool's address space is provisioned through separate `CfnIPAMPoolCidr` resources, add a dependency from the subnet on them so the pool has space to allocate from when the subnet is created. |
There was a problem hiding this comment.
The CIDR block allocated from the pool must lie within the CIDR of the VPC" is true, but users will trip on the pool type. It has to be a resource planning pool for this VPC, and private-scope pools need the IPAM Advanced Tier. Showing the pool in the example makes it copyable:
declare const vpc: ec2.Vpc;
declare const ipam: ec2.CfnIPAM;
const pool = new ec2.CfnIPAMPool(this, 'SubnetPool', {
addressFamily: 'ipv4',
ipamScopeId: ipam.attrPrivateDefaultScopeId,
locale: this.region,
sourceResource: {
resourceId: vpc.vpcId,
resourceOwner: this.account,
resourceRegion: this.region,
resourceType: 'vpc',
},
provisionedCidrs: [{ cidr: vpc.vpcCidrBlock }],
});
new ec2.Subnet(this, 'IpamSubnet', {
vpcId: vpc.vpcId,
availabilityZone: vpc.availabilityZones[0],
ipv4IpamAllocation: { ipamPool: pool, netmaskLength: 24 },
});The ipamPool JSDoc in lib/vpc.ts has the same sentence and could get the same note.
There was a problem hiding this comment.
Done: the README example now creates the resource planning pool from the VPC, and the text after it says the pool must be a resource planning pool for the subnet's VPC and that private-scope pools require the IPAM Advanced Tier. I added the same note to the ipamPool docstring in SubnetIpamAllocation.
| test.each([15, 29])('fails for ipv4IpamAllocation.netmaskLength /%d outside the /16-/28 subnet range', (netmaskLength) => { | ||
| // GIVEN | ||
| const stack = new Stack(); | ||
| const pool = CfnIPAMPool.fromIpamPoolId(stack, 'Pool', ipamPoolId); | ||
|
|
||
| // THEN | ||
| expect(() => new Subnet(stack, 'Subnet', { | ||
| vpcId: 'vpc-1234', | ||
| availabilityZone: 'dummy1a', | ||
| ipv4IpamAllocation: { ipamPool: pool, netmaskLength }, | ||
| })).toThrow(/'ipv4IpamAllocation.netmaskLength' must be between 16 and 28/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
nit: this checks that /15 and /29 are rejected. Checking that /16 and /28 are accepted would catch an off-by-one:
test.each([16, 28])('accepts ipv4IpamAllocation.netmaskLength /%d', (netmaskLength) => {
const stack = new Stack();
const pool = CfnIPAMPool.fromIpamPoolId(stack, 'Pool', ipamPoolId);
new Subnet(stack, 'Subnet', {
vpcId: 'vpc-1234',
availabilityZone: 'dummy1a',
ipv4IpamAllocation: { ipamPool: pool, netmaskLength },
});
Template.fromStack(stack).hasResourceProperties('AWS::EC2::Subnet', {
Ipv4NetmaskLength: netmaskLength,
});
});There was a problem hiding this comment.
Added test.each([16, 28]) that creates the subnet at each bound and checks Ipv4NetmaskLength in the template.
| import * as cdk from 'aws-cdk-lib'; | ||
| import { ExpectedResult, IntegTest } from '@aws-cdk/integ-tests-alpha'; | ||
| import { CfnIPAM, CfnIPAMPool, IpAddresses, Subnet, Vpc } from 'aws-cdk-lib/aws-ec2'; | ||
| import { EC2_RESTRICT_DEFAULT_SECURITY_GROUP } from 'aws-cdk-lib/cx-api'; | ||
|
|
||
| /* | ||
| * Stack verification steps: | ||
| * * The subnet is created without a CidrBlock property; IPAM allocates its CIDR at deploy time | ||
| * * The assertion checks that the allocated CIDR is the first /24 of the pool's provisioned range | ||
| * | ||
| * ### MANUAL CLEAN UP REQUIRED ### | ||
| * | ||
| * As in integ.vpc-ipam.ts, the IPAM and the pool are retained after the test run and must be | ||
| * deleted manually. | ||
| */ | ||
|
|
||
| const app = new cdk.App(); | ||
| const stack = new cdk.Stack(app, 'aws-cdk-ec2-ipam-subnet'); | ||
| stack.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false); | ||
|
|
||
| const ipam = new CfnIPAM(stack, 'IPAM', { | ||
| operatingRegions: [ | ||
| { regionName: stack.region }, | ||
| ], | ||
| tags: [{ | ||
| key: 'stack', | ||
| value: stack.stackId, | ||
| }], | ||
| }); | ||
| ipam.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); | ||
|
|
||
| // A VPC with a concrete CIDR and no subnets of its own | ||
| const vpc = new Vpc(stack, 'Vpc', { | ||
| ipAddresses: IpAddresses.cidr('10.0.0.0/16'), | ||
| subnetConfiguration: [], | ||
| }); | ||
|
|
||
| // A pool that plans the VPC's address space for subnets: it provisions the VPC CIDR | ||
| const pool = new CfnIPAMPool(stack, 'Pool', { | ||
| description: 'Subnet pool for the VPC', | ||
| addressFamily: 'ipv4', | ||
| autoImport: false, | ||
| locale: stack.region, | ||
| ipamScopeId: ipam.attrPrivateDefaultScopeId, | ||
| provisionedCidrs: [{ | ||
| cidr: '10.0.0.0/16', | ||
| }], | ||
| }); | ||
| pool.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); | ||
|
|
||
| const subnet = new Subnet(stack, 'IpamSubnet', { | ||
| vpcId: vpc.vpcId, | ||
| availabilityZone: vpc.availabilityZones[0], | ||
| ipv4IpamAllocation: { | ||
| ipamPool: pool, | ||
| netmaskLength: 24, | ||
| }, | ||
| }); | ||
|
|
||
| const integ = new IntegTest(app, 'SubnetIpam', { | ||
| testCases: [stack], | ||
| allowDestroy: ['EC2::IPAM'], | ||
| }); | ||
|
|
||
| // The first allocation from a fresh pool is the lowest /24 of the provisioned range | ||
| integ.assertions.awsApiCall('EC2', 'describeSubnets', { | ||
| SubnetIds: [subnet.subnetId], | ||
| }).expect(ExpectedResult.objectLike({ | ||
| Subnets: [ | ||
| { | ||
| CidrBlock: '10.0.0.0/24', | ||
| }, | ||
| ], | ||
| })); |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| // The first allocation from a fresh pool is the lowest /24 of the provisioned range | ||
| integ.assertions.awsApiCall('EC2', 'describeSubnets', { | ||
| SubnetIds: [subnet.subnetId], | ||
| }).expect(ExpectedResult.objectLike({ | ||
| Subnets: [ | ||
| { | ||
| CidrBlock: '10.0.0.0/24', | ||
| }, | ||
| ], | ||
| })); |
There was a problem hiding this comment.
⚪ Optional — The deploy-time assertion checks only CidrBlock: '10.0.0.0/24'. That confirms the subnet received a /24, but it relies on an IPAM implementation detail (that the first allocation from a fresh pool is the lowest /24) and never confirms the subnet is actually bound to the IPAM pool — a subnet with a hand-set 10.0.0.0/24 would pass identically. The test would be a stronger behavioral guarantee if it also proved the CIDR came from IPAM rather than coincidentally matching (INTEGRATION_TESTS.md).
Suggested change: Keep the CidrBlock check and additionally assert the IPAM association, e.g. an awsApiCall('EC2', 'getIpamResourceCidrs', ...) or describeIpamPools/pool-allocations call confirming the pool allocated the block, so the test proves the CIDR came from IPAM.
The resource planning pool fails with "The source resource vpc-... is not monitored in the IPAM scope" when it is created right after the VPC, because IPAM takes several minutes to discover a new VPC. Add a custom resource whose isComplete handler calls GetIpamResourceCidrs in the IPAM's private default scope every 30 seconds, for up to 45 minutes, and completes once the VPC is listed. The pool depends on it. Its ec2:GetIpamResourceCidrs grant is scoped to that IPAM scope's ARN. The snapshot comes from a real deployment. The waiter completed after about 8.5 minutes, the subnet was allocated 10.0.0.0/24 from the pool, and the assertion passed.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| /** | ||
| * The netmask length of the CIDR block to allocate from the pool | ||
| * | ||
| * Must be between 16 and 28 (inclusive), the sizes allowed for an IPv4 subnet. | ||
| */ |
There was a problem hiding this comment.
⚪ Optional — SubnetIpamAllocation.netmaskLength is a required field, but the underlying service property Ipv4NetmaskLength is optional because an IPAM pool can carry a default allocation netmask length, so allocating from a pool without specifying a per-subnet netmask is a valid service use case this struct cannot express (aws-resource-ec2-subnet). The current required shape (matching the pre-existing AwsIpamProps.ipv4NetmaskLength) is defensible and the field is easy to widen later, so this does not block — but it is worth a deliberate decision since the prop is hard to change once released.
Suggested change: If covering the pool-default case is desired, make it readonly netmaskLength?: number; with a @default - the pool's default netmask length is used doc, letting ipv4NetmaskLength fall through as undefined.
Remove the RETAIN removal policies from the IPAM and the pool, so the test's destroy deletes them and leaves the Region without an IPAM. Also remove allowDestroy, which named 'EC2::IPAM' instead of 'AWS::EC2::IPAM' and so never matched a resource type. The snapshot comes from a real deployment. The destroy completed without manual cleanup: the pool's delete waited about 19 minutes for IPAM to release the deleted subnet's allocation, then the IPAM was deleted.
Pull request has been modified.
|
|
||||||||||||||
|
|
||||||||||||||
Issue # (if applicable)
Closes #34296.
Reason for this change
Subnet(andPublicSubnet/PrivateSubnet) require a concretecidrBlock, whileAWS::EC2::Subnetcan allocate its IPv4 CIDR from an Amazon VPC IP Address Manager pool (Ipv4IpamPoolId+Ipv4NetmaskLength).Vpcalready supports IPAM for the VPC CIDR throughIpAddresses.awsIpamAllocation(), but a standalone L2 subnet cannot use IPAM without dropping to the L1, or overriding the L1 properties withCfnSubnetPropsMixinand a placeholdercidrBlock.Description of changes
SubnetProps.cidrBlockbecomes optional and a new optionalipv4IpamAllocation: SubnetIpamAllocationprop is added, whereSubnetIpamAllocationis{ ipamPool: IIPAMPoolRef, netmaskLength: number }.Subnetvalidates that exactly one ofcidrBlock/ipv4IpamAllocationis set and thatnetmaskLengthis within /16 to /28 (the IPv4 subnet sizes the package already enforces inNetworkBuilder; skipped for tokens), usingValidationErrorwithlitcodes.Ipv4IpamPoolId/Ipv4NetmaskLengthare passed toCfnSubnet. When IPAM is used,subnet.ipv4CidrBlockisCfnSubnet.attrCidrBlock(a deploy-time token) instead of a concrete string, the same wayVpc.vpcCidrBlockisattrCidrBlockfor an IPAM-allocated VPC.Design decisions, flagged for review:
ipv4IpamPoolId/ipv4NetmaskLength: the two values are only valid together (co-dependent props rule indocs/AGENTS_CONSTRUCT_DESIGN.md). Exclusivity with the pre-existingcidrBlockcannot be made structural without adding a second, factory-style way to express the CIDR, so it is validated at construction time as proposed in the issue.ipamPool: IIPAMPoolRefrather than an ID string, following the preference forI*Refinterfaces in props (as inplacementGroup?: IPlacementGroupRef,networkAcl: INetworkAclRef).CfnIPAMPoolimplements it directly andCfnIPAMPool.fromIpamPoolId()covers pools created elsewhere.AwsIpamProps.ipv4IpamPoolIdis still astring(with a "todo: should be a type" comment); happy to switch to a string if consistency with it is preferred.netmaskLengthis required, mirroringAwsIpamProps.ipv4NetmaskLength; relaxing it to optional later is non-breaking.cidrBlockrequired,ipv4CidrBlockderived from it), which a mixin must not change.CfnSubnetPropsMixinremains the escape hatch, e.g. for IPv6 IPAM allocation, which is left for a follow-up.ipv4CidrBlockthat parse the CIDR (SubnetFilter.byCidrMask(),byCidrRanges(),containsIpAddresses()) cannot work with a deploy-time value; this is documented on the prop and in the README rather than changed. Subnets created byVpcfromsubnetConfigurationare untouched.cidrBlockoptional on an input struct is source-compatible for callers;PublicSubnetProps/PrivateSubnetPropsinherit the new prop.Alternatives considered and rejected: flat props (co-dependent rule); a
SubnetIpAddresses.cidr()/.ipam()factory prop mirroringVpc.ipAddresses(cidrBlockalready exists and cannot be removed); optionalnetmaskLengthusing the pool's default netmask as in #34349 (deferred).Describe any new or updated permissions being added
None. No IAM policies are created; the allocation uses the deploying principal's existing EC2/IPAM permissions.
Description of how you validated changes
packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts(describe('Subnet')): the concrete-CIDR path renders exactly as before (CidrBlock, no IPAM properties); the IPAM path rendersIpv4IpamPoolId/Ipv4NetmaskLengthand noCidrBlock, both withCfnIPAMPool.fromIpamPoolId()and with a pool defined in the same stack;ipv4CidrBlockresolves toFn::GetAtt [<subnet>, CidrBlock];PublicSubnet/PrivateSubnetaccept the prop (includingaddNatGateway()); a tokennetmaskLengthis passed through; the three validation errors.packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.subnet-ipam.ts: an IPAM pool provisioning the VPC's10.0.0.0/16, aSubnetwithipv4IpamAllocation, and anawsApiCall('EC2', 'describeSubnets')assertion that the allocated CIDR is10.0.0.0/24. I do not have an AWS account to deploy it, so the snapshot is not included; see the exemption request comment below.yarn install,lerna run build --scope=aws-cdk-lib(compile, eslint, awslint),jest aws-ec2/test/vpc.test.ts(157 tests),yarn test aws-ec2, andyarn compatall pass: https://github.com/gauranshahuja/aws-cdk/actions/runs/34515935447. Ajsii-rosetta extract --strictrun in the same workflow reports 52 diagnostics, all in pre-existing snippets of other modules' READMEs; the newaws-ec2README snippet produces none.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license