Make dns module resilient to dynamic values (#317)

* refactor module and fix tests * account for wildcard records * account for empty recordset names * align tests * align networking end to end examples * fix behaviour with wildcard and empty names * Update main.tf * fix dumb online edit :)
2021-10-04 18:59:14 +02:00 · 2021-10-04 18:59:14 +02:00 · 5001eb49a4
parent b481d9baff
commit 5001eb49a4
15 changed files with 88 additions and 88 deletions
--- a/modules/dns/README.md
+++ b/modules/dns/README.md
@ -16,9 +16,9 @@ module "private-dns" {
  name            = "test-example"
  domain          = "test.example."
  client_networks = [var.vpc.self_link]
-  recordsets = [
-    { name = "localhost", type = "A", ttl = 300, records = ["127.0.0.1"] }
-  ]
+  recordsets = {
+    "A localhost" = { type = "A", ttl = 300, records = ["127.0.0.1"] }
+  }
 }
 # tftest:modules=1:resources=2
 ```
@ -68,7 +68,7 @@ module "private-dns" {
 | *dnssec_config* | DNSSEC configuration: kind, non_existence, state. | <code title="">any</code> |  | <code title="">{}</code> |
 | *forwarders* | Map of {IPV4_ADDRESS => FORWARDING_PATH} for 'forwarding' zone types. Path can be 'default', 'private', or null for provider default. | <code title="map&#40;string&#41;">map(string)</code> |  | <code title="">{}</code> |
 | *peer_network* | Peering network self link, only valid for 'peering' zone types. | <code title="">string</code> |  | <code title="">null</code> |
-| *recordsets* | List of DNS record objects to manage. | <code title="list&#40;object&#40;&#123;&#10;name    &#61; string&#10;type &#61; string&#10;ttl     &#61; number&#10;records &#61; list&#40;string&#41;&#10;&#125;&#41;&#41;">list(object({...}))</code> |  | <code title="">[]</code> |
+| *recordsets* | None | <code title="map&#40;object&#40;&#123;&#10;ttl     &#61; number&#10;records &#61; list&#40;string&#41;&#10;&#125;&#41;&#41;">map(object({...}))</code> |  | <code title="&#123;&#125;&#10;validation &#123;&#10;condition &#61; alltrue&#40;&#91;&#10;for k, v in var.recordsets &#61;&#61; null &#63; &#123;&#125; : var.recordsets :&#10;length&#40;split&#40;&#34; &#34;, k&#41;&#41; &#61;&#61; 2&#10;&#93;&#41;&#10;error_message &#61; &#34;Recordsets must have keys in the format &#92;&#34;type name&#92;&#34;.&#34;&#10;&#125;">...</code> |
 | *service_directory_namespace* | Service directory namespace id (URL), only valid for 'service-directory' zone types. | <code title="">string</code> |  | <code title="">null</code> |
 | *type* | Type of zone to create, valid values are 'public', 'private', 'forwarding', 'peering', 'service-directory'. | <code title="">string</code> |  | <code title="private&#10;validation &#123;&#10;condition     &#61; contains&#40;&#91;&#34;public&#34;, &#34;private&#34;, &#34;forwarding&#34;, &#34;peering&#34;, &#34;service-directory&#34;&#93;, var.type&#41;&#10;error_message &#61; &#34;Zone must be one of &#39;public&#39;, &#39;private&#39;, &#39;forwarding&#39;, &#39;peering&#39;, &#39;service-directory&#39;.&#34;&#10;&#125;">...</code> |
 | *zone_create* | Create zone. When set to false, uses a data source to reference existing zone. | <code title="">bool</code> |  | <code title="">true</code> |
--- a/modules/dns/main.tf
+++ b/modules/dns/main.tf
@ -15,9 +15,10 @@
 */

 locals {
-  recordsets = var.recordsets == null ? {} : {
-    for record in var.recordsets :
-    join("/", [record.name, record.type]) => record
+  _recordsets = var.recordsets == null ? {} : var.recordsets
+  recordsets = {
+    for key, attrs in local._recordsets :
+    key => merge(attrs, zipmap(["type", "name"], split(" ", key)))
  }
  zone = (
    var.zone_create
@ -152,10 +153,18 @@ resource "google_dns_record_set" "cloud-static-records" {
  )
  project      = var.project_id
  managed_zone = var.name
-  name         = each.value.name != "" ? "${each.value.name}.${var.domain}" : var.domain
-  type         = each.value.type
-  ttl          = each.value.ttl
-  rrdatas      = each.value.records
+  name = (
+    each.value.name == ""
+    ? var.domain
+    : (
+      substr(each.value.name, -1, 1) == "."
+      ? each.value.name
+      : "${each.value.name}.${var.domain}"
+    )
+  )
+  type    = each.value.type
+  ttl     = each.value.ttl
+  rrdatas = each.value.records
  depends_on = [
    google_dns_managed_zone.non-public, google_dns_managed_zone.public
  ]
--- a/modules/dns/variables.tf
+++ b/modules/dns/variables.tf
@ -76,14 +76,19 @@ variable "project_id" {
 }

 variable "recordsets" {
-  type = list(object({
-    name    = string
-    type    = string
+  description = "Map of DNS recordsets in \"type name\" => {ttl, [records]} format."
+  type = map(object({
    ttl     = number
    records = list(string)
  }))
-  description = "List of DNS record objects to manage."
-  default     = []
+  default = {}
+  validation {
+    condition = alltrue([
+      for k, v in var.recordsets == null ? {} : var.recordsets :
+      length(split(" ", k)) == 2
+    ])
+    error_message = "Recordsets must have keys in the format \"type name\"."
+  }
 }

 variable "service_directory_namespace" {
--- a/networking/decentralized-firewall/main.tf
+++ b/networking/decentralized-firewall/main.tf
@ -87,9 +87,9 @@ module "dns-api-prod" {
  name            = "googleapis"
  domain          = "googleapis.com."
  client_networks = [module.vpc-prod.self_link]
-  recordsets = [
-    { name = "*", type = "CNAME", ttl = 300, records = ["private.googleapis.com."] },
-  ]
+  recordsets = {
+    "CNAME *" = { ttl = 300, records = ["private.googleapis.com."] }
+  }
 }

 module "dns-api-dev" {
@ -99,9 +99,9 @@ module "dns-api-dev" {
  name            = "googleapis"
  domain          = "googleapis.com."
  client_networks = [module.vpc-dev.self_link]
-  recordsets = [
-    { name = "*", type = "CNAME", ttl = 300, records = ["private.googleapis.com."] },
-  ]
+  recordsets = {
+    "CNAME *" = { ttl = 300, records = ["private.googleapis.com."] }
+  }
 }

 ###############################################################################
--- a/networking/filtering-proxy/main.tf
+++ b/networking/filtering-proxy/main.tf
@ -118,10 +118,10 @@ module "private-dns" {
  name            = "internal"
  domain          = "internal."
  client_networks = [module.vpc.self_link]
-  recordsets = [
-    { name = "squid", type = "A", ttl = 60, records = [local.squid_address] },
-    { name = "proxy", type = "CNAME", ttl = 3600, records = ["squid.internal."] }
-  ]
+  recordsets = {
+    "A squid"     = { ttl = 60, records = [local.squid_address] }
+    "CNAME proxy" = { ttl = 3600, records = ["squid.internal."] }
+  }
 }

 ###############################################################################
--- a/networking/hub-and-spoke-vpn/main.tf
+++ b/networking/hub-and-spoke-vpn/main.tf
@ -13,10 +13,6 @@
 # limitations under the License.

 locals {
-  vm-instances = [
-    module.vm-spoke-1.instance,
-    module.vm-spoke-2.instance
-  ]
  vm-startup-script = join("\n", [
    "#! /bin/bash",
    "apt-get update && apt-get install -y dnsutils"
@ -287,14 +283,11 @@ module "dns-host" {
  name            = "example"
  domain          = "example.com."
  client_networks = [module.vpc-hub.self_link]
-  # setting instance IPs at first apply fails due to dynamic values
-  recordsets = [
-    { name = "localhost", type = "A", ttl = 300, records = ["127.0.0.1"] }
-    # for instance in local.vm-instances : {
-    #   name    = instance.name, type = "A", ttl = 300,
-    #   records = [instance.network_interface.0.network_ip]
-    # }
-  ]
+  recordsets = {
+    "A localhost"    = { ttl = 300, records = ["127.0.0.1"] }
+    "A spoke-1-test" = { ttl = 300, records = [module.vm-spoke-1.internal_ip] }
+    "A spoke-2-test" = { ttl = 300, records = [module.vm-spoke-2.internal_ip] }
+  }
 }

 module "dns-spoke-1" {
--- a/networking/hub-and-spoke-vpn/outputs.tf
+++ b/networking/hub-and-spoke-vpn/outputs.tf
@ -15,7 +15,7 @@
 output "vms" {
  description = "GCE VMs."
  value = {
-    for instance in local.vm-instances :
+    for instance in [module.vm-spoke-1.instance, module.vm-spoke-2.instance] :
    instance.name => instance.network_interface.0.network_ip
  }
 }
--- a/networking/onprem-google-access-dns/main.tf
+++ b/networking/onprem-google-access-dns/main.tf
@ -170,20 +170,11 @@ module "dns-gcp" {
  name            = "gcp-example"
  domain          = "gcp.example.org."
  client_networks = [module.vpc.self_link]
-  recordsets = concat(
-    [{ name = "localhost", type = "A", ttl = 300, records = ["127.0.0.1"] }],
-    # setting addresses during first apply triggers a dynamic value error
-    # [
-    #   {
-    #     name    = module.vm-test1.instance.name, type = "A", ttl = 300,
-    #     records = [module.vm-test1.internal_ip]
-    #   },
-    #   {
-    #     name    = module.vm-test2.instance.name, type = "A", ttl = 300,
-    #     records = [module.vm-test2.internal_ip]
-    #   }
-    # ]
-  )
+  recordsets = {
+    "A localhost" = { ttl = 300, records = ["127.0.0.1"] }
+    "A test-1"    = { ttl = 300, records = [module.vm-test1.internal_ip] }
+    "A test-2"    = { ttl = 300, records = [module.vm-test2.internal_ip] }
+  }
 }

 module "dns-api" {
@ -193,11 +184,11 @@ module "dns-api" {
  name            = "googleapis"
  domain          = "googleapis.com."
  client_networks = [module.vpc.self_link]
-  recordsets = [
-    { name = "*", type = "CNAME", ttl = 300, records = ["private.googleapis.com."] },
-    { name = "private", type = "A", ttl = 300, records = local.vips.private },
-    { name = "restricted", type = "A", ttl = 300, records = local.vips.restricted },
-  ]
+  recordsets = {
+    "CNAME *"      = { ttl = 300, records = ["private.googleapis.com."] }
+    "A private"    = { ttl = 300, records = local.vips.private }
+    "A restricted" = { ttl = 300, records = local.vips.restricted }
+  }
 }

 module "dns-onprem" {
--- a/networking/private-cloud-function-from-onprem/main.tf
+++ b/networking/private-cloud-function-from-onprem/main.tf
@ -229,12 +229,9 @@ module "private-dns-onprem" {
  name            = var.name
  domain          = "${var.region}-${module.project.project_id}.cloudfunctions.net."
  client_networks = [module.vpc-onprem.self_link]
-  recordsets = [{
-    name    = "",
-    type    = "A",
-    ttl     = 300,
-    records = [module.addresses.psc_addresses[local.psc_name].address]
-  }]
+  recordsets = {
+    "A " = { ttl = 300, records = [module.addresses.psc_addresses[local.psc_name].address] }
+  }
 }

 ###############################################################################
--- a/networking/shared-vpc-gke/main.tf
+++ b/networking/shared-vpc-gke/main.tf
@ -156,10 +156,10 @@ module "host-dns" {
  name            = "example"
  domain          = "example.com."
  client_networks = [module.vpc-shared.self_link]
-  recordsets = [
-    { name = "localhost", type = "A", ttl = 300, records = ["127.0.0.1"] },
-    # { name = "bastion", type = "A", ttl = 300, records = [module.vm-bastion.internal_ip] },
-  ]
+  recordsets = {
+    "A localhost" = { ttl = 300, records = ["127.0.0.1"] }
+    "A bastion"   = { ttl = 300, records = [module.vm-bastion.internal_ip] }
+  }
 }

 ################################################################################
--- a/tests/modules/dns/fixture/variables.tf
+++ b/tests/modules/dns/fixture/variables.tf
@ -32,16 +32,16 @@ variable "peer_network" {
 }

 variable "recordsets" {
-  type = list(object({
-    name    = string
-    type    = string
+  type = map(object({
    ttl     = number
    records = list(string)
  }))
-  default = [
-    { name = "localhost", type = "A", ttl = 300, records = ["127.0.0.1"] },
-    { name = "local-host", type = "A", ttl = 300, records = ["127.0.0.2"] }
-  ]
+  default = {
+    "A localhost"                = { ttl = 300, records = ["127.0.0.1"] }
+    "A local-host.test.example." = { ttl = 300, records = ["127.0.0.2"] }
+    "CNAME *"                    = { ttl = 300, records = ["localhost.example.org."] }
+    "A "                         = { ttl = 300, records = ["127.0.0.3"] }
+  }
 }

 variable "type" {
--- a/tests/modules/dns/test_plan.py
+++ b/tests/modules/dns/test_plan.py
@ -21,9 +21,9 @@ FIXTURES_DIR = os.path.join(os.path.dirname(__file__), 'fixture')


 def test_private(plan_runner):
-  "Test private zone with two recordsets."
+  "Test private zone with three recordsets."
  _, resources = plan_runner(FIXTURES_DIR)
-  assert len(resources) == 3
+  assert len(resources) == 5
  assert set(r['type'] for r in resources) == set([
      'google_dns_record_set', 'google_dns_managed_zone'
  ])
@ -34,13 +34,22 @@ def test_private(plan_runner):
    assert len(r['values']['private_visibility_config']) == 1


+def test_private_recordsets(plan_runner):
+  "Test recordsets in private zone."
+  _, resources = plan_runner(FIXTURES_DIR)
+  recordsets = [r['values']
+                for r in resources if r['type'] == 'google_dns_record_set']
+  assert set(r['name'] for r in recordsets) == set([
+      'localhost.test.example.',
+      'local-host.test.example.',
+      '*.test.example.',
+      "test.example."
+  ])
+
+
 def test_private_no_networks(plan_runner):
  "Test private zone not exposed to any network."
  _, resources = plan_runner(FIXTURES_DIR, client_networks='[]')
-  assert len(resources) == 3
-  assert set(r['type'] for r in resources) == set([
-      'google_dns_record_set', 'google_dns_managed_zone'
-  ])
  for r in resources:
    if r['type'] != 'google_dns_managed_zone':
      continue
@ -83,10 +92,6 @@ def test_peering(plan_runner):
 def test_public(plan_runner):
  "Test public zone with two recordsets."
  _, resources = plan_runner(FIXTURES_DIR, type='public')
-  assert len(resources) == 3
-  assert set(r['type'] for r in resources) == set([
-      'google_dns_record_set', 'google_dns_managed_zone'
-  ])
  for r in resources:
    if r['type'] != 'google_dns_managed_zone':
      continue
--- a/tests/networking/hub_and_spoke_vpn/test_plan.py
+++ b/tests/networking/hub_and_spoke_vpn/test_plan.py
@ -24,4 +24,4 @@ def test_resources(e2e_plan_runner):
  "Test that plan works and the numbers of resources is as expected."
  modules, resources = e2e_plan_runner(FIXTURES_DIR)
  assert len(modules) == 17
-  assert len(resources) == 69
+  assert len(resources) == 71
--- a/tests/networking/onprem_google_access_dns/test_plan.py
+++ b/tests/networking/onprem_google_access_dns/test_plan.py
@ -24,4 +24,4 @@ def test_resources(e2e_plan_runner):
  "Test that plan works and the numbers of resources is as expected."
  modules, resources = e2e_plan_runner(FIXTURES_DIR)
  assert len(modules) == 14
-  assert len(resources) == 46
+  assert len(resources) == 48
--- a/tests/networking/shared_vpc_gke/test_plan.py
+++ b/tests/networking/shared_vpc_gke/test_plan.py
@ -24,4 +24,4 @@ def test_resources(e2e_plan_runner):
  "Test that plan works and the numbers of resources is as expected."
  modules, resources = e2e_plan_runner(FIXTURES_DIR)
  assert len(modules) == 10
-  assert len(resources) == 40
+  assert len(resources) == 41