diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b1e8106e..cc0c80215 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -613,6 +613,16 @@ jobs: terraform -chdir="$d" test done + # The fck-nat egress route must target the persistent ENI, not the + # current instance's own, so it survives ASG replacement (#716). + - name: Test AWS networking module + if: matrix.cloud == 'aws' + run: | + d=terraform/modules/networking/aws + terraform -chdir="$d" init -backend=false -lockfile=readonly -input=false + terraform -chdir="$d" fmt -check -recursive + terraform -chdir="$d" test + # Failure alerts must fire on the markers the app logs (#714). GCP's # module tests run in the /health step below. - name: Test monitoring module diff --git a/terraform/environments/aws/ci-cd-permissions/policy_networking.tf b/terraform/environments/aws/ci-cd-permissions/policy_networking.tf index de56d2941..423cfc27a 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_networking.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_networking.tf @@ -33,6 +33,15 @@ resource "aws_iam_policy" "networking" { "ec2:CreateLaunchTemplateVersion", "ec2:DeleteLaunchTemplateVersions", "ec2:ModifyLaunchTemplate", + # The persistent fck-nat ENI the private routes target (#716): + # CreateNetworkInterface to create it, ModifyNetworkInterfaceAttribute + # because the provider turns source_dest_check off in a second call + # after the create, and DetachNetworkInterface because destroying or + # replacing it detaches it from the NAT instance first. + # DeleteNetworkInterface and DescribeNetworkInterfaces are below. + "ec2:CreateNetworkInterface", + "ec2:ModifyNetworkInterfaceAttribute", + "ec2:DetachNetworkInterface", "ec2:CreateRoute", "ec2:CreateRouteTable", "ec2:CreateSecurityGroup", diff --git a/terraform/modules/networking/aws/.terraform.lock.hcl b/terraform/modules/networking/aws/.terraform.lock.hcl index cdc1668d4..8fbb17124 100644 --- a/terraform/modules/networking/aws/.terraform.lock.hcl +++ b/terraform/modules/networking/aws/.terraform.lock.hcl @@ -6,6 +6,8 @@ provider "registry.terraform.io/hashicorp/aws" { constraints = "~> 5.0" hashes = [ "h1:Ijt7pOlB7Tr7maGQIqtsLFbl7pSMIj06TVdkoSBcYOw=", + "h1:edXOJWE4ORX8Fm+dpVpICzMZJat4AX0VRCAy/xkcOc0=", + "h1:hd45qFU5cFuJMpFGdUniU9mVIr5LYVWP1uMeunBpYYs=", "zh:054b8dd49f0549c9a7cc27d159e45327b7b65cf404da5e5a20da154b90b8a644", "zh:0b97bf8d5e03d15d83cc40b0530a1f84b459354939ba6f135a0086c20ebbe6b2", "zh:1589a2266af699cbd5d80737a0fe02e54ec9cf2ca54e7e00ac51c7359056f274", diff --git a/terraform/modules/networking/aws/README.md b/terraform/modules/networking/aws/README.md index 145cb0528..2804db2b8 100644 --- a/terraform/modules/networking/aws/README.md +++ b/terraform/modules/networking/aws/README.md @@ -114,6 +114,9 @@ The `enable_nat_gateway` option deploys a cost-effective NAT alternative: - Provides IPv4 egress for services without IPv6 support (like AWS SES) - Automatically deployed in the first public subnet - 90% cheaper than AWS NAT Gateway (~$32/month) +- Private IPv4 routes target a persistent ENI that each instance attaches at + boot, so egress survives the ASG replacing the instance. With an existing + VPC, route private subnets to the `nat_network_interface_id` output. ## Security Groups @@ -136,6 +139,8 @@ The module outputs: - `database_security_group_id`: Security group for database - `lambda_vpc_config`: Convenience object for Lambda module - `database_vpc_config`: Convenience object for database module +- `nat_network_interface_id`: Persistent fck-nat ENI to route IPv4 egress to (if enabled) +- `nat_autoscaling_group_name`: fck-nat Auto Scaling group name (if enabled) ## Cost Comparison diff --git a/terraform/modules/networking/aws/main.tf b/terraform/modules/networking/aws/main.tf index 84a3dc797..99dc57e2d 100644 --- a/terraform/modules/networking/aws/main.tf +++ b/terraform/modules/networking/aws/main.tf @@ -209,6 +209,27 @@ resource "aws_iam_instance_profile" "fck_nat" { }) } +# Persistent ENI the private routes target (#716). The route tables must not +# name an instance's own primary ENI: the ASG replaces the instance (failed +# health check, AZ event) and the replacement comes up with a new primary ENI, +# so a route pinned to the old one blackholes IPv4 egress until the next apply. +# This ENI outlives every instance. Each one the ASG launches attaches it at +# boot as its second interface (see user_data), so the route target never +# changes. It lives in the ASG's only subnet because an ENI can only attach to +# an instance in its own AZ. +resource "aws_network_interface" "fck_nat" { + count = var.enable_nat_gateway ? 1 : 0 + + description = "${var.stack_name} fck-nat persistent ENI (private route target)" + subnet_id = local.public_subnet_ids[0] + security_groups = [aws_security_group.fck_nat[0].id] + source_dest_check = false # NAT forwards traffic it is neither source nor destination of + + tags = merge(var.tags, { + Name = "${var.stack_name}-fck-nat-eni" + }) +} + # Launch template for fck-nat instance resource "aws_launch_template" "fck_nat" { count = var.enable_nat_gateway ? 1 : 0 @@ -227,15 +248,16 @@ resource "aws_launch_template" "fck_nat" { security_groups = [aws_security_group.fck_nat[0].id] } - # User data to disable source/destination check for NAT functionality + # Hand the persistent ENI to the AMI's own fck-nat service. With eni_id set + # it disables source/destination checking on the primary ENI, attaches + # eni_id as the second interface (retrying until a predecessor that is + # still terminating has released it), and NATs traffic arriving on it out + # of the primary interface. user_data = base64encode(<<-EOF - #!/bin/bash - # Get instance ID and ENI ID - INSTANCE_ID=$(ec2-metadata --instance-id | cut -d " " -f 2) - ENI_ID=$(aws ec2 describe-instances --instance-ids $INSTANCE_ID --region ${var.region} --query 'Reservations[0].Instances[0].NetworkInterfaces[0].NetworkInterfaceId' --output text) - - # Disable source/destination check - aws ec2 modify-network-interface-attribute --network-interface-id $ENI_ID --region ${var.region} --no-source-dest-check + #!/bin/sh + : > /etc/fck-nat.conf + echo "eni_id=${aws_network_interface.fck_nat[0].id}" >> /etc/fck-nat.conf + service fck-nat restart EOF ) @@ -272,7 +294,7 @@ resource "aws_autoscaling_group" "fck_nat" { desired_capacity = 1 min_size = 1 max_size = 1 - vpc_zone_identifier = [local.public_subnet_ids[0]] # Deploy in first public subnet + vpc_zone_identifier = [aws_network_interface.fck_nat[0].subnet_id] # The persistent ENI's subnet (first public subnet) launch_template { id = aws_launch_template.fck_nat[0].id @@ -282,6 +304,11 @@ resource "aws_autoscaling_group" "fck_nat" { health_check_type = "EC2" health_check_grace_period = 60 + # Grant the attach permission before an instance can boot and ask for it. + # fck-nat retries the attach, but its one-shot source/dest-check call on + # the primary ENI does not, so make the order explicit. + depends_on = [aws_iam_role_policy.fck_nat_ec2] + tag { key = "Name" value = "${var.stack_name}-fck-nat-asg" @@ -299,6 +326,16 @@ resource "aws_autoscaling_group" "fck_nat" { lifecycle { create_before_destroy = true + + # A running instance never re-reads user data, and the launch template + # version is "$Latest", so a user_data change alone would leave the + # current instance on the old config. That matters once, on the upgrade + # that introduces the persistent ENI: the routes would move to an ENI no + # instance has attached. Replacing the ASG (create-before-destroy, so the + # new instance is InService before the old one goes) launches one that + # attaches it. user_data only changes when the ENI itself is replaced, so + # AMI rotations (image_id) still do not cycle the instance. + replace_triggered_by = [aws_launch_template.fck_nat[count.index].user_data] } } @@ -311,51 +348,28 @@ resource "aws_iam_role_policy" "fck_nat_ec2" { policy = jsonencode({ Version = "2012-10-17" - Statement = [{ - Effect = "Allow" - Action = [ - "ec2:DescribeInstances", - "ec2:ModifyNetworkInterfaceAttribute" - ] - Resource = "*" - }] + Statement = [ + { + # Attach the persistent ENI to whichever instance the ASG launched. + # AttachNetworkInterface is authorized against both the ENI and the + # instance, so both ARNs are needed; the ENI one is what scopes it. + Sid = "AttachPersistentENI" + Effect = "Allow" + Action = ["ec2:AttachNetworkInterface"] + Resource = [aws_network_interface.fck_nat[0].arn, "arn:aws:ec2:${var.region}:*:instance/*"] + }, + { + # Disable source/destination checking on the instance's own primary + # ENI, whose ID is only known at boot. + Sid = "DisableSourceDestCheck" + Effect = "Allow" + Action = ["ec2:ModifyNetworkInterfaceAttribute"] + Resource = "*" + }, + ] }) } -# Get the instance from the ASG -data "aws_instances" "fck_nat" { - count = var.enable_nat_gateway ? 1 : 0 - - filter { - name = "tag:aws:autoscaling:groupName" - values = [aws_autoscaling_group.fck_nat[0].name] - } - - filter { - name = "instance-state-name" - values = ["running"] - } - - depends_on = [aws_autoscaling_group.fck_nat] -} - -# Get the ENI of the fck-nat instance for routing -data "aws_network_interfaces" "fck_nat" { - count = var.enable_nat_gateway ? 1 : 0 - - filter { - name = "attachment.instance-id" - values = data.aws_instances.fck_nat[0].ids - } - - filter { - name = "attachment.device-index" - values = ["0"] # Primary network interface - } - - depends_on = [data.aws_instances.fck_nat] -} - # ============================================== # Availability Zones # ============================================== @@ -485,7 +499,12 @@ resource "aws_route" "private_internet_ipv4" { route_table_id = aws_route_table.private[count.index].id destination_cidr_block = "0.0.0.0/0" - network_interface_id = data.aws_network_interfaces.fck_nat[0].ids[0] + network_interface_id = aws_network_interface.fck_nat[0].id # Persistent, survives instance replacement (#716) + + # Move the route only once an instance that attaches the ENI is InService, + # so the upgrade that introduces the ENI does not point egress at an + # unattached interface while the replacement ASG is still launching. + depends_on = [aws_autoscaling_group.fck_nat] } # Associate private subnets with private route tables diff --git a/terraform/modules/networking/aws/outputs.tf b/terraform/modules/networking/aws/outputs.tf index 4258a5ea5..d94906640 100644 --- a/terraform/modules/networking/aws/outputs.tf +++ b/terraform/modules/networking/aws/outputs.tf @@ -98,3 +98,20 @@ output "database_vpc_config" { security_group_id = aws_security_group.database.id } } + +# The persistent fck-nat ENI (#716). The module routes its own private route +# tables to it; with use_existing_vpc, point the VPC's private 0.0.0.0/0 +# routes here rather than at the fck-nat instance, whose primary ENI changes +# every time the ASG replaces it. +output "nat_network_interface_id" { + description = "ID of the persistent fck-nat ENI to route private IPv4 egress to (null when enable_nat_gateway is false)" + value = var.enable_nat_gateway ? aws_network_interface.fck_nat[0].id : null +} + +# The fck-nat ASG, for alarms on its instance replacements. It is replaced +# (new name) when the launch template's user data changes; see the ASG's +# replace_triggered_by. +output "nat_autoscaling_group_name" { + description = "Name of the fck-nat Auto Scaling group (null when enable_nat_gateway is false)" + value = var.enable_nat_gateway ? aws_autoscaling_group.fck_nat[0].name : null +} diff --git a/terraform/modules/networking/aws/tests/fck_nat_eni.tftest.hcl b/terraform/modules/networking/aws/tests/fck_nat_eni.tftest.hcl new file mode 100644 index 000000000..6b6eea5c8 --- /dev/null +++ b/terraform/modules/networking/aws/tests/fck_nat_eni.tftest.hcl @@ -0,0 +1,178 @@ +# fck-nat egress route survives instance replacement (#716). +# +# The private 0.0.0.0/0 routes used to point at the primary ENI of whichever +# fck-nat instance was running when Terraform last read it. The ASG replaces +# that instance on a failed health check or AZ event, the new instance gets a +# new primary ENI, and the route keeps naming the old one until the next +# apply: private-subnet IPv4 egress is blackholed in the meantime. The routes +# must instead target a persistent ENI that Terraform owns and every instance +# the ASG launches attaches at boot, so the route target never changes when +# the instance does. +mock_provider "aws" { + # The AZ list and the VPC's IPv6 block feed cidrsubnet() and indexing, so + # the mock's random defaults cannot stand in for them. + override_data { + target = data.aws_availability_zones.available + values = { + names = ["us-east-1a", "us-east-1b", "us-east-1c"] + } + } + + override_resource { + target = aws_vpc.main + values = { + ipv6_cidr_block = "2600:1f18:1234:5600::/56" + } + } + + # The provider validates the ASG's launch_template.id format. + override_resource { + target = aws_launch_template.fck_nat + values = { + id = "lt-0mock" + } + } +} + +variables { + stack_name = "cudly-test" + permissions_boundary_arn = "arn:aws:iam::123456789012:policy/cudly-deploy-boundary" + region = "us-east-1" + az_count = 3 + enable_nat_gateway = true +} + +# Not covered here: the routes' depends_on on the ASG, which keeps the +# one-time upgrade from moving egress to the new ENI before an instance that +# attaches it is InService, and the ASG's depends_on on the instance role +# policy. Mock providers expose no apply ordering, so deleting either one +# leaves every run below green; check them by hand when editing main.tf. +run "routes_target_persistent_eni" { + command = apply + + assert { + condition = length(aws_route.private_internet_ipv4) == 3 + error_message = "Every private route table needs its IPv4 default route via fck-nat." + } + + assert { + condition = alltrue([ + for r in aws_route.private_internet_ipv4 : + r.network_interface_id == aws_network_interface.fck_nat[0].id + ]) + error_message = "Private IPv4 default routes must target the persistent fck-nat ENI, not the primary ENI of the instance running at apply time: the ASG replaces that instance and its ENI with it, blackholing egress (#716)." + } + + assert { + condition = output.nat_network_interface_id == aws_network_interface.fck_nat[0].id + error_message = "nat_network_interface_id must expose the persistent ENI so existing-VPC route tables can target it too." + } +} + +run "persistent_eni_is_attachable_by_every_instance" { + command = apply + + # NAT forwards traffic it is neither source nor destination of. + assert { + condition = aws_network_interface.fck_nat[0].source_dest_check == false + error_message = "The persistent ENI must have source/destination checking disabled or it drops forwarded traffic." + } + + # An ENI can only attach to an instance in its own AZ. + assert { + condition = toset(aws_autoscaling_group.fck_nat[0].vpc_zone_identifier) == toset([aws_network_interface.fck_nat[0].subnet_id]) + error_message = "The ASG must launch fck-nat only into the persistent ENI's subnet, or a replacement instance in another AZ cannot attach it." + } + + # The fck-nat AMI's own service attaches eni_id at boot, retrying until a + # terminating predecessor has released it. + assert { + condition = strcontains(base64decode(aws_launch_template.fck_nat[0].user_data), "eni_id=${aws_network_interface.fck_nat[0].id}") + error_message = "Launch template user data must hand the persistent ENI to the fck-nat service (eni_id in /etc/fck-nat.conf) so each replacement instance attaches it." + } + + assert { + condition = strcontains(base64decode(aws_launch_template.fck_nat[0].user_data), "service fck-nat restart") + error_message = "User data must restart the fck-nat service after writing its config, or the ENI is never attached." + } + + assert { + condition = anytrue([ + for s in jsondecode(aws_iam_role_policy.fck_nat_ec2[0].policy).Statement : + contains(flatten([s.Action]), "ec2:AttachNetworkInterface") && contains(flatten([s.Resource]), aws_network_interface.fck_nat[0].arn) + ]) + error_message = "The fck-nat instance role must be allowed ec2:AttachNetworkInterface on the persistent ENI." + } +} + +# A running instance never re-reads user data, so when the ENI it must attach +# changes (the upgrade that introduces it, or the ENI being replaced later), +# the ASG itself has to be replaced to launch an instance that attaches the +# new one. Mock providers do not reproduce the real provider's ForceNew +# planning, so the ENI replacement is requested explicitly here. +run "eni_replacement_replaces_asg" { + command = apply + + plan_options { + replace = [aws_network_interface.fck_nat[0]] + } + + assert { + condition = output.nat_network_interface_id != run.persistent_eni_is_attachable_by_every_instance.nat_network_interface_id + error_message = "Test precondition: the persistent ENI should have been replaced." + } + + assert { + condition = output.nat_autoscaling_group_name != run.persistent_eni_is_attachable_by_every_instance.nat_autoscaling_group_name + error_message = "The fck-nat ASG must be replaced when its user data names a new ENI; updated in place, the running instance keeps the old config and never attaches the ENI the routes now target." + } +} + +# The fck-nat AMI lookup is most_recent, so an upstream AMI release changes the +# launch template's image_id on the next apply with nothing else changing. +# That must not cycle the NAT instance. +run "ami_rotation_keeps_asg" { + command = apply + + override_data { + target = data.aws_ami.fck_nat + values = { + id = "ami-0rotated" + } + } + + assert { + condition = aws_launch_template.fck_nat[0].image_id == "ami-0rotated" + error_message = "Test precondition: the launch template should have picked up the rotated AMI." + } + + assert { + condition = output.nat_autoscaling_group_name == run.eni_replacement_replaces_asg.nat_autoscaling_group_name + error_message = "An AMI rotation alone must not replace the fck-nat ASG (and its running instance)." + } +} + +run "no_nat_no_eni" { + command = plan + + variables { + use_existing_vpc = true + existing_vpc_id = "vpc-0existing" + existing_public_subnet_ids = ["subnet-0public"] + existing_private_subnet_ids = ["subnet-0private"] + enable_nat_gateway = false + } + + override_data { + target = data.aws_vpc.existing + values = { + cidr_block = "10.20.0.0/16" + ipv6_cidr_block = "2600:1f18:abcd:ef00::/56" + } + } + + assert { + condition = length(aws_network_interface.fck_nat) == 0 && output.nat_network_interface_id == null + error_message = "No fck-nat ENI should exist when enable_nat_gateway is false." + } +}