Skip to content

feat: Allow specifying additional TGW routes in attached VPCs #132

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −3
Open

feat: Allow specifying additional TGW routes in attached VPCs #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ No modules.
| [aws_ram_resource_association.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ram_resource_association) | resource |
| [aws_ram_resource_share.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ram_resource_share) | resource |
| [aws_ram_resource_share_accepter.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ram_resource_share_accepter) | resource |
| [aws_route.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/route) | resource |
| [aws_route.additional_cidrs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/route) | resource |
| [aws_route.destination_cidr](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/route) | resource |

## Inputs

Expand Down
3 changes: 2 additions & 1 deletion examples/multi-account/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ module "tgw_peer" {
transit_gateway_default_route_table_propagation = false

vpc_route_table_ids = module.vpc1.private_route_table_ids
tgw_destination_cidr = "0.0.0.0/0"
tgw_destination_cidr = "10.0.0.0/8"
tgw_additional_cidrs = ["172.0.0/12"]

tgw_routes = [
{
Expand Down
32 changes: 31 additions & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ locals {
}
]
])

vpc_route_table_additional_cidrs = flatten([
for k, v in var.vpc_attachments : [
for rtb_id in try(v.vpc_route_table_ids, []) : [
for cidr in try(v.tgw_additional_cidrs, []) : {
rtb_id = rtb_id
cidr = cidr
tgw_id = var.create_tgw ? aws_ec2_transit_gateway.this[0].id : v.tgw_id
}
]
]
])
}

################################################################################
Expand Down Expand Up @@ -127,7 +139,7 @@ resource "aws_ec2_transit_gateway_route" "this" {
transit_gateway_attachment_id = tobool(try(local.vpc_attachments_with_routes[count.index][1].blackhole, false)) == false ? aws_ec2_transit_gateway_vpc_attachment.this[local.vpc_attachments_with_routes[count.index][0].key].id : null
}

resource "aws_route" "this" {
Copy link
Contributor

@yyarmoshyk yyarmoshyk Oct 24, 2025

Choose a reason for hiding this comment

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

There is no need to introduce additional resources resource "aws_route" "additional_cidrs" and resource "aws_route" "destination_cidr"

Let's simplify the code a bit by looping over the merged map of local.vpc_route_table_destination_cidr and local.vpc_route_table_additional_cidrs like the following:

resource "aws_route" "this" {
  for_each = { for x in merge(local.vpc_route_table_destination_cidr,local.vpc_route_table_additional_cidrs) : ${x.rtb_id}_${x.cidr} => {
    cidr   = x.cidr,
    tgw_id = x.tgw_id
    rtb_id = x.rtb_id
  } }

  region = var.region

  route_table_id              = each.value["rtb_id"]
  destination_cidr_block      = try(each.value.ipv6_support, false) ? null : each.value["cidr"]
  destination_ipv6_cidr_block = try(each.value.ipv6_support, false) ? each.value["cidr"] : null
  transit_gateway_id          = each.value["tgw_id"]

  depends_on = [aws_ec2_transit_gateway_vpc_attachment.this]
}

Copy link
Author

@partcyborg partcyborg Oct 24, 2025

Choose a reason for hiding this comment

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

@yyarmoshyk this change isn't possible without causing the deletion of all existing aws_route.this resources, as you can't do dynamic moved blocks.

Given that the key for the default route will depend on the current cidr block, there is no way to write a moved block that will move the existing route entry into a key for the current aws_route.this resource.

As I stated in the PR description, this leaves us only two options:

  1. Keep the aws_route.this resource as is, but add the aws_route.additional_cidrs resource which has a map with key. This violates terraform style, but if we want to drop the moved block I would be okay with that if the module owners are okay with ignoring terraform style.
  2. Keep the change as written which allows us to not destroy the existing route resource but move it to the new entry, while also keeping the additional_cidrs object which is a map.

If we just do what you suggest, I would have to remove the moved block which would mean that when users upgrade the terraform module the route entry will be deleted and re-created which could cause traffic disruption. This is highly undesirable in my opinion

resource "aws_route" "destination_cidr" {
for_each = { for x in local.vpc_route_table_destination_cidr : x.rtb_id => {
cidr = x.cidr,
tgw_id = x.tgw_id
Expand All @@ -143,6 +155,24 @@ resource "aws_route" "this" {
depends_on = [aws_ec2_transit_gateway_vpc_attachment.this]
}

moved {
Copy link
Contributor

@yyarmoshyk yyarmoshyk Oct 24, 2025

Choose a reason for hiding this comment

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

with the change above this becomes unneeded.

Copy link
Author

@partcyborg partcyborg Oct 24, 2025

Choose a reason for hiding this comment

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

As stated above, your suggestion will cause an outage for all users of this module. I really don't think this is the right approach here.

from = aws_route.this
to = aws_route.destination_cidr
}

resource "aws_route" "additional_cidrs" {
for_each = { for x in local.vpc_route_table_additional_cidrs : "${x.rtb_id}_${x.cidr}" => {
cidr = x.cidr
rtb_id = x.rtb_id
tgw_id = x.tgw_id
} }

route_table_id = each.value["rtb_id"]
destination_cidr_block = try(each.value.ipv6_support, false) ? null : each.value["cidr"]
destination_ipv6_cidr_block = try(each.value.ipv6_support, false) ? each.value["cidr"] : null
transit_gateway_id = each.value["tgw_id"]
}

resource "aws_ec2_transit_gateway_route_table_association" "this" {
for_each = {
for k, v in var.vpc_attachments : k => v if var.create_tgw && var.create_tgw_routes && try(v.transit_gateway_default_route_table_association, true) != true
Expand Down