[ Design Issue ] Allow and prefer reference by id instead of name in all resources

[jpbuecken]

Affected resources:

• "vcd_vapp"
• "vcd_vapp_vm"

Version

• provider 2.6
• Terraform v0.12.18

Most important example for this issue is vApp and VM resource

In the current design, you reference resources by name.

This has two big issues:
a) Rebuild of a resource will not rebuild the dependency, unless you change the name
b) Changing the name of vApp is possible online, but with this design, it will force rebuild of VM. (see https://github.com/terraform-providers/terraform-provider-vcd/issues/387 as well)

This will lead to an inconsistent terraform plan:
If you change the value of vApp that forces a rebuild of the vApp, terraform plan will only show changes of the vApp. If you run apply, your VM is destroyed, but terraform plan did not reflect this.
Your VM is lost without the possibility for review.

How terraform will detect needed rebuild is explained here:
hashicorp/terraform#8099 (comment)

I think you are expecting an additional behavior: if there is an update to foo.bar then there will always be an automatic update to bar.foo. But that is not actually how Terraform works, by design: the dependency edges are used for ordering, but the direct attribute values are used for diffing.

HCL used to test:

resource "vcd_vapp" "vapp" {
  name = var.hostname
}

resource "vcd_vapp_vm" "instance" {
  name          = var.hostname
  computer_name = var.hostname
  vapp_name     = vcd_vapp.vapp.name
  catalog_name  = var.image_catalog
  template_name = var.image_string
  cpus          = var.cpu
  cpu_cores     = var.core
  memory        = var.memory

  network {
    type               = "org"
    name               = var.network
    ip_allocation_mode = var.private_ip == null ? "POOL" : "MANUAL"
    ip                 = var.private_ip
    is_primary         = true
  }
}

Example for a)
terraform plan if you change vdc only for vApp. If you run apply your VM is deleted without any notice:

-/+ destroy and then create replacement

Terraform will perform the following actions:

  # vcd_vapp.vapp must be replaced
-/+ resource "vcd_vapp" "vapp" {
        accept_all_eulas = true
      ~ href             = "https://access.devcloud.arvato-systems.de/api/vApp/vapp-40b7d417-8408-4712-b520-d83dcff59481" -> (known after apply)
      ~ id               = "urn:vcloud:vapp:40b7d417-8408-4712-b520-d83dcff59481" -> (known after apply)
      ~ ip               = "allocated" -> (known after apply)
      - metadata         = {} -> null
        name             = "test1"
        power_on         = true
      ~ status           = 4 -> (known after apply)
      ~ status_text      = "POWERED_ON" -> (known after apply)
      + vdc              = "bla" # forces replacement
    }

Plan: 1 to add, 0 to change, 1 to destroy.

------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

Example for b)
Change of name of vApp forces rebuild of VM which is not needed:

-/+ destroy and then create replacement

Terraform will perform the following actions:

  # vcd_vapp.vapp must be replaced
-/+ resource "vcd_vapp" "vapp" {
        accept_all_eulas = true
      - guest_properties = {} -> null
      ~ href             = "https://access.devcloud.arvato-systems.de/api/vApp/vapp-40b7d417-8408-4712-b520-d83dcff59481" -> (known after apply)
      ~ id               = "urn:vcloud:vapp:40b7d417-8408-4712-b520-d83dcff59481" -> (known after apply)
      ~ ip               = "allocated" -> (known after apply)
      - metadata         = {} -> null
      ~ name             = "test1" -> "newname" # forces replacement
        power_on         = true
      ~ status           = 4 -> (known after apply)
      ~ status_text      = "POWERED_ON" -> (known after apply)
    }
  # vcd_vapp_vm.instance must be replaced
-/+ resource "vcd_vapp_vm" "instance" {
        accept_all_eulas               = true
[...]

Suggested solution:
Add reference by id to each resource, maybe allow use of both, reference by name or id for a while, then deprecate reference by name:

e.g. for vcd_vapp_vm

resource "vcd_vapp_vm" "instance" {
   vapp_id = vcd_vapp.vapp.id
  [...]

[jpbuecken]

For reference: Issue detected during tests for https://github.com/terraform-providers/terraform-provider-vcd/pull/412#issuecomment-572471499

[jpbuecken]

This will also resolve a statement from hashicorp in the documentation about depends_on:

The depends_on argument should be used only as a last resort.

https://www.terraform.io/docs/configuration/resources.html#depends_on-explicit-resource-dependencies

Since the reference by id is a required field, you do not need "depends_on" anymore (see other discussions about dependecies in
https://github.com/terraform-providers/terraform-provider-vcd/pull/412#issuecomment-571558229 ,
https://github.com/terraform-providers/terraform-provider-vcd/pull/412#issuecomment-571596658 ,
https://github.com/terraform-providers/terraform-provider-vcd/pull/412#issuecomment-571610984
as well. I used strings to reference vApp and VM name instead of the reference by the information provided by the resource. This will most likely not happen if I need to reference the ID)

[lvirbalas]

Thank you for a good write-up. Would you update the links to PR to the exact place with relevant comments? As you know, that PR is huge. This will help us get our heads around it when the time comes.

[dataclouder]

Suggested solution:
Add reference by id to each resource, maybe allow use of both, reference by name or id for a while, then deprecate reference by name:

Adding support by ID is relatively easy. Terraform is already dealing with entities by ID.

Removing reference by name, however, is not recommendable, as it makes the system harder to use.

I propose adding support for ID-based references, but leaving names in place, as mutually exclusive with IDs, and users will decide which they need to adopt.

Note also that the rename problem is a genera issue in all the library entities, and should be resolved, in due time, independently from ID reference.

[jpbuecken]

Thank you for a good write-up. Would you update the links to PR to the exact place with relevant comments? As you know, that PR is huge. This will help us get our heads around it when the time comes.

Done

[jpbuecken]

Removing reference by name, however, is not recommendable, as it makes the system harder to use.

I propose adding support for ID-based references, but leaving names in place, as mutually exclusive with IDs, and users will decide which they need to adopt.

Note also that the rename problem is a genera issue in all the library entities, and should be resolved, in due time, independently from ID reference.

The rename problem is just an example to trigger the issue.
On the one hand:
If you leave reference by name in place, there is a risk that customer destroy whole production environments because terraform plan did not show the deletion of dependencies

If terraform plan is not consistent to what will actually happen, I consider it very dangrous to use for production!

On the other hand:
It is not more difficult to use ids than name , you only change some bytes in your template
(vcd_vapp = vcd_vapp.vapp.name -> vcd_vapp_id = vcd_vapp.vapp.id)
If you have experience with another provider on any other cloud, you are used to it already.
This is a very low barrier if I compare it with destruction of whole production environments.
IMHO it is grossly negligent to leave reference by name in place

And at least it looks like it is important for you to show a terraform plan that will show clean data about changes, e.g.
https://github.com/terraform-providers/terraform-provider-vcd/pull/412/files#r364670810
" Mess up a lot - the result, user don't understand what will be final result. For e.g. change one internal disk - it will show that will change all of them, though in reality code will change only one"

[jpbuecken]

One additional comment:
This is why data sources are important in other clouds:
As a user, you usually work with the name.
So you use data source to get id.

Example: Assume the vApp is not build by terraform, but you want to deploy a VM in it.
Ofcourse the system is harder to use if you need to lookup id manually:

resource vcd_vapp_vm {
   vapp_id = "urn:vapp:12345"
   [...]
}

But this is not what you do.
You use the name, which is more familar, and extend your template by a couple of lines.

data vcd_vapp vappdata {
   name = "XXX"
}

resource vcd_vapp_vm {
   vapp_id = data.vcd_vapp.vappdata.id
   [...]
}

@dataclouder If you work like this, it may mitigate your assumption that it will be harder to use.
IMHO if you work like this, the concept of terraform is coherent.

[Didainius]

We are always using IDs now and while there are some legacy places with names, they will gradually be deprecated and removed either with resource EOL, or with a new replacement (like we recently did with vcd_catalog_vapp_template and vcd_vapp_vm/vcd_vm support for it). I don't think we require a separate issue to track this anymore.