PaloAltoNetworks / terraform-aws-vmseries-modules

Terraform Reusable Modules for VM-Series on AWS
https://registry.terraform.io/modules/PaloAltoNetworks/vmseries-modules/aws
MIT License
40 stars 49 forks source link

Unable to use security_group_ids in vmseries module #291

Closed jamesholland-uk closed 1 year ago

jamesholland-uk commented 1 year ago

Describe the bug

Type declaration for interfaces in vmseries modules appears to make security_group_ids unusable.

Expected behavior

Should be able to use security_group_ids in the interfaces section of the vmseries module.

Current behavior

The HCL code showing the module usage (below) works fine, until I uncomment the security_group_ids line, then I receive the following error: The given value is not suitable for module.the_vmseries.var.interfaces declared at .terraform/modules/the_vmseries/modules/vmseries/variables.tf:72,1-22: attribute types must all match for conversion to map.

module "the_vmseries" {
  source  = "PaloAltoNetworks/vmseries-modules/aws//modules/vmseries"
  version = "0.4.1"

  name                  = var.name_prefix
  vmseries_product_code = var.vm_series_product_code
  vmseries_version      = var.vm_series_version
  ssh_key_name          = aws_key_pair.ssh_key_pair.key_name

  interfaces = {
    mgmt = {
      device_index     = 0
      subnet_id        = aws_subnet.mgmt_subnet.id
      name             = "mgmt"
      create_public_ip = true
      #security_group_ids = ["sg-05996161e0be5332a"]
    },
    public = {
      device_index     = 1
      subnet_id        = aws_subnet.outside_subnet.id
      name             = "outside"
      create_public_ip = true
    },
    private = {
      device_index     = 2
      subnet_id        = aws_subnet.inside_subnet.id
      name             = "inside"
      create_public_ip = false
    },
  }
}

Possible solution

I think that this error occurs because it introduces a list of strings parameter value. Before uncommenting that line, all parameter values are strings, uncommenting introduces a list of strings. I believe that the type declaration of map means that all parameter values must be of the same type (ref). I think the type declaration would either need to be very permissive (just type = any) or much more specific where every parameter variable is listed in the type declaration, like this generic example:

variable "test" {
  type = object({
    param1 = string
    param2 = string
    param3 = object({
      mylist = list(string)
    })
  })
}

Steps to reproduce

HCL code snippet above

Screenshots

Screenshot 2023-03-08 at 10 07 17

Context

Unable to set a Security Group for a VM-Series created with the vmseries module.

Your Environment

Terraform v1.3.8
on darwin_arm64
+ provider registry.terraform.io/hashicorp/aws v4.57.1
sebastianczech commented 1 year ago

hi @jamesholland-uk . While checking the issue, I noticed, that you defined security_group_ids only for first network interface. Could you please define security_group_ids for all interfaces ? As we have in error message: attribute types must all match for conversion to map, then we need to use security_group_ids for all interfaces or for no one.

Yes, it would be better to have defined variable not like map(any), but map of objects with defined attributes. The problem is that in our case some attributes are optional as in the variable interfaces description (in module vmseries):

  - `name`               = (Optional|string) Name tag for the ENI. Defaults to instance name suffixed by map's key.
  - `description`        = (Optional|string) A descriptive name for the ENI.
  - `create_public_ip`   = (Optional|bool) Whether to create a public IP for the ENI. Defaults to false.
  - `eip_allocation_id`  = (Optional|string) Associate an existing EIP to the ENI.
  - `private_ips`        = (Optional|list) List of private IPs to assign to the ENI. If not set, dynamic allocation is used.
  - `public_ipv4_pool`   = (Optional|string) EC2 IPv4 address pool identifier. 
  - `source_dest_check`  = (Optional|bool) Whether to enable source destination checking for the ENI. Defaults to false.
  - `security_group_ids` = (Optional|list) A list of Security Group IDs to assign to this interface. Defaults to null.

Unfortunately only Terraform 1.3 or above supports optional object type attributes with defaults and as our modules are supported for Terraform starting from 1.0.0:

https://github.com/PaloAltoNetworks/terraform-aws-vmseries-modules/blob/fdb85b3f07fa161aa7f2523fcaeedba9c345ae77/modules/vmseries/versions.tf#L1-L2

we cannot use optional object type attributes in vmseries module.

Please check you Terraform code with security_group_ids for all interfaces (even as empty list [] for other interfaces only to check that it helps) and please let us know if it's ok for you and we can close that issue.

jamesholland-uk commented 1 year ago

Hi @sebastianczech, empty list worked great for the other two interfaces so I was able to get past the error. Thanks for your assistance :-)