From 36ed53924fefd39f75421132a941c542dafd09a5 Mon Sep 17 00:00:00 2001
From: Dalton Hubble <dghubble@gmail.com>
Date: Sun, 6 Oct 2019 11:53:49 -0700
Subject: [PATCH] Add stricter types for bare-metal modules

* Review variables available in bare-metal kubernetes modules
for Container Linux and Fedora CoreOS
* Deprecate cluster_domain_suffix variable
* Remove deprecated container_linux_oem variable
---
 CHANGES.md                                    | 11 ++--
 .../kubernetes/cl/install.yaml.tmpl           |  1 -
 .../container-linux/kubernetes/groups.tf      |  2 +-
 .../container-linux/kubernetes/profiles.tf    |  2 -
 .../container-linux/kubernetes/variables.tf   | 52 ++++++++-----------
 .../fedora-coreos/kubernetes/profiles.tf      |  6 +--
 .../fedora-coreos/kubernetes/variables.tf     | 46 ++++++++--------
 docs/cl/bare-metal.md                         | 19 ++++---
 docs/fedora-coreos/bare-metal.md              | 23 ++++----
 9 files changed, 76 insertions(+), 86 deletions(-)

diff --git a/CHANGES.md b/CHANGES.md
index de1bf06eb..5ac814b63 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -7,30 +7,31 @@ Notable changes between versions.
 * Kubernetes [v1.16.1](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG-1.16.md#v1161)
 * Update etcd from v3.3.15 to [v3.4.1](https://github.com/etcd-io/etcd/releases/tag/v3.4.1)
 * Update Calico from v3.8.2 to [v3.9.1](https://docs.projectcalico.org/v3.9/release-notes/)
+* Add Terraform v0.12 variables types ([#553](https://github.com/poseidon/typhoon/pull/553), [#557](https://github.com/poseidon/typhoon/pull/557), [#560](https://github.com/poseidon/typhoon/pull/560), [#556](https://github.com/poseidon/typhoon/pull/556), [#562](https://github.com/poseidon/typhoon/pull/562))
+  * Deprecate `cluster_domain_suffix` variable
 
 #### AWS
 
-* Add Terraform v0.12 variables types ([#553](https://github.com/poseidon/typhoon/pull/553))
 * Add `node_labels` variable to internal `workers` module ([#550](https://github.com/poseidon/typhoon/pull/550))
 
 #### Azure
 
-* Add Terraform v0.12 variables types ([#557](https://github.com/poseidon/typhoon/pull/557))
-* Change `workers` module default `vm_type` to `Standard_DS1_v2` (followup to [#539](https://github.com/poseidon/typhoon/pull/539))
+* Promote `networking` provider Calico VXLAN out of experimental (set `networking = "calico"`)
 * Add `node_labels` variable to internal `workers` module ([#550](https://github.com/poseidon/typhoon/pull/550))
+* Change `workers` module default `vm_type` to `Standard_DS1_v2` (followup to [#539](https://github.com/poseidon/typhoon/pull/539))
 
 #### Bare-Metal
 
 * Fix Terraform missing comma error ([#549](https://github.com/poseidon/typhoon/pull/549))
+* Remove deprecated `container_linux_oem` variable ([#562](https://github.com/poseidon/typhoon/pull/562))
 
 #### DigitalOcean
 
-* Add Terraform v0.12 variables types ([#560](https://github.com/poseidon/typhoon/pull/560))
+* Promote `networking` provider Calico VXLAN out of experimental (set `networking = "calico"`)
 * Fix Terraform missing comma error ([#549](https://github.com/poseidon/typhoon/pull/549))
 
 #### Google Cloud
 
-* Add Terraform v0.12 variables types ([#556](https://github.com/poseidon/typhoon/pull/556))
 * Add `node_labels` variable to internal `workers` module ([#550](https://github.com/poseidon/typhoon/pull/550))
 
 #### Addons
diff --git a/bare-metal/container-linux/kubernetes/cl/install.yaml.tmpl b/bare-metal/container-linux/kubernetes/cl/install.yaml.tmpl
index 0acca9abf..e8562c932 100644
--- a/bare-metal/container-linux/kubernetes/cl/install.yaml.tmpl
+++ b/bare-metal/container-linux/kubernetes/cl/install.yaml.tmpl
@@ -35,7 +35,6 @@ storage:
             -d ${install_disk} \
             -C ${os_channel} \
             -V ${os_version} \
-            -o "${container_linux_oem}" \
             ${baseurl_flag} \
             -i ignition.json
           udevadm settle
diff --git a/bare-metal/container-linux/kubernetes/groups.tf b/bare-metal/container-linux/kubernetes/groups.tf
index 2cc410d90..d7ee3c4e4 100644
--- a/bare-metal/container-linux/kubernetes/groups.tf
+++ b/bare-metal/container-linux/kubernetes/groups.tf
@@ -4,7 +4,7 @@ resource "matchbox_group" "install" {
   name = format("install-%s", element(concat(var.controller_names, var.worker_names), count.index))
 
   # pick one of 4 Matchbox profiles (Container Linux or Flatcar, cached or non-cached)
-  profile = local.flavor == "flatcar" ? var.cached_install == "true" ? element(matchbox_profile.cached-flatcar-linux-install.*.name, count.index) : element(matchbox_profile.flatcar-install.*.name, count.index) : var.cached_install == "true" ? element(matchbox_profile.cached-container-linux-install.*.name, count.index) : element(matchbox_profile.container-linux-install.*.name, count.index)
+  profile = local.flavor == "flatcar" ? var.cached_install ? element(matchbox_profile.cached-flatcar-linux-install.*.name, count.index) : element(matchbox_profile.flatcar-install.*.name, count.index) : var.cached_install ? element(matchbox_profile.cached-container-linux-install.*.name, count.index) : element(matchbox_profile.container-linux-install.*.name, count.index)
 
   selector = {
     mac = element(concat(var.controller_macs, var.worker_macs), count.index)
diff --git a/bare-metal/container-linux/kubernetes/profiles.tf b/bare-metal/container-linux/kubernetes/profiles.tf
index 0a6ba49e3..78f958f59 100644
--- a/bare-metal/container-linux/kubernetes/profiles.tf
+++ b/bare-metal/container-linux/kubernetes/profiles.tf
@@ -40,7 +40,6 @@ data "template_file" "container-linux-install-configs" {
     os_version          = var.os_version
     ignition_endpoint   = format("%s/ignition", var.matchbox_http_endpoint)
     install_disk        = var.install_disk
-    container_linux_oem = var.container_linux_oem
     ssh_authorized_key  = var.ssh_authorized_key
     # only cached-container-linux profile adds -b baseurl
     baseurl_flag = ""
@@ -82,7 +81,6 @@ data "template_file" "cached-container-linux-install-configs" {
     os_version          = var.os_version
     ignition_endpoint   = format("%s/ignition", var.matchbox_http_endpoint)
     install_disk        = var.install_disk
-    container_linux_oem = var.container_linux_oem
     ssh_authorized_key  = var.ssh_authorized_key
     # profile uses -b baseurl to install from matchbox cache
     baseurl_flag = "-b ${var.matchbox_http_endpoint}/assets/${local.flavor}"
diff --git a/bare-metal/container-linux/kubernetes/variables.tf b/bare-metal/container-linux/kubernetes/variables.tf
index a95a808b3..5f6d5809f 100644
--- a/bare-metal/container-linux/kubernetes/variables.tf
+++ b/bare-metal/container-linux/kubernetes/variables.tf
@@ -62,8 +62,8 @@ variable "clc_snippets" {
 # configuration
 
 variable "k8s_domain_name" {
-  description = "Controller DNS name which resolves to a controller instance. Workers and kubeconfig's will communicate with this endpoint (e.g. cluster.example.com)"
   type        = string
+  description = "Controller DNS name which resolves to a controller instance. Workers and kubeconfig's will communicate with this endpoint (e.g. cluster.example.com)"
 }
 
 variable "ssh_authorized_key" {
@@ -72,63 +72,55 @@ variable "ssh_authorized_key" {
 }
 
 variable "asset_dir" {
-  description = "Path to a directory where generated assets should be placed (contains secrets)"
   type        = string
+  description = "Absolute path to a directory where generated assets should be placed (contains secrets)"
 }
 
 variable "networking" {
-  description = "Choice of networking provider (flannel or calico)"
   type        = string
+  description = "Choice of networking provider (flannel or calico)"
   default     = "calico"
 }
 
 variable "network_mtu" {
+  type        = number
   description = "CNI interface MTU (applies to calico only)"
-  type        = string
-  default     = "1480"
+  default     = 1480
 }
 
 variable "network_ip_autodetection_method" {
-  description = "Method to autodetect the host IPv4 address (applies to calico only)"
   type        = string
+  description = "Method to autodetect the host IPv4 address (applies to calico only)"
   default     = "first-found"
 }
 
 variable "pod_cidr" {
-  description = "CIDR IPv4 range to assign Kubernetes pods"
   type        = string
+  description = "CIDR IPv4 range to assign Kubernetes pods"
   default     = "10.2.0.0/16"
 }
 
 variable "service_cidr" {
+  type = string
   description = <<EOD
 CIDR IPv4 range to assign Kubernetes services.
 The 1st IP will be reserved for kube_apiserver, the 10th IP will be reserved for coredns.
 EOD
-
-
-  type = string
   default = "10.3.0.0/16"
 }
 
 # optional
 
-variable "cluster_domain_suffix" {
-  description = "Queries for domains with the suffix will be answered by coredns. Default is cluster.local (e.g. foo.default.svc.cluster.local) "
-  type = string
-  default = "cluster.local"
-}
-
 variable "download_protocol" {
   type = string
-  default = "https"
   description = "Protocol iPXE should use to download the kernel and initrd. Defaults to https, which requires iPXE compiled with crypto support. Unused if cached_install is true."
+  default = "https"
 }
 
 variable "cached_install" {
-  type = string
-  default = "false"
+  type = bool
   description = "Whether Container Linux should PXE boot and install from matchbox /assets cache. Note that the admin must have downloaded the os_version into matchbox assets."
+  default = false
 }
 
 variable "install_disk" {
@@ -137,27 +129,29 @@ variable "install_disk" {
   description = "Disk device to which the install profiles should install Container Linux (e.g. /dev/sda)"
 }
 
-variable "container_linux_oem" {
-  type = string
-  default = ""
-  description = "DEPRECATED: Specify an OEM image id to use as base for the installation (e.g. ami, vmware_raw, xen) or leave blank for the default image"
-}
-
 variable "kernel_args" {
-  description = "Additional kernel arguments to provide at PXE boot."
   type = list(string)
+  description = "Additional kernel arguments to provide at PXE boot."
   default = []
 }
 
 variable "enable_reporting" {
-  type = string
+  type = bool
   description = "Enable usage or analytics reporting to upstreams (Calico)"
-  default = "false"
+  default = false
 }
 
 variable "enable_aggregation" {
+  type = bool
   description = "Enable the Kubernetes Aggregation Layer (defaults to false)"
+  default = false
+}
+
+# unofficial, undocumented, unsupported
+
+variable "cluster_domain_suffix" {
   type = string
-  default = "false"
+  description = "Queries for domains with the suffix will be answered by coredns. Default is cluster.local (e.g. foo.default.svc.cluster.local) "
+  default = "cluster.local"
 }
 
diff --git a/bare-metal/fedora-coreos/kubernetes/profiles.tf b/bare-metal/fedora-coreos/kubernetes/profiles.tf
index 80771c9b7..018d69b2b 100644
--- a/bare-metal/fedora-coreos/kubernetes/profiles.tf
+++ b/bare-metal/fedora-coreos/kubernetes/profiles.tf
@@ -21,9 +21,9 @@ locals {
     "coreos.inst.install_dev=${var.install_disk}"
   ]
 
-  kernel = var.cached_install == "true" ? local.cached_kernel : local.remote_kernel
-  initrd = var.cached_install == "true" ? local.cached_initrd : local.remote_initrd
-  args   = var.cached_install == "true" ? local.cached_args : local.remote_args
+  kernel = var.cached_install ? local.cached_kernel : local.remote_kernel
+  initrd = var.cached_install ? local.cached_initrd : local.remote_initrd
+  args   = var.cached_install ? local.cached_args : local.remote_args
 }
 
 
diff --git a/bare-metal/fedora-coreos/kubernetes/variables.tf b/bare-metal/fedora-coreos/kubernetes/variables.tf
index 30411c9ed..84f821f60 100644
--- a/bare-metal/fedora-coreos/kubernetes/variables.tf
+++ b/bare-metal/fedora-coreos/kubernetes/variables.tf
@@ -63,8 +63,8 @@ variable "snippets" {
 # configuration
 
 variable "k8s_domain_name" {
-  description = "Controller DNS name which resolves to a controller instance. Workers and kubeconfig's will communicate with this endpoint (e.g. cluster.example.com)"
   type        = string
+  description = "Controller DNS name which resolves to a controller instance. Workers and kubeconfig's will communicate with this endpoint (e.g. cluster.example.com)"
 }
 
 variable "ssh_authorized_key" {
@@ -73,80 +73,80 @@ variable "ssh_authorized_key" {
 }
 
 variable "asset_dir" {
-  description = "Path to a directory where generated assets should be placed (contains secrets)"
   type        = string
+  description = "Absolute path to a directory where generated assets should be placed (contains secrets)"
 }
 
 variable "networking" {
-  description = "Choice of networking provider (flannel or calico)"
   type        = string
+  description = "Choice of networking provider (flannel or calico)"
   default     = "calico"
 }
 
 variable "network_mtu" {
+  type        = number
   description = "CNI interface MTU (applies to calico only)"
-  type        = string
-  default     = "1480"
+  default     = 1480
 }
 
 variable "network_ip_autodetection_method" {
-  description = "Method to autodetect the host IPv4 address (applies to calico only)"
   type        = string
+  description = "Method to autodetect the host IPv4 address (applies to calico only)"
   default     = "first-found"
 }
 
 variable "pod_cidr" {
-  description = "CIDR IPv4 range to assign Kubernetes pods"
   type        = string
+  description = "CIDR IPv4 range to assign Kubernetes pods"
   default     = "10.2.0.0/16"
 }
 
 variable "service_cidr" {
+  type = string
   description = <<EOD
 CIDR IPv4 range to assign Kubernetes services.
 The 1st IP will be reserved for kube_apiserver, the 10th IP will be reserved for coredns.
 EOD
-
-
-  type = string
   default = "10.3.0.0/16"
 }
 
 # optional
 
-variable "cluster_domain_suffix" {
-  description = "Queries for domains with the suffix will be answered by coredns. Default is cluster.local (e.g. foo.default.svc.cluster.local) "
-  type = string
-  default = "cluster.local"
-}
-
 variable "cached_install" {
-  type = string
-  default = "false"
+  type = bool
   description = "Whether Fedora CoreOS should PXE boot and install from matchbox /assets cache. Note that the admin must have downloaded the os_version into matchbox assets."
+  default = false
 }
 
 variable "install_disk" {
   type = string
-  default = "sda"
   description = "Disk device to install Fedora CoreOS (e.g. sda)"
+  default = "sda"
 }
 
 variable "kernel_args" {
-  description = "Additional kernel arguments to provide at PXE boot."
   type = list(string)
+  description = "Additional kernel arguments to provide at PXE boot."
   default = []
 }
 
 variable "enable_reporting" {
-  type = string
+  type = bool
   description = "Enable usage or analytics reporting to upstreams (Calico)"
-  default = "false"
+  default = false
 }
 
 variable "enable_aggregation" {
+  type = bool
   description = "Enable the Kubernetes Aggregation Layer (defaults to false)"
+  default = false
+}
+
+# unofficial, undocumented, unsupported
+
+variable "cluster_domain_suffix" {
+  description = "Queries for domains with the suffix will be answered by coredns. Default is cluster.local (e.g. foo.default.svc.cluster.local) "
   type = string
-  default = "false"
+  default = "cluster.local"
 }
 
diff --git a/docs/cl/bare-metal.md b/docs/cl/bare-metal.md
index 2d5f0b0ac..fb739f1e4 100644
--- a/docs/cl/bare-metal.md
+++ b/docs/cl/bare-metal.md
@@ -144,9 +144,9 @@ Configure the Matchbox provider to use your Matchbox API endpoint and client cer
 provider "matchbox" {
   version     = "0.3.0"
   endpoint    = "matchbox.example.com:8081"
-  client_cert = "${file("~/.config/matchbox/client.crt")}"
-  client_key  = "${file("~/.config/matchbox/client.key")}"
-  ca          = "${file("~/.config/matchbox/ca.crt")}"
+  client_cert = file("~/.config/matchbox/client.crt")
+  client_key  = file("~/.config/matchbox/client.key")
+  ca          = file("~/.config/matchbox/ca.crt")
 }
 
 provider "ct" {
@@ -327,13 +327,13 @@ Check the [variables.tf](https://github.com/poseidon/typhoon/blob/master/bare-me
 
 | Name | Description | Example |
 |:-----|:------------|:--------|
-| cluster_name | Unique cluster name | mercury |
-| matchbox_http_endpoint | Matchbox HTTP read-only endpoint | http://matchbox.example.com:port |
+| cluster_name | Unique cluster name | "mercury" |
+| matchbox_http_endpoint | Matchbox HTTP read-only endpoint | "http://matchbox.example.com:port" |
 | os_channel | Channel for a Container Linux derivative | coreos-stable, coreos-beta, coreos-alpha, flatcar-stable, flatcar-beta, flatcar-alpha, flatcar-edge |
-| os_version | Version for a Container Linux derivative to PXE and install | 1632.3.0 |
+| os_version | Version for a Container Linux derivative to PXE and install | "1632.3.0" |
 | k8s_domain_name | FQDN resolving to the controller(s) nodes. Workers and kubectl will communicate with this endpoint | "myk8s.example.com" |
 | ssh_authorized_key | SSH public key for user 'core' | "ssh-rsa AAAAB3Nz..." |
-| asset_dir | Path to a directory where generated assets should be placed (contains secrets) | "/home/user/.secrets/clusters/mercury" |
+| asset_dir | Absolute path to a directory where generated assets should be placed (contains secrets) | "/home/user/.secrets/clusters/mercury" |
 | controller_names | Ordered list of controller short names | ["node1"] |
 | controller_macs | Ordered list of controller identifying MAC addresses | ["52:54:00:a1:9c:ae"] |
 | controller_domains | Ordered list of controller FQDNs | ["node1.example.com"] |
@@ -351,9 +351,8 @@ Check the [variables.tf](https://github.com/poseidon/typhoon/blob/master/bare-me
 | networking | Choice of networking provider | "calico" | "calico" or "flannel" |
 | network_mtu | CNI interface MTU (calico-only) | 1480 | - | 
 | clc_snippets | Map from machine names to lists of Container Linux Config snippets | {} | [example](/advanced/customization/#usage) |
-| network_ip_autodetection_method | Method to detect host IPv4 address (calico-only) | first-found | can-reach=10.0.0.1 |
+| network_ip_autodetection_method | Method to detect host IPv4 address (calico-only) | "first-found" | "can-reach=10.0.0.1" |
 | pod_cidr | CIDR IPv4 range to assign to Kubernetes pods | "10.2.0.0/16" | "10.22.0.0/16" |
 | service_cidr | CIDR IPv4 range to assign to Kubernetes services | "10.3.0.0/16" | "10.3.0.0/24" |
-| cluster_domain_suffix | FQDN suffix for Kubernetes services answered by coredns. | "cluster.local" | "k8s.example.com" |
-| kernel_args | Additional kernel args to provide at PXE boot | [] | "kvm-intel.nested=1" |
+| kernel_args | Additional kernel args to provide at PXE boot | [] | ["kvm-intel.nested=1"] |
 
diff --git a/docs/fedora-coreos/bare-metal.md b/docs/fedora-coreos/bare-metal.md
index 570d4135e..49be5032f 100644
--- a/docs/fedora-coreos/bare-metal.md
+++ b/docs/fedora-coreos/bare-metal.md
@@ -147,9 +147,9 @@ Configure the Matchbox provider to use your Matchbox API endpoint and client cer
 provider "matchbox" {
   version     = "0.3.0"
   endpoint    = "matchbox.example.com:8081"
-  client_cert = "${file("~/.config/matchbox/client.crt")}"
-  client_key  = "${file("~/.config/matchbox/client.key")}"
-  ca          = "${file("~/.config/matchbox/ca.crt")}"
+  client_cert = file("~/.config/matchbox/client.crt")
+  client_key  = file("~/.config/matchbox/client.key")
+  ca          = file("~/.config/matchbox/ca.crt")
 }
 
 provider "ct" {
@@ -163,14 +163,14 @@ Define a Kubernetes cluster using the module `bare-metal/fedora-coreos/kubernete
 
 ```tf
 module "bare-metal-mercury" {
-  source = "git::https://github.com/poseidon/typhoon//bare-metal/fedora-coreos/kubernetes?ref=DEVELOPMENT_SHA"
+  source = "git::https://github.com/poseidon/typhoon//bare-metal/fedora-coreos/kubernetes?ref=v1.16.1"
   
   # bare-metal
   cluster_name            = "mercury"
   matchbox_http_endpoint  = "http://matchbox.example.com"
   os_stream               = "testing"
   os_version              = "30.20190801.0"
-  cached_install          = "true"
+  cached_install          = true
 
   # configuration
   k8s_domain_name    = "node1.example.com"
@@ -318,10 +318,10 @@ Check the [variables.tf](https://github.com/poseidon/typhoon/blob/master/bare-me
 
 | Name | Description | Example |
 |:-----|:------------|:--------|
-| cluster_name | Unique cluster name | mercury |
-| matchbox_http_endpoint | Matchbox HTTP read-only endpoint | http://matchbox.example.com:port |
-| os_stream | Fedora CoreOS release stream | testing |
-| os_version | Fedora CoreOS version to PXE and install | 30.20190716.1 |
+| cluster_name | Unique cluster name | "mercury" |
+| matchbox_http_endpoint | Matchbox HTTP read-only endpoint | "http://matchbox.example.com:port" |
+| os_stream | Fedora CoreOS release stream | "testing" |
+| os_version | Fedora CoreOS version to PXE and install | "30.20190716.1" |
 | k8s_domain_name | FQDN resolving to the controller(s) nodes. Workers and kubectl will communicate with this endpoint | "myk8s.example.com" |
 | ssh_authorized_key | SSH public key for user 'core' | "ssh-rsa AAAAB3Nz..." |
 | asset_dir | Absolute path to a directory where generated assets should be placed (contains secrets) | "/home/user/.secrets/clusters/mercury" |
@@ -341,9 +341,8 @@ Check the [variables.tf](https://github.com/poseidon/typhoon/blob/master/bare-me
 | networking | Choice of networking provider | "calico" | "calico" or "flannel" |
 | network_mtu | CNI interface MTU (calico-only) | 1480 | - | 
 | snippets | Map from machine names to lists of Fedora CoreOS Config snippets | {} | UNSUPPORTED |
-| network_ip_autodetection_method | Method to detect host IPv4 address (calico-only) | first-found | can-reach=10.0.0.1 |
+| network_ip_autodetection_method | Method to detect host IPv4 address (calico-only) | "first-found" | "can-reach=10.0.0.1" |
 | pod_cidr | CIDR IPv4 range to assign to Kubernetes pods | "10.2.0.0/16" | "10.22.0.0/16" |
 | service_cidr | CIDR IPv4 range to assign to Kubernetes services | "10.3.0.0/16" | "10.3.0.0/24" |
-| cluster_domain_suffix | FQDN suffix for Kubernetes services answered by coredns. | "cluster.local" | "k8s.example.com" |
-| kernel_args | Additional kernel args to provide at PXE boot | [] | "kvm-intel.nested=1" |
+| kernel_args | Additional kernel args to provide at PXE boot | [] | ["kvm-intel.nested=1"] |