From e3a03a76fff248b08a04440dc4a29d874463ca4b Mon Sep 17 00:00:00 2001 From: Ludovico Magnocavallo Date: Wed, 3 Feb 2021 08:00:08 +0100 Subject: [PATCH] Add support for rePD and existing disk attachment to compute VM (#194) * initial untested support for repd in compute-vm * fix repd reference in instance disks * add support for snapshot source, change disk variable * fix attach existing disk to instance * fix examples, add documentation on disk sources * fix attaching disk for instance templates, add examples * fix data e2e examples * update vars table in README --- .../cmek-via-centralized-kms/main.tf | 14 +- .../gcs-to-bq-with-dataflow/main.tf | 14 +- modules/compute-vm/README.md | 98 ++++++++++-- modules/compute-vm/main.tf | 140 +++++++++++++----- modules/compute-vm/variables.tf | 37 +++-- 5 files changed, 226 insertions(+), 77 deletions(-) diff --git a/data-solutions/cmek-via-centralized-kms/main.tf b/data-solutions/cmek-via-centralized-kms/main.tf index 6f7a388cd..66cfdc677 100644 --- a/data-solutions/cmek-via-centralized-kms/main.tf +++ b/data-solutions/cmek-via-centralized-kms/main.tf @@ -111,15 +111,11 @@ module "kms_vm_example" { }] attached_disks = [ { - name = "attacheddisk" - size = 10 - image = null - options = { - auto_delete = true - mode = null - source = null - type = null - } + name = "attacheddisk" + size = 10 + source = null + source_type = null + options = null } ] instance_count = 1 diff --git a/data-solutions/gcs-to-bq-with-dataflow/main.tf b/data-solutions/gcs-to-bq-with-dataflow/main.tf index 1f85beaa0..6f7aec043 100644 --- a/data-solutions/gcs-to-bq-with-dataflow/main.tf +++ b/data-solutions/gcs-to-bq-with-dataflow/main.tf @@ -212,15 +212,11 @@ module "vm_example" { }] attached_disks = [ { - name = "attacheddisk" - size = 10 - image = null - options = { - auto_delete = true - mode = null - source = null - type = null - } + name = "attacheddisk" + size = 10 + source = null + source_type = null + options = null } ] instance_count = 2 diff --git a/modules/compute-vm/README.md b/modules/compute-vm/README.md index 48b069797..3bee1daa7 100644 --- a/modules/compute-vm/README.md +++ b/modules/compute-vm/README.md @@ -33,6 +33,80 @@ module "simple-vm-example" { ``` +### Disk sources + +Attached disks can be created and optionally initialized from a pre-existing source, or attached to VMs when pre-existing. The `source` and `source_type` attributes of the `attached_disks` variable allows several modes of operation: + +- `source_type = "image"` can be used with zonal disks in instances and templates, set `source` to the image name or link +- `source_type = "snapshot"` can be used with instances only, set `source` to the snapshot name or link +- `source_type = "attach"` can be used for both instances and templates to attach an existing disk, set source to the name (for zonal disks) or link (for regional disks) of the existing disk to attach; no disk will be created +- `source_type = null` can be used where an empty disk is needed, `source` becomes irrelevant and can be left null + +This is an example of attaching a pre-existing regional PD to a new instance: + +```hcl +module "simple-vm-example" { + source = "./modules/compute-vm" + project_id = var.project_id + region = var.region + name = "test" + network_interfaces = [{ + network = var.vpc.self_link + subnetwork = var.subnet.self_link + nat = false + addresses = null + alias_ips = null + }] + attached_disks = [{ + name = "repd-1" + size = null + source_type = "attach" + source = "regions/${var.region}/disks/repd-test-1" + options = { + auto_delete = false + mode = null + regional = true + type = null + } + }] + service_account_create = true +} +# tftest:modules=1:resources=2 +``` + +And the same example for an instance template (where not using the full self link of the disk triggers recreation of the template) + +```hcl +module "simple-vm-example" { + source = "./modules/compute-vm" + project_id = var.project_id + region = var.region + name = "test" + network_interfaces = [{ + network = var.vpc.self_link + subnetwork = var.subnet.self_link + nat = false + addresses = null + alias_ips = null + }] + attached_disks = [{ + name = "repd" + size = null + source_type = "attach" + source = "https://www.googleapis.com/compute/v1/projects/${var.project_id}/regions/${var.region}/disks/repd-test-1" + options = { + auto_delete = false + mode = null + regional = true + type = null + } + }] + service_account_create = true + use_instance_template = true +} +# tftest:modules=1:resources=2 +``` + ### Disk encryption with Cloud KMS This example shows how to control disk encryption via the the `encryption` variable, in this case the self link to a KMS CryptoKey that will be used to encrypt boot and attached disk. Managing the key with the `../kms` module is of course possible, but is not shown here. @@ -53,14 +127,10 @@ module "kms-vm-example" { attached_disks = [ { name = "attached-disk" - size = 10 - image = null - options = { - auto_delete = true - mode = null - source = null - type = null - } + size = 10 + source = null + source_type = null + options = null } ] service_account_create = true @@ -132,7 +202,13 @@ module "cos-test" { size = 10 } attached_disks = [ - { name = "disk-1", size = 10, image = null, options = null } + { + name = "disk-1" + size = 10 + source = null + source_type = null + options = null + } ] service_account = "vm-default@my-project.iam.gserviceaccount.com" use_instance_template = true @@ -185,8 +261,8 @@ module "instance-group" { | network_interfaces | Network interfaces configuration. Use self links for Shared VPC, set addresses and alias_ips to null if not needed. | list(object({...})) | ✓ | | | project_id | Project id. | string | ✓ | | | region | Compute region. | string | ✓ | | -| *attached_disk_defaults* | Defaults for attached disks options. | object({...}) | | ... | -| *attached_disks* | Additional disks, if options is null defaults will be used in its place. | list(object({...})) | | [] | +| *attached_disk_defaults* | Defaults for attached disks options. | object({...}) | | ... | +| *attached_disks* | Additional disks, if options is null defaults will be used in its place. Source type is one of 'image' (zonal disks in vms and template), 'snapshot' (vm), 'existing', and null. | list(object({...})) | | ... | | *boot_disk* | Boot disk properties. | object({...}) | | ... | | *can_ip_forward* | Enable IP forwarding. | bool | | false | | *confidential_compute* | Enable Confidential Compute for these instances. | bool | | false | diff --git a/modules/compute-vm/main.tf b/modules/compute-vm/main.tf index f4423691c..dddd1e67e 100644 --- a/modules/compute-vm/main.tf +++ b/modules/compute-vm/main.tf @@ -25,6 +25,14 @@ locals { for pair in setproduct(keys(local.names), keys(local.attached_disks)) : "${pair[0]}-${pair[1]}" => { disk_name = pair[1], name = pair[0] } } + attached_region_disks_pairs = { + for k, v in local.attached_disks_pairs : + k => v if local.attached_disks[v.disk_name].options.regional + } + attached_zone_disks_pairs = { + for k, v in local.attached_disks_pairs : + k => v if !local.attached_disks[v.disk_name].options.regional + } on_host_maintenance = ( var.options.preemptible || var.confidential_compute ? "TERMINATE" @@ -68,24 +76,34 @@ locals { } resource "google_compute_disk" "disks" { - for_each = var.use_instance_template ? {} : local.attached_disks_pairs - project = var.project_id - zone = local.zones[each.value.name] - name = each.key - type = local.attached_disks[each.value.disk_name].options.type - size = local.attached_disks[each.value.disk_name].size - image = local.attached_disks[each.value.disk_name].image + for_each = var.use_instance_template ? {} : { + for k, v in local.attached_zone_disks_pairs : + k => v if local.attached_disks[v.disk_name].source_type != "attach" + } + project = var.project_id + zone = local.zones[each.value.name] + name = each.key + type = local.attached_disks[each.value.disk_name].options.type + size = local.attached_disks[each.value.disk_name].size + image = ( + local.attached_disks[each.value.disk_name].source_type == "image" + ? local.attached_disks[each.value.disk_name].source + : null + ) + snapshot = ( + local.attached_disks[each.value.disk_name].source_type == "snapshot" + ? local.attached_disks[each.value.disk_name].source + : null + ) labels = merge(var.labels, { disk_name = local.attached_disks[each.value.disk_name].name disk_type = local.attached_disks[each.value.disk_name].options.type - # Disk images usually have slashes, which is against label # restrictions # image = local.attached_disks[each.value.disk_name].image }) - dynamic disk_encryption_key { + dynamic "disk_encryption_key" { for_each = var.encryption != null ? [""] : [] - content { raw_key = var.encryption.disk_encryption_key_raw kms_key_self_link = var.encryption.kms_key_self_link @@ -93,6 +111,36 @@ resource "google_compute_disk" "disks" { } } +resource "google_compute_region_disk" "disks" { + provider = google-beta + for_each = var.use_instance_template ? {} : { + for k, v in local.attached_region_disks_pairs : + k => v if local.attached_disks[v.disk_name].source_type != "attach" + } + project = var.project_id + region = var.region + replica_zones = var.zones + name = each.key + type = local.attached_disks[each.value.disk_name].options.type + size = local.attached_disks[each.value.disk_name].size + snapshot = ( + local.attached_disks[each.value.disk_name].source_type == "snapshot" + ? local.attached_disks[each.value.disk_name].source + : null + ) + labels = merge(var.labels, { + disk_name = local.attached_disks[each.value.disk_name].name + disk_type = local.attached_disks[each.value.disk_name].options.type + }) + dynamic "disk_encryption_key" { + for_each = var.encryption != null ? [""] : [] + content { + raw_key = var.encryption.disk_encryption_key_raw + kms_key_name = var.encryption.kms_key_self_link + } + } +} + resource "google_compute_instance" "default" { provider = google-beta for_each = var.use_instance_template ? {} : local.names @@ -113,16 +161,25 @@ resource "google_compute_instance" "default" { var.metadata, try(element(var.metadata_list, each.value), {}) ) - dynamic attached_disk { + dynamic "attached_disk" { for_each = { for resource_name, pair in local.attached_disks_pairs : - resource_name => local.attached_disks[pair.disk_name] if pair.name == each.key + resource_name => local.attached_disks[pair.disk_name] + if pair.name == each.key } iterator = config content { device_name = config.value.name mode = config.value.options.mode - source = google_compute_disk.disks[config.key].name + source = ( + config.value.source_type == "attach" + ? config.value.source + : ( + config.value.options.regional + ? google_compute_region_disk.disks[config.key].id + : google_compute_disk.disks[config.key].name + ) + ) } } @@ -136,14 +193,14 @@ resource "google_compute_instance" "default" { kms_key_self_link = var.encryption != null ? var.encryption.kms_key_self_link : null } - dynamic confidential_instance_config { + dynamic "confidential_instance_config" { for_each = var.confidential_compute ? [""] : [] content { enable_confidential_compute = true } } - dynamic network_interface { + dynamic "network_interface" { for_each = var.network_interfaces iterator = config content { @@ -154,7 +211,7 @@ resource "google_compute_instance" "default" { ? null : config.value.addresses.internal[each.value] ) - dynamic access_config { + dynamic "access_config" { for_each = config.value.nat ? [config.value.addresses] : [] iterator = addresses content { @@ -163,7 +220,7 @@ resource "google_compute_instance" "default" { ) } } - dynamic alias_ip_range { + dynamic "alias_ip_range" { for_each = config.value.alias_ips != null ? config.value.alias_ips : {} iterator = alias_ips content { @@ -175,12 +232,12 @@ resource "google_compute_instance" "default" { } scheduling { - automatic_restart = ! var.options.preemptible + automatic_restart = !var.options.preemptible on_host_maintenance = local.on_host_maintenance preemptible = var.options.preemptible } - dynamic scratch_disk { + dynamic "scratch_disk" { for_each = [ for i in range(0, var.scratch_disks.count) : var.scratch_disks.interface ] @@ -195,7 +252,7 @@ resource "google_compute_instance" "default" { scopes = local.service_account_scopes } - dynamic shielded_instance_config { + dynamic "shielded_instance_config" { for_each = var.shielded_config != null ? [var.shielded_config] : [] iterator = config content { @@ -239,35 +296,48 @@ resource "google_compute_instance_template" "default" { boot = true } - dynamic confidential_instance_config { + dynamic "confidential_instance_config" { for_each = var.confidential_compute ? [""] : [] content { enable_confidential_compute = true } } - dynamic disk { + dynamic "disk" { for_each = local.attached_disks iterator = config content { - auto_delete = config.value.options.auto_delete - device_name = config.value.name - disk_type = config.value.options.type - disk_size_gb = config.value.size - mode = config.value.options.mode - source_image = config.value.image - source = config.value.options.source - type = "PERSISTENT" + auto_delete = config.value.options.auto_delete + device_name = config.value.name + # Cannot use `source` with any of the fields in + # [disk_size_gb disk_name disk_type source_image labels] + disk_type = ( + config.value.source_type != "attach" ? config.value.options.type : null + ) + disk_size_gb = ( + config.value.source_type != "attach" ? config.value.size : null + ) + mode = config.value.options.mode + source_image = ( + config.value.source_type == "image" ? config.value.source : null + ) + source = ( + config.value.source_type == "attach" ? config.value.source : null + ) + disk_name = ( + config.value.source_type != "attach" ? config.value.name : null + ) + type = "PERSISTENT" } } - dynamic network_interface { + dynamic "network_interface" { for_each = var.network_interfaces iterator = config content { network = config.value.network subnetwork = config.value.subnetwork - dynamic access_config { + dynamic "access_config" { for_each = config.value.nat ? [""] : [] content {} } @@ -275,7 +345,7 @@ resource "google_compute_instance_template" "default" { } scheduling { - automatic_restart = ! var.options.preemptible + automatic_restart = !var.options.preemptible on_host_maintenance = local.on_host_maintenance preemptible = var.options.preemptible } @@ -292,7 +362,7 @@ resource "google_compute_instance_template" "default" { resource "google_compute_instance_group" "unmanaged" { count = ( - var.group != null && ! var.use_instance_template ? 1 : 0 + var.group != null && !var.use_instance_template ? 1 : 0 ) project = var.project_id network = ( @@ -306,7 +376,7 @@ resource "google_compute_instance_group" "unmanaged" { instances = [ for name, instance in google_compute_instance.default : instance.self_link ] - dynamic named_port { + dynamic "named_port" { for_each = var.group.named_ports != null ? var.group.named_ports : {} iterator = config content { diff --git a/modules/compute-vm/variables.tf b/modules/compute-vm/variables.tf index 3f83a9e1c..4f4ac27e9 100644 --- a/modules/compute-vm/variables.tf +++ b/modules/compute-vm/variables.tf @@ -15,19 +15,30 @@ */ variable "attached_disks" { - description = "Additional disks, if options is null defaults will be used in its place." + description = "Additional disks, if options is null defaults will be used in its place. Source type is one of 'image' (zonal disks in vms and template), 'snapshot' (vm), 'existing', and null." type = list(object({ - name = string - image = string - size = string + name = string + size = string + source = string + source_type = string options = object({ auto_delete = bool mode = string - source = string + regional = bool type = string }) })) default = [] + validation { + condition = length([ + for d in var.attached_disks : d if( + d.source_type == null + || + contains(["image", "snapshot", "attach"], coalesce(d.source_type, "1")) + ) + ]) == length(var.attached_disks) + error_message = "Source type must be one of 'image', 'snapshot', 'attach', null." + } } variable "attached_disk_defaults" { @@ -35,13 +46,13 @@ variable "attached_disk_defaults" { type = object({ auto_delete = bool mode = string + regional = bool type = string - source = string }) default = { auto_delete = true - source = null mode = "READ_WRITE" + regional = false type = "pd-ssd" } } @@ -72,6 +83,12 @@ variable "confidential_compute" { default = false } +variable "enable_display" { + description = "Enable virtual display on the instances" + type = bool + default = false +} + variable "encryption" { description = "Encryption options. Only one of kms_key_self_link and disk_encryption_key_raw may be set. If needed, you can specify to encrypt or not the boot disk." type = object({ @@ -240,9 +257,3 @@ variable "shielded_config" { }) default = null } - -variable "enable_display" { - description = "Enable virtual display on the instances" - type = bool - default = false -}