Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/google: Fix instance/template metadata support #10537

Merged
merged 1 commit into from
Dec 6, 2016
Merged

provider/google: Fix instance/template metadata support #10537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions builtin/providers/google/resource_compute_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
89 changes: 70 additions & 19 deletions builtin/providers/google/resource_compute_instance_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
Expand Down
68 changes: 68 additions & 0 deletions builtin/providers/google/resource_compute_instance_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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))
5 changes: 5 additions & 0 deletions website/source/docs/providers/google/r/compute_instance_template.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👣 🔫

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍


* `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.
Expand Down