Skip to content

Conversation

@s2504s
Copy link
Contributor

@s2504s s2504s commented Aug 8, 2017

What

  • Create TF Module that implements a public & privatesubnet strategy using existing VPC

Why

  • Encapsulate business logic of VPCs and Subnets in a module

@s2504s s2504s requested review from const-bon and osterman August 8, 2017 11:56
@s2504s s2504s self-assigned this Aug 8, 2017
@osterman
Copy link
Member

osterman commented Aug 8, 2017

@s2504s looks great! Just add README.md

@osterman osterman changed the title Create TF Module that implements subnets strategy to existing VPC Create TF Module that implements public/private subnet strategy in existing VPC Aug 8, 2017
Copy link
Contributor

@const-bon const-bon left a comment

Choose a reason for hiding this comment

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

terraform plan has ended with errors.

Error running plan: 1 error(s) occurred:

* module.tf_subnets.aws_route_table_association.public: 2 error(s) occurred:

* module.tf_subnets.aws_route_table_association.public[0]: Resource 'aws_route_table.public' not found for variable 'aws_route_table.public.id'
* module.tf_subnets.aws_route_table_association.public[1]: Resource 'aws_route_table.public' not found for variable 'aws_route_table.public.id'

@const-bon
Copy link
Contributor

There was an issue if we try to use existing AWS Internet Gateway and Default Route Table.

Even if we use code like this it doesn't work.

resource "aws_route_table_association" "public" {
  count = "${length(var.availability_zones)}"
  subnet_id      = "${element(aws_subnet.public.*.id, count.index)}"
  route_table_id = "${signum(length(var.vpc_default_route_table)) == 1 ? var.vpc_default_route_table : aws_route_table.public.id }"
}

Because if we do not create aws_route_table.public (when we use existing route table), Terraform is unable to resolve aws_route_table.public.id and we got an error.

Error running plan: 1 error(s) occurred:

* module.tf_subnets.aws_route_table_association.public: 2 error(s) occurred:

* module.tf_subnets.aws_route_table_association.public[0]: Resource 'aws_route_table.public' not found for variable 'aws_route_table.public.id'
* module.tf_subnets.aws_route_table_association.public[1]: Resource 'aws_route_table.public' not found for variable 'aws_route_table.public.id'

I've pinned igw_id as required value and added resources for two cases: use existing aws_route_table and create new aws_route_table.

resource "aws_route_table_association" "public_new" {
  count = "${signum(length(var.vpc_default_route_table)) == 1 ? 0 : length(var.availability_zones)}"
  subnet_id      = "${element(aws_subnet.public.*.id, count.index)}"
  route_table_id = "${aws_route_table.public.id}"
}

resource "aws_route_table_association" "public_exists" {
  count = "${signum(length(var.vpc_default_route_table)) == 1 ? length(var.availability_zones) : 0}"
  subnet_id      = "${element(aws_subnet.public.*.id, count.index)}"
  route_table_id = "${var.vpc_default_route_table}"
}

@const-bon
Copy link
Contributor

I've tested terraform plan with two cases:

module "tf_subnets" {
  source = "git::https://github.com/cloudposse/tf_subnets.git?ref=init"

  availability_zones      = ["us-east-1a", "us-east-1b"]
  namespace               = "test"
  name                    = "test"
  stage                   = "test"
  region                  = "us-east-1"
  vpc_id                  = "vpc-aceb27ca"
  igw_id                  = "igw-9c26adfb"
  vpc_default_route_table = "rtb-f4f0ce92"
}
module "tf_subnets" {
  source = "git::https://github.com/cloudposse/tf_subnets.git?ref=init"

  availability_zones      = ["us-east-1a", "us-east-1b"]
  namespace               = "test"
  name                    = "test"
  stage                   = "test"
  region                  = "us-east-1"
  vpc_id                  = "vpc-aceb27ca"
  igw_id                  = "igw-9c26adfb"
}

Both are ok.

@const-bon
Copy link
Contributor

@osterman please review

public.tf Outdated
resource "aws_route_table_association" "public_exists" {
count = "${signum(length(var.vpc_default_route_table)) == 1 ? length(var.availability_zones) : 0}"
subnet_id = "${element(aws_subnet.public.*.id, count.index)}"
route_table_id = "${var.vpc_default_route_table}"
Copy link
Member

Choose a reason for hiding this comment

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

We should suffix with _id if it refers to an ID (for consistency).

Copy link
Contributor

Choose a reason for hiding this comment

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

Added.

public.tf Outdated
route_table_id = "${aws_route_table.public.id}"
}

resource "aws_route_table_association" "public_exists" {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call this public_default

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed.

public.tf Outdated
tags = "${module.tf_label.tags}"
}

resource "aws_route_table_association" "public_new" {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this public so it's symetrical with aws_route_table.public

Copy link
Contributor

Choose a reason for hiding this comment

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

Renamed.

@const-bon
Copy link
Contributor

Added required fixes.
@osterman please approve.

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