Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ec2.Vpc to support using ipam #21333

Closed
2 tasks
mrpackethead opened this issue Jul 27, 2022 · 25 comments · Fixed by #22458
Closed
2 tasks

ec2.Vpc to support using ipam #21333

mrpackethead opened this issue Jul 27, 2022 · 25 comments · Fixed by #22458
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1

Comments

@mrpackethead
Copy link

Describe the feature

ec2.Vpc currently requires a prop cidr

It would be very helpful if vpc could either take cidr, or optionally take an Ipv4IpamPoolId. This would allow the construct to use IPAM allocated address's. which is exceptionally helpful.

Use Case

We have migrated to the automatic allocation of IP address from IPAM.

Proposed Solution

Add the optional prop to the ec2.Vpc Construct.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.33

Environment details (OS name and version, etc.)

Generic.

@mrpackethead mrpackethead added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 27, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Jul 27, 2022
@mrpackethead
Copy link
Author

This problem has a more squirly problme layered in underneath, as i've discovered today.. So this is the guts of the code i created. I thought oh yeah, easy i can extract the Cidr using a custom resource. the error is all telling..

jsii.errors.JSIIError: 'cidr' property must be a concrete CIDR string, got a Token (we need to parse it for automatic subdivision)

So, heres the reality of this situation.. We can use ec2.VPC has it sits, becuase it needs that IP address range at synth time.

Maybe this is a case for a custom context lookup, which will get the the range?

`class TeRotoVPC(constructs.Construct):

def __init__(self, scope: constructs.Construct, id: str, *,
	stage_name=None):
	super().__init__(scope, id)
	

	cidr_allocation = ec2.CfnIPAMAllocation(self, "MyCfnIPAMAllocation",
		ipam_pool_id="ipam-pool-0afcad222ed575c58",
		description= f'terito-{stage_name}',
		netmask_length=24,
	)

	
	getcidr = cr.AwsCustomResource(self, "GetCidr",
		on_create=cr.AwsSdkCall(
			service='EC2',
			action='getIpamPoolAllocations',
			parameters = {
				'IpamPoolAllocationId': cidr_allocation.attr_ipam_pool_allocation_id,
				'IpamPoolId': 'ipam-pool-0afcad222ed575c58'
			},
			physical_resource_id=cr.PhysicalResourceId.of("GetCidr")
		),
		policy=cr.AwsCustomResourcePolicy.from_sdk_calls(
			resources=cr.AwsCustomResourcePolicy.ANY_RESOURCE
		)
	)

	#get the allocation.. 


	# create a VPC, assign address space from ipam
	self.vpc = ec2.Vpc(self, f'terito{stage_name}',
		nat_gateways = 0, 
		availability_zones= ['ap-southeast-2a','ap-southeast-2c'],
		cidr = getcidr.get_response_field('IpamPoolAllocations.0.Cidr'),
		vpc_name = f'terito-{stage_name}',
		subnet_configuration= [
			ec2.SubnetConfiguration(
				name="database",
				subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
				cidr_mask = 28,
			)
		]
	)`

@mrpackethead
Copy link
Author

This is a bit of a clanger of a problem. ec2.Vpc requires a concrete IP range to work. because its subvidiing it.. if we we want to use a IP range thats been allocated by IPAM we need a new strategy...
The underlying cfn type for ec2 supports taking a prop for an ipv4IpamPoolId.. which can work, as it doe'snt need to a concrete value at synth time.
I'm thinking.. Is this a good fit for a context lookup?? Maybe. Its not so much a lookup. Its a create. Your asking for a range, which is created. That rattles my cage a little bit, in terms of appropriateness of that.
Another approach is to create a vpc with the L1 contruct, and then bring back a vpc using vpc.fromattributes...

@Crycham
Copy link

Crycham commented Jul 28, 2022

Hey guys, after struggling weeks with IPAM with the L2 construct ( ec2.vpc ) and tried all the solutions provided :

First solution : override to add the property path pool id
> This solution works fine but the problem is when I modify the cidr block of the vpc, subnets didn't take the override in consideration and works with the previous cidr block. but I tried also to override the cidr block of subnet with the adapted cidr from ipam but the problem is not dynamically possible I have to write directly "10.1.64.0/18" for example , and knowing that 10.1.0.0/16 was allocated because when I tried to get the cidr block after allocating with IPAM I got a token string and I can't manipulate the string to modify for example 9.0.64.0/18 to 10.1.64.0/18 dynamically.

Second solution : Custom resource to get ipam allocations
> Same problem the response field return a token string not exploitable to override subnets cidr block.

I also tried a hard solution : first create a midle vpc with CfnVpc using Ipam pool id just to get a no token string cidr range we deploy the stack and get the string of cidr then we release the vpc with aws cli ( problem here ipam take few minutes to release...) after that we create the desire vpc with the string cidr ( not a token thanks god ^^' ) and we override the property ipam pool Id to have it in count in ipam and bingo worked ! but and didn't like this solution so much due to the time lost waiting the release.

Third solution : Working with CfnVpc
> so i give up using the ec2.vpc function to use the CfnVpc function which supports the ipam pool id, fine, but the problem is the subnets that I create after that cannot be dynamically allocated with the appropriate cidr block from the vpc. I have to write a string like 10.1.0.0/18 and that makes no sense because I want ipam take care of it or the vpc himself using the vpc id as parameter.
Also I found a contradiction with the doc and the run time , in the cdk doc they mentionned that the cidr block parameter is optionnal
https://docs.aws.amazon.com/cdk/api/v1/python/aws_cdk.aws_ec2/CfnSubnet.html

while when I run a subnet lke this :
self.pub_subnet = ec2.CfnSubnet(
self,
id='Pub0',
vpc_id=self.vpc.attr_vpc_id,
)
I got an error:
12:08:02 | CREATE_FAILED | AWS::EC2::Subnet | Priv01
Resource handler returned message: "Invalid request provided: Property CidrBlock cannot be empty." (RequestToken: 04eec8d4-184a-aafb-44de-5de81ad1a720, HandlerErrorCode: InvalidRequest)

Do you think that is logic for you to use Ipam and also give the cidr block ? I think it's the ipam job to do that or a back end function that provides available cidr block from the main cidr block of the vpc to the subnets created after.

So even the normal use of ipam stopped at vpc level and subnets could not be used !
the CfnSubnet function must be clever enough to have a available cidr block from the vpc using vpc_id paramter.

Noted that the configuration of IPAM works fine but the problem is with after using the ipam pool id !

I hope that you could help me and i hope there is a way to solve this problem...

@peterwoodworth peterwoodworth added p1 effort/large Large work item – several weeks of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jul 29, 2022
@mrpackethead
Copy link
Author

@peterwoodworth , thanks for marking this as effort/large. This ones import, and we need to do it the right way or we are going to cause our selves a lot of pain moving forward....

Ive been doing more work on this problem in the last few days, and looking at the exisiting construct code..

The things i've established;

(a) the construct must return a fully formed ec2.IVpc. Do do anything else will have so much down stream impact, it wont' be workable. So many cdk constructs rely on this construct.

(b) The ec2.Vpc construct underlying uses the l1. CfnVpc

(c) We cant' establish the cidr block untill deploy time. so the 'task' of dividing the Cidr up and calcuating the Subnets cant' be done at synth time. I propose that we move this task to a Custom Resource Lambda,.. Largely we can take the exisiting code and run it in the lambda.. we can then pass the attrbutes of the custom resource to the CfnVPC. Lastly we will need to create the iVPC..

I think its doable inside the exisiting construct, and keeping it backwards compatible and non breaking..

The prop cidr would be come optional, we would create a new prop ipv4 ( and possibly ipv6 ), that was also optional, but you'd need to one. ( and not both ).

vpc = ec2.Vpc(self, f'terito{stage_name}',
    cidr: '10.10.10.0/24',
    ipv4ipampoolid: [ipamId],
)

This sounds like a good wee project which i probalby can contribute too..

@Crycham
Copy link

Crycham commented Aug 1, 2022

I have may found I workaround to solve this problem, the idea is to exploit the auto import option in the ipam pool.
1- Create a temporary L1 vpc using the ipam pool id to allocate for example 10.0.0.0/16
2- deploy the tmp vcp stack
3- get the cidr of the vpc with Cfnoutput and stock it in a file cidr.txt
4- destroy the tm vpc stack to release the allocation in the ipam
5- read the file in a variable vpc_cidr in the app.py and pass it as a parametre in the final vpc stack
6- Create the final L2 vpc using the cidr parametre vpc_cidr
7- repeat for another vpc the same steps without waiting for release ( in few minutes the ipam will attach automatically the previous vpc to the allocation no overlapping -> compliant )

Note : to be sure that the allocation of the tmp vpc will take on consideration others ipam allocation and avoid to attach it while the allocation is empty or not complete even if this case is rare we could use this check in the bash script :

aws ec2 get-ipam-pool-allocations --ipam-pool-id $(<ipamid) | grep $(<cidr2.txt) | cut -d\" -f4 | tr -d "\n" > check;
while [[ $(<cidr2.txt) != $(<check) ]]; do echo "waiting for allocation in ipam..."; sleep 5; done;

Hope that could help or to be improved to use the ipam param directly in ec2.vpc :)

@corymhall
Copy link
Contributor

My initial thought is that we may need to refactor the Vpc construct to use Fn::Cidr to allocate the subnet addresses instead of calculating it at synth time. I'm not sure if this is something that can be done easily or should be part of a larger effort (i.e. #5927)

@mrpackethead
Copy link
Author

@corymhall, thats possibly a good approach. it may be simpler than what i had contemplated, with creating a custom resource to do the computation, ( essentially the required function exisits in CF, so why reinvent it. ) One of my collegues is using Fn::Cidr to allocate subnets, Hes done that in native Cloudformation.. I'll look at the exisiting code and see what effort might be involved. but my gut feeling is that its not a big task, and could be a resonable bolt on..

@Crycham
Copy link

Crycham commented Aug 5, 2022

@corymhall @mrpackethead I really appreciate the quality and the efficiency of your response. the solution worked well for me in addition to another workaround that I added I found out that your suggestion what I was missing to have a something working properly. So I want to know what do you think about it, the solution is this following steps :

1- create a VPC with subnets using ec2.vpc ( to have the advantage of automatic configuration )

2- override the vpc's cidr to add ipam pool id using :

    Ipv4NetmaskLength = 16
    cfn_vpc: ec2.CfnVPC = self.vpc.node.default_child
    cfn_vpc.add_property_deletion_override("CidrBlock")
    cfn_vpc.add_property_override("Ipv4IpamPoolId", Ipam_pool_id)
    cfn_vpc.add_property_override("Ipv4NetmaskLength", Ipv4NetmaskLength)

3- use the ipam cidr allocation in the function Fn.Cidr to generate subnet's cidr and :

    #make sure to have dynamically the proper division of subnets
    # if N is a power of two simply return it
       N = max_azs*2
    if not (N & (N - 1)):
        size_max = str(32-(Ipv4NetmaskLength+int(math.log2(N))))
    else :
    # else set only the left bit of most significant bit
        size_max = str(32-(Ipv4NetmaskLength+int(math.log2(int("1" + (len(bin(N)) - 2) * "0", 2))))) 

    cidrs_subnet = Fn.cidr(ip_block=cfn_vpc.attr_cidr_block,count=max_azs*2,size_mask= size_max) 

4- override the cidr subnets of the vpc previously created :

      #i is the number of Availability zone

      for i in range(max_azs) :

        cidr_pub_subnet = Fn.select(i,cidrs_subnet)
        cfn_subnet: ec2.CfnSubnet = self.vpc.public_subnets[i].node.default_child
        cfn_subnet.add_property_deletion_override("CidrBlock")
        cfn_subnet.add_property_override("CidrBlock", cidr_pub_subnet)

        cidr_priv_subnet = Fn.select(i+max_azs,cidrs_subnet)
        cfn_subnet: ec2.CfnSubnet = self.vpc.private_subnets[i].node.default_child
        cfn_subnet.add_property_deletion_override("CidrBlock")
        cfn_subnet.add_property_override("CidrBlock", cidr_priv_subnet)

Thank you guys for talking about this issue.

@mrpackethead
Copy link
Author

@Crycham , could you provide a working example of what you have done.. I hav'tn quite followed..

@Crycham
Copy link

Crycham commented Aug 8, 2022

@Crycham , could you provide a working example of what you have done.. I hav'tn quite followed..

