From 7332327b6baacd73d29d54b99a72a0f74727753f Mon Sep 17 00:00:00 2001 From: Riley Karson Date: Mon, 25 Oct 2021 09:48:46 -0700 Subject: [PATCH] Change the import behaviour of GCE instance metadata startup-script (#5329) --- .../resource_compute_instance.go.erb | 14 +++++------- .../resource_compute_instance_template.go.erb | 2 ++ .../resource_compute_instance_test.go.erb | 4 ++-- .../guides/version_4_upgrade.html.markdown | 22 +++++++++++++++++++ .../docs/r/compute_instance.html.markdown | 20 ++++++++--------- 5 files changed, 41 insertions(+), 21 deletions(-) diff --git a/mmv1/third_party/terraform/resources/resource_compute_instance.go.erb b/mmv1/third_party/terraform/resources/resource_compute_instance.go.erb index 3581a97421e7..a83c4945b3d0 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1148,19 +1148,15 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error } md := flattenMetadataBeta(instance.Metadata) - existingMetadata := d.Get("metadata").(map[string]interface{}) - // If the existing config specifies "metadata.startup-script" instead of "metadata_startup_script", - // we shouldn't move the remote metadata.startup-script to metadata_startup_script. Otherwise, - // we should. - if _, ok := existingMetadata["startup-script"]; !ok { + // If the existing state contains "metadata_startup_script" instead of "metadata.startup-script", + // we should move the remote metadata.startup-script to metadata_startup_script to avoid + // specifying it in two places. + if _, ok := d.GetOk("metadata_startup_script"); ok { if err := d.Set("metadata_startup_script", md["startup-script"]); err != nil { return fmt.Errorf("Error setting metadata_startup_script: %s", err) } - // Note that here we delete startup-script from our metadata list. This is to prevent storing the startup-script - // as a value in the metadata since the config specifically tracks it under 'metadata_startup_script' - delete(md, "startup-script") - } else if _, ok := d.GetOk("metadata_startup_script"); ok { + delete(md, "startup-script") } diff --git a/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb b/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb index 95a653c28b93..ae3171a2e983 100644 --- a/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb +++ b/mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb @@ -1324,8 +1324,10 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{ if err = d.Set("metadata_startup_script", script); err != nil { return fmt.Errorf("Error setting metadata_startup_script: %s", err) } + delete(_md, "startup-script") } + if err = d.Set("metadata", _md); err != nil { return fmt.Errorf("Error setting metadata: %s", err) } diff --git a/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb b/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb index 1be448bad928..bb7b5d517533 100644 --- a/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -77,8 +77,8 @@ func testSweepComputeInstance(region string) error { func computeInstanceImportStep(zone, instanceName string, additionalImportIgnores []string) resource.TestStep { // metadata is only read into state if set in the config - // since importing doesn't know whether metadata.startup_script vs metadata_startup_script is set in the config, - // it guesses metadata_startup_script + // importing doesn't know whether metadata.startup_script vs metadata_startup_script is set in the config, + // it always takes metadata.startup-script ignores := []string{"metadata.%", "metadata.startup-script", "metadata_startup_script"} return resource.TestStep{ diff --git a/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown b/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown index 022dbdf38783..3eeeffe7425e 100644 --- a/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown +++ b/mmv1/third_party/terraform/website/docs/guides/version_4_upgrade.html.markdown @@ -277,6 +277,28 @@ The provider will now enforce at plan time that one of these fields be set. Previously, if all of these fields were left empty, the firewall defaulted to allowing traffic from 0.0.0.0/0, which is a suboptimal default. +## Resource: `google_compute_instance` + +### `metadata_startup_script` is no longer set on import + +Earlier versions of the provider set the `metadata_startup_script` value on +import, omitting the value of `metadata.startup-script` for historical backwards +compatibility. This was dangerous in practice, as `metadata_startup_script` +would flag an instance for recreation if the values differed rather than for +just an update. + +In `4.0.0` the behaviour has been flipped, and `metadata.startup-script` is the +default value that gets written. Users who want `metadata_startup_script` set +on an imported instance will need to modify their state manually. This is more +consistent with our expectations for the field, that a user who manages an +instance **only** through Terraform uses it but that most users should prefer +the `metadata` block. + +No action is required for user configs with instances already imported. If you +have a config or module where neither is specified- where `import` will be run, +or an old config that is not reconciled with the API- the value that gets set +will change. + ## Resource: `google_compute_instance_group_manager` ### `update_policy.min_ready_sec` is removed from the GA provider diff --git a/mmv1/third_party/terraform/website/docs/r/compute_instance.html.markdown b/mmv1/third_party/terraform/website/docs/r/compute_instance.html.markdown index 0ef367216aa0..95d40c677d9b 100644 --- a/mmv1/third_party/terraform/website/docs/r/compute_instance.html.markdown +++ b/mmv1/third_party/terraform/website/docs/r/compute_instance.html.markdown @@ -132,16 +132,16 @@ The following arguments are supported: we provide a special attribute, `metadata_startup_script`, which is documented below. * `metadata_startup_script` - (Optional) An alternative to using the - startup-script metadata key, except this one forces the instance to be - recreated (thus re-running the script) if it is changed. This replaces the - startup-script metadata key on the created instance and thus the two - mechanisms are not allowed to be used simultaneously. Users are free to use - either mechanism - the only distinction is that this separate attribute - will cause a recreate on modification. On import, `metadata_startup_script` - will be set, but `metadata.startup-script` will not - if you choose to use the - other mechanism, you will see a diff immediately after import, which will cause a - destroy/recreate operation. You may want to modify your state file manually - using `terraform state` commands, depending on your use case. +startup-script metadata key, except this one forces the instance to be recreated +(thus re-running the script) if it is changed. This replaces the startup-script +metadata key on the created instance and thus the two mechanisms are not +allowed to be used simultaneously. Users are free to use either mechanism - the +only distinction is that this separate attribute will cause a recreate on +modification. On import, `metadata_startup_script` will not be set - if you +choose to specify it you will see a diff immediately after import causing a +destroy/recreate operation. If importing an instance and specifying this value +is desired, you will need to modify your state file manually using +`terraform state` commands. * `min_cpu_platform` - (Optional) Specifies a minimum CPU platform for the VM instance. Applicable values are the friendly names of CPU platforms, such as `Intel Haswell` or `Intel Skylake`. See the complete list [here](https://cloud.google.com/compute/docs/instances/specify-min-cpu-platform).