Skip to content

Commit

Permalink
Fix VPC legacy default subnet distribution (#1210)
Browse files Browse the repository at this point in the history
Fixes #1204 

- Assuming all subnets are the same size is overly cautious and breaks
some existing setups.
- Maintain the new special case for single subnet layouts to use the
whole of small VPCs.
- This will now fail and require manual layout for smaller VPCs with
either:
   - More than 1 private subnets
   - More than 2 public subnets
   - More than 1 public subnets and more than 4 isolated subnets
  • Loading branch information
danielrbradley committed Jan 30, 2024
1 parent 63b3e79 commit 61a7560
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 2 deletions.
18 changes: 18 additions & 0 deletions awsx/ec2/knownWorkingSubnets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1323,4 +1323,22 @@ export const knownWorkingSubnets: {
],
vpcCidr: "10.0.0.0/20",
},
{
vpcCidr: "10.1.0.0/18",
subnetSpecs: [
{
type: "Private",
},
{
type: "Public",
},
{
type: "Isolated",
},
{
type: "Isolated",
},
],
result: ["10.1.0.0/19", "10.1.32.0/20", "10.1.48.0/24", "10.1.49.0/24"],
},
];
38 changes: 38 additions & 0 deletions awsx/ec2/subnetDistributorLegacy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,44 @@ describe("default subnet layout", () => {
);
});

describe("default sizes private, public and isolated subnet", () => {
it("gives /19, /20 and /24 for /16 AZ", () => {
expect(getDefaultSubnetSizes(16)).toEqual([19, 20, 24]);
});
it("gives /19, /20 and /24 for /17 AZ", () => {
expect(getDefaultSubnetSizes(17)).toEqual([19, 20, 24]);
});
it("gives /19, /20 and /24 for /18 AZ", () => {
expect(getDefaultSubnetSizes(18)).toEqual([19, 20, 24]);
});
it("gives /20, /20 and /24 for /19 AZ", () => {
expect(getDefaultSubnetSizes(19)).toEqual([20, 20, 24]);
});
it("gives /21, /21 and /24 for /20 AZ", () => {
expect(getDefaultSubnetSizes(20)).toEqual([21, 21, 24]);
});
it("gives /22, /22 and /24 for /21 AZ", () => {
expect(getDefaultSubnetSizes(21)).toEqual([22, 22, 24]);
});
it("gives /23, /23 and /24 for /22 AZ", () => {
expect(getDefaultSubnetSizes(22)).toEqual([23, 23, 24]);
});
it("gives /24, /24 and /24 for /23 AZ", () => {
expect(getDefaultSubnetSizes(23)).toEqual([24, 24, 24]);
});

function getDefaultSubnetSizes(azSize: number) {
const vpcCidr = `10.0.0.0/${azSize}`;
const result = getSubnetSpecsLegacy(
"vpcName",
vpcCidr,
["us-east-1a"],
[{ type: "Private" }, { type: "Public" }, { type: "Isolated" }],
);
return result.map((s) => getCidrMask(s.cidrBlock));
}
});

it("should use default values if VPC is large", () => {
fc.assert(
fc.property(
Expand Down
5 changes: 3 additions & 2 deletions awsx/ec2/subnetDistributorLegacy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ export function getSubnetSpecsLegacy(

let subnetsOut: SubnetSpec[] = [];

// How many bits do we need if just dividing up evenly?
const newBitsPerSubnet = Math.log2(nextPow2(subnetInputs.length));
// How many bits do we need if just assuming a "normal" layout with at least a private and public subnet?
// Special case where there's only 1 subnet - with a small VPC, we'll let it use the whole space.
const newBitsPerSubnet = subnetInputs.length === 1 ? 0 : 1;

for (let i = 0; i < azNames.length; i++) {
const privateSubnetsOut: SubnetSpec[] = [];
Expand Down
54 changes: 54 additions & 0 deletions awsx/ec2/subnetDistributorNew.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,30 @@ function subnetSpec() {
}

describe("default subnet layout", () => {
describe("when no layout specified", () => {
it.each([16, 17, 18, 19, 20, 21, 22, 23, 24])(
"/%i AZ creates single private & public with staggered sizes",
(azCidrMask) => {
expect(getDefaultSubnetSizes(azCidrMask)).toMatchObject([
{
type: "Private",
cidrMask: azCidrMask + 1,
},
{
type: "Public",
cidrMask: azCidrMask + 2,
},
]);
},
);

function getDefaultSubnetSizes(azSize: number) {
const vpcCidr = `10.0.0.0/${azSize}`;
const result = getSubnetSpecs("vpcName", vpcCidr, ["us-east-1a"], undefined);
return result.map((s) => ({ type: s.type, cidrMask: getCidrMask(s.cidrBlock) }));
}
});

it("should have smaller subnets than the vpc", () => {
fc.assert(
fc.property(
Expand Down Expand Up @@ -92,6 +116,36 @@ describe("default subnet layout", () => {
);
});

describe("default sizes for private, public and isolated subnet", () => {
it.each([16, 17, 18, 19, 20, 21, 22, 23, 24])(
"/%i AZ evenly distributes space",
(azCidrMask) => {
const vpcCidr = `10.0.0.0/${azCidrMask}`;
const result = getSubnetSpecs(
"vpcName",
vpcCidr,
["us-east-1a"],
[{ type: "Private" }, { type: "Public" }, { type: "Isolated" }],
);
const masks = result.map((s) => ({ type: s.type, cidrMask: getCidrMask(s.cidrBlock) }));
expect(masks).toMatchObject([
{
type: "Private",
cidrMask: azCidrMask + 2,
},
{
type: "Public",
cidrMask: azCidrMask + 2,
},
{
type: "Isolated",
cidrMask: azCidrMask + 2,
},
]);
},
);
});

it("should not have overlapping ranges", () => {
fc.assert(
fc.property(
Expand Down

0 comments on commit 61a7560

Please sign in to comment.