fix: Ensure database route table output works

[henworth]

Description

On initial plan/apply the output database_route_table_ids is not available due to the values not being known until after apply. Switching the logic to test the length of the array of resource ids instead of a try() fixes this issue. Credit to @martin566 for discovering the solution.

Motivation and Context

• This change is required to allow the output to be able to be used by other resources in the same root module
• Resolves tuple vs list problem with output database_route_table_ids during first plan #857.

Breaking Changes

None.

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects

• I have executed pre-commit run -a on my pull request

Have also confirmed in my use case that before this patch, example code like this failed (truncated for brevity):

locals {
  subnet_route_tables = merge(
    zipmap(["public1"], module.vpc.public_route_table_ids),
    zipmap(["private1", "private2", "private3"], module.vpc.private_route_table_ids),
    zipmap(["database1", "database2", "database3"], module.vpc.database_route_table_ids)
  )
}

resource "aws_route" "tgw" {
  for_each = local.subnet_route_tables

  route_table_id         = each.value
  destination_cidr_block = "10.0.0.0/8"
  transit_gateway_id     = data.aws_ec2_transit_gateway.this.id
}

│ Error: Invalid for_each argument
│ 
│   on main.tf line 110, in resource "aws_route" "tgw":
│  110:   for_each = local.subnet_route_tables
│     ├────────────────
│     │ local.subnet_route_tables will be known only after apply
│ 
│ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify the instances of this resource.
│ 

After this patch, the error was not present and the plan/apply succeeded.

[github-actions]

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

[henworth]

Bump

[github-actions]

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

[henworth]

Bump

[srini-hv]

+1
We've been running into this for a while now. We had to create VPC first and then pass the output to our peering module in the next run. Now, I'm trying to add a new subnet and the plan fails. Painful workaround is to delete and create peering which I would like to avoid. This might really help us.
@antonbabenko @bryantbiggs?

[bryantbiggs]

I would like to see a reproduction and then something I can test that validates this change. Right now I am not sold and this statement still stands #857 (comment)

[srini-hv]

@bryantbiggs Thanks for responding. I did manage to reproduce but I had to make another change along with this. If it looks decent to you, I will create new PR.

https://github.com/terraform-aws-modules/terraform-aws-vpc/compare/master...srini-hv:terraform-aws-vpc:fix/using-length-instead?expand=1

module "common_vpc" {
  count   = var.create_vpc ? 1 : 0
  source  = "terraform-aws-modules/vpc/aws"
  version = "4.0.2"

  name                 = local.vpc_name
  cidr                 = var.vpc_cidr
  azs                  = data.aws_availability_zones.available.names
  enable_dns_hostnames = true
  enable_nat_gateway = true
  single_nat_gateway = true
  create_database_subnet_route_table = true

  database_subnets = [
    cidrsubnet(var.vpc_cidr, var.common_vpc_subnet_newbits, 0),
    cidrsubnet(var.vpc_cidr, var.common_vpc_subnet_newbits, 1),
    cidrsubnet(var.vpc_cidr, var.common_vpc_subnet_newbits, 2)
  ]

  private_subnets = [
    cidrsubnet(var.vpc_cidr, var.common_vpc_subnet_newbits, 3)
  ]
  
  public_subnets = [
    cidrsubnet(var.vpc_cidr, var.common_vpc_subnet_newbits, 4)
  ]

  database_subnet_assign_ipv6_address_on_creation = false
  map_public_ip_on_launch                         = false
}

resource "aws_vpc_peering_connection" "requester_cross_account" {

  peer_owner_id = "7645838754" #fake
  peer_vpc_id   = "vpc-7465735w45sd" #fake
  vpc_id        = "vpc-35768fjdg" #fake
  peer_region   = "eu-west-2"
  auto_accept   = false

}
resource "aws_route" "requester_multi_account" {
  for_each = { for index, id in module.common_vpc[0].database_route_table_ids : index => id }

  route_table_id            = each.value
  destination_cidr_block    = "10.0.0.0/16"
  vpc_peering_connection_id = aws_vpc_peering_connection.requester_cross_account.id
}

This Produces error

module.common_vpc[0].database_route_table_ids will be known only after apply
│ 
│ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full
│ set of keys that will identify the instances of this resource.
│ 
│ When working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only
│ in the map values.
│ 
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a
│ second time to fully converge.

Changes:

