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

RFC 507: Full control over VPC and subnet configuration #508

Merged
merged 22 commits into from
Jun 21, 2023

Conversation

otaviomacedo
Copy link
Contributor

@otaviomacedo otaviomacedo commented May 26, 2023

This is a request for comments about a new API in the aws-ec2 module to give users full control over VPC and subnet configuration. See #507 for additional details.

APIs are signed off by @corymhall

Rendered version


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where this is going!

text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
Comment on lines 155 to 166
const natGateway = publicSubnet.addNatGateway('natgw', {elasticIp});

const routeTable = vpc.addRouteTable('routeTable', {
routes: [
Route.to({
destination: '0.0.0.0/0',

// targets must implement the IRouter interface
target: natGateway,
}),
],
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect people to do this 3 times? What does that look like, and are we still happy with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an alternative that allows users to define 3 identical route tables without repeating the properties? A clone() method, perhaps?

const routeTable = vpc.addRouteTable(...);

vpc.addSubnet('sn1', {
  ...,
  routeTable,
});

vpc.addSubnet('sn2', {
  ...,
  routeTable: routeTable.clone(),
});

vpc.addSubnet('sn3', {
  ...,
  routeTable: routeTable.clone(),
});

Then again, if you are defining your own route tables, why would you do this, instead of reusing the same one?

text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
text/0507-subnets.md Outdated Show resolved Hide resolved
@otaviomacedo otaviomacedo requested a review from rix0rrr May 30, 2023 10:35
@otaviomacedo otaviomacedo marked this pull request as ready for review June 3, 2023 04:08
Copy link

@tmokmss tmokmss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments and questions from a frequent user of Vpc. Looking forward to using this feature soon! 🚀

```ts
const subnet = vpc.addSubnet('subnet', {
cidrBlock: '10.2.0.0/20',
availabilityZone: 'us-west-2a'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is availabilityZone optional? If not, is it possible to make this construct region-agnostic?

more secondary IP blocks, by providing them in the CIDR format:

```ts
const vpc = new VpcV2(this, 'vpc', {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does VpcV2 implements IVpc interface? I think it's necessary for compatibility with other constructs.


They will serve two separate sets of customers, with different use cases.
For example, `Vpc` creates all the subnets on behalf of the user, while
`VpcV2` will not create any subnet other than the ones requested by the user.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first impression to the name VpcV2 was the current Vpc construct will be deprecated. But after reading this RFC carefully, it seems that they will coexist for different purposes. (Vpc for simple use case, and VpcV2 for advanced users.)

I'm afraid that users might get confused and misunderstand that they should not use the Vpc construct any more.

### Why all the `.addXxx()` methods?

To keep the current way of organizing constructs in the tree, in which
everything that is logically part of the VPC is a direct or indirect
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, so migrating from Vpc to VpcV2 is not very difficult, right? I'd like to see reference to migration in this FAQ section as well.

@mrpackethead
Copy link
Contributor

Its not clear to me, but it seems SubnetGroups have dissapeared somewhere.

Subnet groups are an exceptionally powerful things.; I like having an abstracted 'routing table' associated with a subnetgroup. ( but underneath it still has individual routing tables for each subnet )..

I don't think that its desirable to have people needing to add a routing table...

If you want to do this kind of thing, you can ( using the L1 Constructs which is painful ).

ec2.Vpc, is a L3 Construct, and this ec2.Vpc2 will be to. I think the way forward here, is not to create another L3 Construct in the library. ( But by all means, create them for your own use cases .. What would be much more valuable is to create a solid set of L2 Constructs.

For me, I' have been able to extend ec2.Vpc to do some things that are not easy otherwise. But really, if there had been some Good L2 Constructs, i'd probalby have started there.

Underneath, i need subnets to use endpoints ( for cloudwan, transit gateways, firewalls etc ) that are in their specific AZ, but i don't want to have to consider that when i'm building my vpc.. Its a deployment detail, that the construct can deal with.

I also need to be able to route to specific subnets, within my vpc from other subnets, so that i can force traffic via firewalls. this is a feature that was added a while ago, so you can route to smaller routes than the vpc's cidr.

I've essentially created my own EVpc which wraps around eVpc to provide some extra methods to the exisiting vpc. This is to meet MY specific use-cases. They are opinionated and solve my problems.

My fear is that the Vpc2 construct seems more opinionated than the Vpc construct, and while it will meet the needs of some people, for their quite specific use cases, it wont' actually help most people.

Could we have some examples of where the current ec2.Vpc does not

// Define subnetGroups, for use in the enterprise VPC

    const linknet = new network.SubnetGroup(this, 'linknet', {name: 'linknet', subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS, cidrMask: 26});
    const outside = new network.SubnetGroup(this, 'outside', {name: 'outside', subnetType: ec2.SubnetType.PUBLIC, cidrMask: 28});
    const endpoints = new network.SubnetGroup(this, 'endpoints', {name: 'endpoints', subnetType: ec2.SubnetType.PRIVATE_ISOLATED, cidrMask: 24});


sharedServiceVpc.router([
      {
        subnetGroup: linknet,
        routes: [
          { cidr: '10.0.0.0/8', destination: network.Destination.CLOUDWAN, description: 'linknetAllPrivateNetworks' }
        ]
      },
      {
        subnetGroup: endpoints,
        routes: [
          { cidr: '10.0.0.0/8', destination: network.Destination.CLOUDWAN, description: 'endpointsAllPrivateNetworks' }
        ]
      },
      {
        subnetGroup: outside,
        routes: [
          { cidr: '10.0.0.0/8', destination: network.Destination.CLOUDWAN, description: 'outsideAllPrivateNetworks' }
        ]
      },
    ]);

@corymhall
Copy link
Contributor

ec2.Vpc, is a L3 Construct, and this ec2.Vpc2 will be to. I think the way forward here, is not to create another L3 Construct in the library. ( But by all means, create them for your own use cases .. What would be much more valuable is to create a solid set of L2 Constructs.

+1 to this. I think the first step is to create solid L2s for everything so a user could create their own Vpc from scratch. We could then build more utility on top of that.

* **Tracking Issue**: #507
* **API Bar Raiser**: @{BAR_RAISER_USER}

New L2 constructs to allow for greater control over VPC design. Among other
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be interesting to see a breakdown of all the L1s that are used in creating a Vpc and which ones we do not have L2 coverage for. To me this breaks down into 3 steps

  1. Show how to create the current default Vpc with a new complete set of L2s.
  2. Show how to create a complicated non-default Vpc with the new set of L2s.
  3. Can we make the above two easier with opinionated abstractions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be quite a short list.

  • the vpc itself. ( its kind of unforuantate that ec2.Vpc was called Vpc, becuase a vpc in cdk and a vpc in the rest of amazon are two differnet thigns )
  • subnets
  • route tables.

@mergify mergify bot dismissed corymhall’s stale review June 6, 2023 07:51

Pull request has been modified.

text/0507-subnets.md Outdated Show resolved Hide resolved
@otaviomacedo otaviomacedo merged commit 82577ee into master Jun 21, 2023
1 of 2 checks passed
@otaviomacedo otaviomacedo deleted the otaviom/subnets branch June 21, 2023 16:08
@evgenyka evgenyka added the l2-request request for new L2 construct label Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l2-request request for new L2 construct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants