Change the import behaviour of GCE instance metadata startup-script (#…

…5329) (#3765)

Signed-off-by: Modular Magician <[email protected]>

.changelog/5329.txt

@@ -0,0 +1,3 @@
+```release-note:breaking-change
+compute: changed the import / drift detection behaviours for `metadata_startup_script`, `metadata.startup-script` in `google_compute_instance`. Now, `metadata.startup-script` will be set by default, and `metadata_startup_script` will only be set if present.
+```

google-beta/resource_compute_instance.go

@@ -1141,19 +1141,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")
 	}
 

google-beta/resource_compute_instance_template.go

@@ -1313,8 +1313,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)
 		}

google-beta/resource_compute_instance_test.go

@@ -76,8 +76,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{

google-beta/resource_dataproc_cluster_test.go

@@ -13,9 +13,8 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 
-	"google.golang.org/api/googleapi"
-
 	dataproc "google.golang.org/api/dataproc/v1beta2"
+	"google.golang.org/api/googleapi"
 )
 
 func TestDataprocExtractInitTimeout(t *testing.T) {

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

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).