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: use filtered subnet cidr blocks instead of the VPC cidr #83

Merged
merged 19 commits into from
Oct 10, 2024

Conversation

bmbferreira
Copy link
Contributor

@bmbferreira bmbferreira commented Jan 22, 2024

what

This PR changes the module to start using the filtered subnet cidrs to create the routes, instead of using the whole VPC cidr.

why

Currently even when filtering the accepter or requester subnets by tags, the route table on the requester/accepter side is always created to the vpc cidr and doesn't create individual routes for each filtered subnet. This is not flexible enough as we might need for example peering to two different VPCs with overlapping CIDRs and we might want to use the subnet cidrs to be able to route to individual cidrs within the subnet, dodging the VPC cidr overlapping.

references

@bmbferreira bmbferreira requested review from a team as code owners January 22, 2024 11:50
@bmbferreira bmbferreira changed the title fix: access to accepter cidr block fix: use filtered subnet cidr bloccks for instead of the VPC cidr Jan 22, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d hans-d added wip Work in Progress: Not ready for final review or merge terratest-failing and removed wip Work in Progress: Not ready for final review or merge labels Mar 2, 2024
@bmbferreira
Copy link
Contributor Author

@hans-d any plans to merge this? the test failure doesn't seem to be related with my change, as I see the same test failin in other PRs as well: https://github.com/cloudposse/actions/actions/runs/8132588929/job/22223186662

@hans-d
Copy link

hans-d commented Mar 4, 2024

Unfortunately, I cannot bypass this check. Getting some internal visability on these failing tests as well, as they are now popping up in various places (for various different reasons).
Once the blocking bits are gone (soon I hope), this will be revisited.

@mergify mergify bot added the triage Needs triage label Apr 1, 2024
@bmbferreira
Copy link
Contributor Author

@joe-niland @dotCipher Hi! What can I do to have this reviewed? Thank you

@bmbferreira bmbferreira changed the title fix: use filtered subnet cidr bloccks for instead of the VPC cidr fix: use filtered subnet cidr blocks for instead of the VPC cidr Sep 25, 2024
@bmbferreira
Copy link
Contributor Author

@goruha just fixed format that was failing. Can you check if the PR is ok to merge? thank you

Copy link
Member

@goruha goruha left a comment

Choose a reason for hiding this comment

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

@bmbferreira why do not we have ?

data "aws_subnet" "requester" {

@bmbferreira bmbferreira changed the title fix: use filtered subnet cidr blocks for instead of the VPC cidr fix: use filtered subnet cidr blocks for accepter_cidr_block_associations instead of the VPC cidr Sep 26, 2024
@bmbferreira bmbferreira changed the title fix: use filtered subnet cidr blocks for accepter_cidr_block_associations instead of the VPC cidr fix: use filtered subnet cidr blocks instead of the VPC cidr Sep 28, 2024
@bmbferreira
Copy link
Contributor Author

@bmbferreira why do not we have ?

data "aws_subnet" "requester" {

Fixed. I applied the same logic to the requester as well. I also did an improvement to fallback to the VPC cidr if no tag filters are passed as inputs. I think it only makes sense to use the filtered subnet cidr blocks if we are actually filtering it. If not we can continue using the vpc cidr. WDYT?

accepter.tf Outdated Show resolved Hide resolved
@goruha
Copy link
Member

goruha commented Oct 2, 2024

@bmbferreira I have one request for change.
Beside of that it looks fine

@goruha
Copy link
Member

goruha commented Oct 2, 2024

/terratest

@bmbferreira
Copy link
Contributor Author

/terratest

@goruha done!

@goruha
Copy link
Member

goruha commented Oct 10, 2024

/terratest

@goruha goruha added the major Breaking changes (or first stable release) label Oct 10, 2024
@goruha
Copy link
Member

goruha commented Oct 10, 2024

@bmbferreira Thanks for your contribution

@mergify mergify bot removed the triage Needs triage label Oct 10, 2024
@goruha
Copy link
Member

goruha commented Oct 10, 2024

/terratest

@goruha goruha merged commit 18ffc2c into cloudposse:main Oct 10, 2024
17 checks passed
Copy link

These changes were released in v1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Breaking changes (or first stable release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants