Should not try to parse empty strings

[athak]

We are trying to encapsulate some functionality in a custom module, trying to expose as many arguments from the underlying resources as possible through the module. One of the resources used inside the module is softlayer_virtual_guest. So for example the module has these variables declared among others:

variable "image_id" {
  default     = ""
}

variable "private_vlan_id" {
  default     = ""
}

variable "public_vlan_id" {
  default     = ""
}

And then passed to the resource:

resource "softlayer_virtual_guest" "instance" {
  ...
  image_id                 = "${var.image_id}"
  ...
  private_vlan_id          = "${var.private_vlan_id}"
  public_vlan_id           = "${var.public_vlan_id}"
  ...
}

The problem is that if no value is passed to the module, it still tries to interpret/parse the empty strings resulting in:

Errors:

  * softlayer_virtual_guest.instance: cannot parse '' as int: strconv.ParseInt: parsing "": invalid syntax
  * softlayer_virtual_guest.instance: cannot parse '' as int: strconv.ParseInt: parsing "": invalid syntax
  * softlayer_virtual_guest.instance: "image_id": conflicts with os_reference_code ("UBUNTU_14_64")
  * softlayer_virtual_guest.instance: "os_reference_code": conflicts with image_id ("")

The first two are the public and private_vlan_id arguments. Commenting those in the resource makes the errors go away.

Given that these are optional arguments, empty strings should be interpreted as nil. This is with terraform 0.7.13 and latest provider release, 1.3.0.

Thanks for the awesome work!

Cheers,
Atha

[renier]

@athak You have to pass the variables to the inputs of the module, but I see the vars being interpolated directly into the underlying resources instead.

can you show all the tf files including the module?

[athak]

@renier thanks for the prompt reply. The module in this case is being called without those arguments since they are optional:

module "srv" {
  source                   = "../../../modules/instance/softlayer"
  amount                   = 1
  tags                     = ["dev", "test"]
  name                     = "srv"

  os                       = "ubuntu"
  os_version               = "14"
  register_private_domain  = false
  register_public_domain   = false
  customer_dns_name        = "mgmt"
  cnames                   = {
    "region"      = "dev, test"
    "provider"    = "alias"
    "environment" = "envalias"
  }
}

So the defaults kick in which are the empty strings.

[renier]

I see what you mean now. I don't know how to get around this currently without removing the vars you don't intend to use. Don't think this is an actual issue with the provider. Just how you setup the vars to drive the module.

[athak]

I just made a quick experiment using the old softlayer_virtual_guest resource included in terraform, adjusting the argument names:

resource "softlayer_virtual_guest" "instance" {
  ...
  block_device_template_group_gid                 = "${var.image_id}"
  ...
  backend_vlan_id          = "${var.private_vlan_id}"
  frontend_vlan_id           = "${var.public_vlan_id}"
  ...
}

Same module call, default empty strings. No errors.

[athak]

I see in the source code of the old softlayer_virtual_guest resource that block_device_template_group_gid, backend_vlan_id and frontend_vlan_id are defined as TypeString and then parsed to int. Maybe that's the workaround?

[athak]

This appears to be a related issue: hashicorp/terraform#6254

If converting the arguments to TypeString is not possible, how about using a special value like -1 to be interpreted as nil? There seems currently to be no way to pass integers to modules.

[athak]

Ok, so setting the default for public_vlan_id and private_vlan_id to -1 seems to work with the current code, since it checks for values > 0. The instance is correctly created. However, the terraform plan shows:

+ module.srv.softlayer_virtual_guest.instance
    ...
    private_network_only:     "false"
    private_subnet:           "<computed>"
    private_vlan_id:          "-1"
    public_subnet:            "<computed>"
    public_vlan_id:           "-1"

The terraform.tfstate does have the assigned vlan numbers though. So we can go with this for now for VLAN ids but it feels a bit dirty for the planning phase.

The conflict between image_id with default empty string and os_reference code remains.

[renier]

@athak that is a problem in terraform when you mark a fields conflicting with each other. It looks up the values and that's how it deals with explicit empty strings when the type is TypeInt. You should look at creating a module that uses images and another that uses os reference codes.

