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.61k stars 3.9k forks source link

ec2: `NatProvider.instanceV2` does not work when `mapPublicIpOnLaunch=false` for public subnets #31711

Open tmokmss opened 1 week ago

tmokmss commented 1 week ago

Describe the bug

When we set mapPublicIpOnLaunch=false for public subnets, NAT instances does not get public IP addresses assigned, resulting in non-working NAT instances.

Disabling mapPublicIpOnLaunch is recommended as AWS Config rule (and cdk-nag as well.) https://docs.aws.amazon.com/config/latest/developerguide/subnet-auto-assign-public-ip-disabled.html

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

NAT instance works.

Current Behavior

NAT instance does not work.

Reproduction Steps

Deploy the below VPC, and you can see the NAT instances does not have public IP addresses assigned; traffic cannot go to the Internet because of that.

new Vpc(this, 'Vpc', {
  natGatewayProvider: NatProvider.instanceV2({
    instanceType: InstanceType.of(InstanceClass.T4G, InstanceSize.MICRO),
  }),
  subnetConfiguration: [
    {
      subnetType: SubnetType.PUBLIC,
      name: 'Public',
      // NAT instance does not work when this set to false.
      mapPublicIpOnLaunch: false,
    },
    {
      subnetType: SubnetType.PRIVATE_WITH_EGRESS,
      name: 'Private',
    },
  ],
});

Possible Solution

Expose associatePublicIpAddress property maybe?

Additional Information/Context

No response

CDK CLI Version

2.160.0

Framework Version

No response

Node.js Version

20

OS

macos

Language

TypeScript

Language Version

No response

Other information

No response

khushail commented 1 week ago

Hi @tmokmss , thanks for reaching out.

The property mapPublicIpOnLaunch in CDK docs states that -

The property mapPublicIpOnLaunch controls if a public IPv4 address will be assigned. This defaults to false for dual stack VPCs to avoid inadvertant costs of having the public address. However, a public IP must be enabled (or otherwise configured with BYOIP or IPAM) in order for services that rely on the address to function.

The ipv6AssignAddressOnCreation property controls the same behavior for the IPv6 address. It defaults to true.

Using IPv6 specific properties in an IPv4 only VPC will result in errors.

As mentioned in CDK Docs, not assigning public IP when mapPublicIpOnLaunch=false, is intentional as mentioned in here -

Disabling the auto-assigning of a public IPv4 address by default can avoid the cost of public IPv4 addresses starting 2/1/2024. For use cases that need an IPv4 address, the mapPublicIpOnLaunch property in subnetConfiguration can be set to auto-assign the IPv4 address. Note that private IPv4 address allocation will not be changed.

and one can always assign public Ip addresses manually, by using Elastic IP Addresses.

AFAIU, and if my understanding is correct, this is by design to keep it disabled. Please feel free to correct me if something is misunderstood.

I would also reach out to core team and request their insights on the implementation of this concept. Thanks

tmokmss commented 1 week ago

Thanks @khushail for the investigation!

this is by design to keep it disabled

I'm afraid not. Without public IP addresses, NAT instances won't work. This is a surprising behavior because users expect NatProvider.instanceV2 just works as soon as the deployment finished. Forcing users to assign public ip address manally is clearly a sub-optimal solution.

So a straightforward solution would be to expose a prop to assign a public IP address to the NAT instance when configuring NatProvider.instanceV2. In Instance construct, we have associatePublicIpAddress prop, which we can expose in NatInstanceProps and pass it when defining an Instance construct (below code).

https://github.com/aws/aws-cdk/blob/fbc28bcd5892768bb436b93c09c6d925b57daf0f/packages/aws-cdk-lib/aws-ec2/lib/nat.ts#L536-L547

khushail commented 6 days ago

thanks @tmokmss for the clarification there. Your solution makes sense.