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

VPC construct with full control #5927

Open
2 tasks
rix0rrr opened this issue Jan 23, 2020 · 21 comments
Open
2 tasks

VPC construct with full control #5927

rix0rrr opened this issue Jan 23, 2020 · 21 comments
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. in-progress This issue is being actively worked on. p1

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 23, 2020

Many people are not satisfied with the out of the box VPC experience we provide.

They have slightly different IP layout needs, or AZ selection or subnet layout or routing table, or don't like the hierarchy/naming used. The current VPC is not flexible enough.

Provide a compatible Vpc construct that can be configured to users' heart's content.

Creating this issue to link all others to.

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

This is a 🚀 Feature Request

Related: #6683

@rix0rrr rix0rrr added feature-request A feature should be added or improved. @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud needs-triage This issue or PR still needs to be triaged. package/vpc labels Jan 23, 2020
@rix0rrr rix0rrr added the effort/large Large work item – several weeks of effort label Jan 23, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jan 28, 2020
@Vlaaaaaaad
Copy link

It seems that by default there is no way to add an additional CIDR to the VPC. This is a blocker for larger infrastructures: in my case, I cannot use the CDK to create the VPC for any large EKS clusters.

@kert-io
Copy link

kert-io commented Feb 18, 2020

CDK should follow the cloud-formation interface! Do not tie our hands, let us build. If you want to add a "convenience" wrapper, do so, but do not force this on us. CDK is a tool not a prescription for how to build.

@mikepietruszka
Copy link

mikepietruszka commented Feb 26, 2020

I agree that this is an issue. Base VPC construct has way too many assumptions. For example it tries to create 3x public and 3x private subnets even if optional subnet_configuration is not specified.

Should we be using CfnVpc construct instead?

@trondhindenes
Copy link

We're finding the same. We use Cfn classes for 90% of the stuff we do in CDK because of the highly opinionated nature of the higher-level constructs. I wish AWS would find a better way to strike a balance between abastraction and opinionated-ness.
It also needs to be much easier to use the two (Constructs and Cfn) in unison. Right now if you use Cfn your essentially forced to use it all over, since constructs often can't reference Cfn resources.

I LOVE the idea behind CDK, but the current implementation and direction just isn't right. In the meantime, we'll use Cfn resources pretty much exclusively.

@kevinslin
Copy link

kevinslin commented Apr 9, 2020

would like for ability to blacklist/whitelist AZ by name/zone id

usecase:

  • working inside an old AWS account in us-east-1. use1-az3 is an available AZ and unfortunately its mapped to us-east-1a which means that the VPC construct always selects this. this az does not have new instances (eg. no t3.* family) which forces us to use CfnVpc to select newer azs.

proposed addition to Vpc options:

// list of availability zones that won't be used when creating a vpc
+blacklistAzs?: string[]

happy to put in a pull request if we feel good about this change. this would also completely address this issue

@moatazelmasry2
Copy link
Contributor

moatazelmasry2 commented Apr 30, 2020

Hi all, I submitted a PR #7720 implementing more control over the subnet. Would be glad if you can reivew and submit feedback. Thanks

@MentalPower
Copy link

Is this something that needs an RFC?

@foriequal0
Copy link

I've subclassed ec2.Vpc this way, and I found that this is more flexible than before.

export class Vpc extends ec2.Vpc {
  private internetGateway?: CfnInternetGateway;
  private _myInternetConnectivityEstablished = new ConcreteDependable();

  constructor(scope: Construct, id: string, props: Omit<ec2.VpcProps, "subnetConfiguration">) {
    super(scope, id, { ...props, subnetConfiguration: [] });

    // @ts-ignore
    this.internetGatewayId = Lazy.stringValue({
      produce: (_context: IResolveContext): string | undefined => {
        return this.internetGateway?.ref;
      },
    });
    // @ts-ignore
    this.internetConnectivityEstablished = this._internetConnectivityEstablished;
  }

  private getInternetGateway(): [CfnInternetGateway, IDependable] {
    if (this.internetGateway === undefined) {
      const igw = new ec2.CfnInternetGateway(this, "IGW", {});
      const att = new ec2.CfnVPCGatewayAttachment(this, "VPCGW", {
        vpcId: this.vpcId,
        internetGatewayId: igw.ref,
      });

      this.internetGateway = igw;
      this._myInternetConnectivityEstablished.add(att);
    }

    return [this.internetGateway, this.internetConnectivityEstablished];
  }

  public addPublicSubnet(props: Omit<PublicSubnetProps, "vpcId">): PublicSubnet {
    assert(this.availabilityZones.indexOf(props.availabilityZone) !== -1);

    const subnetId = this.publicSubnets.length + 1;
    const [igw, att] = this.getInternetGateway();
    const pub = new ec2.PublicSubnet(this, `PublicSubnet${subnetId}`, {
      availabilityZone: props.availabilityZone,
      vpcId: this.vpcId,
      cidrBlock: props.cidrBlock,
      mapPublicIpOnLaunch: props.mapPublicIpOnLaunch ?? true,
    });
    pub.addDefaultInternetRoute(igw.ref, att);
    this.publicSubnets.push(pub);
    return pub;
  }

  public addPrivateSubnet(props: Omit<PrivateSubnetProps, "vpcId">): PrivateSubnet {
    assert(this.availabilityZones.indexOf(props.availabilityZone) !== -1);

    const subnetId = this.privateSubnets.length + 1;
    const priv = new ec2.PrivateSubnet(this, `PrivateSubnet${subnetId}`, {
      availabilityZone: props.availabilityZone,
      vpcId: this.vpcId,
      cidrBlock: props.cidrBlock,
      mapPublicIpOnLaunch: props.mapPublicIpOnLaunch ?? false,
    });
    this.privateSubnets.push(priv);
    return priv;
  }
}

Then I can design my subnet as I want.

const vpc = new Vpc(...);

const networkBuilder = new NetworkBuilder(props.cidrBlock);
for(const availabilityZone of this.availabilityZones) {
  const nb = new NetworkBuilder(networkBuilder.addSubnet(18));
  const priv = vpc.addPrivateSubnet({
    availabilityZone,
    cidrBlock: nb.addSubnet(19),
  });
  const pub = vpc.addPublicSubnet({
    availabilityZone,
    cidrBlock: nb.addSubnet(20),
  });
  const nat = pub.addNatGateway();
  priv.addDefaultNatRoute(nat.ref);
}

Also it solves some issues with CfnVpc (#11406)

@flemjame-at-amazon
Copy link
Contributor

I think there's value in the existing VPC construct -- it does a lot of heavy lifting for you in the constructor, like error checking and creating the more mundane resources like route tables. Custom VPCs might want to keep lots of that functionality, but modify specific aspects of the VPC -- for example, creating subnets might be a function that could be overridden, but everything else about the current VPC construct is desired.

What about refactoring the VPC constructor code, breaking it down into a more modular form? That way, custom logic could be provided for only some aspects, with the "default" logic being exactly what's there now.

@calvinwyoung
Copy link

@flemjame-at-amazon IMO this would be the ideal implementation

@venkat513
Copy link

venkat513 commented Jan 10, 2022

vpc construct is at high level where the new vpc creation is made very easy at case. and subnets can be referred easily by type. But now the problem :
for a existing vpc
1.iam trying to add subnets it is creating individual route tables for each subnet.
2.The subnet association to a common Route Table is hard for a group of new subnets (for ex DB subnets)

i have to pass on the cidr's and hard code the subnets in my cdk.json for creation and I cannot run the [deploy all ]stacks in my case.

Have to deploy the vpc stuff in the beggining and the use the subnet values created in the console again as subnets list in the cdk.json.

@fitzoh
Copy link

fitzoh commented Jun 14, 2022

Control over AZ selection just shipped in 2.28 with #20562

@azatoth
Copy link
Contributor

azatoth commented Sep 29, 2023

@rix0rrr I notice a few sub-tickets has been closed due to staleness, for example #7073
Would it be possible for you to address this issue. imo it doesn't make sense to close an issue just because no one has fixed it.

@lumattr
Copy link

lumattr commented Apr 5, 2024

Honestly just having the ability to manually define the cidr would go a long way.

As with a lot on companies im sure (basically every one iv worked at), we have a manually created network that was created a number of years ago, before CDK, before the new subnet grouping and nicer management features we have now. I want to be able to import our existing network into CDK, but there doesnt seem to be a clean way to do that at the moment.

@adamjkeller
Copy link
Contributor

Hey y’all,

As part of our goal to improve service coverage with CDK constructs, we are actively working on this feature! While we can’t commit to specific dates, we’re making progress and will keep you updated along the way. Your feedback and support are helping us shape the roadmap, so thank you!

@adamjkeller adamjkeller added the in-progress This issue is being actively worked on. label May 23, 2024
mergify bot pushed a commit that referenced this issue Aug 7, 2024
### Issue # (if applicable)

Closes [RFC#507](https://github.com/aws/aws-cdk-rfcs/blob/57fd92a7f20e242b96885264c12567493f5e867f/text/0507-subnets.md).

Issue#[5927](#5927)

Tracking Ticket #30762

### Reason for this change

This PR implements below RFC for Full Control VPC configuration
Implementing RFC [Full Control VPC](https://github.com/aws/aws-cdk-rfcs/blob/57fd92a7f20e242b96885264c12567493f5e867f/text/0507-subnets.md)


### Description of changes



Experimental API for VPC
Lifecycle Doc: https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md

- Introduced a new alpha module for VPCV2
- Both new class VPCV2 and SubnetV2 implement IVPC and ISubnet for compatibility with other constructs
- Introduced a new IPAM class to support IP address alllocation through IPAM pool.
- Validations of IP ranges assigned to subnet.
- L2 class(Route) to define custom routes under the subnet
- L2s for gateways like Egress only IGW and NATGW.

### Next Steps: 
Iterate on the API with the feedback from community and team to make it ergonomic.
Close on the features listed in [tracking ticket](#30762)  

Will follow the exit criteria for this experimental API as outlined in below doc: 
https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md

### Description of how you validated changes

Added unit tests with current coverage ~70% 

Added integration tests for subnet, vpc and routing features.




### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

Adding a description of #31162 to make sure the use-case is considered for the VPC construct being developed as part of this issue.

The existing Vpc construct does not support adding AZs to a VPC without breaking, but it comes close. The SubnetConfiguration allows for a stable cidrMask to be specified, so that adding subnets doesn't impact the CIDRs of existing subnets. The below talks about the case when cidrMask is specified, because when it isn't adding new subnets without changing existing ones will never work.

The existing code loops on subnet cofiguration first then on AZ when creating subnets. For each configuration it adds subnets for each AZ.

this.subnetConfiguration.forEach((configuration)=> (

This means that when adding a new subnet configuration to an existing VPC, the new subnets are added at the end and therefore the update can be performed without changing all existing subnets.
When adding an AZ however, subnets from the new AZ come before subnets from existing AZs and this throws off the CIDR allocations.

This could be addressed without breaking existing customers by adding a configuration parameter to the existing Vpc to specify whether to allocate subnets by configuration first or by AZ first. The default should be to allocate by configuration first so that it's backwards compatible, and users who want to keep the same configuration but add AZs will be able to change the option.

This would allow me to specify a Vpc like this and add AZs without replacing any existing subnets.

    var v = new Vpc(this, "MyVpc", {
      NEW_PARAM: byAz // the new param
      subnetConfiguration: [
        {
          cidrMask: 22,
          subnetType: SubnetType.PUBLIC,
          name: "Public"
        },
        {
          cidrMask: 22,
          subnetType: SubnetType.PRIVATE_WITH_EGRESS,
          name: "Private"
        },
      ],
        availabilityZones: this.availabilityZones.slice(0, N) // here N can be increased to add AZs
    })

Use Case

I have an existing VPC and I want to add AZs. I can't do this today because it will require replacement of all subnets, and this will fail even if it could be tolerated because the new subnets will have CIDRs that clash with existing ones.

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. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

No branches or pull requests