modules/vpc: support re-apply of terraform when aws AZ number changes

modules/aws/vpc/common.tf

@@ -0,0 +1,69 @@
+# Canonical internal state definitions for this module.
+# read only: only locals and data source definitions allowed. No resources or module blocks in this file
+data "aws_region" "current" {
+  current = true
+}
+
+// Fetch a list of available AZs
+data "aws_availability_zones" "azs" {}
+
+// Only reference data sources which are gauranteed to exist at any time (above) in this locals{} block
+locals {
+  // Define canonical source of truth for this
+  external_vpc_mode = "${var.external_vpc_id != ""}"
+
+  // List of possible AZs for each type of subnet
+  new_worker_subnet_azs = ["${coalescelist(keys(var.new_worker_subnet_configs), data.aws_availability_zones.azs.names)}"]
+  new_master_subnet_azs = ["${coalescelist(keys(var.new_master_subnet_configs), data.aws_availability_zones.azs.names)}"]
+
+  // How many AZs to create worker and master subnets in (always zero if external_vpc_mode)
+  new_worker_az_count = "${local.external_vpc_mode ? 0 : length(local.new_worker_subnet_azs)}"
+  new_master_az_count = "${local.external_vpc_mode ? 0 : length(local.new_master_subnet_azs)}"
+
+  // The base set of ids needs to build rest of vpc data sources
+  // This is crux of dealing with existing vpc / new vpc incongruity
+  vpc_id = "${local.external_vpc_mode ? var.external_vpc_id : element(concat(aws_vpc.new_vpc.*.id,list("")),0)}"
+
+  // When referencing the _ids arrays or data source arrays via count = , always use the *_count variable rather than taking the length of the list
+  worker_subnet_ids   = ["${coalescelist(aws_subnet.worker_subnet.*.id,var.external_worker_subnet_ids)}"]
+  master_subnet_ids   = ["${coalescelist(aws_subnet.master_subnet.*.id,var.external_master_subnet_ids)}"]
+  worker_subnet_count = "${local.external_vpc_mode ? length(var.external_worker_subnet_ids) : local.new_worker_az_count}"
+  master_subnet_count = "${local.external_vpc_mode ? length(var.external_master_subnet_ids) : local.new_master_az_count}"
+}
+
+# all data sources should be input variable-agnostic and used as canonical source for querying "state of resources" and building outputs
+# (ie: we don't want "data.aws_subnet.external-worker" and "data.aws_subnet.worker". just "data.aws_subnet.worker" used everwhere for list of worker subnets for any valid input var state)
+
+data "aws_vpc" "cluster_vpc" {
+  id = "${local.vpc_id}"
+}
+
+data "aws_subnet" "worker" {
+  count  = "${local.worker_subnet_count}"
+  id     = "${local.worker_subnet_ids[count.index]}"
+  vpc_id = "${local.vpc_id}"
+}
+
+data "aws_subnet" "master" {
+  count  = "${local.master_subnet_count}"
+  id     = "${local.master_subnet_ids[count.index]}"
+  vpc_id = "${local.vpc_id}"
+}
+
+data "aws_route_table" "worker" {
+  count = "${local.worker_subnet_count}"
+
+  filter = {
+    name   = "association.subnet-id"
+    values = ["${list(local.worker_subnet_ids[count.index])}"]
+  }
+}
+
+data "aws_route_table" "master" {
+  count = "${local.master_subnet_count}"
+
+  filter = {
+    name   = "association.subnet-id"
+    values = ["${list(local.master_subnet_ids[count.index])}"]
+  }
+}

modules/aws/vpc/variables.tf

@@ -1,11 +1,3 @@
-variable "master_az_count" {
-  type = "string"
-}
-
-variable "worker_az_count" {
-  type = "string"
-}
-
 variable "cidr_block" {
   type = "string"
 }
@@ -26,11 +18,11 @@ variable "external_vpc_id" {
   type = "string"
 }
 
