From 61a756069b307d784db7301374ce4b7fa698a887 Mon Sep 17 00:00:00 2001 From: Daniel Bradley Date: Tue, 30 Jan 2024 17:09:25 +0000 Subject: [PATCH] Fix VPC legacy default subnet distribution (#1210) 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 --- awsx/ec2/knownWorkingSubnets.ts | 18 ++++++++ awsx/ec2/subnetDistributorLegacy.test.ts | 38 +++++++++++++++++ awsx/ec2/subnetDistributorLegacy.ts | 5 ++- awsx/ec2/subnetDistributorNew.test.ts | 54 ++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 2 deletions(-) diff --git a/awsx/ec2/knownWorkingSubnets.ts b/awsx/ec2/knownWorkingSubnets.ts index 46f2089f1..6f4ae67ac 100644 --- a/awsx/ec2/knownWorkingSubnets.ts +++ b/awsx/ec2/knownWorkingSubnets.ts @@ -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"], + }, ]; diff --git a/awsx/ec2/subnetDistributorLegacy.test.ts b/awsx/ec2/subnetDistributorLegacy.test.ts index d47cdeb13..25f188199 100644 --- a/awsx/ec2/subnetDistributorLegacy.test.ts +++ b/awsx/ec2/subnetDistributorLegacy.test.ts @@ -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( diff --git a/awsx/ec2/subnetDistributorLegacy.ts b/awsx/ec2/subnetDistributorLegacy.ts index c0aadb69e..5b396bc15 100644 --- a/awsx/ec2/subnetDistributorLegacy.ts +++ b/awsx/ec2/subnetDistributorLegacy.ts @@ -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[] = []; diff --git a/awsx/ec2/subnetDistributorNew.test.ts b/awsx/ec2/subnetDistributorNew.test.ts index b4970ea07..407da6303 100644 --- a/awsx/ec2/subnetDistributorNew.test.ts +++ b/awsx/ec2/subnetDistributorNew.test.ts @@ -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( @@ -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(