From 01831f1ab0da75db1368e5705eff73d3da88fdf5 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Thu, 14 Sep 2023 14:29:02 +0100 Subject: [PATCH 1/6] fix: Don't require `var.zone` for `read_replica` instances Currently, a value is required for either `var.read_replicas[replica]["zone"]` or `var.zone` as a fallback. This commit tweaks that behaviour to use _either_ `var.zone` or a random available zone as the fallback. Backwards compatibility should be retained, as `local.zone` already prefers the value of `var.zone` over a random available zone. Also worth noting that `local.zone` is already used in the `location_preference {}` block. --- modules/mysql/read_replica.tf | 4 ++-- modules/postgresql/read_replica.tf | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/mysql/read_replica.tf b/modules/mysql/read_replica.tf index d0885cd2..f7866fed 100644 --- a/modules/mysql/read_replica.tf +++ b/modules/mysql/read_replica.tf @@ -34,10 +34,10 @@ resource "google_sql_database_instance" "replicas" { project = var.project_id name = each.value.name_override == null || each.value.name_override == "" ? "${local.master_instance_name}-replica${var.read_replica_name_suffix}${each.value.name}" : each.value.name_override database_version = var.replica_database_version != "" ? var.replica_database_version : var.database_version - region = join("-", slice(split("-", lookup(each.value, "zone", var.zone)), 0, 2)) + region = join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2)) master_instance_name = google_sql_database_instance.default.name deletion_protection = var.read_replica_deletion_protection - encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", var.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name + encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name replica_configuration { failover_target = false diff --git a/modules/postgresql/read_replica.tf b/modules/postgresql/read_replica.tf index 653c8569..e3910371 100644 --- a/modules/postgresql/read_replica.tf +++ b/modules/postgresql/read_replica.tf @@ -34,10 +34,10 @@ resource "google_sql_database_instance" "replicas" { project = var.project_id name = each.value.name_override == null || each.value.name_override == "" ? "${local.master_instance_name}-replica${var.read_replica_name_suffix}${each.value.name}" : each.value.name_override database_version = var.database_version - region = join("-", slice(split("-", lookup(each.value, "zone", var.zone)), 0, 2)) + region = join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2)) master_instance_name = google_sql_database_instance.default.name deletion_protection = var.read_replica_deletion_protection - encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", var.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name + encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name replica_configuration { failover_target = false From f5007c6ee263bfe9a9d122d488c0be9250575b1d Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 15 Sep 2023 11:09:34 +0100 Subject: [PATCH 2/6] Switch from `lookup()` to `coalesce()` function. The `lookup()` func was causing errors such as `Invalid value for "str" parameter: argument must not be null.` on the `region` and `encryption_key_name` fields. I believe this is related to https://github.com/hashicorp/terraform/issues/32157. By switching to `coalesce()`, the first non-null value will be returned, which I believe is the intended behaviour with the previous `lookup()` usage. --- modules/mysql/read_replica.tf | 22 +++++++++++----------- modules/postgresql/read_replica.tf | 22 +++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/modules/mysql/read_replica.tf b/modules/mysql/read_replica.tf index f7866fed..545010e8 100644 --- a/modules/mysql/read_replica.tf +++ b/modules/mysql/read_replica.tf @@ -34,20 +34,20 @@ resource "google_sql_database_instance" "replicas" { project = var.project_id name = each.value.name_override == null || each.value.name_override == "" ? "${local.master_instance_name}-replica${var.read_replica_name_suffix}${each.value.name}" : each.value.name_override database_version = var.replica_database_version != "" ? var.replica_database_version : var.database_version - region = join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2)) + region = join("-", slice(split("-", coalesce(each.value["zone"], local.zone)), 0, 2)) master_instance_name = google_sql_database_instance.default.name deletion_protection = var.read_replica_deletion_protection - encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name + encryption_key_name = (join("-", slice(split("-", coalesce(each.value["zone"], local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name replica_configuration { failover_target = false } settings { - tier = lookup(each.value, "tier", var.tier) - edition = lookup(each.value, "edition", var.edition) + tier = coalesce(each.value["tier"], var.tier) + edition = coalesce(each.value["edition"], var.edition) activation_policy = "ALWAYS" - availability_type = lookup(each.value, "availability_type", var.availability_type) + availability_type = coalesce(each.value["availability_type"], var.availability_type) deletion_protection_enabled = var.read_replica_deletion_protection_enabled dynamic "backup_configuration" { @@ -97,12 +97,12 @@ resource "google_sql_database_instance" "replicas" { } } - disk_autoresize = lookup(each.value, "disk_autoresize", var.disk_autoresize) - disk_autoresize_limit = lookup(each.value, "disk_autoresize_limit", var.disk_autoresize_limit) - disk_size = lookup(each.value, "disk_size", var.disk_size) - disk_type = lookup(each.value, "disk_type", var.disk_type) + disk_autoresize = coalesce(each.value["disk_autoresize"], var.disk_autoresize) + disk_autoresize_limit = coalesce(each.value["disk_autoresize_limit"], var.disk_autoresize_limit) + disk_size = coalesce(each.value["disk_size"], var.disk_size) + disk_type = coalesce(each.value["disk_type"], var.disk_type) pricing_plan = "PER_USE" - user_labels = lookup(each.value, "user_labels", var.user_labels) + user_labels = coalesce(each.value["user_labels"], var.user_labels) dynamic "database_flags" { for_each = lookup(each.value, "database_flags", []) @@ -113,7 +113,7 @@ resource "google_sql_database_instance" "replicas" { } location_preference { - zone = lookup(each.value, "zone", local.zone) + zone = coalesce(each.value["zone"], local.zone) } } diff --git a/modules/postgresql/read_replica.tf b/modules/postgresql/read_replica.tf index e3910371..6a24013e 100644 --- a/modules/postgresql/read_replica.tf +++ b/modules/postgresql/read_replica.tf @@ -34,20 +34,20 @@ resource "google_sql_database_instance" "replicas" { project = var.project_id name = each.value.name_override == null || each.value.name_override == "" ? "${local.master_instance_name}-replica${var.read_replica_name_suffix}${each.value.name}" : each.value.name_override database_version = var.database_version - region = join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2)) + region = join("-", slice(split("-", coalesce(each.value["zone"], local.zone)), 0, 2)) master_instance_name = google_sql_database_instance.default.name deletion_protection = var.read_replica_deletion_protection - encryption_key_name = (join("-", slice(split("-", lookup(each.value, "zone", local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name + encryption_key_name = (join("-", slice(split("-", coalesce(each.value["zone"], local.zone)), 0, 2))) == var.region ? null : each.value.encryption_key_name replica_configuration { failover_target = false } settings { - tier = lookup(each.value, "tier", var.tier) - edition = lookup(each.value, "edition", var.edition) + tier = coalesce(each.value["tier"], var.tier) + edition = coalesce(each.value["edition"], var.edition) activation_policy = "ALWAYS" - availability_type = lookup(each.value, "availability_type", var.availability_type) + availability_type = coalesce(each.value["availability_type"], var.availability_type) deletion_protection_enabled = var.read_replica_deletion_protection_enabled dynamic "ip_configuration" { @@ -88,12 +88,12 @@ resource "google_sql_database_instance" "replicas" { } } - disk_autoresize = lookup(each.value, "disk_autoresize", var.disk_autoresize) - disk_autoresize_limit = lookup(each.value, "disk_autoresize_limit", var.disk_autoresize_limit) - disk_size = lookup(each.value, "disk_size", var.disk_size) - disk_type = lookup(each.value, "disk_type", var.disk_type) + disk_autoresize = coalesce(each.value["disk_autoresize"], var.disk_autoresize) + disk_autoresize_limit = coalesce(each.value["disk_autoresize_limit"], var.disk_autoresize_limit) + disk_size = coalesce(each.value["disk_size"], var.disk_size) + disk_type = coalesce(each.value["disk_type"], var.disk_type) pricing_plan = "PER_USE" - user_labels = lookup(each.value, "user_labels", var.user_labels) + user_labels = coalesce(each.value["user_labels"], var.user_labels) dynamic "database_flags" { for_each = lookup(each.value, "database_flags", []) @@ -104,7 +104,7 @@ resource "google_sql_database_instance" "replicas" { } location_preference { - zone = lookup(each.value, "zone", local.zone) + zone = coalesce(each.value["zone"], local.zone) } } From b431ee91c657d0547860ba46cf2dcb406d458257 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Fri, 15 Sep 2023 11:13:06 +0100 Subject: [PATCH 3/6] Update `edition` variable to default to `ENTERPRISE`. This mirrors the API behaviour, and fixes an issue with the `coalesce()` function not finding a value for `edition`. --- modules/mysql/variables.tf | 2 +- modules/postgresql/variables.tf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/mysql/variables.tf b/modules/mysql/variables.tf index 9e1e6727..c5f3ddc9 100644 --- a/modules/mysql/variables.tf +++ b/modules/mysql/variables.tf @@ -59,7 +59,7 @@ variable "tier" { variable "edition" { description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS." type = string - default = null + default = "ENTERPRISE" } variable "zone" { diff --git a/modules/postgresql/variables.tf b/modules/postgresql/variables.tf index 5adc094f..ed0cad6e 100644 --- a/modules/postgresql/variables.tf +++ b/modules/postgresql/variables.tf @@ -57,7 +57,7 @@ variable "tier" { variable "edition" { description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS." type = string - default = null + default = "ENTERPRISE" } variable "zone" { From 5e5e922f547e213544e3d1895f84c11dae3d0453 Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Mon, 2 Oct 2023 11:42:02 +0100 Subject: [PATCH 4/6] Set the "default" for `edition` as a local This avoids a constant flapping reported by Terraform if the variable default is `ENTERPRISE`. --- modules/mysql/read_replica.tf | 4 +++- modules/mysql/variables.tf | 4 ++-- modules/postgresql/read_replica.tf | 4 +++- modules/postgresql/variables.tf | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/modules/mysql/read_replica.tf b/modules/mysql/read_replica.tf index 545010e8..6fd9ccc0 100644 --- a/modules/mysql/read_replica.tf +++ b/modules/mysql/read_replica.tf @@ -18,6 +18,8 @@ locals { replicas = { for x in var.read_replicas : "${var.name}-replica${var.read_replica_name_suffix}${x.name}" => x } + // Edition should default to Enterprise + edition = var.edition != null ? var.edition : "ENTERPRISE" // Zone for replica instances zone = var.zone == null ? data.google_compute_zones.available[0].names[0] : var.zone } @@ -45,7 +47,7 @@ resource "google_sql_database_instance" "replicas" { settings { tier = coalesce(each.value["tier"], var.tier) - edition = coalesce(each.value["edition"], var.edition) + edition = coalesce(each.value["edition"], local.edition) activation_policy = "ALWAYS" availability_type = coalesce(each.value["availability_type"], var.availability_type) deletion_protection_enabled = var.read_replica_deletion_protection_enabled diff --git a/modules/mysql/variables.tf b/modules/mysql/variables.tf index c5f3ddc9..302aa873 100644 --- a/modules/mysql/variables.tf +++ b/modules/mysql/variables.tf @@ -57,9 +57,9 @@ variable "tier" { } variable "edition" { - description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS." + description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS. Defaults to ENTERPRISE." type = string - default = "ENTERPRISE" + default = null } variable "zone" { diff --git a/modules/postgresql/read_replica.tf b/modules/postgresql/read_replica.tf index 6a24013e..7559c3f9 100644 --- a/modules/postgresql/read_replica.tf +++ b/modules/postgresql/read_replica.tf @@ -18,6 +18,8 @@ locals { replicas = { for x in var.read_replicas : "${var.name}-replica${var.read_replica_name_suffix}${x.name}" => x } + // Edition should default to Enterprise + edition = var.edition != null ? var.edition : "ENTERPRISE" // Zone for replica instances zone = var.zone == null ? data.google_compute_zones.available[0].names[0] : var.zone } @@ -45,7 +47,7 @@ resource "google_sql_database_instance" "replicas" { settings { tier = coalesce(each.value["tier"], var.tier) - edition = coalesce(each.value["edition"], var.edition) + edition = coalesce(each.value["edition"], local.edition) activation_policy = "ALWAYS" availability_type = coalesce(each.value["availability_type"], var.availability_type) deletion_protection_enabled = var.read_replica_deletion_protection_enabled diff --git a/modules/postgresql/variables.tf b/modules/postgresql/variables.tf index ed0cad6e..c61b7289 100644 --- a/modules/postgresql/variables.tf +++ b/modules/postgresql/variables.tf @@ -55,9 +55,9 @@ variable "tier" { } variable "edition" { - description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS." + description = "The edition of the instance, can be ENTERPRISE or ENTERPRISE_PLUS. Defaults to ENTERPRISE." type = string - default = "ENTERPRISE" + default = null } variable "zone" { From ff3ee1019d2c306228e1ce3c2ba2884de612b1fa Mon Sep 17 00:00:00 2001 From: Gavin Williams Date: Mon, 2 Oct 2023 11:42:48 +0100 Subject: [PATCH 5/6] Remove `count` from `google_compute_zones.available` to prevent error With a count that depends on `var.zone`, I was getting the following error: ``` The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. ``` Update references to use resource without the count index. --- modules/mysql/read_replica.tf | 3 +-- modules/postgresql/read_replica.tf | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/mysql/read_replica.tf b/modules/mysql/read_replica.tf index 6fd9ccc0..fe4f5f56 100644 --- a/modules/mysql/read_replica.tf +++ b/modules/mysql/read_replica.tf @@ -21,11 +21,10 @@ locals { // Edition should default to Enterprise edition = var.edition != null ? var.edition : "ENTERPRISE" // Zone for replica instances - zone = var.zone == null ? data.google_compute_zones.available[0].names[0] : var.zone + zone = var.zone == null ? data.google_compute_zones.available.names[0] : var.zone } data "google_compute_zones" "available" { - count = var.zone == null ? 0 : 1 project = var.project_id region = var.region } diff --git a/modules/postgresql/read_replica.tf b/modules/postgresql/read_replica.tf index 7559c3f9..c37cc28d 100644 --- a/modules/postgresql/read_replica.tf +++ b/modules/postgresql/read_replica.tf @@ -21,11 +21,10 @@ locals { // Edition should default to Enterprise edition = var.edition != null ? var.edition : "ENTERPRISE" // Zone for replica instances - zone = var.zone == null ? data.google_compute_zones.available[0].names[0] : var.zone + zone = var.zone == null ? data.google_compute_zones.available.names[0] : var.zone } data "google_compute_zones" "available" { - count = var.zone == null ? 0 : 1 project = var.project_id region = var.region } From 9204771b8d0acdce78b695fc41ea0e023a644c2f Mon Sep 17 00:00:00 2001 From: Awais Malik Date: Mon, 5 Feb 2024 15:31:43 -0800 Subject: [PATCH 6/6] generate docs --- modules/mysql/README.md | 2 +- modules/postgresql/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/mysql/README.md b/modules/mysql/README.md index fdb98ae8..ee3ec296 100644 --- a/modules/mysql/README.md +++ b/modules/mysql/README.md @@ -28,7 +28,7 @@ Note: CloudSQL provides [disk autoresize](https://cloud.google.com/sql/docs/mysq | disk\_autoresize\_limit | The maximum size to which storage can be auto increased. | `number` | `0` | no | | disk\_size | The disk size for the master instance | `number` | `10` | no | | disk\_type | The disk type for the master instance. | `string` | `"PD_SSD"` | no | -| edition | The edition of the instance, can be ENTERPRISE or ENTERPRISE\_PLUS. | `string` | `null` | no | +| edition | The edition of the instance, can be ENTERPRISE or ENTERPRISE\_PLUS. Defaults to ENTERPRISE. | `string` | `null` | no | | enable\_default\_db | Enable or disable the creation of the default database | `bool` | `true` | no | | enable\_default\_user | Enable or disable the creation of the default user | `bool` | `true` | no | | enable\_random\_password\_special | Enable special characters in generated random passwords. | `bool` | `false` | no | diff --git a/modules/postgresql/README.md b/modules/postgresql/README.md index 55c3055c..46380005 100644 --- a/modules/postgresql/README.md +++ b/modules/postgresql/README.md @@ -29,7 +29,7 @@ Note: CloudSQL provides [disk autoresize](https://cloud.google.com/sql/docs/mysq | disk\_autoresize\_limit | The maximum size to which storage can be auto increased. | `number` | `0` | no | | disk\_size | The disk size for the master instance. | `number` | `10` | no | | disk\_type | The disk type for the master instance. | `string` | `"PD_SSD"` | no | -| edition | The edition of the instance, can be ENTERPRISE or ENTERPRISE\_PLUS. | `string` | `null` | no | +| edition | The edition of the instance, can be ENTERPRISE or ENTERPRISE\_PLUS. Defaults to ENTERPRISE. | `string` | `null` | no | | enable\_default\_db | Enable or disable the creation of the default database | `bool` | `true` | no | | enable\_default\_user | Enable or disable the creation of the default user | `bool` | `true` | no | | enable\_random\_password\_special | Enable special characters in generated random passwords. | `bool` | `false` | no |