aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.51k stars 3.85k forks source link

VPC construct with full control #5927

Open rix0rrr opened 4 years ago

rix0rrr commented 4 years ago

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.


This is a :rocket: Feature Request

Related: #6683

Vlaaaaaaad commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

usecase:

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 commented 4 years ago

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 commented 4 years ago

Is this something that needs an RFC?

catalyst0 commented 4 years ago

Hello. Is there any update or ETA on when the Subnet CIDR (example, 10.0.1.0/24) can be explicitly specified when using a Builder construct?

foriequal0 commented 4 years ago

I want fo follow some VPC design guides from AWS: Practical VPC Design : it divides VPC into AZs first then into tiers. Building a Modular and Scalable Virtual Network Architecture with Amazon VPC: it divides VPC into tiers and then into AZs. I already have a poorly designed subnets. I need to work around them first and plan for refactoring. However current ec2.Vpc doesn't let me to do that. It would be better if I can list subnet CIDR and AZ specifications to ec2.Vpc.

catalyst0 commented 4 years ago

@foriequal0 Hello. You might want to look into the cfn* constructs.

https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.CfnNatGateway.html

foriequal0 commented 4 years ago

@catalyst0 Thanks! I ended up with using cfn constructs. But I wish there is a better way than wiring manually cfn constructs. ec2.Vpc provides some default routings for such as IGW, NAT. However, cfn* constructs force me to wiring them from bottom to top by myself since L2 constructs doesn't play well with L1 constructs. It was much easier if I could import existing resources to L1 or L2, or converting L1 to L2.

catalyst0 commented 4 years ago

@catalyst0 Thanks! I ended up with using cfn constructs. But I wish there is a better way than wiring manually cfn constructs. ec2.Vpc provides some default routings for such as IGW, NAT. However, cfn* constructs force me to wiring them from bottom to top by myself since L2 constructs doesn't play well with L1 constructs. It was much easier if I could import existing resources to L1 or L2, or converting L1 to L2.

@foriequal0 You are welcome. Yes, the HL constructs are definitely lacking when it comes to more granular control over how you want to deploy your IaC. Another option is to generate your own HL classes from typical patterns that suit your needs where you can have your own classes referencing the Cfn* classes to do things. For example. a class called MakeSNPrivate that accepts a subnet object and creates a route table for it and also creates a route to the NGW supplied.

foriequal0 commented 3 years ago

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 (https://github.com/aws/aws-cdk/issues/11406)

flemjame-at-amazon commented 3 years ago

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 commented 3 years ago

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

venkat513 commented 2 years ago

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 commented 2 years ago

Control over AZ selection just shipped in 2.28 with https://github.com/aws/aws-cdk/pull/20562

azatoth commented 11 months ago

@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 commented 5 months ago

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 commented 3 months ago

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!

baumand-amazon commented 3 weeks ago

Adding a description of https://github.com/aws/aws-cdk/issues/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. https://github.com/aws/aws-cdk/blob/9295a85a8fb893d7f5eae06108b68df864096c4c/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts#L1748 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.