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

[Issue 1051] Fix CIDR overlap #1093

Merged
merged 1 commit into from
Jan 25, 2024
Merged

[Issue 1051] Fix CIDR overlap #1093

merged 1 commit into from
Jan 25, 2024

Conversation

coilysiren
Copy link
Collaborator

Summary

Rolls up to #1051

Time to review: 5 mins

Changes proposed

  • Pipes in some DMS stuff are variables.
  • Changes the second octet (eg. x in 10.x.0.0) in the VPC cidr to be a variable. devs cidr is staying the same, staging and prod are changing.

Context for reviewers

  • I decided on this course of action after thinking about the DMS overnight. I concluded that the overlapping CIDRs might create a networking problem on the MicroHealth side.
  • I'll need to redeploy staging and prod's VPCs in order to finish this PR. That'll take me a bit.

Copy link
Collaborator

@jamesbursa jamesbursa left a comment

Choose a reason for hiding this comment

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

Are there descriptions we can use in the comments, instead of vendor names?

public_subnets = ["10.0.10.0/24", "10.0.11.0/24", "10.0.12.0/24"]
private_subnets = ["10.0.0.0/24", "10.0.1.0/24", "10.0.2.0/24"]
database_subnets = ["10.0.5.0/24", "10.0.6.0/24", "10.0.7.0/24"]
public_subnets = ["10.${var.second_octet}.10.0/24", "10.${var.second_octet}.11.0/24", "10.${var.second_octet}.12.0/24"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the cidrsubnet function might be another option here if you prefer, instead of string interpolation. https://developer.hashicorp.com/terraform/language/functions/cidrsubnet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jamesbursa I looked into that! But unfortunately I didn't really parse the math TBH.

@coilysiren coilysiren merged commit e541b0d into main Jan 25, 2024
7 checks passed
@coilysiren coilysiren deleted the cidr-overlap branch January 25, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants