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

feat(ec2): Vpc supports allocating CIDR from AWS IPAM #22458

Merged
merged 36 commits into from Oct 27, 2022

Conversation

nbaillie
Copy link
Contributor

@nbaillie nbaillie commented Oct 11, 2022

Allows Vpc to Use Aws IPAM for Ip address assignment:

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:

New Features

  • Have you added the new feature to an integration test?
    • 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

@gitpod-io
Copy link

gitpod-io bot commented Oct 11, 2022

@github-actions github-actions bot added effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1 labels Oct 11, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team October 11, 2022 08:46
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 11, 2022
packages/@aws-cdk/aws-ec2/lib/vpc.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ec2/README.md Outdated Show resolved Hide resolved
@mergify mergify bot dismissed rix0rrr’s stale review October 18, 2022 16:20

Pull request has been modified.

@mrpackethead
Copy link

This looks sensible, however i'd urge care here that this does not cause unintended breaking changes.

I noted that Network Builder as been removed, and replaced with IPAMProvider.

There are many cases where someone is not going to use IPAM. Its confusing to use the term IPAM, when the IPAM service is not part of things doesn't make sense. Its not the job of 'IPAM' to calculate subnets.. Its the job of IPAM to assign Pools of Address space, ( and monitor them )..

NetworkBuilder was not a bad name, becuase it described what it was doing.. IPAM is not a good name.

@nbaillie nbaillie marked this pull request as ready for review October 24, 2022 11:45
@nbaillie nbaillie requested a review from rix0rrr October 26, 2022 08:43
@rix0rrr rix0rrr self-assigned this Oct 26, 2022
@rix0rrr rix0rrr changed the title feat(ec2): Vpc Support for AWS Ipam integration feat(ec2): Vpc supports allocating CIDR from AWS IPAM Oct 26, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 26, 2022

I have some final commits pushed to Neil's branch, but they're not appearing here. Can't approve until I see them and I know they are in the validation pipeline.

rix0rrr
rix0rrr previously approved these changes Oct 26, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 26, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2022

refresh

✅ Pull request refreshed

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mrpackethead
Copy link

Sorry a late comment, but only about docs.

Something that might cause pain , is if we try to request an ipam allocation from a pool that has rules applied to it, restricting which size networks are able to be allocated. eg, if you asked for a /20 but the rules say you can only have between /22 - /24.

A once sentance addtion could save a lot of fustration.

@mergify mergify bot dismissed rix0rrr’s stale review October 26, 2022 20:18

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 132f18d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7ed9cd1 into aws:main Oct 27, 2022
@iamed2
Copy link

iamed2 commented Jan 20, 2023

Thank you very much for this. We have been stuck with L1 constructs only until now; this change now makes CDK a usable and useful product for us to replace raw CloudFormation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK 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 this pull request may close these issues.

ec2.Vpc to support using ipam
6 participants