-variable "external_master_subnets" {
+variable "external_master_subnet_ids" {
   type = "list"
 }
 
-variable "external_worker_subnets" {
+variable "external_worker_subnet_ids" {
   type = "list"
 }
 
@@ -45,20 +37,14 @@ variable "enable_etcd_sg" {
   default     = true
 }
 
-variable "master_subnets" {
-  type = "list"
-}
-
-variable "worker_subnets" {
-  type = "list"
-}
-
-variable "master_azs" {
-  type = "list"
+variable "new_master_subnet_configs" {
+  description = "{az_name = new_subnet_cidr}: Empty map means create new subnets in all availability zones in region with generated cidrs"
+  type        = "map"
 }
 
-variable "worker_azs" {
-  type = "list"
+variable "new_worker_subnet_configs" {
+  description = "{az_name = new_subnet_cidr}: Empty map means create new subnets in all availability zones in region with generated cidrs"
+  type        = "map"
 }
 
 variable "private_master_endpoints" {

modules/aws/vpc/vpc-private.tf

@@ -1,46 +1,43 @@
 resource "aws_route_table" "private_routes" {
-  count  = "${var.external_vpc_id == "" ? var.worker_az_count : 0}"
+  count  = "${local.new_worker_az_count}"
   vpc_id = "${data.aws_vpc.cluster_vpc.id}"
 
   tags = "${merge(map(
-      "Name", "${var.cluster_name}-private-${data.aws_availability_zones.azs.names[count.index]}",
+      "Name","${var.cluster_name}-private-${local.new_worker_subnet_azs[count.index]}",
       "kubernetes.io/cluster/${var.cluster_name}", "shared",
       "tectonicClusterID", "${var.cluster_id}"
     ), var.extra_tags)}"
 }
 
 resource "aws_route" "to_nat_gw" {
-  count                  = "${var.external_vpc_id == "" ? var.worker_az_count : 0}"
+  count                  = "${local.new_worker_az_count}"
   route_table_id         = "${aws_route_table.private_routes.*.id[count.index]}"
   destination_cidr_block = "0.0.0.0/0"
   nat_gateway_id         = "${element(aws_nat_gateway.nat_gw.*.id, count.index)}"
   depends_on             = ["aws_route_table.private_routes"]
 }
 
 resource "aws_subnet" "worker_subnet" {
-  count = "${var.external_vpc_id == "" ? var.worker_az_count : 0}"
+  count = "${local.new_worker_az_count}"
 
   vpc_id = "${data.aws_vpc.cluster_vpc.id}"
 
-  cidr_block = "${length(var.worker_subnets) > 1 ?
-    "${element(var.worker_subnets, count.index)}" :
-    "${cidrsubnet(data.aws_vpc.cluster_vpc.cidr_block, 4, count.index + var.worker_az_count)}"
-  }"
-
-  availability_zone = "${var.worker_azs[count.index]}"
+  cidr_block = "${lookup(var.new_worker_subnet_configs,
+    local.new_worker_subnet_azs[count.index],
+    cidrsubnet(local.new_worker_cidr_range, 3, count.index),
+  )}"
 
   tags = "${merge(map(
-      "Name", "${var.cluster_name}-worker-${ "${length(var.worker_azs)}" > 0 ?
-    "${var.worker_azs[count.index]}" :
-    "${data.aws_availability_zones.azs.names[count.index]}" }",
-      "kubernetes.io/cluster/${var.cluster_name}", "shared",
-      "kubernetes.io/role/internal-elb", "",
-      "tectonicClusterID", "${var.cluster_id}"
-    ), var.extra_tags)}"
+    "Name", "${var.cluster_name}-worker-${local.new_worker_subnet_azs[count.index]}",
+    "kubernetes.io/cluster/${var.cluster_name}","shared",
+    "kubernetes.io/role/internal-elb", "",
+    "tectonicClusterID", "${var.cluster_id}",
+    ),
+    var.extra_tags)}"
 }
 
 resource "aws_route_table_association" "worker_routing" {
-  count          = "${var.external_vpc_id == "" ? var.worker_az_count : 0}"
+  count          = "${local.new_worker_az_count}"
   route_table_id = "${aws_route_table.private_routes.*.id[count.index]}"
   subnet_id      = "${aws_subnet.worker_subnet.*.id[count.index]}"
 }

