この記事は公開されてから1年以上経過しています。情報が古い可能性がありますので、ご注意ください。
こんにちは!大阪オフィスのかずえです。今回は、自分で書いたTerraformコードがイマイチだったのでセルフリファクタリングしてみた過程を共有したいと思います。
ぜひ「こうすればもっと良くなるよ」といったご意見ください!
やりたいこと
VPCを二つ(VPCAとVPCB)作成する
- 各VPCはパブリックサブネットを二つ持つ
- (後ほどTransit GatewayでVPC間を接続予定)
初回構築時のコード
data aws_availability_zones az {}
variable vpc_cidr {
type = list(string)
default = [
"10.0.0.0/16",
"172.16.0.0/16"
]
}
variable vpc_name {
type = list(string)
default = [
"A",
"B"
]
}
resource aws_vpc vpc {
count = length(var.vpc_cidr)
cidr_block = var.vpc_cidr[count.index]
enable_dns_support = true
enable_dns_hostnames = true
tags = {
Name = "${var.basename}-${var.env}-vpc-${element(var.vpc_name, count.index)}"
}
}
resource aws_internet_gateway igw {
count = length(var.vpc_cidr)
vpc_id = aws_vpc.vpc[count.index].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${element(var.vpc_name, count.index)}-igw"
}
}
resource aws_subnet A-public {
count = 2
vpc_id = aws_vpc.vpc[index(var.vpc_name, "A")].id
cidr_block = cidrsubnet(aws_vpc.vpc[0].cidr_block, 8, count.index)
availability_zone = data.aws_availability_zones.az.names[count.index]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-A-public-subnet-${count.index + 1}"
}
}
resource aws_subnet B-public {
count = 2
vpc_id = aws_vpc.vpc[index(var.vpc_name, "B")].id
cidr_block = cidrsubnet(aws_vpc.vpc[1].cidr_block, 8, count.index)
availability_zone = data.aws_availability_zones.az.names[count.index]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-B-public-subnet-${count.index + 1}"
}
}
resource aws_route_table public {
count = length(var.vpc_cidr)
vpc_id = aws_vpc.vpc[count.index].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.vpc_name[count.index]}-public-subnet-rtb"
}
}
resource aws_route public {
count = length(var.vpc_cidr)
route_table_id = aws_route_table.public[count.index].id
gateway_id = aws_internet_gateway.igw[count.index].id
destination_cidr_block = "0.0.0.0/0"
}
resource aws_route_table_association A-public {
count = 2
subnet_id = aws_subnet.A-public[count.index].id
route_table_id = aws_route_table.public[index(var.vpc_name, "A")].id
}
resource aws_route_table_association B-public {
count = 2
subnet_id = aws_subnet.B-public[count.index].id
route_table_id = aws_route_table.public[index(var.vpc_name, "B")].id
}
resource aws_network_acl acl {
count = length(var.vpc_cidr)
vpc_id = aws_vpc.vpc[count.index].id
subnet_ids = setunion(
count.index == index(var.vpc_name, "A") ? aws_subnet.A-public.*.id : aws_subnet.B-public.*.id
)
ingress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
egress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.vpc_name[count.index]}-acl"
}
}
皆さんならどのようにリファクタリングされますでしょうか。それではリファクタリング、開始します。
変更1 : listからmapに変える
countの0がVPCA、1がVPCBと暗黙的に決まっているのがイマイチだなと感じました。
aws_route_table.public[index(var.vpc_name, "A")]
といった処理もわかりにくいですよね。最初はaws_route_table.public[0]
と書いていました。ですが、できるだけマジックナンバーを無くしたいと思ってindex()
を使う方法にしたのです。が、わかりにくい。
v0.12の新文法 for_eachを(mapを代入して)使うと、1resouce項で複数リソース作成した場合のlistのインデックス部分(例:aws_route_table.public[0]
の0の部分)が、mapのindex値に変わります。わかりやすくなりそうなので、変数をlistからmapに変えて、countをfor_eachに変えてみました。
data aws_availability_zones az {}
variable vpc {
type = map(string)
default = {
A = "10.0.0.0/16",
B = "172.16.0.0/16"
}
}
resource aws_vpc vpc {
for_each = var.vpc
cidr_block = each.value
enable_dns_support = true
enable_dns_hostnames = true
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}"
}
}
resource aws_internet_gateway igw {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-igw"
}
}
resource aws_subnet A-public {
count = 2
vpc_id = aws_vpc.vpc["A"].id
cidr_block = cidrsubnet(aws_vpc.vpc["A"].cidr_block, 8, count.index)
availability_zone = data.aws_availability_zones.az.names[count.index]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-A-public-subnet-${count.index + 1}"
}
}
resource aws_subnet B-public {
count = 2
vpc_id = aws_vpc.vpc["B"].id
cidr_block = cidrsubnet(aws_vpc.vpc["B"].cidr_block, 8, count.index)
availability_zone = data.aws_availability_zones.az.names[count.index]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-B-public-subnet-${count.index + 1}"
}
}
resource aws_route_table public {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-public-subnet-rtb"
}
}
resource aws_route public {
for_each = var.vpc
route_table_id = aws_route_table.public[each.key].id
gateway_id = aws_internet_gateway.igw[each.key].id
destination_cidr_block = "0.0.0.0/0"
}
resource aws_route_table_association A-public {
count = 2
subnet_id = aws_subnet.A-public[count.index].id
route_table_id = aws_route_table.public["A"].id
}
resource aws_route_table_association B-public {
count = 2
subnet_id = aws_subnet.B-public[count.index].id
route_table_id = aws_route_table.public["B"].id
}
resource aws_network_acl acl {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
subnet_ids = setunion(
each.key == "A" ? aws_subnet.A-public.*.id : aws_subnet.B-public.*.id
)
ingress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
egress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-acl"
}
}
aws_route_table.public["A"].id
などと書けるようになったので、少しわかりやすくなったのではないでしょうか。
変更2 : VPC毎に書いている箇所をまとめたい
大体のリソースはVPCAとVPCBでまとめて作成できているのですが、サブネットなど1VPCあたり複数個作成するものはリソースを分けて記述しています。どうせならこういったコードも駆逐してすべてVPCAとVPCBまとめて作成できるようにしたい。ということでやってみました。
data aws_availability_zones az {}
variable vpc {
type = map(string)
default = {
A = "10.0.0.0/16",
B = "172.16.0.0/16"
}
}
locals {
vpc_name = keys(var.vpc)
}
resource aws_vpc vpc {
for_each = var.vpc
cidr_block = each.value
enable_dns_support = true
enable_dns_hostnames = true
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}"
}
}
resource aws_internet_gateway igw {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-igw"
}
}
resource aws_subnet public {
for_each = { for index, name in concat(local.vpc_name, local.vpc_name) : index => name }
vpc_id = aws_vpc.vpc[each.value].id
cidr_block = cidrsubnet(aws_vpc.vpc[each.value].cidr_block, 8, floor(each.key / 2))
availability_zone = data.aws_availability_zones.az.names[floor(each.key / 2)]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.value}-public-subnet-${floor(each.key / 2) + 1}"
}
}
resource aws_route_table public {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-public-subnet-rtb"
}
}
resource aws_route public {
for_each = var.vpc
route_table_id = aws_route_table.public[each.key].id
gateway_id = aws_internet_gateway.igw[each.key].id
destination_cidr_block = "0.0.0.0/0"
}
resource aws_route_table_association public {
for_each = { for index, name in concat(local.vpc_name, local.vpc_name) : index => name }
subnet_id = aws_subnet.public[each.key].id
route_table_id = aws_route_table.public[each.value].id
}
resource aws_network_acl acl {
for_each = var.vpc
vpc_id = aws_vpc.vpc[each.key].id
subnet_ids = [for subnet in aws_subnet.public : subnet.id if subnet.vpc_id == aws_vpc.vpc[each.key].id]
ingress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
egress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
tags = {
Name = "${var.basename}-${var.env}-vpc-${each.key}-acl"
}
}
できました。が、可読性最悪。。これはだめでしょう。
変更3 : モジュール化する
どうにかわかりやすく、かつまとめて記述する方法はないものかと試行錯誤していたのですが、「あれ、これモジュールを利用するのが一番まっとうな方法なのでは?」と気づいてしまいました。
variable vpc {
type = map(string)
default = {
A = "10.0.0.0/16",
B = "172.16.0.0/16"
}
}
module A {
source = "./modules/vpc"
env = var.env
basename = var.basename
name = "A"
cidr = var.vpc["A"]
}
module B {
source = "./modules/vpc"
env = var.env
basename = var.basename
name = "B"
cidr = var.vpc["B"]
}
./modules/vpc/vpc.tf
variable env {}
variable basename {}
variable name {}
variable cidr {}
data aws_availability_zones az {}
resource aws_vpc vpc {
cidr_block = var.cidr
enable_dns_support = true
enable_dns_hostnames = true
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.name}"
}
}
resource aws_internet_gateway igw {
vpc_id = aws_vpc.vpc.id
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.name}-igw"
}
}
resource aws_subnet public {
count = 2
vpc_id = aws_vpc.vpc.id
cidr_block = cidrsubnet(aws_vpc.vpc.cidr_block, 8, count.index)
availability_zone = data.aws_availability_zones.az.names[count.index]
map_public_ip_on_launch = false
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.name}-public-subnet-${count.index + 1}"
}
}
resource aws_route_table public {
vpc_id = aws_vpc.vpc.id
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.name}-public-subnet-rtb"
}
}
resource aws_route public {
route_table_id = aws_route_table.public.id
gateway_id = aws_internet_gateway.igw.id
destination_cidr_block = "0.0.0.0/0"
}
resource aws_route_table_association public {
count = 2
subnet_id = aws_subnet.public[count.index].id
route_table_id = aws_route_table.public.id
}
resource aws_network_acl acl {
vpc_id = aws_vpc.vpc.id
subnet_ids = aws_subnet.public.*.id
ingress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
egress {
protocol = "-1"
rule_no = 100
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
}
tags = {
Name = "${var.basename}-${var.env}-vpc-${var.name}-acl"
}
}
はい。わかりやすく、かつ重複処理を共通化することができています。
おまけ : モジュール内でもfor_eachすればいいんじゃないの?
こんな感じ
./modules/vpc/vpc.tf
module vpc {
for_each = var.vpc
source = "./modules/vpc"
env = var.env
basename = var.basename
name = each.key
cidr = each.value
}
2019/10/30現時点ではこれはできません。
Moduleでのfor_each(とcount)の利用は現在準備中だそうですよ。
感想、学び
moduleの利便性を改めて実感しました。
「変更2 : VPC毎に書いている箇所をまとめたい」で書いたコードの可読性が最悪になったのは、Terraformはネストループを記述できないからだと考えています。Subnet部分などの記述はVPCごとに二つずつリソース作成 = 2x2のリソースを作成する処理です。for_eachでネストループが書けるとか、for_eachとcountを併用できると言ったことができればもう少し簡潔に書けたのではないかと思います。
そこで、moduleを活用することで
- moduleを複数回利用することで二つのVPCの処理を共通化する
- module内ではcount(for_eachでも良い)を使ってVPC内の複数回リソース作成する処理(Subnetなど)を共通化する
と、moduleとcount(for_each)で2階層作成し、ネストループのようなことが実現できました。当たり前といえば当たり前のことではありますが、個人的には今回のリファクタリングでこの部分の理解が深まりとても良かったです。
「こうすればもっと良くに書けるよ」というご意見、お待ちしております!