[athak]

@renier I understand the current limitations in terraform and the fact that we still have to go through a lot of workarounds to achieve certain goals due to lack of certain features. I go through that pain every day. However, maintaining an almost exact duplicate copy of a module because of this kind of shortcoming is not a realistic solution.

Sometimes certain improvements/workarounds can be made to accommodate shortcomings in the underlying framework.

[renier]

@athak open to looking at a PR showing your idea on solving this at the provider level. However, a PR up to Terraform would be more appropriate.

[athak]

@renier unfortunately my knowledge in Go is not up to par for this. Anyway, thanks for your help.

[renier]

@athak do you have an idea though on what the provider could do in this case? The conflict resolution is managed above the provider plugin, so I can't immediately think of a solution, unless we lift that conflict relationship. You can tinker around and try it. We would have to manage the potential conflict locally in the provider. Assuming that would help you.

[minsikl]

DiffSuppressFunc can be added to private_vlan_id and public_vlan_id arguments to ignore updated vlan_id values for terraform plan or terraform apply. I also think that conflict checking logic can be implemented in a create function instead of ConflictsWith to provide more flexibility.

[renier]

DiffSuppressFunc is not without its problems.

[minsikl]

private_vlan_id and public_vlan_id don't have to have a default value. The following DiffSuppressFunc ignores difference when private_vlan_id = 0 or public_vlan_id = 0

DiffSuppressFunc: func(k, o, n string, d *schema.ResourceData) bool {
	intO, _ := strconv.Atoi(o)
	intN, _ := strconv.Atoi(n)
	if intN <= 0 {
		return true
	}
	return intO == intN
}

[athak]

I came across a comment by @mitchellh in this issue which basically is about the same functionalluty. He states that

Most resources at the moment must just be written to accept some "zero" value as unset.

hoping to provide a way in the future to unset a value.

Is this something that can be implemented? Our current pain points are with image_id, {public,private}_vlan_id. Allowing the use of 0 or other value as "unset" would greatly simplify our current code.

[renier]

@athak Have you tried setting those defaults to 0?

variable "image_id" {
  default     = 0
}

variable "private_vlan_id" {
  default     = 0
}

variable "public_vlan_id" {
  default     = 0
}

[athak]

@renier yes, in the case of image_id it conflicts with os_reference_code. In the case of the vlan ids, 0 is initially accepted as a value and the instance is created and a correct vlan id is assigned. However, once the instance is created, issuing a plan produces the following for example:

    cores:                    "1" => "1"
    datacenter:               "wdc04" => "wdc04"
    dedicated_acct_host_only: "false" => "false"
    disks.#:                  "1" => "1"
    disks.0:                  "25" => "25"
    domain:                   "dev.vurbia.net" => "dev.vurbia.net"
    hostname:                 "test" => "test"
    hourly_billing:           "true" => "true"
    ip_address_id:            "74154291" => "<computed>"
    ip_address_id_private:    "74154417" => "<computed>"
    ipv4_address:             "169.45.194.237" => "<computed>"
    ipv4_address_private:     "10.170.108.180" => "<computed>"
    ipv6_address:             "" => "<computed>"
    ipv6_address_id:          "" => "<computed>"
    ipv6_enabled:             "false" => "false"
    local_disk:               "false" => "false"
    memory:                   "1024" => "1024"
    network_speed:            "100" => "100"
    os_reference_code:        "UBUNTU_16_64" => "UBUNTU_16_64"
    private_network_only:     "false" => "false"
    private_subnet:           "10.170.108.128/26" => "<computed>"
    private_vlan_id:          "1589655" => "0" (forces new resource)
    public_ipv6_subnet:       "" => "<computed>"
    public_subnet:            "169.45.194.224/28" => "<computed>"
    public_vlan_id:           "1589653" => "0" (forces new resource)
    secondary_ip_addresses.#: "0" => "<computed>"
    secondary_ip_count:       "0" => "0"
    wait_time_minutes:        "90" => "90"