Permit aws.ec2.Vpc to use IPAM-allocated cidrBlock ranges #1352

t0yv0 · 2024-07-30T01:02:18Z

Fixes issues with supporting the ipv4IpamPoolId parameter that ties a VPC to an IPAM pool.

You should now be able to write the following to allows IPAM to allocate and manage a cidrBlock range. The VPC component now uses that dynamically allocated block to automatically configure subnets.

new awsx.ec2.Vpc("myVpc", {
  ipv4IpamPoolId: myVpcIpamPool.id,
  ipv4NetmaskLength: 24,
  subnetStrategy: "Auto",
});

It is also possible to constrain the allocated subnets with subnetSpecs, while still using IPAM to manage the overall cidrBlock range:

new awsx.ec2.Vpc("myVpc", {
  numberOfAvailabilityZones: 3,
  subnetStrategy: "Auto",
  ipv4IpamPoolId: myVpcIpamPool.id,
  ipv4NetmaskLength: 22,
  subnetSpecs: [
    {
      type: "Private",
      name: "private",
      cidrMask: 25,
    },
    {
      type: "Public",
      name: "public",
      cidrMask: 27,
    },
  ],
  tags: tags,
});

Fixes #872

Note that subnetStrategy: "Auto" is required with this functionality, and "Legacy" strategy is not supported.

examples/vpc/nodejs/vpc-ipam-ipv4-auto-cidrblock/index.ts

t0yv0 · 2024-07-30T01:07:18Z

awsx/ec2/vpc.ts

+
+    const subnetSpecs = (() => {
+
+      // cidrBlock is allocated dynamically but there are no explicit specs; use subnetDistributorNew.


The case analysis here is a bit uncertain for me, appreciate some help formulating which test cases we want to add.

One particular complication is that getSubnetSpecsLegacy cannot yet accept a cidrBlock: Input<string> and therefore this does not support IPAM-allocated cidrBlock with Legacy strategy properly. I've only done the Input threading for getSubnetSpecs which implements the newer Auto strategy.

Would it be acceptable to only support IPAM when using the Auto strategy? Users won't be adding IPAM to an existing VPC, so I don't think we should have to support "legacy" stuff.

It now only supports the Auto strategy. Indeed cleaner.

awsx/ec2/vpc.ts

t0yv0 · 2024-07-31T21:35:52Z

awsx/ec2/vpc.ts

+    );
+
+    const subnetSpecs = validatePartialSubnetSpecs(decidedSpecs, (subnetSpecs) => {
+      validateSubnets(subnetSpecs, getOverlappingSubnets);


Hmm, self-reviewing this change, there's a problem here that validations now happen after the new aws.ec2.Vpc call, and in the case when they used to terminate the flow promptly, that's a visible change, as now new aws.ec2.Vpc attempts to provision before we reject the validations. It's a bit of a chicken and egg problem.

I think it's the best that we can do. From the user's perspective I would have to have created the VPC resource in order to have the cidr allocated.

How does pulumi run the validation in this case? Are the creation of the other resources dependent on the validation completing or would they be run in parallel?

It runs after the internal vpc resource registers but before subnets do. If there's error reported by the internal vpc resource they now will be reported in addition to validation failures reported by awsx.Vpc. At a cost of a little bit of extra code complexity we can get the previous behavior back while using the new validation ordering behavior for the IPAM case.

Yeah pulumi runs resource allocations in parallel, so when we say new Vpc() we fork off a thread creating that VPC. Validations can run in parallel or they can block resource creation.

awsx/ec2/vpc.ts

corymhall

Had a first pass and mostly things look good!

corymhall · 2024-08-01T18:13:14Z

awsx/ec2/subnetDistributorNew.ts

+    azCidrMask,
+  );
+  const subnetSpecs = subnetInputs ?? defaultSubnetInputsBare();
+  return azNames.flatMap((azName, azIndex) => {


Can we combine this with the allocateSubnetCidrBlocksInput/allocationSubnetCidrBlocks function? It looks like we perform the same type of loop a bunch of different places which makes it a little hard to follow what is happening.

Yeah, I don't know how to make this cleaner. allocateSubnetCidrBlocksInput is very intentionally broken out. Our problem here is that we're trying to keep the allocateSubnetCidrBlocks preserve the original algo as-is. But we need to interface with this algo whenvpcCidr: pulumi.Input<string> is not known yet. Even in this case where it's not know yet, we can't assume that the entire set of SubnetSpecPartial[] is unknown - we know how many there will be, and we know parts of information about them right away at preview, just not their respective cidrBlock values.

This indirection is accomplished by the means of Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }> helper data structure.

allocateSubnetCidrBlocks is needed to preserve original algo in as simple code as possible

allocateSubnetCidrBlocksInput is needed to do tricks to make allocateSubnetCidrBlocks callable with vpcCidr that is now an Input but return a Record, not an Output<Record> - tricks are used to not lose information

getSubnetSpecsWithPartialCidr/getSubnetSpecs is needed to do the rest of SubnetSpecPartial computation, not just the cidrBlock

What if we used union types and type checking? Something like this:

export function getSubnetSpecsWithPartialCidr( vpcName: string, vpcCidr: pulumi.Input<string>, azNames: string[], subnetInputs: SubnetSpecInputs[] | undefined, azCidrMask?: number, ): SubnetSpecPartial[] { let alloc: | pulumi.Output<Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }>> | Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }>; if (typeof vpcCidr === "string") { alloc = allocateSubnetCidrBlocks(vpcName, vpcCidr, azNames, subnetInputs, azCidrMask); } else { alloc = pulumi .output(vpcCidr) .apply((vpcCidr) => allocateSubnetCidrBlocks(vpcName, vpcCidr, azNames, subnetInputs, azCidrMask), ); } const subnetSpecs = subnetInputs ?? defaultSubnetInputsBare(); return azNames.flatMap((azName, azIndex) => { const azNum = azIndex + 1; return subnetSpecs.map((subnetSpec, subnetIndex) => { const subnetAllocID = subnetAllocationID(vpcName, subnetSpec, azNum, subnetIndex); let cidrBlock: pulumi.Input<string>; if (pulumi.Output.isInstance(alloc)) { const allocOutput = alloc as pulumi.Output< Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }> >; cidrBlock = allocOutput.apply((a) => pulumi.output(a[subnetAllocID].cidrBlock)); } else { cidrBlock = alloc[subnetAllocID].cidrBlock as pulumi.Input<string>; } return { cidrBlock: cidrBlock, type: subnetSpec.type, azName, subnetName: subnetName(vpcName, subnetSpec, azNum), tags: subnetSpec.tags, }; }); }); }

This is a nice idea, I think you are right. This lets me delete one of the functions. I opted for an even simpler form as I think we do not need to if (typeof vpcCidr === "string") { for cidrBlock we can just completely commit to pulumi.Output without loss of pertinent information in this case. Have a look at the latest version and thanks for pushing on this 🙏

corymhall · 2024-08-01T18:20:06Z

awsx/ec2/subnetSpecs.ts

+function detectPromptSubnetSpecs(specs: SubnetSpecPartial[]): SubnetSpec[] | undefined {
+  if (specs.every((s) => typeof s.cidrBlock === "string")) {
+    return specs.map((s) => {
+      const cidrBlock: string = typeof s.cidrBlock === "string" ? s.cidrBlock : "";


Is this one of those typescript issues where you've already established the type in the outer loop, but it forgets?

corymhall · 2024-08-01T18:27:52Z

awsx/ec2/vpc.ts

+
+    const subnetSpecs = (() => {
+
+      // cidrBlock is allocated dynamically but there are no explicit specs; use subnetDistributorNew.


Would it be acceptable to only support IPAM when using the Auto strategy? Users won't be adding IPAM to an existing VPC, so I don't think we should have to support "legacy" stuff.

awsx/ec2/vpc.ts

corymhall · 2024-08-01T18:39:38Z

awsx/ec2/vpc.ts

+    );
+
+    const subnetSpecs = validatePartialSubnetSpecs(decidedSpecs, (subnetSpecs) => {
+      validateSubnets(subnetSpecs, getOverlappingSubnets);


I think it's the best that we can do. From the user's perspective I would have to have created the VPC resource in order to have the cidr allocated.

How does pulumi run the validation in this case? Are the creation of the other resources dependent on the validation completing or would they be run in parallel?

corymhall · 2024-08-01T18:44:49Z

awsx/ec2/vpc.ts


    const sharedTags = { Name: name, ...args.tags };

+    const cidrBlock = Vpc.decideCidrBlockVpcInput(args);


Should we also validate here that if the user provides the ipamPoolId then they also provide the netmask?

This validation is done by the underlying awx.ec2.Vpc resource. It probably is slightly better UX to lift the validation to the awsx.ec2.Vpc layer. We can do this.

t0yv0 · 2024-08-03T03:24:19Z

The tests pass but they take a very long time, and they start timing out the token used for other tests. I think it's actually the destroy bit that's slow for some reason, likely for the aws Ipam resource. I wonder what's a practical way forward here, is there a way to keep an IPAM resource alive for testing and import it instead of create/delete on each test. Could also trivially combine the tests into one to run against only one IPAM resource.

 TestVpcIpam (49m0.33s)
✅ TestVpcIpam/vpc-ipam-ipv4-auto-cidrblock (29m0.69s)
✅ TestVpcIpam/vpc-ipam-ipv4-auto-cidrblock-with-specs (19m59.64s)

corymhall · 2024-08-05T12:16:25Z

I wonder what's a practical way forward here, is there a way to keep an IPAM resource alive for testing and import it instead of create/delete on each test. Could also trivially combine the tests into one to run against only one IPAM resource.

IPAM has a free tier so I wonder if it would be better to just create one and leave it there and have any tests that need it import it.

t0yv0 · 2024-08-05T20:22:03Z

It turns out the slowness comes from pulumi/pulumi-aws#4346 which makes deleting a VPC wait for IPAM to remove allocations. This aspect will not be helped by keeping an always-created IPAM for testing. Looks like customTimeout setting does not work either.

t0yv0 · 2024-08-05T20:27:54Z

Suggesting to go ahead with a workaround to the problem simply admitting longer running tests for the moment: #1357

corymhall

Just a couple of comments, otherwise looks good!

corymhall · 2024-08-06T12:30:49Z

awsx/ec2/subnetDistributorNew.ts

@@ -35,7 +70,48 @@ export function getSubnetSpecs(
  azNames: string[],
  subnetInputs: SubnetSpecInputs[] | undefined,
  azCidrMask?: number,
-): SubnetSpec[] {
+): SubnetSpecPartial[] {
+  return getSubnetSpecsWithPartialCidr(vpcName, vpcCidr, azNames, subnetInputs, azCidrMask);


I know you mentioned that we need to keep both getSubnetSpecs & getSubnetSpecsWithPartialCidr, but I'm not sure I understand why. They both have the same input and output types and getSubnetSpecs does not have any additional logic.

You might be right, I must have made a mistake of thinking about other functions. This can just be getSubnetSpecs.

corymhall · 2024-08-06T13:13:41Z

awsx/ec2/subnetDistributorNew.ts

+    azCidrMask,
+  );
+  const subnetSpecs = subnetInputs ?? defaultSubnetInputsBare();
+  return azNames.flatMap((azName, azIndex) => {


What if we used union types and type checking? Something like this:

export function getSubnetSpecsWithPartialCidr( vpcName: string, vpcCidr: pulumi.Input<string>, azNames: string[], subnetInputs: SubnetSpecInputs[] | undefined, azCidrMask?: number, ): SubnetSpecPartial[] { let alloc: | pulumi.Output<Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }>> | Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }>; if (typeof vpcCidr === "string") { alloc = allocateSubnetCidrBlocks(vpcName, vpcCidr, azNames, subnetInputs, azCidrMask); } else { alloc = pulumi .output(vpcCidr) .apply((vpcCidr) => allocateSubnetCidrBlocks(vpcName, vpcCidr, azNames, subnetInputs, azCidrMask), ); } const subnetSpecs = subnetInputs ?? defaultSubnetInputsBare(); return azNames.flatMap((azName, azIndex) => { const azNum = azIndex + 1; return subnetSpecs.map((subnetSpec, subnetIndex) => { const subnetAllocID = subnetAllocationID(vpcName, subnetSpec, azNum, subnetIndex); let cidrBlock: pulumi.Input<string>; if (pulumi.Output.isInstance(alloc)) { const allocOutput = alloc as pulumi.Output< Record<SubnetAllocationID, { cidrBlock: pulumi.Input<string> }> >; cidrBlock = allocOutput.apply((a) => pulumi.output(a[subnetAllocID].cidrBlock)); } else { cidrBlock = alloc[subnetAllocID].cidrBlock as pulumi.Input<string>; } return { cidrBlock: cidrBlock, type: subnetSpec.type, azName, subnetName: subnetName(vpcName, subnetSpec, azNum), tags: subnetSpec.tags, }; }); }); }

corymhall · 2024-08-06T13:19:12Z

awsx/ec2/vpc.ts

+      // If cidrBlock is known because the user provided it, validations should run as early as possible. Failing
+      // validations will short-circuit trying to create the inner aws.ec2.Vpc resource.


It is possible for cidrBlock to be unknown until the VPC is created. To accommodate this, this change introduces a SubnetSpecPartial type where the type of cidrBlock is now `pulumi.Input<string>`.

t0yv0 · 2024-08-06T15:56:25Z

Looks like unit tests do not like it if we lift prompt values to pulumi.Output around cidrBlocks and that may delay validations as well. To be safe I've coded up an inputApply that allows to use apply on pulumi.Input without losing prompt-ness.

I acknowledge it may be bad form to warp toward test constraints but unfortunately I still haven't worked out a ncie way to test failing Output values per pulumi/pulumi#16665

flostadler

Awesome!

The only thing I'm wondering is if we could add some unit tests like these here to make the testing easier&faster: https://www.pulumi.com/docs/using-pulumi/testing/unit/. Might be a cool&fast hackathon project to test those

flostadler · 2024-08-07T17:20:06Z

awsx/ec2/subnetDistributorNew.ts

+// This utility function is like pulumi.output(x).apply(fn) but tries to stay in the Input layer so that prompt
+// validations and test cases are not disturbed. wrap* functions are usually identity. Ideally something like this could
+// be handled in the core Pulumi Node SDK.
+function inputApply<T, U>(


flostadler · 2024-08-07T17:26:16Z

awsx/ec2/vpc.ts

+      // If cidrBlock is known because the user provided it, validations should run as early as possible. Failing
+      // validations will short-circuit trying to create the inner aws.ec2.Vpc resource.


flostadler · 2024-08-07T17:30:55Z

awsx/ec2/vpcConverters.ts

+  return s.map(convSubnetSpecInputsToResolvedSubnetSpecOutputs);
+}
+
+function convSubnetSpecInputsToResolvedSubnetSpecOutputs(


Yikes, inputs, outputs, unwrap... Good job with taming that complexity!

I've fixed Haskell singletons code before, this is fun and games.

t0yv0 · 2024-08-07T19:26:13Z

The only thing I'm wondering is if we could add some unit tests

Indeed, testing via the integration loop was an absolute bear. I'll leave this out of this PR but return to it if doing any more serious work around this repo.

t0yv0 commented Jul 30, 2024

View reviewed changes

examples/vpc/nodejs/vpc-ipam-ipv4-auto-cidrblock/index.ts Outdated Show resolved Hide resolved

t0yv0 commented Jul 30, 2024

View reviewed changes

t0yv0 requested review from flostadler and corymhall July 30, 2024 13:52

t0yv0 commented Jul 30, 2024

View reviewed changes

awsx/ec2/vpc.ts Outdated Show resolved Hide resolved

t0yv0 force-pushed the t0yv0/fix-872 branch from 4d38ac9 to 7b1eba4 Compare July 31, 2024 18:23

t0yv0 commented Jul 31, 2024

View reviewed changes

awsx/ec2/vpc.ts Outdated Show resolved Hide resolved

corymhall reviewed Aug 1, 2024

View reviewed changes

t0yv0 force-pushed the t0yv0/fix-872 branch from 6367748 to 603bfd3 Compare August 2, 2024 19:44

mjeffryes assigned t0yv0 Aug 2, 2024

t0yv0 force-pushed the t0yv0/fix-872 branch from 603bfd3 to 37eb011 Compare August 2, 2024 22:13

t0yv0 force-pushed the t0yv0/fix-872 branch from c9ecd1a to 12884b7 Compare August 5, 2024 17:55

t0yv0 marked this pull request as ready for review August 5, 2024 21:31

t0yv0 requested a review from corymhall August 5, 2024 21:31

corymhall requested changes Aug 6, 2024

View reviewed changes

t0yv0 added 10 commits August 6, 2024 10:24

refactor: add an explicit return type to Vpc.initialize

8370af2

refactor: factor out decideSubnetSpecs

749ac58

Permit returning outputs in the ResolvedSubnetSpec structure

8368687

Unify SubnetSpec type definition

b8fc895

Introduce SubnetSpecPartial

9d8fc2b

It is possible for cidrBlock to be unknown until the VPC is created. To accommodate this, this change introduces a SubnetSpecPartial type where the type of cidrBlock is now `pulumi.Input<string>`.

subnetDistributorNew: factor out allocateSubnetCidrBlocks

34b7f01

Ensure subnetDistributorNew can work with inputty vpcCidr

bf365e1

Colocate validation code for subnetSpecs

a7ab495

decideSubnetSpecs is now using inputty cidr under the Auto allocator

76ace7e

Fix: do not set cidrBlock default with ipv4NetmaskLength

58d71cf

t0yv0 added 10 commits August 6, 2024 10:24

Do not default cidrBlock when an IPAM pool is used

84c3a73

Add examples and integration tests for VPC IPAM support

cd36e5b

make lint

6b8bf33

The example needs to specify subnetStrategy: Auto

ba7ad11

Consolidate tests so parallel execution does not hit IPAM=1 limit

2155094

NoParallel setting for IPAM tests

c3bee8e

Ensure validations order vs Vpc construction order is not changing

4d25dae

Validate the use of ipv4NetmaskLength and cidrBlock

2412b7a

Simplify per PR comments

666894c

Simplify the code per PR review

edc3d77

t0yv0 force-pushed the t0yv0/fix-872 branch from dc94fa4 to edc3d77 Compare August 6, 2024 15:24

t0yv0 requested a review from corymhall August 6, 2024 15:25

Fix tests by preserving Input distinction

b701f86

corymhall approved these changes Aug 7, 2024

View reviewed changes

flostadler approved these changes Aug 7, 2024

View reviewed changes

t0yv0 merged commit b805061 into master Aug 7, 2024
11 checks passed

t0yv0 deleted the t0yv0/fix-872 branch August 7, 2024 19:26

mjeffryes added this to the 0.108 milestone Aug 16, 2024


		const subnetSpecs = (() => {

		// cidrBlock is allocated dynamically but there are no explicit specs; use subnetDistributorNew.


		const sharedTags = { Name: name, ...args.tags };

		const cidrBlock = Vpc.decideCidrBlockVpcInput(args);

		// If cidrBlock is known because the user provided it, validations should run as early as possible. Failing
		// validations will short-circuit trying to create the inner aws.ec2.Vpc resource.

Permit aws.ec2.Vpc to use IPAM-allocated cidrBlock ranges #1352

Permit aws.ec2.Vpc to use IPAM-allocated cidrBlock ranges #1352

Conversation

t0yv0 commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 3, 2024

corymhall commented Aug 5, 2024

t0yv0 commented Aug 5, 2024

t0yv0 commented Aug 5, 2024

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 6, 2024 • edited Loading

flostadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 7, 2024

t0yv0 commented Jul 30, 2024 •

edited

Loading

t0yv0 commented Aug 6, 2024 •

edited

Loading