From 5001eb49a4efb4fd810d18703d770d436598bbe3 Mon Sep 17 00:00:00 2001 From: Ludovico Magnocavallo Date: Mon, 4 Oct 2021 18:59:14 +0200 Subject: [PATCH] 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 :) --- modules/dns/README.md | 8 ++--- modules/dns/main.tf | 23 ++++++++++----- modules/dns/variables.tf | 15 ++++++---- networking/decentralized-firewall/main.tf | 12 ++++---- networking/filtering-proxy/main.tf | 8 ++--- networking/hub-and-spoke-vpn/main.tf | 17 ++++------- networking/hub-and-spoke-vpn/outputs.tf | 2 +- networking/onprem-google-access-dns/main.tf | 29 +++++++------------ .../main.tf | 9 ++---- networking/shared-vpc-gke/main.tf | 8 ++--- tests/modules/dns/fixture/variables.tf | 14 ++++----- tests/modules/dns/test_plan.py | 25 +++++++++------- .../networking/hub_and_spoke_vpn/test_plan.py | 2 +- .../onprem_google_access_dns/test_plan.py | 2 +- tests/networking/shared_vpc_gke/test_plan.py | 2 +- 15 files changed, 88 insertions(+), 88 deletions(-) diff --git a/modules/dns/README.md b/modules/dns/README.md index 5980fa29d..7fdfd296b 100644 --- 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. | any | | {} | | *forwarders* | Map of {IPV4_ADDRESS => FORWARDING_PATH} for 'forwarding' zone types. Path can be 'default', 'private', or null for provider default. | map(string) | | {} | | *peer_network* | Peering network self link, only valid for 'peering' zone types. | string | | null | -| *recordsets* | List of DNS record objects to manage. | list(object({...})) | | [] | +| *recordsets* | None | map(object({...})) | | ... | | *service_directory_namespace* | Service directory namespace id (URL), only valid for 'service-directory' zone types. | string | | null | | *type* | Type of zone to create, valid values are 'public', 'private', 'forwarding', 'peering', 'service-directory'. | string | | ... | | *zone_create* | Create zone. When set to false, uses a data source to reference existing zone. | bool | | true | diff --git a/modules/dns/main.tf b/modules/dns/main.tf index 738794c23..b6a3bfaad 100644 --- 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 ] diff --git a/modules/dns/variables.tf b/modules/dns/variables.tf index 010693b1a..b34487595 100644 --- 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" { diff --git a/networking/decentralized-firewall/main.tf b/networking/decentralized-firewall/main.tf index 2502d41f6..d8fa029e0 100644 --- 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."] } + } } ############################################################################### diff --git a/networking/filtering-proxy/main.tf b/networking/filtering-proxy/main.tf index 39f79a762..c01aa4d4a 100644 --- 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."] } + } } ############################################################################### diff --git a/networking/hub-and-spoke-vpn/main.tf b/networking/hub-and-spoke-vpn/main.tf index e1d61bc34..e1886e5e4 100644 --- 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" { diff --git a/networking/hub-and-spoke-vpn/outputs.tf b/networking/hub-and-spoke-vpn/outputs.tf index 9775595d5..2ccd2ca70 100644 --- 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 } } diff --git a/networking/onprem-google-access-dns/main.tf b/networking/onprem-google-access-dns/main.tf index caf665630..5782bda51 100644 --- 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" { diff --git a/networking/private-cloud-function-from-onprem/main.tf b/networking/private-cloud-function-from-onprem/main.tf index 2e2de0023..a51fa363a 100644 --- 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] } + } } ############################################################################### diff --git a/networking/shared-vpc-gke/main.tf b/networking/shared-vpc-gke/main.tf index fa11ebf4e..69879a2e9 100644 --- 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] } + } } ################################################################################ diff --git a/tests/modules/dns/fixture/variables.tf b/tests/modules/dns/fixture/variables.tf index 813817146..a08a04207 100644 --- 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" { diff --git a/tests/modules/dns/test_plan.py b/tests/modules/dns/test_plan.py index 091356974..682c2e1a6 100644 --- 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 diff --git a/tests/networking/hub_and_spoke_vpn/test_plan.py b/tests/networking/hub_and_spoke_vpn/test_plan.py index 5549e7cc1..d27f74180 100644 --- 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 diff --git a/tests/networking/onprem_google_access_dns/test_plan.py b/tests/networking/onprem_google_access_dns/test_plan.py index edcbd3b40..e1b1fb714 100644 --- 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 diff --git a/tests/networking/shared_vpc_gke/test_plan.py b/tests/networking/shared_vpc_gke/test_plan.py index e315dbd05..d367412ef 100644 --- 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