Skip to content

data/aws/vpc: Drop new_az_count and vpc_id locals #2437

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Oct 2, 2019
Merged

data/aws/vpc: Drop new_az_count and vpc_id locals #2437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions data/data/aws/vpc/common.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@

// Only reference data sources which are guaranteed to exist at any time (above) in this locals{} block
locals {
// How many AZs to create subnets in
new_az_count = length(var.availability_zones)

// The VPC ID to use to build the rest of the vpc data sources
vpc_id = aws_vpc.new_vpc.id

private_subnet_ids = aws_subnet.private_subnet.*.id
public_subnet_ids = aws_subnet.public_subnet.*.id
}
Expand All @@ -17,6 +11,6 @@ locals {
# (ie: we don't want "aws.new_vpc" and "data.aws_vpc.cluster_vpc", just "data.aws_vpc.cluster_vpc" used everwhere).

data "aws_vpc" "cluster_vpc" {
id = local.vpc_id
id = aws_vpc.new_vpc.id
}

6 changes: 3 additions & 3 deletions data/data/aws/vpc/master-elb.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ resource "aws_lb_target_group" "api_internal" {
name = "${var.cluster_id}-aint"
protocol = "TCP"
port = 6443
vpc_id = local.vpc_id
vpc_id = data.aws_vpc.cluster_vpc.id
Copy link
Contributor

@abhinavdahiya abhinavdahiya Oct 1, 2019

Choose a reason for hiding this comment

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

using the data in all the clients, ties the clients to how vpc information is fetched.
using resource -> data source makes CREATE, READ, READ api calls which isn't bad but tying our clients to data source to get the vpc id means we can't change those transparently to all the clients.

So i do prefer the local.vpc_id approach.

Copy link
Member Author

@wking wking Oct 1, 2019

Choose a reason for hiding this comment

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

using resource -> data source makes CREATE, READ, READ api calls...

We're already doing that, aren't we? And we're already consuming it here and in other places in master.

which isn't bad but tying our clients to data source to get the vpc id means we can't change those transparently to all the clients.

We can change this transparently by adjusting what we feed into the data block (like this). The consumers just look at data.aws_vpc.cluster_vpc and don't have to care about whether this is a new or preexisting VPC.

So I don't see any functional difference between local.vpc_id and data.aws_vpc.cluster_vpc.id besides the latter being slightly slower due to the latency of the fetch we need to run anyway. And I think adding that latency for consumers that need only the VPC ID is a small price to pay for the benefit of reducing the number of moving parts. Am I missing something? Or do you just think that it's worth having local variables to save the latency when the VPC ID is the only thing we care about?

Copy link
Contributor

@abhinavdahiya abhinavdahiya Oct 1, 2019

Choose a reason for hiding this comment

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

And I think adding that latency for consumers that need only the VPC ID is a small price to pay for the benefit of reducing the number of moving parts. Am I missing something?

it's not super important and we can reconsider if it does later on.


target_type = "ip"

Expand All @@ -71,7 +71,7 @@ resource "aws_lb_target_group" "api_external" {
name = "${var.cluster_id}-aext"
protocol = "TCP"
port = 6443
vpc_id = local.vpc_id
vpc_id = data.aws_vpc.cluster_vpc.id

target_type = "ip"

Expand All @@ -96,7 +96,7 @@ resource "aws_lb_target_group" "services" {
name = "${var.cluster_id}-sint"
protocol = "TCP"
port = 22623
vpc_id = local.vpc_id
vpc_id = data.aws_vpc.cluster_vpc.id

target_type = "ip"

Expand Down
8 changes: 4 additions & 4 deletions data/data/aws/vpc/vpc-private.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
resource "aws_route_table" "private_routes" {
count = local.new_az_count
count = length(var.availability_zones)
vpc_id = data.aws_vpc.cluster_vpc.id

tags = merge(
Expand All @@ -11,7 +11,7 @@ resource "aws_route_table" "private_routes" {
}

resource "aws_route" "to_nat_gw" {
count = local.new_az_count
count = length(var.availability_zones)
route_table_id = aws_route_table.private_routes[count.index].id
destination_cidr_block = "0.0.0.0/0"
nat_gateway_id = element(aws_nat_gateway.nat_gw.*.id, count.index)
Expand All @@ -23,7 +23,7 @@ resource "aws_route" "to_nat_gw" {
}

resource "aws_subnet" "private_subnet" {
count = local.new_az_count
count = length(var.availability_zones)

vpc_id = data.aws_vpc.cluster_vpc.id

Expand All @@ -41,7 +41,7 @@ resource "aws_subnet" "private_subnet" {
}

resource "aws_route_table_association" "private_routing" {
count = local.new_az_count
count = length(var.availability_zones)
route_table_id = aws_route_table.private_routes[count.index].id
subnet_id = aws_subnet.private_subnet[count.index].id
}
Expand Down
8 changes: 4 additions & 4 deletions data/data/aws/vpc/vpc-public.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ resource "aws_route" "igw_route" {
}

resource "aws_subnet" "public_subnet" {
count = local.new_az_count
count = length(var.availability_zones)
vpc_id = data.aws_vpc.cluster_vpc.id

cidr_block = cidrsubnet(local.new_public_cidr_range, 3, count.index)
Expand All @@ -52,13 +52,13 @@ resource "aws_subnet" "public_subnet" {
}

resource "aws_route_table_association" "route_net" {
count = local.new_az_count
count = length(var.availability_zones)
route_table_id = aws_route_table.default.id
subnet_id = aws_subnet.public_subnet[count.index].id
}

resource "aws_eip" "nat_eip" {
count = local.new_az_count
count = length(var.availability_zones)
vpc = true

tags = merge(
Expand All @@ -75,7 +75,7 @@ resource "aws_eip" "nat_eip" {
}

resource "aws_nat_gateway" "nat_gw" {
count = local.new_az_count
count = length(var.availability_zones)
allocation_id = aws_eip.nat_eip[count.index].id
subnet_id = aws_subnet.public_subnet[count.index].id

Expand Down