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

Significant refactor #109

Closed
bmurphey opened this issue Apr 26, 2023 · 13 comments · May be fixed by #113
Closed

Significant refactor #109

bmurphey opened this issue Apr 26, 2023 · 13 comments · May be fixed by #113

Comments

@bmurphey
Copy link

Is your request related to a problem? Please describe.

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

Describe the solution you'd like.

I have a large refactor of the module that I can submit as a PR, which adds the following features:

  • 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

Describe alternatives you've considered.

I considered using different modules or writing my own from scratch, but I thought the best approach would be to perform this refactor and give it back to the community.

Additional context

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

Would you like me to open a PR? This was a massive effort and I would love for the community to benefit from it.

@mikkoc
Copy link

mikkoc commented May 2, 2023

FWIW, I'd like to see that 👍

@virtualGain
Copy link

Plus one from me as well. Glad I read this as I am going down the path of creating this landing zone right now with a similar architecture and you just saved me a ton of time trying to fiddle with this to make it work. Any chance you can throw this up on your account so I can fork it in the mean time?

@shamil
Copy link

shamil commented May 30, 2023

I'm specifically interested in "Allow for adding multiple CIDR blocks to VPC route tables per-attachment", currently only single CIDR is possible per attachment, which is very limiting, unless there is other way that I'm not aware of.

@bmurphey
Copy link
Author

Thank you all for your comments. I haven't heard back from maintainers yet, but I've put up a PR here. Please try it out, and let me know if anything is confusing or broken. I've tried to test all of my changes but it's certainly possible something slipped through. I am using this in production, though, so if something isn't working it's likely I didn't document it thoroughly enough.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale label Jul 21, 2023
@bmurphey
Copy link
Author

The PR is still open, so this issue should still stay open.

@github-actions github-actions bot removed the stale label Aug 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

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

@github-actions github-actions bot added the stale label Sep 1, 2023
@bmurphey
Copy link
Author

bmurphey commented Sep 2, 2023

The PR is still open, so this issue should still stay open.

@github-actions github-actions bot removed the stale label Sep 3, 2023
@Lincon-Freitas
Copy link

Wow great job! Will test your work from the PR, thanks for doing this!

@chrisduong
Copy link

looking for this too.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale label Oct 29, 2023
Copy link

github-actions bot commented Nov 9, 2023

This issue was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2023
Copy link

github-actions bot commented Dec 9, 2023

I'm going to lock this issue 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 similar to this, 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 Dec 9, 2023
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 a pull request may close this issue.

6 participants