From e1a580583352f6aa572ef477d248345c5f2c6f94 Mon Sep 17 00:00:00 2001 From: Paddy Date: Fri, 2 Dec 2016 13:40:55 -0800 Subject: [PATCH] Fix instance/template metadata support 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. --- .../google/resource_compute_instance.go | 18 +--- .../resource_compute_instance_template.go | 89 +++++++++++++++---- ...resource_compute_instance_template_test.go | 68 ++++++++++++++ .../r/compute_instance_template.html.markdown | 5 ++ 4 files changed, 147 insertions(+), 33 deletions(-) diff --git a/builtin/providers/google/resource_compute_instance.go b/builtin/providers/google/resource_compute_instance.go index a34c0ca9d..cdcf19a27 100644 --- a/builtin/providers/google/resource_compute_instance.go +++ b/builtin/providers/google/resource_compute_instance.go @@ -111,10 +111,9 @@ func resourceComputeInstance() *schema.Resource { }, "metadata": &schema.Schema{ - Type: schema.TypeMap, - Optional: true, - Elem: schema.TypeString, - ValidateFunc: validateInstanceMetadata, + Type: schema.TypeMap, + Optional: true, + Elem: schema.TypeString, }, "metadata_startup_script": &schema.Schema{ @@ -634,10 +633,10 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error md := instance.Metadata _md := MetadataFormatSchema(d.Get("metadata").(map[string]interface{}), md) - delete(_md, "startup-script") if script, scriptExists := d.GetOk("metadata_startup_script"); scriptExists { d.Set("metadata_startup_script", script) + delete(_md, "startup-script") } if err = d.Set("metadata", _md); err != nil { @@ -1010,12 +1009,3 @@ func resourceInstanceTags(d *schema.ResourceData) *compute.Tags { return tags } - -func validateInstanceMetadata(v interface{}, k string) (ws []string, es []error) { - mdMap := v.(map[string]interface{}) - if _, ok := mdMap["startup-script"]; ok { - es = append(es, fmt.Errorf( - "Use metadata_startup_script instead of a startup-script key in %q.", k)) - } - return -} diff --git a/builtin/providers/google/resource_compute_instance_template.go b/builtin/providers/google/resource_compute_instance_template.go index f9d28ec35..da0708b3b 100644 --- a/builtin/providers/google/resource_compute_instance_template.go +++ b/builtin/providers/google/resource_compute_instance_template.go @@ -173,6 +173,12 @@ func resourceComputeInstanceTemplate() *schema.Resource { ForceNew: true, }, + "metadata_startup_script": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + ForceNew: true, + }, + "metadata_fingerprint": &schema.Schema{ Type: schema.TypeString, Computed: true, @@ -477,6 +483,7 @@ func resourceComputeInstanceTemplateCreate(d *schema.ResourceData, meta interfac return err } instanceProperties.Disks = disks + metadata, err := resourceInstanceMetadata(d) if err != nil { return err @@ -693,7 +700,6 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{ log.Printf("[WARN] Removing Instance Template %q because it's gone", d.Get("name").(string)) // The resource doesn't exist anymore d.SetId("") - return nil } @@ -702,44 +708,89 @@ func resourceComputeInstanceTemplateRead(d *schema.ResourceData, meta interface{ // Set the metadata fingerprint if there is one. if instanceTemplate.Properties.Metadata != nil { - d.Set("metadata_fingerprint", instanceTemplate.Properties.Metadata.Fingerprint) + if err = d.Set("metadata_fingerprint", instanceTemplate.Properties.Metadata.Fingerprint); err != nil { + return fmt.Errorf("Error setting metadata_fingerprint: %s", err) + } + + md := instanceTemplate.Properties.Metadata + + _md := flattenMetadata(md) + + if script, scriptExists := d.GetOk("metadata_startup_script"); scriptExists { + 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) + } } // Set the tags fingerprint if there is one. if instanceTemplate.Properties.Tags != nil { - d.Set("tags_fingerprint", instanceTemplate.Properties.Tags.Fingerprint) + if err = d.Set("tags_fingerprint", instanceTemplate.Properties.Tags.Fingerprint); err != nil { + return fmt.Errorf("Error setting tags_fingerprint: %s", err) + } + } + if err = d.Set("self_link", instanceTemplate.SelfLink); err != nil { + return fmt.Errorf("Error setting self_link: %s", err) + } + if err = d.Set("name", instanceTemplate.Name); err != nil { + return fmt.Errorf("Error setting name: %s", err) } - d.Set("self_link", instanceTemplate.SelfLink) - d.Set("name", instanceTemplate.Name) if instanceTemplate.Properties.Disks != nil { - d.Set("disk", flattenDisks(instanceTemplate.Properties.Disks, d)) + if err = d.Set("disk", flattenDisks(instanceTemplate.Properties.Disks, d)); err != nil { + return fmt.Errorf("Error setting disk: %s", err) + } } - d.Set("description", instanceTemplate.Description) - d.Set("machine_type", instanceTemplate.Properties.MachineType) - d.Set("can_ip_forward", instanceTemplate.Properties.CanIpForward) - if instanceTemplate.Properties.Metadata != nil { - d.Set("metadata", flattenMetadata(instanceTemplate.Properties.Metadata)) + if err = d.Set("description", instanceTemplate.Description); err != nil { + return fmt.Errorf("Error setting description: %s", err) + } + if err = d.Set("machine_type", instanceTemplate.Properties.MachineType); err != nil { + return fmt.Errorf("Error setting machine_type: %s", err) + } + + if err = d.Set("can_ip_forward", instanceTemplate.Properties.CanIpForward); err != nil { + return fmt.Errorf("Error setting can_ip_forward: %s", err) + } + + if err = d.Set("instance_description", instanceTemplate.Properties.Description); err != nil { + return fmt.Errorf("Error setting instance_description: %s", err) + } + if err = d.Set("project", project); err != nil { + return fmt.Errorf("Error setting project: %s", err) } - d.Set("instance_description", instanceTemplate.Properties.Description) - d.Set("project", project) if instanceTemplate.Properties.NetworkInterfaces != nil { networkInterfaces, region := flattenNetworkInterfaces(instanceTemplate.Properties.NetworkInterfaces) - d.Set("network_interface", networkInterfaces) + if err = d.Set("network_interface", networkInterfaces); err != nil { + return fmt.Errorf("Error setting network_interface: %s", err) + } // region is where to look up the subnetwork if there is one attached to the instance template if region != "" { - d.Set("region", region) + if err = d.Set("region", region); err != nil { + return fmt.Errorf("Error setting region: %s", err) + } } } if instanceTemplate.Properties.Scheduling != nil { scheduling, autoRestart := flattenScheduling(instanceTemplate.Properties.Scheduling) - d.Set("scheduling", scheduling) - d.Set("automatic_restart", autoRestart) + if err = d.Set("scheduling", scheduling); err != nil { + return fmt.Errorf("Error setting scheduling: %s", err) + } + if err = d.Set("automatic_restart", autoRestart); err != nil { + return fmt.Errorf("Error setting automatic_restart: %s", err) + } } if instanceTemplate.Properties.Tags != nil { - d.Set("tags", instanceTemplate.Properties.Tags.Items) + if err = d.Set("tags", instanceTemplate.Properties.Tags.Items); err != nil { + return fmt.Errorf("Error setting tags: %s", err) + } } if instanceTemplate.Properties.ServiceAccounts != nil { - d.Set("service_account", flattenServiceAccounts(instanceTemplate.Properties.ServiceAccounts)) + if err = d.Set("service_account", flattenServiceAccounts(instanceTemplate.Properties.ServiceAccounts)); err != nil { + return fmt.Errorf("Error setting service_account: %s", err) + } } return nil } diff --git a/builtin/providers/google/resource_compute_instance_template_test.go b/builtin/providers/google/resource_compute_instance_template_test.go index b1521aa3f..642e0e57f 100644 --- a/builtin/providers/google/resource_compute_instance_template_test.go +++ b/builtin/providers/google/resource_compute_instance_template_test.go @@ -115,6 +115,26 @@ func TestAccComputeInstanceTemplate_subnet_custom(t *testing.T) { }) } +func TestAccComputeInstanceTemplate_metadata_startup_script(t *testing.T) { + var instanceTemplate compute.InstanceTemplate + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeInstanceTemplateDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeInstanceTemplate_startup_script, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeInstanceTemplateExists( + "google_compute_instance_template.foobar", &instanceTemplate), + testAccCheckComputeInstanceTemplateStartupScript(&instanceTemplate, "echo 'Hello'"), + ), + }, + }, + }) +} + func testAccCheckComputeInstanceTemplateDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) @@ -268,6 +288,31 @@ func testAccCheckComputeInstanceTemplateTag(instanceTemplate *compute.InstanceTe } } +func testAccCheckComputeInstanceTemplateStartupScript(instanceTemplate *compute.InstanceTemplate, n string) resource.TestCheckFunc { + return func(s *terraform.State) error { + if instanceTemplate.Properties.Metadata == nil && n == "" { + return nil + } else if instanceTemplate.Properties.Metadata == nil && n != "" { + return fmt.Errorf("Expected metadata.startup-script to be '%s', metadata wasn't set at all", n) + } + for _, item := range instanceTemplate.Properties.Metadata.Items { + if item.Key != "startup-script" { + continue + } + if item.Value != nil && *item.Value == n { + return nil + } else if item.Value == nil && n == "" { + return nil + } else if item.Value == nil && n != "" { + return fmt.Errorf("Expected metadata.startup-script to be '%s', wasn't set", n) + } else if *item.Value != n { + return fmt.Errorf("Expected metadata.startup-script to be '%s', got '%s'", n, *item.Value) + } + } + return fmt.Errorf("This should never be reached.") + } +} + var testAccComputeInstanceTemplate_basic = fmt.Sprintf(` resource "google_compute_instance_template" "foobar" { name = "instancet-test-%s" @@ -421,3 +466,26 @@ resource "google_compute_instance_template" "foobar" { foo = "bar" } }`, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10)) + +var testAccComputeInstanceTemplate_startup_script = fmt.Sprintf(` +resource "google_compute_instance_template" "foobar" { + name = "instance-test-%s" + machine_type = "n1-standard-1" + + disk { + source_image = "debian-8-jessie-v20160803" + auto_delete = true + disk_size_gb = 10 + boot = true + } + + metadata { + foo = "bar" + } + + network_interface{ + network = "default" + } + + metadata_startup_script = "echo 'Hello'" +}`, acctest.RandString(10)) diff --git a/website/source/docs/providers/google/r/compute_instance_template.html.markdown b/website/source/docs/providers/google/r/compute_instance_template.html.markdown index 27a86b737..201abc333 100644 --- a/website/source/docs/providers/google/r/compute_instance_template.html.markdown +++ b/website/source/docs/providers/google/r/compute_instance_template.html.markdown @@ -136,6 +136,11 @@ The following arguments are supported: * `metadata` - (Optional) Metadata key/value pairs to make available from within instances created from this template. +* `metadata_startup_script` - (Optional) An alternative to using the + startup-script metadata key, mostly to match the compute_instance resource. + This replaces the startup-script metadata key on the created instance and + thus the two mechanisms are not allowed to be used simultaneously. + * `network_interface` - (Required) Networks to attach to instances created from this template. This can be specified multiple times for multiple networks. Structure is documented below.