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

feat!: Refactor count/lists into for_each/maps #113

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

bmurphey
Copy link

@bmurphey bmurphey commented Jun 20, 2023

Description

  • Add an "accepter" resource for VPC attachments, to avoid the "auto accept shared attachments" feature when using Resource Access Manager (RAM)
  • Add support for TGW Peering attachments
  • Add Flow Logs for whole-TGW and/or individual TGW Peering/VPC attachments, publishing to S3 and/or CloudWatch Logs
  • Convert tgw_routes from a list of maps to a map of maps, to avoid potential downtime associated with destroying routes when adding new ones
  • Enable multiple TGW route tables to allow for more granular network segmentation
  • Allow for adding multiple CIDR blocks to VPC route tables per-attachment, and rename the parameter from tgw_destination_cidrs to vpc_route_table_destination_cidrs to reflect its true purpose
  • Add parameters to help transform implementation steps into a more cohesive order of operations
  • Convert TGW route destination CIDR block to list, to allow multiple CIDR blocks per attachment
  • Allow for disabling non-default route table propagation, to ensure VPC CIDR block can be left out of TGW route table when only certain subnets should be routable

Motivation and Context

This module was missing quite a few features that are necessary for true multi-account operation, as well as operational concerns like logging.

Breaking Changes

This PR includes breaking changes due to changes in data structures. The changes to data structures (converting lists/sets to maps, mostly) were necessary to prevent downtime during applies due to deleting/re-creating routes.

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

I am running my refactor in production currently, using one AWS account as the hub, and several other AWS accounts as the spokes.

bmurphey added 13 commits June 19, 2023 16:50
To avoid downtime when adding/removing attachments.  With the previous approach, any new or removed attachments would cause the attachment routes to be deleted and re-created according to the new order.
And unifying peering/VPC attachments under one parameter, VPC attachments are filtered out of the parameter to create the VPC attachement resources.
For fetching attachment details.  Add VPC attachment accepter resource to avoid the need for "auto-accept shared attachments" when sharing via RAM.
When route destinations are defined
With composite keys to allow associating/propagating custom route tables.
@bmurphey bmurphey mentioned this pull request Jun 20, 2023
@bmurphey bmurphey changed the title Significant refactor feat/Significant refactor Jun 20, 2023
@bmurphey bmurphey changed the title feat/Significant refactor feat: Significant refactor Jun 20, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added the stale label Jul 21, 2023
@bryantbiggs bryantbiggs added wip and removed stale labels Jul 26, 2023
@bryantbiggs bryantbiggs changed the title feat: Significant refactor feat!: Refactor count/lists into for_each/maps Jul 26, 2023
@bmurphey
Copy link
Author

bmurphey commented Jan 4, 2024

@bryantbiggs anything I can help with to move this along?

@@ -162,9 +270,9 @@ resource "aws_ram_resource_association" "this" {
}

resource "aws_ram_principal_association" "this" {
count = var.create_tgw && var.share_tgw ? length(var.ram_principals) : 0
for_each = var.create_tgw && var.share_tgw ? var.ram_principals : []
Copy link

@ebarros29 ebarros29 Jan 19, 2024

Choose a reason for hiding this comment

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

Suggested change
for_each = var.create_tgw && var.share_tgw ? var.ram_principals : []
for_each = var.create_tgw && var.share_tgw ? var.ram_principals : toset([])

The right side should be a set...

Even fixing it, we will face the for_each problem for computed values as mentioned here by @bryantbiggs

However, the current version of this module is using countwhich will produce the same error for computed values 🤔

@dash-aug
Copy link

Could we have a separate PR for adding flow logs? They're quite useful, and it's a shame they're "stuck" in this PR.

@bryantbiggs bryantbiggs removed the wip label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Significant refactor
4 participants