cloudposse/terraform-aws-ec2-autoscale-group

block_device_mappings doesn't function as expected

ChrisMcKee opened this issue · 3 comments

The type definition in the variables.tf file seems to stop you being able to use simple setups

  block_device_mappings = [
    {
      device_name = "/dev/sda1"
      no_device    = false
      virtual_name = "root"
      ebs = {
        delete_on_termination = true
        volume_size           = 22
      }
    }
  ]

Fails with

Error: Invalid value for module argument

  on main.tf line 133, in module "autoscale_group":
 133:   block_device_mappings = [
 134:     {
 135:       device_name = "/dev/sda1"
 136:       no_device    = false
 137:       virtual_name = "root"
 138:       ebs = {
 139:         delete_on_termination = true
 140:         volume_size           = 22
 141:       }
 142:     }
 143:   ]

The given value is not suitable for child module variable
"block_device_mappings" defined at
.terraform/modules/autoscale_group/variables.tf:108,1-33: element 0: attribute
"ebs": attributes "encrypted", "iops", "kms_key_id", "snapshot_id", and
"volume_type" are required.

Which was after adding

      no_device    = false
      virtual_name = "root"

as it wouldnt work without those either.

Removing the type restrictions from the vars allows it to work as expected

  type = list(object({
    device_name  = string
    no_device    = bool
    virtual_name = string
    ebs = object({
      delete_on_termination = bool
      encrypted             = bool
      iops                  = number
      kms_key_id            = string
      snapshot_id           = string
      volume_size           = number
      volume_type           = string
    })
  }))

The AWS module defines it as

variable "ebs_block_device" {
  description = "Additional EBS block devices to attach to the instance"
  type        = list(map(string))
  default     = []
}

https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/variables.tf#L145-L161

and define it using

  dynamic "ebs_block_device" {
    for_each = var.ebs_block_device
    content {
      delete_on_termination = lookup(ebs_block_device.value, "delete_on_termination", null)
      device_name           = ebs_block_device.value.device_name
      encrypted             = lookup(ebs_block_device.value, "encrypted", null)
      iops                  = lookup(ebs_block_device.value, "iops", null)
      no_device             = lookup(ebs_block_device.value, "no_device", null)
      snapshot_id           = lookup(ebs_block_device.value, "snapshot_id", null)
      volume_size           = lookup(ebs_block_device.value, "volume_size", null)
      volume_type           = lookup(ebs_block_device.value, "volume_type", null)
    }
  }

  dynamic "ephemeral_block_device" {
    for_each = var.ephemeral_block_device
    content {
      device_name  = ephemeral_block_device.value.device_name
      virtual_name = ephemeral_block_device.value.virtual_name
    }
  }

  dynamic "root_block_device" {
    for_each = var.root_block_device
    content {
      delete_on_termination = lookup(root_block_device.value, "delete_on_termination", null)
      iops                  = lookup(root_block_device.value, "iops", null)
      volume_size           = lookup(root_block_device.value, "volume_size", null)
      volume_type           = lookup(root_block_device.value, "volume_type", null)
      encrypted             = lookup(root_block_device.value, "encrypted", null)
    }
  }

But tbh I just ripped out the type to get it working.

Indeed it seems to require all fields.
Setting a value to null seems to result in the AWS default.

    block_device_mappings = [
      {
        device_name  = "/dev/sda1"
        no_device    = "false"
        virtual_name = "root"
        ebs = {
          encrypted             = true
          volume_size           = 200
          delete_on_termination = true
          iops                  = null
          kms_key_id            = null
          snapshot_id           = null
          volume_type           = "standard"
        }
      }
    ]

As above; tbh in this case no validation is better than requiring explicit null contrary to how the provider uses it (imo)