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

Add/ignore cidr blocks #43

Merged
merged 11 commits into from
Nov 2, 2022
Merged

Conversation

colinh6
Copy link
Contributor

@colinh6 colinh6 commented Oct 25, 2022

what

  • This PR will introduce logic to ignore certain CIDR ranges from either the requestor or acceptor VPC(s), without affecting the existing functionality.

why

  • We add the CIDR range 100.64.0.0/16 to each of our EKS cluster VPCs to increase the maximum amount of assignable IP addresses when using the AWS VPC CNI.
  • This causes issues when attempting to leverage this existing peering module, as the 100.64.0.0/16 routes will conflict in certain cases.
  • One specific case is where we want to peer and route a single database or elasticsearch VPC to multiple different EKS cluster VPCs that all contain the 100.64.0.0/16 additional CIDR association

references

@colinh6 colinh6 marked this pull request as ready for review October 25, 2022 21:23
@colinh6 colinh6 requested review from a team as code owners October 25, 2022 21:23
@colinh6 colinh6 requested review from Gowiem and woz5999 October 25, 2022 21:23
@aknysh
Copy link
Member

aknysh commented Oct 26, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Oct 26, 2022

@colinh6 thank you for the PR, looks good, but please fix these errors https://github.com/cloudposse/actions/actions/runs/3325626731/jobs/5498510030

@colinh6 colinh6 force-pushed the add/ignore_cidr_blocks branch from ed080bc to b95e170 Compare October 26, 2022 02:38
@colinh6
Copy link
Contributor Author

colinh6 commented Oct 26, 2022

@colinh6 thank you for the PR, looks good, but please fix these errors https://github.com/cloudposse/actions/actions/runs/3325626731/jobs/5498510030

@aknysh Thank you for the review, I believe my latest commit should (hopefully) fix the errors. Could you please re-run the tests?

examples/complete/main.tf Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@colinh6
Copy link
Contributor Author

colinh6 commented Oct 26, 2022

please see comments

@aknysh I believe I have fixed that error. I have attempted to run terratest locally but am having a number of issues. Would you mind re-running the tests again for me? Much appreciated.

@ngoyal16
Copy link

/test all

@colinh6 colinh6 force-pushed the add/ignore_cidr_blocks branch from 911b5cf to 91fc93a Compare November 1, 2022 18:21
@colinh6
Copy link
Contributor Author

colinh6 commented Nov 1, 2022

@aknysh I found some time and got the tests running locally, I have them passing now. Could you check again for me?

@aknysh
Copy link
Member

aknysh commented Nov 2, 2022

/test all

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@colinh6 thanks again, please see the comments (please fix the error in terratest)

@aknysh
Copy link
Member

aknysh commented Nov 2, 2022

/test all

@aknysh aknysh merged commit c9316cb into cloudposse:master Nov 2, 2022
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.

4 participants