Skip to content
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.

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

fix: Avoid error looking up tgw_route_table when resource is not crea… #67

Closed
wants to merge 2 commits into from

Conversation

gmherb
Copy link

@gmherb gmherb commented Mar 15, 2022

Description

When sharing a tgw across aws accounts (tgw_share=true and tgw_create=false), the module runs into an error when looking up the transit gateway route table. By introducing a local variable with 'try' we can circumvent the issue.

Motivation and Context

Module fails when trying to share across accounts. Previous issues reported:
#61
#46
#47
#56

Breaking Changes

none

@gmherb gmherb changed the title fix: avoid error looking up tgw_route_table when resource is not crea… fix: Avoid error looking up tgw_route_table when resource is not crea… Mar 15, 2022
@ndobbs
Copy link

ndobbs commented Mar 17, 2022

Looks like this PR solves the issues I'm running into as well.

terraform_version: v1.1.7
module_version: v2.5.1

@cryptk
Copy link

cryptk commented Mar 25, 2022

I'm hitting this now as well trying to do a cross-account TGW attachment. I hate to be a pest, but @antonbabenko is there any chance we can get a 2.5.2 released with this fix in it?

@@ -123,15 +125,15 @@ resource "aws_ec2_transit_gateway_route_table_association" "this" {

# Create association if it was not set already by aws_ec2_transit_gateway_vpc_attachment resource
transit_gateway_attachment_id = aws_ec2_transit_gateway_vpc_attachment.this[each.key].id
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), var.transit_gateway_route_table_id, aws_ec2_transit_gateway_route_table.this[0].id)
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), local.tgw_route_table_id)
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to do:

Suggested change
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), local.tgw_route_table_id)
transit_gateway_route_table_id = var.create_tgw ? aws_ec2_transit_gateway_route_table.this[0].id : try(each.value.transit_gateway_route_table_id, var.transit_gateway_route_table_id)

Copy link
Member

Choose a reason for hiding this comment

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

actually, this won't solve the issue either. Please see PR #68 and let me know if this resolves the issue

Copy link
Member

Choose a reason for hiding this comment

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

ignore - this won't work either

}

resource "aws_ec2_transit_gateway_route_table_propagation" "this" {
for_each = local.vpc_attachments_without_default_route_table_propagation

# Create association if it was not set already by aws_ec2_transit_gateway_vpc_attachment resource
transit_gateway_attachment_id = aws_ec2_transit_gateway_vpc_attachment.this[each.key].id
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), var.transit_gateway_route_table_id, aws_ec2_transit_gateway_route_table.this[0].id)
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), local.tgw_route_table_id)
Copy link
Member

Choose a reason for hiding this comment

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

and same as above

Suggested change
transit_gateway_route_table_id = coalesce(lookup(each.value, "transit_gateway_route_table_id", null), local.tgw_route_table_id)
transit_gateway_route_table_id = var.create_tgw ? aws_ec2_transit_gateway_route_table.this[0].id : try(each.value.transit_gateway_route_table_id, var.transit_gateway_route_table_id)

Copy link
Member

Choose a reason for hiding this comment

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

ignore - this won't work either

@bryantbiggs
Copy link
Member

bryantbiggs commented Mar 26, 2022

hey all, would anyone be willing to double check PR #68 and let me know if this resolves the issue? The proposed changes here won't work either because doing try(var.some_variable, ....) will always succeed and take var.some_variable. It will only fail if you do a lookup on an attribute/value that doesn't exist. For example, these all will fail the first try() check and continue down the chain (resulting in null in these examples):

  • try(var.foo.bar, null) if var.foo doesn't contain bar in its map attributes
  • try(aws_some_resource[0].id, null) if aws_some_resource` is not created
  • try(each.value.foo, null) if the map that is being looped over with for_each doesn't contain foo in its attributes

@antonbabenko
Copy link
Member

This issue has been resolved in version 2.6.0 🎉

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants