Skip to content

Commit

Permalink
fix(ec2): IPAM allocated subnets cannot split more than 256 times (#2…
Browse files Browse the repository at this point in the history
…8027)

Because of IPAM allocation, we can't know the parent CIDR at synth time, so we cannot calculate the CIDR split at synth time either.

This forces us to rely on the `{ Fn::Cidr }` function provided by CloudFormation. For resource consumption reasons, this function is limited to splitting any range into at most 256 subranges, which means the IPAM allocated VPC cannot split into more subranges either.

This PR adds a recursive split feature: if we need to split an CIDR range more than 256 times, we will do multiple splits:

```ts
Fn.select(300, Fn.cidr(range, 4096, 4)) // <-- illegal

// ==

Fn.select(44, Fn.cidr(Fn.select(1, Fn.cidr(range, 4, 12)), 256, 4))
```

Fixes #25537.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr committed Nov 17, 2023
1 parent 95538a1 commit 91a3e8c
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 4 deletions.
43 changes: 41 additions & 2 deletions packages/aws-cdk-lib/aws-ec2/lib/ip-addresses.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { calculateCidrSplits } from './cidr-splits';
import { CidrSplit, calculateCidrSplits } from './cidr-splits';
import { NetworkBuilder } from './network-util';
import { SubnetConfiguration } from './vpc';
import { Fn, Token } from '../../core';
Expand Down Expand Up @@ -230,7 +230,7 @@ class AwsIpam implements IIpAddresses {

const allocatedSubnets: AllocatedSubnet[] = cidrSplit.map(subnet => {
return {
cidr: Fn.select(subnet.index, Fn.cidr(input.vpcCidr, subnet.count, `${32-subnet.netmask}`)),
cidr: cidrSplitToCfnExpression(input.vpcCidr, subnet),
};
});

Expand All @@ -241,6 +241,45 @@ class AwsIpam implements IIpAddresses {
}
}

/**
* Convert a CIDR split command to a CFN expression that calculates the same CIDR
*
* Can recursively produce multiple `{ Fn::Cidr }` expressions.
*
* This is necessary because CFN's `{ Fn::Cidr }` reifies the split to an actual list of
* strings, and to limit resource consumption `count` may never be higher than 256. So
* if we need to split deeper, we need to do more than one split.
*
* (Function public for testing)
*/
export function cidrSplitToCfnExpression(parentCidr: string, split: CidrSplit) {
const MAX_COUNT = 256;
const MAX_COUNT_BITS = 8;

if (split.count === 1) {
return parentCidr;
}

if (split.count <= MAX_COUNT) {
return Fn.select(split.index, Fn.cidr(parentCidr, split.count, `${32-split.netmask}`));
}

if (split.netmask - MAX_COUNT_BITS < 1) {
throw new Error(`Cannot split an IP range into ${split.count} /${split.netmask}s`);
}

const parentSplit = {
netmask: split.netmask - MAX_COUNT_BITS,
count: Math.ceil(split.count / MAX_COUNT),
index: Math.floor(split.index / MAX_COUNT),
};
return cidrSplitToCfnExpression(cidrSplitToCfnExpression(parentCidr, parentSplit), {
netmask: split.netmask,
count: MAX_COUNT,
index: split.index - (parentSplit.index * MAX_COUNT),
});
}

/**
* Implements static Ip assignment locally.
*
Expand Down
35 changes: 33 additions & 2 deletions packages/aws-cdk-lib/aws-ec2/test/ip-addresses.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@

import { Template } from '../../assertions';
import { Stack } from '../../core';
import { IpAddresses, SubnetType, Vpc } from '../lib';
import { IpAddresses, SubnetType, Vpc, cidrSplitToCfnExpression } from '../lib';
import { CidrSplit } from '../lib/cidr-splits';

describe('Cidr vpc allocation', () => {

Expand Down Expand Up @@ -406,5 +407,35 @@ describe('AwsIpam Vpc Integration', () => {
template.resourceCountIs('AWS::EC2::Subnet', 2);

});

});

type CfnSplit = {
count: number;
hostbits: number;
select: number;
}

test.each([
// Index into first block
[{ count: 4096, netmask: 28, index: 123 }, /* -> */ { count: 16, hostbits: 12, select: 0 }, { count: 256, hostbits: 4, select: 123 }],
// Index into second block
[{ count: 4096, netmask: 28, index: 300 }, /* -> */ { count: 16, hostbits: 12, select: 1 }, { count: 256, hostbits: 4, select: 44 }],
// Index into third block
[{ count: 4096, netmask: 28, index: 513 }, /* -> */ { count: 16, hostbits: 12, select: 2 }, { count: 256, hostbits: 4, select: 1 }],
// Count too low for netmask (wasting space)
[{ count: 4000, netmask: 28, index: 300 }, /* -> */ { count: 16, hostbits: 12, select: 1 }, { count: 256, hostbits: 4, select: 44 }],
])('recursive splitting when CIDR needs to be split more than 256 times: %p', (split: CidrSplit, first: CfnSplit, second: CfnSplit) => {
const stack = new Stack();
expect(stack.resolve(cidrSplitToCfnExpression('10.0.0.0/16', split))).toEqual({
'Fn::Select': [
second.select,
{
'Fn::Cidr': [
{ 'Fn::Select': [first.select, { 'Fn::Cidr': ['10.0.0.0/16', first.count, `${first.hostbits}`] }] },
second.count,
`${second.hostbits}`,
],
},
],
});
});

0 comments on commit 91a3e8c

Please sign in to comment.