Skip to content

Commit

Permalink
Improve VPC usability (#1153)
Browse files Browse the repository at this point in the history
1. Add a new `Unused` subnet type. This has no effect in the legacy subnet allocation method.
2. Add `subnetStrategy` field to VPC as enum of `Legacy`, `Auto`, `Exact`. 
	- `Legacy` maintains current complex behaviour.
	- `Auto` lays out in specified order with gaps where required. Either all mask must be specified, or no masks specified - no mixing of auto-allocated and manual is allowed.
	- `Exact` requires the user to specify what every part of the range is used for - using the `Unused` subnet spec type for expected gaps between subnets.

Anyone using the legacy subnet allocation method can definitely migrate to the exact layout method, they can also test migrating to the Auto layout, but it's not guaranteed to be compatible.

In the next major version, we should change the default subnet strategy to `Auto`.

Adds optional `size` and `cidrBlocks` fields to subnet spec as an alternative to `cidrMask` as a more intuitive way of specifying masks. The size must be a power of two (2, 4, 8, 16, 32, 64, 128, etc). The cidrBlocks property is an array of strings, which must match the number of AZs being used. If using `cidrBlocks`, then all subnet specs MUST specify `cidrBlocks`. If multiple of the cidrMask, cidrBlocks and size properties are specified, they must agree.

Adds a new output called `subnetLayout` which describes the resolved subnet layout to make it a little more transparent to the user what's happening. This is also used to help users move from the legacy strategy to Auto or Exact.

Example of warning for anyone using the implicit "Legacy" strategy:

```
  awsx:ec2:Vpc (vpc):
    warning: The default subnetStrategy will change from "Legacy" to "Auto" in the next major version. Please specify the subnetStrategy explicitly. The current subnet layout can be specified via "Auto" as:
    
    [
      {
        "type": "Private",
        "cidrMask": 21
      },
      {
        "type": "Unused",
        "cidrMask": 21
      },
      {
        "type": "Public",
        "cidrMask": 20
      }
    ]
```

Fixes #1135 
Fixes #951
Fixes #321 - we can now add subnets in custom positions to avoid conflicts or recreation

## Changes:

* Add SubnetStrategy to VPC

- Add "Unused" spec type.
- Draft the `cidrBlock` variation for complete custom layouts.

* Make plain

* Rename existing subnet to legacy

* Implement new subnet distributor

- Copy existing tests.
- Exclude known cases test as we specifically don't want this to be backward compatible - only legacy is backward compatible to v1.x.
- Use netmask over ip-address as it's a simpler API and includes a `.next()` function.

* Validate for gaps in exact mode

* Fixup lint

* Generate SDKs

* Make default layout match legacy

- Remove isolated subnet.
- Simplify maths.

* Implement fully custom subnet layouts

* Add warning about users on the legacy strategy by default

* Separate codepaths for auto and explicit layouts

- Refine type of explicit layouts to avoid existence checks.
- Extract subnet name function.
- Rework normalisation to be more imperative and ensure type correctness.

* Add custom documentation for VPC

* Improve subnet strategy warning

- Add explanation of condition.
- Attach to correct resource

* Make it easier to debug the provider

* Fix review feedback

Remove redundant undefined check.

Replace `Math.log2(nextPow2(x))` with `newBits(x)`. Proven compatibility locally by the test:

test("log2calc", () => {
  fc.assert(
    fc.property(fc.integer({ min: 0, max: 32 }), (i) => {
      expect(newBits(i)).toEqual(Math.log2(nextPow2(i)));
    }),
  );
});

* Add availabilityZoneCidrMask option

Allow users to reserve more space for additional AZs to be added later.

* Add `subnetLayout` output

Reverse engineer layout from legacy calculation by looking at just the first AZ repetition.
Add calculated "Auto" to the warning message for legacy users to transition to the new "Auto" setting.

* Un-skip tests
  • Loading branch information
danielrbradley authored Nov 20, 2023
1 parent 713ceda commit 5d5870e
Show file tree
Hide file tree
Showing 37 changed files with 2,721 additions and 58 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ JAVA_GEN_VERSION := v0.9.7

AWSX_SRC := $(wildcard awsx/*.*) $(wildcard awsx/*/*.ts)
AWSX_CLASSIC_SRC:= $(wildcard awsx-classic/*.*) $(wildcard awsx-classic/*/*.ts)
CODEGEN_SRC := $(wildcard schemagen/go.*) $(wildcard schemagen/pkg/*/*.go) $(wildcard schemagen/pkg/cmd/${CODEGEN}/*.go)
CODEGEN_SRC := $(wildcard schemagen/go.*) $(wildcard schemagen/pkg/*/*) $(wildcard schemagen/pkg/cmd/${CODEGEN}/*.go)

WORKING_DIR := $(shell pwd)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@

import fc from "fast-check";
import { SubnetSpecInputs, SubnetTypeInputs } from "../schema-types";
import { getSubnetSpecs, SubnetSpec, validateRanges } from "./subnetDistributor";
import { getSubnetSpecsLegacy, SubnetSpec, validateRanges } from "./subnetDistributorLegacy";
import { knownWorkingSubnets } from "./knownWorkingSubnets";
import { extractSubnetSpecInputFromLegacyLayout } from "./vpc";
import { getSubnetSpecs } from "./subnetDistributorNew";
import { Netmask } from "netmask";

function cidrMask(args?: { min?: number; max?: number }): fc.Arbitrary<number> {
return fc.integer({ min: args?.min ?? 16, max: args?.max ?? 27 });
Expand Down Expand Up @@ -52,7 +55,7 @@ describe("default subnet layout", () => {
({ vpcCidrMask, azs, subnetSpecs }) => {
const vpcCidr = `10.0.0.0/${vpcCidrMask}`;

const result = getSubnetSpecs("vpcName", vpcCidr, azs, subnetSpecs);
const result = getSubnetSpecsLegacy("vpcName", vpcCidr, azs, subnetSpecs);

for (const subnet of result) {
const subnetMask = getCidrMask(subnet.cidrBlock);
Expand All @@ -74,7 +77,7 @@ describe("default subnet layout", () => {
({ vpcCidrMask, subnetSpec }) => {
const vpcCidr = `10.0.0.0/${vpcCidrMask}`;

const result = getSubnetSpecs("vpcName", vpcCidr, ["us-east-1a"], [subnetSpec]);
const result = getSubnetSpecsLegacy("vpcName", vpcCidr, ["us-east-1a"], [subnetSpec]);

expect(result.length).toBe(1);
expect(result[0].cidrBlock).toBe(vpcCidr);
Expand All @@ -93,7 +96,7 @@ describe("default subnet layout", () => {
({ vpcCidrMask }) => {
const vpcCidr = `10.0.0.0/${vpcCidrMask}`;

const result = getSubnetSpecs(
const result = getSubnetSpecsLegacy(
"vpcName",
vpcCidr,
["us-east-1a"],
Expand Down Expand Up @@ -144,7 +147,7 @@ describe("default subnet layout", () => {
({ vpcCidrMask, subnetSpecs }) => {
const vpcCidr = `10.0.0.0/${vpcCidrMask}`;

const result = getSubnetSpecs("vpcName", vpcCidr, ["us-east-1a"], subnetSpecs);
const result = getSubnetSpecsLegacy("vpcName", vpcCidr, ["us-east-1a"], subnetSpecs);

validateRanges(result);
},
Expand All @@ -154,10 +157,11 @@ describe("default subnet layout", () => {

describe("known working subnets", () => {
for (const knownCase of knownWorkingSubnets) {
it(`should work for ${knownCase.vpcCidr} with subnets ${knownCase.subnetSpecs
const specDescription = knownCase.subnetSpecs
.map((s) => `${s.type}:${s.cidrMask}`)
.join(", ")}`, () => {
const result = getSubnetSpecs(
.join(", ");
it(`should work for ${knownCase.vpcCidr} with subnets ${specDescription}`, () => {
const result = getSubnetSpecsLegacy(
"vpcName",
knownCase.vpcCidr,
["us-east-1a"],
Expand All @@ -167,8 +171,57 @@ describe("default subnet layout", () => {

expect(actual).toEqual(knownCase.result);
});
it(`should be convertible to new format (${knownCase.vpcCidr}, ${specDescription})`, () => {
const vpcName = "vpcName";
const availabilityZones = ["us-east-1a"];
const legacyResult = getSubnetSpecsLegacy(
vpcName,
knownCase.vpcCidr,
availabilityZones,
knownCase.subnetSpecs,
);
const extracted = extractSubnetSpecInputFromLegacyLayout(
legacyResult,
vpcName,
availabilityZones,
);

try {
const autoResult = getSubnetSpecs(
vpcName,
knownCase.vpcCidr,
availabilityZones,
extracted,
);
const normalizedAutoResult = autoResult
.filter((s) => s.type !== "Unused")
.map((s) => s.cidrBlock);
// Legacy sometimes returns odd netmasks like 10.0.1.128/24 which should actually be 10.0.1.0/24
const normalizedLegacyNetmasks = legacyResult.map((s) =>
new Netmask(s.cidrBlock).toString(),
);
expect(normalizedAutoResult).toEqual(normalizedLegacyNetmasks);
} catch (err: any) {
// Some cases don't actually fit inside their VPCs.
if (!err.message.includes("Subnets are too large for VPC")) {
throw err;
}
}
});
}
});
it("can override az cidr mask", () => {
const vpcCidr = "10.0.0.0/16";
const result = getSubnetSpecsLegacy(
"vpcName",
vpcCidr,
["us-east-1a"],
[{ type: "Public" }],
21,
);
// Would default to /20 as that's the hard coded max size for a public subnet
expect(result[0].cidrBlock).toBe("10.0.0.0/21");
});
});

function getCidrMask(cidrBlock: string): number {
Expand All @@ -181,7 +234,7 @@ describe("getSubnetSpecs", () => {
const vpcName = "vpcname";

it("should return the default subnets with no parameters and 3 AZs", () => {
const result = getSubnetSpecs(vpcName, vpcCidr, azs);
const result = getSubnetSpecsLegacy(vpcName, vpcCidr, azs);
const expected: SubnetSpec[] = [
{
type: "Private",
Expand Down Expand Up @@ -271,7 +324,7 @@ describe("getSubnetSpecs", () => {
},
];

expect(getSubnetSpecs(vpcName, vpcCidr, azs, inputs)).toEqual(expected);
expect(getSubnetSpecsLegacy(vpcName, vpcCidr, azs, inputs)).toEqual(expected);
},
);

Expand Down Expand Up @@ -332,6 +385,6 @@ describe("getSubnetSpecs", () => {
},
];

expect(getSubnetSpecs(vpcName, vpcCidr, azs, inputs)).toEqual(expected);
expect(getSubnetSpecsLegacy(vpcName, vpcCidr, azs, inputs)).toEqual(expected);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ export interface SubnetSpec {
}>;
}

export function getSubnetSpecs(
export function getSubnetSpecsLegacy(
vpcName: string,
vpcCidr: string,
azNames: string[],
subnetInputs?: SubnetSpecInputs[],
azCidrMask?: number,
): SubnetSpec[] {
// Design:
// 1. Split the VPC CIDR block evenly between the AZs.
Expand All @@ -46,7 +47,10 @@ export function getSubnetSpecs(
// 1. Attempt to use the legacy method (cidrSubnetV4) to get the next block.
// 2. Check if the generated next block overlaps with the previous block.
// 3. If there's overlap, use the new method (nextBlock) to get the next block.
const newBitsPerAZ = Math.log2(nextPow2(azNames.length));
const newBitsPerAZ =
azCidrMask !== undefined
? azCidrMask - new ipAddress.Address4(vpcCidr).subnetMask
: Math.log2(nextPow2(azNames.length));

const azBases: string[] = [];
for (let i = 0; i < azNames.length; i++) {
Expand Down
Loading

0 comments on commit 5d5870e

Please sign in to comment.