--- a/main.tf
+++ b/main.tf
@@ -403,7 +403,7 @@ resource "aws_route_table" "database" {
 }
 
 resource "aws_route_table_association" "database" {
-  count = local.create_database_subnets ? local.len_database_subnets : 0
+  count = local.create_database_subnets && (length(aws_route_table.database[*].id) >0 || length(aws_route_table.private[*].id) > 0) ? local.len_database_subnets : 0

--- a/outputs.tf
+++ b/outputs.tf
@@ -279,7 +279,7 @@ output "database_subnet_group_name" {
 
 output "database_route_table_ids" {
   description = "List of IDs of database route tables"
-  value       = try(coalescelist(aws_route_table.database[*].id, local.private_route_table_ids), [])
+  count = local.create_database_subnets && (length(aws_route_table.database[*].id) >0 || length(aws_route_table.private[*].id) > 0) ? local.len_database_subnets : 0
 }

After the changes, there is no more error. Although, I'm not entirely sure if additional condition should be applied to rest of the aws_route_table_association resources.

[bryantbiggs]

folks - lets stick with discussing the issue and how to reproduce the issue before we start opening pull requests. I will kindly wait for a reproduction until proceeding any further on this topic

[srini-hv]

Sorry, Is there a particular way of reproducing?
The example I mentioned above does produce the exact error and 2 line changes I mentioned seem to eliminate it.

[bryantbiggs]

https://github.com/bryantbiggs/how-to-create-reproduction

[srini-hv]

Reproduction Code:

terraform {
  required_version = "~> 1.0"

  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = "~> 4.67"
    }
  }
}

provider "aws" {
  region = local.region
}

################################################################################
# Common Locals
################################################################################

locals {
  vpc_name = "my_vpc"
  region   = "eu-west-2"

  vpc_cidr = "10.9.0.0/16"
  azs      = slice(data.aws_availability_zones.available.names, 0, 3)
}

data "aws_availability_zones" "available" {
  state = "available"
}

################################################################################
# VPC
################################################################################

module "my_vpc" {
  source  = "terraform-aws-modules/vpc/aws"
  version = "4.0.2"

  name                               = local.vpc_name
  cidr                               = local.vpc_cidr
  azs                                = data.aws_availability_zones.available.names
  enable_dns_hostnames               = true
  enable_nat_gateway                 = true
  single_nat_gateway                 = true
  create_database_subnet_route_table = true

  ## Subnets
  database_subnets = [
    cidrsubnet(local.vpc_cidr, 8, 0),
    cidrsubnet(local.vpc_cidr, 8, 1),
    cidrsubnet(local.vpc_cidr, 8, 2)
  ]
  database_subnet_tags = {"subnet_type" = "database"}

  private_subnets = [
    cidrsubnet(local.vpc_cidr, 8, 3)
  ]
  private_subnet_tags = {"subnet_type" = "private"}

  public_subnets = [
    cidrsubnet(local.vpc_cidr, 8, 4)
  ]
  public_subnet_tags = {"subnet_type" = "public"}

  database_subnet_assign_ipv6_address_on_creation = false
  map_public_ip_on_launch                         = false
}

################################################################################
# VPC Peering
################################################################################

resource "aws_vpc_peering_connection" "requester" {

  peer_owner_id = "8273486537546"    # dummy
  peer_vpc_id   = "vpc-278536734534" # dummy
  vpc_id        = module.my_vpc.vpc_id
  peer_region   = "eu-west-2"
  auto_accept   = false

}

resource "aws_route" "requester_routes" {
  for_each = { for index, id in module.my_vpc.database_route_table_ids : index => id }

  route_table_id            = each.value
  destination_cidr_block    = "10.0.0.0/16"
  vpc_peering_connection_id = aws_vpc_peering_connection.requester.id
}

Steps to reproduce the behavior:

1. terraform init
2. terraform plan

Expected behaviour:

All the resources including VPC, Subnets, Peering Connection and & new routes created.

Actual behaviour:

Error: Invalid for_each argument
│ 
│   on main.tf line 77, in resource "aws_route" "requester_multi_account":
│   77:   for_each = { for index, id in module.my_vpc.database_route_table_ids : index => id }
│     ├────────────────
│     │ module.my_vpc.database_route_table_ids will be known only after apply
│ 
│ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full
│ set of keys that will identify the instances of this resource.
│ 
│ When working with unknown values in for_each, it's better to define the map keys statically in your configuration and place apply-time results only
│ in the map values.
│ 
│ Alternatively, you could use the -target planning option to first apply only the resources that the for_each value depends on, and then apply a
│ second time to fully converge.

Hope this helps!

[bryantbiggs]

Awesome, thank you @srini-hv - that was extremely helpful to test and validate. Will merge once CI checks pass

[antonbabenko]

This PR is included in version 5.1.1 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.