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: Manage route tables for multiple database subnets #494

Conversation

y-batsianouski
Copy link
Contributor

Description

Module fails when I try to create multiple database subnets with create_database_subnet_route_table = true and single_nat_gateway = false

Motivation and Context

When I try to create multiple database subnets with it's own route tables, module tries to create:

  1. multiple db subnets
  2. One route table for multiple database subnets
  3. Multiple routes for each NAT gateway. Points all this routes to one route table and fails.

Breaking Changes

There was no possibility to create multiple db subnets with create_database_subnet_route_table = true and single_nat_gateway = false. So there are shouldn't be breaking changes.

How Has This Been Tested?

Case 1

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  azs              = ["us-east-1a", "us-east-1b"]
  public_subnets   = ["10.40.1.0/24", "10.40.2.0/24"]
  private_subnets  = ["10.40.3.0/24", "10.40.4.0/24"]
  database_subnets = ["10.40.5.0/24", "10.40.6.0/24"]

  single_nat_gateway                     = false
  create_database_subnet_route_table     = true
  create_database_nat_gateway_route      = true
  ...
}

Creates 2 route tables for database subnets. Points each route table to appropriate NAT gateway

Case 2

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  azs              = ["us-east-1a", "us-east-1b"]
  public_subnets   = ["10.40.1.0/24", "10.40.2.0/24"]
  private_subnets  = ["10.40.3.0/24", "10.40.4.0/24"]
  database_subnets = ["10.40.5.0/24", "10.40.6.0/24"]

  single_nat_gateway                     = true
  create_database_subnet_route_table     = true
  create_database_nat_gateway_route      = true
  ...
}

Creates 1 route table for all database subnets. Points route table to single NAT gateway

Case 3

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  azs              = ["us-east-1a", "us-east-1b"]
  public_subnets   = ["10.40.1.0/24", "10.40.2.0/24"]
  private_subnets  = ["10.40.3.0/24", "10.40.4.0/24"]
  database_subnets = ["10.40.5.0/24", "10.40.6.0/24"]
  
  create_database_subnet_route_table     = true
  create_database_nat_gateway_route      = false
  create_database_internet_gateway_route = true
  ...
}

Creates 1 route table for all database subnets. Points route table to internet gateway

@DrFaust92
Copy link
Contributor

Looks good to me, ill try to test all the cases.

@DrFaust92 DrFaust92 self-requested a review September 15, 2020 18:42
@y-batsianouski
Copy link
Contributor Author

@antonbabenko @DrFaust92 Did you have a chance to look at this? Thanks!

@Chili-Man
Copy link
Contributor

Chili-Man commented Oct 6, 2020

@y-batsianouski Can you please update this with the base branch?

@antonbabenko @DrFaust92 I can confirm that this fixes the issue I reported #516

@y-batsianouski
Copy link
Contributor Author

@y-batsianouski Can you please update this with the base branch?

@antonbabenko @DrFaust92 I can confirm that this fixes the issue I reported #516

@Chili-Man I'll try to manage this soon. But It's not very interesting for me now because I've written my own tf module for VPC which was inspired by this module but provides much more flexibility: https://registry.terraform.io/modules/y-batsianouski/vpc/aws.
Unfortunately full documentation isn't completed yet, but I'm going to update it soon.

@Chili-Man
Copy link
Contributor

@y-batsianouski I understand, thanks for the update; I can carry this forward then; I'll open a new pr with these changes

@Chili-Man
Copy link
Contributor

Chili-Man commented Oct 6, 2020

@y-batsianouski @antonbabenko @DrFaust92 I've opened this pr to carry the changes forward here: #518 . Feel free to close this PR

@y-batsianouski
Copy link
Contributor Author

Closining. @Chili-Man handover this changes in it's PR: #518

@bryantbiggs
Copy link
Member

hi all, this is fixed now in #518

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 Oct 31, 2022
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 this pull request may close these issues.

4 participants