実録!Terraform セルフリファクタリング

2019.10.30

こんにちは!大阪オフィスのかずえです。今回は、自分で書いた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階層作成し、ネストループのようなことが実現できました。当たり前といえば当たり前のことではありますが、個人的には今回のリファクタリングでこの部分の理解が深まりとても良かったです。

「こうすればもっと良くに書けるよ」というご意見、お待ちしております!