You need before that to create the ipam pool to have an ipam pool ID than you deploy this stack :

    max_azs = 2
    self.vpc = ec2.Vpc(self, 'VPN',
        cidr = '9.0.0.0/16',
        max_azs = max_azs,
        enable_dns_hostnames = True,
        enable_dns_support = True, 
        # configuration will create 2 subnets in a 2 AZ.
        subnet_configuration=[
            ec2.SubnetConfiguration(
                name = 'Public-Subnet',
                subnet_type = ec2.SubnetType.PUBLIC,
            ),
            ec2.SubnetConfiguration(
                name = 'Private-Subnet',
                subnet_type = ec2.SubnetType.PRIVATE_WITH_NAT,
            )
        ],
        nat_gateways = 2,
        nat_gateway_subnets=ec2.SubnetSelection(subnet_group_name="Public-Subnet"),
        nat_gateway_provider=ec2.NatProvider.gateway(eip_allocation_ids=[elastic_ip_id,elastic_ip_id2]),
  
    )

    #import the pool id
    Ipam_pool_id = Fn.import_value("Poolid")
    Ipv4NetmaskLength = 16
    Ipv4NetmaskLength = Ipv4NetmaskLength.numerator
    # Override vpc construct
    cfn_vpc: ec2.CfnVPC = self.vpc.node.default_child
    cfn_vpc.add_property_deletion_override("CidrBlock")
    cfn_vpc.add_property_override("Ipv4IpamPoolId", Ipam_pool_id)
    cfn_vpc.add_property_override("Ipv4NetmaskLength", Ipv4NetmaskLength)

    # make sure to have dynamically the proper division of subnets
    # if N is a power of two simply return it
    N = max_azs*2
    if not (N & (N - 1)):
        size_max = str(32-(Ipv4NetmaskLength+int(math.log2(N))))
    else :
    # else set only the left bit of most significant bit
        size_max = str(32-(Ipv4NetmaskLength+int(math.log2(int("1" + (len(bin(N)) - 2) * "0", 2)))))

    cidrs_subnet = Fn.cidr(ip_block=cfn_vpc.attr_cidr_block,count=max_azs*2,size_mask= size_max)
                                                                                   
    for i in range(max_azs) :
        cidr_pub_subnet = Fn.select(i,cidrs_subnet)
        cfn_subnet: ec2.CfnSubnet = self.vpc.public_subnets[i].node.default_child
        cfn_subnet.add_property_deletion_override("CidrBlock")
        cfn_subnet.add_property_override("CidrBlock", cidr_pub_subnet)

        cidr_priv_subnet = Fn.select(i+max_azs,cidrs_subnet)
        cfn_subnet: ec2.CfnSubnet = self.vpc.private_subnets[i].node.default_child
        cfn_subnet.add_property_deletion_override("CidrBlock")
        cfn_subnet.add_property_override("CidrBlock", cidr_priv_subnet) 

You will observe after deploying that the cidr : 9.0.0.0/16 was not used by CF and all it works like you have given the ipam pool id instead

@mrpackethead
Copy link
Author

I like the idea your following.. your code works for the case when all the subnets are equal, so a useful corner case, but it will not quite cover all the use cases.

I my example here, I'm wanting to get 6 subnets that are of /28.. and leave some spares so that i can add them as i need. and they might not be of the same mask.

Some food for thought, here though.. Modifying the subnets with escape hatches is an interesting idea. Off to try something

self.vpc = ec2.Vpc(self, f'thing{stage_name}',
			nat_gateways = 0, 
			max_azs=2,
			cidr = '10.18.0.0/24',
			#cidr = getcidr.get_response_field('IpamPoolAllocations.0.Cidr'),
			vpc_name = f'thing-{stage_name}',
			subnet_configuration= [
				ec2.SubnetConfiguration(
					name="database",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="glue",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="endpoints",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				)
			]
		)

@Crycham
Copy link

Crycham commented Aug 8, 2022

I like the idea your following.. your code works for the case when all the subnets are equal, so a useful corner case, but it will not quite cover all the use cases.

I my example here, I'm wanting to get 6 subnets that are of /28.. and leave some spares so that i can add them as i need. and they might not be of the same mask.

Some food for thought, here though.. Modifying the subnets with escape hatches is an interesting idea. Off to try something

self.vpc = ec2.Vpc(self, f'thing{stage_name}',
			nat_gateways = 0, 
			max_azs=2,
			cidr = '10.18.0.0/24',
			#cidr = getcidr.get_response_field('IpamPoolAllocations.0.Cidr'),
			vpc_name = f'thing-{stage_name}',
			subnet_configuration= [
				ec2.SubnetConfiguration(
					name="database",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="glue",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="endpoints",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				)
			]
		)

Actually we could have a custom distribution of subnets I have just choosen in my case to provide the max spaces to my subnets from the vpc Cidr block but we could control that from this function :

  cidrs_subnet = Fn.cidr(ip_block=cfn_vpc.attr_cidr_block,count=max_azs*2,size_mask= size_max)

In your case if you want to allocate 6 subnets having as a net mask /28 and saving some spaces you need to calculcate first the max subnets you could have for /28 which is 16 subnets https://www.site24x7.com/fr/tools/ipv4-sous-reseau-calculatrice.html
use this as a parameter for "count" and size max from /28 ( 32 - 28 = 4 ) :

  cidrs_subnet = Fn.cidr(ip_block=cfn_vpc.attr_cidr_block,count=16,size_mask= 4)

note : the size mask here is not the net mask but the host mask ( host mask = 32 - netmask ) so here size mask is = 32-28 = 4
try this configuration you will have spaces free for futur subnets. cidrs_subnets is a list of all subnet cidr you could override after with the loop as much as you need.

@mrpackethead
Copy link
Author

mrpackethead commented Aug 8, 2022

I think this will cover the general case. I've taken a slightly different approach, to what you did. think of this more like a 'address' translation... This should work for any method where the ec2.Vpc construct allocates subnets..

@corymhall , this was a quick hack up in python.. I think the method will work just fine for the ec2.Vpc.. For the time being, i'll rewrite this as a jsii construct, so we can move on.. but then i think we can probalby make this work inside cdk natively.

from aws_cdk import (
	aws_ec2 as ec2,
)
import constructs
import aws_cdk as cdk
import struct
import socket

def ip2long(ip):
	packedIP = socket.inet_aton(ip)
	return struct.unpack("!L", packedIP)[0]


class IpamvpcStack(cdk.Stack):

	def __init__(self, scope: constructs.Construct, construct_id: str, **kwargs) -> None:
		super().__init__(scope, construct_id, **kwargs)
		
		vpc_cidr_mask = 24

		cidr_allocation = ec2.CfnIPAMAllocation(self, "IPAMAllocation",
			ipam_pool_id="ipam-pool-0afcad222ed575c58",
			netmask_length=vpc_cidr_mask,
		)

		# create a VPC, assign address space from ipam
		self.vpc = ec2.Vpc(self, f'vpc',
			nat_gateways = 0, 
			max_azs=2,
			cidr = f'0.0.0.0/{vpc_cidr_mask}',  # THIS IS DELIBERATE! dont' change from 0/0
			vpc_name = f'vpc',
			subnet_configuration= [
				ec2.SubnetConfiguration(
					name="subnetgroup1",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="subnetgroup2",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				),
				ec2.SubnetConfiguration(
					name="subnetgroup3",
					subnet_type=ec2.SubnetType.PRIVATE_ISOLATED,
					cidr_mask = 28,
				)
			]
		)
		
		
		# Override vpc construct
		cfn_vpc: ec2.CfnVPC = self.vpc.node.default_child
		cfn_vpc.add_property_deletion_override("CidrBlock")
		cfn_vpc.add_property_override("Ipv4IpamPoolId", cidr_allocation.ipam_pool_id)
		cfn_vpc.add_property_override("Ipv4NetmaskLength", cidr_allocation.netmask_length)


		# Overide the subnets
		for subnet_type in [self.vpc.private_subnets, self.vpc.isolated_subnets, self.vpc.public_subnets]:
			for index, subnet in enumerate(subnet_type):				
			
				subnet_addr = subnet.ipv4_cidr_block.split('/')[0]
				subnet_mask = int(subnet.ipv4_cidr_block.split('/')[1])
				subnet_postion = (int(ip2long(subnet_addr) / (32-subnet_mask)**2) + 1 )
				new_subnet_cidr = cdk.Fn.cidr(ip_block=cfn_vpc.attr_cidr_block,count=subnet_postion,size_mask= str(32-subnet_mask))
				cidr_pub_subnet = cdk.Fn.select(subnet_postion,new_subnet_cidr)
				cfn_subnet: ec2.CfnSubnet = subnet_type[index].node.default_child
				cfn_subnet.add_property_override("CidrBlock", cidr_pub_subnet)

@Crycham
Copy link

Crycham commented Aug 8, 2022

@mrpackethead Well done , this will cover all possible use cases. @corymhall @mrpackethead I'm glad to have this discussion with you and brainstorming about this issue that is finally solved with this hack up, hope that this gonna be taken is consideration for next updates to use it directly in ec2.vcp.

@mrpackethead
Copy link
Author

mrpackethead commented Aug 9, 2022

Typescript construct

import * as cdk from 'aws-cdk-lib'
import * as constructs from 'constructs';
import { 
  aws_ec2 as ec2
}
from 'aws-cdk-lib';

function ipToInt32(ip: string){
	const ipSplit = ip.split('.');
	var ipInt: number = 0;
	ipSplit.forEach((value, index) => {
		var octect: number = value as unknown as number;
		ipInt += octect << (24-index*8)
	})
	return ipInt
}

export interface IpamVpcProps extends ec2.VpcProps {
	netmaskLength?: number;
	ipamPoolId?: string;
	cidr?: string
}
  
export class IpamVpc extends constructs.Construct {

	public readonly ipamAllocationId: ec2.CfnIPAMAllocation;
	public readonly Vpc: ec2.Vpc

	constructor(scope: constructs.Construct, id: string, props: IpamVpcProps) {
		super(scope, id);

		const ipamPoolId = props.ipamPoolId
		const netmaskLength = props.netmaskLength

		// remove these from the props object...
		delete props.ipamPoolId
		delete props.netmaskLength

		// set the cidr to 0.0.0.0 so we can overide later
		props.cidr = `0.0.0.0/${netmaskLength}`

		// get an IP address allocation for the VPC
		const cidrAllocation = new ec2.CfnIPAMAllocation(this, "IPAMAllocation", {
			ipamPoolId: <string>ipamPoolId,
			netmaskLength: netmaskLength
		})

		this.ipamAllocationId = cidrAllocation

		const vpc = new ec2.Vpc(this, 'Vpc', {
			...props
		})

		this.Vpc = vpc

		// override the VPC
		const CfnVPC = vpc.node.defaultChild as ec2.CfnVPC;
		CfnVPC.addPropertyDeletionOverride("CidrBlock");
		CfnVPC.addPropertyOverride("Ipv4IpamPoolId", cidrAllocation.ipamPoolId);
		CfnVPC.addPropertyOverride("Ipv4NetmaskLength", cidrAllocation.netmaskLength);


		// Overide the subnets
		[vpc.privateSubnets, vpc.isolatedSubnets, vpc.publicSubnets].forEach(( subnetType ) => {
	
			subnetType.forEach((subnet, index) => {
				var subnetInt = ipToInt32(subnet.ipv4CidrBlock.split('/')[0])
				var subnetMask: number = subnet.ipv4CidrBlock.split('/')[1] as unknown as number
				var subnetPostion = subnetInt / (32-subnetMask)**2 + 1
				
				var cfnSubnet = subnet.node.defaultChild as ec2.CfnSubnet
				cfnSubnet.addPropertyOverride("CidrBlock",
					cdk.Fn.select(
						subnetPostion,
						cdk.Fn.cidr(CfnVPC.attrCidrBlock, subnetPostion, `${32-subnetMask}`)
					)
				)
			}) // end of subnet
		})
	}
}

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 10, 2022

What I think we're going to want in the Vpc construct is something like an IpAllocation class. It will have multiple implementations, maybe IpAllocation.fromCidr(), and then we can also have IpAllocation.fromIpam(). It could also have some more variants, and be a subclassable class so people can substitute their own strategies.

I have to say I'm not too familiar with what IPAM is, I'll have to read up on that.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 10, 2022

After looking into IPAM a bit, it seems we need:

  • An IPAM
  • A scope
  • A pool
  • An allocation with a netmask for each subnet.

It's not clear to me why a pool doesn't seem to have a size, the implication being that it dynamically grows but there must be a maximum to this?

And the scope can potentially come for free from "an IPAM" itself, as they have default "public" and "private" scopes.

But potentially IpAllocation.fromIpam(ipamPool) would be sufficient?

@Crycham
Copy link

Crycham commented Aug 10, 2022

Heyy @rix0rrr hope you have a good day, I'm glad that you have joined our discussion. Actually a pool does have a size block but you need to make difference between pools type : the top level pool and subpools.

What IPAM needs in general is the cidr block from which we gonna dynamically allocate IP addresses to VPCs.

Imagine the following use case :
I want that VPCs in two different region to communicate and make sure they'll have a dynamic IP allocation without overlapping.
We first create a top level pool using this cidr block 10.0.0.0/8, then we create for each region a regional pool (subpool) using a sub IP addresses 10.0.0.0/12 for region1 and 10.1.0.0/12 for region 2.
Finally we create the vpcs in the two regions and attach each one with its proper pool id from which it gonna retrieve Cidr block, we have to mention also the net mask which must be higher than 12, exemple 16 like my previous example.

You can find my full IPAM and vpc configuration in my pull request here : aws-samples/aws-cdk-examples#708 and https://github.com/Crycham/aws-cdk-examples/tree/03e2c6ba39972606c1a3d1786065c0ac65f1d536/python/Ipam-L2-vpc

And this is the IPAM configuration alone :

from constructs import Construct
from aws_cdk import (
    CfnOutput,
    Stack,
    aws_ec2 as ec2,
)


class IpamStack(Stack):

 def __init__(self, scope: Construct, construct_id: str,cidr_range : str,region_cidr_range : str, **kwargs) -> None:
    super().__init__(scope, construct_id, **kwargs)

    # The code that defines your stack goes here

    
    # Ipam creation
    cfn_iPAM = ec2.CfnIPAM(self, "MyCfnIPAM",
        description="description",
        operating_regions=[ec2.CfnIPAM.IpamOperatingRegionProperty(region_name=self.region)]
    )

    # Top level ipam pool creation used by accounts or regions
    cfn_Top_IpamPool = ec2.CfnIPAMPool(self, "TOP-CfnIPAMPool",
        address_family="ipv4",
        ipam_scope_id=cfn_iPAM.attr_private_default_scope_id,

        description="top-level-pool",
        provisioned_cidrs=[ec2.CfnIPAMPool.ProvisionedCidrProperty(
            cidr=cidr_range
        )],
    )

    # region level ipam pool used by regions

    cfn_Region_iPAMPool = ec2.CfnIPAMPool(self, "Local-CfnIPAMPool",
        address_family="ipv4",
        ipam_scope_id=cfn_iPAM.attr_private_default_scope_id,
        source_ipam_pool_id=cfn_Top_IpamPool.attr_ipam_pool_id,

        description="region-level-pool",
        locale=self.region,
        provisioned_cidrs=[ec2.CfnIPAMPool.ProvisionedCidrProperty(
            cidr=region_cidr_range
        )],
        auto_import= True
        
    )


    # dependencies 
    cfn_Region_iPAMPool.add_depends_on(cfn_Top_IpamPool)
    cfn_Top_IpamPool.add_depends_on(cfn_iPAM)

    # output ressources
    ipam_pool_id = cfn_Region_iPAMPool.attr_ipam_pool_id
    self.ipam_pool_id = ipam_pool_id

    # ipam pool id used in VPC stack and ipamid for deletion
    CfnOutput(self,"ipampoolid",value=ipam_pool_id,description="Ipam pool id",export_name="Poolid")
    CfnOutput(self,"ipamid",value=cfn_iPAM.attr_ipam_id,description="Ipamid",export_name="ipamid")

@nbaillie
Copy link
Contributor

Hi.. Just wanted to mention here.

I have been working on an early draft RFC to look at VPC and how it could change for the better.
Started by collating some of the items raised on GitHub along with some of my experience working in larger multi account enterprises and organisations.

Integration of IPAM, provision to work with NetworkFirewall and changes to the API are some of topics covered.

As i say its in early draft, rix0rrr has had a quick look and provided some comments that i am working through and expanding on before submitting as a PR.

My expectation is the initial draft would be far from perfect but provide a starter for the discussion.

@Crycham
Copy link

Crycham commented Aug 10, 2022

Hi @nbaillie, thank you for your attention about this subject, I'm happy to hear that the integration of IPAM will be one of the future features in the L2 vpc Construct. Glad to help as much as I can :)

@mrpackethead
Copy link
Author

  • An IPAM
  • A scope
  • A pool

These are all sufficently supported now with the cfn Types, there may be some value in creating some convience L2 things, but functionally they work..

  • An allocation with a netmask for each subnet.

