provider/google: Fix instance/template metadata support

[paddycarver]

Update our instance template to include metadata_startup_script, to
match our instance resource. Also, we've resolved the diff errors around
metadata.startup-script, and people want to use that to create startup
scripts that don't force a restart when they're changed, so let's stop
disallowing it.

Also, we had a bunch of calls to schema.ResourceData.Set that ignored
the errors, so I added error handling for those calls. It's mostly
bundled with this code because I couldn't be sure whether it was the
root of bugs or not, so I took care of it while addressing the startup
script issue.

Notes:

• When importing instance templates, we assume any startup scripts are for metadata.startup-script. This means we can't do an import test for metadata_startup_script because it will check for metadata_startup_script in the state, and it will get metadata.startup-script instead. We have to make this assumption because until we have state to work off, we can't detect that metadata_startup_script was used. Imports (to my knowledge?) don't have any state, so we can't figure out what the user specified. This does mean when a template is imported, then its conf file is created, if the conf file uses metadata_startup_script, the next terraform plan will show changes to use metadata_startup_script in the state, instead.
• Changing from metadata.startup-script to metadata_startup_script requires a destroy/create. I don't know of a good way around this, or if this should even be the desired behaviour.
• In the event that both metadata.startup-script and metadata_startup_script are specified, metadata_startup_script wins.
• metadata_startup_script is for instances that should be destroyed/recreated when the startup script changes. metadata.startup-script is for instances that want to modify it without a destroy/recreate. Instance templates destroy and recreate on metadata changes, anyways, so there's no real reason to use metadata_startup_script in an instance template, but cutting down on the inconsistencies between resources seems like a nice thing to do.

This undoes the work @phinze did in #3507, so pinging him. See also #10227, where @JDiPierro, @cblecker, and @sparkprime weighed in on the importance of having both supported for instances.

[phinze]

We have a ConflictsWith key in helper/schema to enforce this as a validation - I think we should do that on both resources to prevent folks from shooting themselves in the foot. 👣 🔫

[paddycarver]

ConflictsWith would be inappropriate here, wouldn't it? It would make metadata_startup_script incompatible with setting any metadata at all, which seems bad. I could do what we do on instances, though that looks like it's only applied on Update, not Create (no idea why). So I could just add similar code to the Create functions for both to test for the presence of a user aiming at their foot?

[phinze]

It would make metadata_startup_script incompatible with setting any metadata at all

Ah I thought that was true. Given:

  metadata {
    foo = "bar"
  }
  metadata_startup_script = "somescript"

Won't you get a perpetual diff in the metadata map as the two fields fight over whether or not startup-script should be in there?

[paddycarver]

Using this instance template conf:

resource "google_compute_instance_template" "template" {
  name        = "terraform-test"
  description = "test instance template"

  tags = ["testing"]

  machine_type = "f1-micro"

  disk {
    source_image = "debian-cloud/debian-8"
    auto_delete  = true
    boot         = true
  }

  network_interface {
    network = "default"
  }

  metadata {
    foo = "bar"
  }

  metadata_startup_script = "somescript"
}

and this instance conf:

resource "google_compute_instance" "instance" {
  name        = "terraform-test"
  description = "test instance"
  zone        = "us-central1-a"

  tags = ["testing"]

  machine_type = "f1-micro"

  disk {
    image = "debian-cloud/debian-8"
  }

  network_interface {
    network = "default"
  }

  metadata {
    foo = "bar"
  }

  metadata_startup_script = "somescript"
}

I'm able to create, then plan, and get no diff. My understanding is that's what this part is for.

[phinze]

Ah yep you are right. So the read method filters out startup-script if and only if metadata_startup_script is set on both resources.

If the user clears out metadata_startup_script in the config there might be an odd edge case there, but I think that's far enough off the beaten path to accept some slightly confusing behavior.

Thanks for helping clarify! 👍

[mitchellh]

LGTM!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.