modules/aws/vpc/vpc-public.tf

@@ -1,5 +1,5 @@
 resource "aws_internet_gateway" "igw" {
-  count  = "${var.external_vpc_id == "" ? 1 : 0}"
+  count  = "${local.external_vpc_mode ? 0 : 1}"
   vpc_id = "${data.aws_vpc.cluster_vpc.id}"
 
   tags = "${merge(map(
@@ -16,52 +16,49 @@ resource "aws_route_table" "default" {
   tags = "${merge(map(
       "Name", "${var.cluster_name}-public",
       "kubernetes.io/cluster/${var.cluster_name}", "shared",
-      "tectonicClusterID", "${var.cluster_id}"
+      "tectonicClusterID", "${var.cluster_id}",
     ), var.extra_tags)}"
 }
 
 resource "aws_main_route_table_association" "main_vpc_routes" {
-  count          = "${var.external_vpc_id == "" ? 1 : 0}"
+  count          = "${local.external_vpc_mode ? 0 : 1}"
   vpc_id         = "${data.aws_vpc.cluster_vpc.id}"
   route_table_id = "${aws_route_table.default.id}"
 }
 
 resource "aws_route" "igw_route" {
-  count                  = "${var.external_vpc_id == "" ? 1 : 0}"
+  count                  = "${local.external_vpc_mode ? 0 : 1}"
   destination_cidr_block = "0.0.0.0/0"
   route_table_id         = "${aws_route_table.default.id}"
   gateway_id             = "${aws_internet_gateway.igw.id}"
 }
 
 resource "aws_subnet" "master_subnet" {
-  count = "${var.external_vpc_id == "" ? var.master_az_count : 0}"
-
+  count  = "${local.new_master_az_count}"
   vpc_id = "${data.aws_vpc.cluster_vpc.id}"
 
-  cidr_block = "${length(var.master_subnets) > 1 ?
-    "${element(var.master_subnets, count.index)}" :
-    "${cidrsubnet(data.aws_vpc.cluster_vpc.cidr_block, 4, count.index)}"
-  }"
+  cidr_block = "${lookup(var.new_master_subnet_configs,
+    local.new_master_subnet_azs[count.index],
+    cidrsubnet(local.new_master_cidr_range, 3, count.index),
+  )}"
 
-  availability_zone = "${var.master_azs[count.index]}"
+  availability_zone = "${local.new_master_subnet_azs[count.index]}"
 
   tags = "${merge(map(
-      "Name", "${var.cluster_name}-master-${ "${length(var.master_azs)}" > 0 ?
-     "${var.master_azs[count.index]}" :
-     "${data.aws_availability_zones.azs.names[count.index]}" }",
+    "Name", "${var.cluster_name}-master-${local.new_master_subnet_azs[count.index]}",
       "kubernetes.io/cluster/${var.cluster_name}", "shared",
       "tectonicClusterID", "${var.cluster_id}"
     ), var.extra_tags)}"
 }
 
 resource "aws_route_table_association" "route_net" {
-  count          = "${var.external_vpc_id == "" ? var.master_az_count : 0}"
+  count          = "${local.new_master_az_count}"
   route_table_id = "${aws_route_table.default.id}"
   subnet_id      = "${aws_subnet.master_subnet.*.id[count.index]}"
 }
 
 resource "aws_eip" "nat_eip" {
-  count = "${var.external_vpc_id == "" ? min(var.master_az_count, var.worker_az_count) : 0}"
+  count = "${min(local.new_master_az_count,local.new_worker_az_count)}"
   vpc   = true
 
   # Terraform does not declare an explicit dependency towards the internet gateway.
@@ -71,7 +68,7 @@ resource "aws_eip" "nat_eip" {
 }
 
 resource "aws_nat_gateway" "nat_gw" {
-  count         = "${var.external_vpc_id == "" ? min(var.master_az_count, var.worker_az_count) : 0}"
+  count         = "${min(local.new_master_az_count,local.new_worker_az_count)}"
   allocation_id = "${aws_eip.nat_eip.*.id[count.index]}"
   subnet_id     = "${aws_subnet.master_subnet.*.id[count.index]}"
 }

modules/aws/vpc/vpc.tf

@@ -1,4 +1,7 @@
-data "aws_availability_zones" "azs" {}
+locals {
+  new_worker_cidr_range = "${cidrsubnet(data.aws_vpc.cluster_vpc.cidr_block,1,1)}"
+  new_master_cidr_range = "${cidrsubnet(data.aws_vpc.cluster_vpc.cidr_block,1,0)}"
+}
 
 resource "aws_vpc" "new_vpc" {
   count                = "${var.external_vpc_id == "" ? 1 : 0}"
@@ -12,19 +15,3 @@ resource "aws_vpc" "new_vpc" {
       "tectonicClusterID", "${var.cluster_id}"
     ), var.extra_tags)}"
 }
-
-data "aws_vpc" "cluster_vpc" {
-  # The join() hack is required because currently the ternary operator
-  # evaluates the expressions on both branches of the condition before
-  # returning a value. When providing and external VPC, the template VPC
-  # resource gets a count of zero which triggers an evaluation error.
-  #
-  # This is tracked upstream: https://github.com/hashicorp/hil/issues/50
-  #
-  id = "${var.external_vpc_id == "" ? join(" ", aws_vpc.new_vpc.*.id) : var.external_vpc_id }"
-}
-
-locals {
-  master_subnet_ids = ["${split(",", var.external_vpc_id == "" ? join(",", aws_subnet.master_subnet.*.id) :  join(",", data.aws_subnet.external_master.*.id))}"]
-  worker_subnet_ids = ["${split(",", var.external_vpc_id == "" ? join(",", aws_subnet.worker_subnet.*.id) :  join(",", data.aws_subnet.external_worker.*.id))}"]
-}

platforms/govcloud/main.tf

@@ -21,50 +21,24 @@ module "container_linux" {
 module "vpc" {
   source = "../../modules/aws/vpc"
 
-  base_domain              = "${var.tectonic_base_domain}"
-  cidr_block               = "${var.tectonic_govcloud_vpc_cidr_block}"
-  cluster_id               = "${module.tectonic.cluster_id}"
-  cluster_name             = "${var.tectonic_cluster_name}"
-  custom_dns_name          = "${var.tectonic_dns_name}"
-  enable_etcd_sg           = "${length(compact(var.tectonic_etcd_servers)) == 0 ? 1 : 0}"
-  external_master_subnets  = "${compact(var.tectonic_govcloud_external_master_subnet_ids)}"
-  external_vpc_id          = "${var.tectonic_govcloud_external_vpc_id}"
-  external_worker_subnets  = "${compact(var.tectonic_govcloud_external_worker_subnet_ids)}"
-  extra_tags               = "${var.tectonic_govcloud_extra_tags}"
-  private_master_endpoints = true
-  public_master_endpoints  = false
+  base_domain     = "${var.tectonic_base_domain}"
+  cidr_block      = "${var.tectonic_govcloud_vpc_cidr_block}"
+  cluster_id      = "${module.tectonic.cluster_id}"
+  cluster_name    = "${var.tectonic_cluster_name}"
+  custom_dns_name = "${var.tectonic_dns_name}"
+  enable_etcd_sg  = "${length(compact(var.tectonic_etcd_servers)) == 0 ? 1 : 0}"
+  external_vpc_id = "${var.tectonic_govcloud_external_vpc_id}"
+
+  external_master_subnet_ids = "${compact(var.tectonic_govcloud_external_master_subnet_ids)}"
+  external_worker_subnet_ids = "${compact(var.tectonic_govcloud_external_worker_subnet_ids)}"
+  extra_tags                 = "${var.tectonic_govcloud_extra_tags}"
 
-  # VPC layout settings.
-  #
-  # The following parameters control the layout of the VPC accross availability zones.
-  # Two modes are available:
-  # A. Explicitly configure a list of AZs + associated subnet CIDRs
-  # B. Let the module calculate subnets accross a set number of AZs
-  #
-  # To enable mode A, configure a set of AZs + CIDRs for masters and workers using the
-  # "tectonic_govcloud_master_custom_subnets" and "tectonic_govcloud_worker_custom_subnets" variables.
-  #
-  # To enable mode B, make sure that "tectonic_govcloud_master_custom_subnets" and "tectonic_govcloud_worker_custom_subnets"
-  # ARE NOT SET.
+  // empty map subnet_configs will have the vpc module creating subnets in all availabile AZs
+  new_master_subnet_configs = "${var.tectonic_govcloud_master_custom_subnets}"
+  new_worker_subnet_configs = "${var.tectonic_govcloud_worker_custom_subnets}"
 
-  # These counts could be deducted by length(keys(var.tectonic_govcloud_master_custom_subnets))
-  # but there is a restriction on passing computed values as counts. This approach works around that.
-  master_az_count = "${length(keys(var.tectonic_govcloud_master_custom_subnets)) > 0 ? "${length(keys(var.tectonic_govcloud_master_custom_subnets))}" : "${length(data.aws_availability_zones.azs.names)}"}"
-  worker_az_count = "${length(keys(var.tectonic_govcloud_worker_custom_subnets)) > 0 ? "${length(keys(var.tectonic_govcloud_worker_custom_subnets))}" : "${length(data.aws_availability_zones.azs.names)}"}"
-  # The appending of the "padding" element is required as workaround since the function
-  # element() won't work on empty lists. See https://github.com/hashicorp/terraform/issues/11210
-  master_subnets = "${concat(values(var.tectonic_govcloud_master_custom_subnets),list("padding"))}"
-  worker_subnets = "${concat(values(var.tectonic_govcloud_worker_custom_subnets),list("padding"))}"
-  # The split() / join() trick works around the limitation of ternary operator expressions
-  # only being able to return strings.
-  master_azs = "${ split("|", "${length(keys(var.tectonic_govcloud_master_custom_subnets))}" > 0 ?
-    join("|", keys(var.tectonic_govcloud_master_custom_subnets)) :
-    join("|", data.aws_availability_zones.azs.names)
-  )}"
-  worker_azs = "${ split("|", "${length(keys(var.tectonic_govcloud_worker_custom_subnets))}" > 0 ?
-    join("|", keys(var.tectonic_govcloud_worker_custom_subnets)) :
-    join("|", data.aws_availability_zones.azs.names)
-  )}"
+  private_master_endpoints = true
+  public_master_endpoints  = false
 }
 
 module "etcd" {

platforms/govcloud/variables.tf

@@ -140,7 +140,7 @@ variable "tectonic_govcloud_external_master_subnet_ids" {
 
   description = <<EOF
 (optional) List of subnet IDs within an existing VPC to deploy master nodes into.
-Required to use an existing VPC and the list must match the AZ count.
+Required to use an existing VPC, not applicable otherwise.
 
 Example: `["subnet-111111", "subnet-222222", "subnet-333333"]`
 EOF
@@ -153,7 +153,7 @@ variable "tectonic_govcloud_external_worker_subnet_ids" {
 
   description = <<EOF
 (optional) List of subnet IDs within an existing VPC to deploy worker nodes into.
-Required to use an existing VPC and the list must match the AZ count.
+Required to use an existing VPC, not applicable otherwise.
 
 Example: `["subnet-111111", "subnet-222222", "subnet-333333"]`
 EOF