So the underlying AWS::EC2::Subnet does not have a Ipam property.. its expecting an actual string to represent the Cidr for the subnet. As far as I can see the cloudformation design intent is that your subnets are numbered using Fn::Cidr, based on the Vpcs address. ( added complication is that a Cidr now can have more than one block ! though i've never used that. That being the case there is no need for allocations for each subnet.. just for the VPC itself..

My workaround did'tn attempt to work out the address allocations.. it essentially 'shifts' the allocations that the construct had already done..

It's not clear to me why a pool doesn't seem to have a size, the implication being that it dynamically grows but there must be a maximum to this?

I guess there will be some limit, but you can add add additional ( non contigous too ) address space to your pools as required.

But potentially IpAllocation.fromIpam(ipamPool) would be sufficient?

yes.

@mrpackethead
Copy link
Author

@nbaillie , as my current need is addressed by my workaround, The way forward seems to be to work together on this.. ec2.Vpc is one of the constructs that we really dont' want to break.. So much stuff has upstream dependancys on it...

@corymhall corymhall removed their assignment Aug 25, 2022
@mrpackethead
Copy link
Author

I've been looking at the ec2.Vpc code and how to integrate iPam in.. based on my previous work above..
This is part of what i'm thinking about..

Essentially, adding a couple of extra props for the ipam pool and length.

If the Ipam pool is set then the cidr will get set to 0.0.0.0/mask, (for network builder to use)
and then by using some overides we delete cidr and set the ipam pool address in the underlying cf resource.

this is the bit that gets changed in ec2.Vpc

    if (props.ipv4IpamPoolId !== undefined) {
      
      if (!Token.isUnresolved(props.ipv4IpamPoolId)){
        if (!props.ipv4IpamPoolId.startsWith('ipam-pool-')) {
          throw new Error('ipv4IpamPoolId must be a valid ipam-pool');
        }
      }
      if (props.ipv4NetworkmaskLength !== undefined) {
        if (props.ipv4NetworkmaskLength  < 16 || props.ipv4NetworkmaskLength > 28) {
          throw new Error('ipv4NetworkmaskLength must be in the range 16-28');
        }
        const networkMaskLength = props.ipv4NetworkmaskLength
      }
      else {
        throw new Error('ipv4NetworkmaskLength must be in the range 16-28')
      }      
      const cidrBlock = `0.0.0.0/${props.ipv4NetworkmaskLength}`;
      this.networkBuilder = new NetworkBuilder(cidrBlock);
    }
    else {
      const cidrBlock = ifUndefined(props.cidr, Vpc.DEFAULT_CIDR_RANGE);
      if (Token.isUnresolved(cidrBlock)) {
        throw new Error('\'cidr\' property must be a concrete CIDR string, got a Token (we need to parse it for automatic subdivision)');
      }
      this.networkBuilder = new NetworkBuilder(cidrBlock);
    }

    this.dnsHostnamesEnabled = props.enableDnsHostnames == null ? true : props.enableDnsHostnames;
    this.dnsSupportEnabled = props.enableDnsSupport == null ? true : props.enableDnsSupport;
    const instanceTenancy = props.defaultInstanceTenancy || 'default';
    this.internetConnectivityEstablished = this._internetConnectivityEstablished;

    
    this.resource = new CfnVPC(this, 'Resource', {
      cidrBlock,
      enableDnsHostnames: this.dnsHostnamesEnabled,
      enableDnsSupport: this.dnsSupportEnabled,
      instanceTenancy,
    });
    
    // if the ipam address pool exisit delete the CidrBlock and add Ipam pool and mask
    // for the vpc to obtain.
    if (props.ipv4IpamPoolId) {
      this.resource.addPropertyDeletionOverride("CidrBlock");
      this.resource.addPropertyOverride("Ipv4IpamPoolId", props.ipv4IpamPoolId);
      this.resource.addPropertyOverride("Ipv4NetmaskLength", props.ipv4NetworkmaskLength);
    }

I'll now have a look at the networkbuilder code to set the subnet address as a Fn.Cidr rather than a concrete value.

@mrpackethead
Copy link
Author

I think its very important to distinguish what IPAM does, and what a 'network' builder does..

An IPAM provider provides an IP address allocation..
The network builder does the job of 'shaping' your VPC, subnets, and the likes..

Lets not confuse the two, and keep them seperate.

@mergify mergify bot closed this as completed in #22458 Oct 27, 2022
mergify bot pushed a commit that referenced this issue Oct 27, 2022
Allows Vpc to Use [Aws IPAM](https://docs.aws.amazon.com/vpc/latest/ipam/what-it-is-ipam.html) for Ip address assignment:

```ts
import { IpAddresses } from '@aws-cdk/aws-ec2';

declare const pool: ec2.CfnIPAMPool;

new ec2.Vpc(stack, 'TheVPC', {
  ipAddresses: ec2.IpAddresses.awsIpamAllocation({
    ipv4IpamPoolId: pool.ref,
    ipv4NetmaskLength: 18,
    defaultSubnetIpv4NetmaskLength: 24
  })
});
```

This is useful for enterprise users that wish to adopt the benefits of centralised IP address management.

It introduces `ipAddresses` property to allow the new configuration.

----

Thanks to @rix0rrr for support on this.

---

closes #21333

----

#22443 - Issue adds a fix to allow the clean up of the AWS Ipam resource used in ingeg-test testing. Would be better to implement something like this later. for now disclaimer added to integ-test clean up needed on Ipam.

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### New Features

* [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants