Vpc creates an InternetGateway even if there are no public subnets

peterwooden commented 1 year ago

What happened?

When I create a VPC with no public subnets, it still creates an InternetGateway. This shouldn't exist because there is no way for subnets in a VPC to access the internet if there are no public subnets in that VPC.

Steps to reproduce

new awsx.ec2.Vpc(
  "db-vpc",
  {
    cidrBlock: "10.0.0.0/24",
    subnets: [{ type: "isolated" }],
  }
);

Expected Behavior

The component should only set up the subnets, route tables, and route table associations; and not the internet gateway.

Actual Behavior

See the aws:ec2:InternetGateway being created below:

    Type                                        Name                                                            Plan       Info
     pulumi:pulumi:Stack                         notes-service-infra-staging_us_a
 +   ├─ awsx:x:ec2:Vpc                           db-vpc                                                          create
 +   │  ├─ awsx:x:ec2:Subnet                     db-vpc-isolated-1                                               create
 +   │  │  ├─ aws:ec2:Subnet                     db-vpc-isolated-1                                               create
 +   │  │  ├─ aws:ec2:RouteTable                 db-vpc-isolated-1                                               create
 +   │  │  └─ aws:ec2:RouteTableAssociation      db-vpc-isolated-1                                               create
 +   │  ├─ awsx:x:ec2:InternetGateway            db-vpc                                                          create
 +   │  │  └─ aws:ec2:InternetGateway            db-vpc                                                          create
 +   │  ├─ awsx:x:ec2:Subnet                     db-vpc-isolated-0                                               create
 +   │  │  ├─ aws:ec2:RouteTable                 db-vpc-isolated-0                                               create
 +   │  │  ├─ aws:ec2:Subnet                     db-vpc-isolated-0                                               create
 +   │  │  └─ aws:ec2:RouteTableAssociation      db-vpc-isolated-0                                               create
 +   │  └─ aws:ec2:Vpc                           db-vpc                                                          create

Output of `pulumi about`

No response

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

jaxxstorm commented 1 year ago

This is expected behaviour, see the code comment here

Internet gateways are free AWS resources. Is there a concern with this approach you could share?

mrjackdavis commented 1 year ago

Plus one here.

We have a problem where this is an issue for security compliance. In my circumstance, we have a highly protected resource that has automated security compliance tests run against it. The automatically created IGW violates these tests :(

jaxxstorm commented 1 year ago

thanks for the feedback, we'll get this triaged

flostadler commented 6 months ago

I started looking into this. The one caveat of this is that it would result in a breaking change (at least for typescript, need to check the other SDKs) because the internetGateway output of the VPC would change to being optional (https://github.com/pulumi/pulumi-awsx/blob/adae402632bd5a2b5907745ad1c3b699f2165755/awsx/schema-types.ts#L64). But it's a rather minor fix for the breaking change: adding a null/undefined check.

Additionally, while most users will want an IGW, there's only few use cases for using the output of it. The main one being the creation of routes for the public subnets, but that's done internally in the component (https://github.com/pulumi/pulumi-awsx/blob/adae402632bd5a2b5907745ad1c3b699f2165755/awsx/ec2/vpc.ts#L298). Users would only do that themselves if they wanted to add custom subnets that don't fit into the existing stereotypes (public, private, isolated).

So to summarize:

this breaking change would only affect users with advanced VPC setups
rather minor fix for the breaking change

Based on that reasoning, most users shouldn't be affected by the breaking change of allowing to toggle the internet gateway on/off. And if they are, it's an easy enough fix.

I'm gonna add an optional toggle to enable/disable the IGW and add some guardrails so users don't end up in scenarios where they have public subnets but no IGW as that would break internet connectivity.

@mjeffryes @t0yv0 @corymhall thoughts about this approach?

corymhall commented 6 months ago

I'm gonna add an optional toggle to enable/disable the IGW and add some guardrails so users don't end up in scenarios where they have public subnets but no IGW as that would break internet connectivity.

We should be able to just do this automatically based on the subnet configuration the user passes in. If they have public subnets then create an IGW, otherwise do not.

flostadler commented 6 months ago

I'm gonna add an optional toggle to enable/disable the IGW and add some guardrails so users don't end up in scenarios where they have public subnets but no IGW as that would break internet connectivity.

We should be able to just do this automatically based on the subnet configuration the user passes in. If they have public subnets then create an IGW, otherwise do not.

But that would mean that resources are getting deleted after making this change. I'd argue that this is an even bigger breaking change

For example:

User creates VPC with internal subnets only in this stack; right now this creates an IGW
In another stack (or manually) they create public subnets using that IGW
User upgrades provider
IGW gets deleted and connectivity is broken

corymhall commented 6 months ago

What we need here is some sort of feature flag. Since I don't think we have a good way of doing global feature flags in components (unless there is a way that I am unaware of), maybe we can accomplish it by adding the toggle as a deprecated property.

/**
  * @default true
  * @deprecated This will be removed in the next major release and an internet gateway
  * will only be created if the VPC is configured with public subnets.
  */
createDefaultInternetGateway?: bool

The reason I don't want to just add a toggle is that it would degrade the API experience. At least with a deprecated property we are only temporarily degrading it.

flostadler commented 6 months ago

Yeah feature flags would be great here!

API wise I agree with you, I was worried about a customer nuking their connectivity. But I found that there's actually protection for that: an IGW can only be deleted if there's no EIPs referencing that.

I'm a bit worried about the signals that adding that parameter as a deprecated property is sending. We'd have to make the change in a major version release (because of the changes to the outputs of the component) and then directly queue up the next breaking change. Given that there's protection around disruptive actions, it might be better to just go for the change without the toggle.

pulumi / pulumi-awsx