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.

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

Add boot_disk property to google_compute_instance #122

Merged
merged 5 commits into from
Jun 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
194 changes: 186 additions & 8 deletions google/resource_compute_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"

"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
)
Expand All @@ -26,6 +27,86 @@ func resourceComputeInstance() *schema.Resource {
MigrateState: resourceComputeInstanceMigrateState,

Schema: map[string]*schema.Schema{
"boot_disk": &schema.Schema{
Type: schema.TypeList,
Optional: true,
ForceNew: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"auto_delete": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Default: true,
ForceNew: true,
},

"device_name": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be ForceNew?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Fixed.

Copy link

Choose a reason for hiding this comment

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

The Forcenew:true would cause the current resource such as a db instance to be recreated. is this desired? maybe I am asking a terraform basic question instead of a google provider one here. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that if the field in question changes in value then it would, yes.

ForceNew: true,
},

"disk_encryption_key_raw": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Sensitive: true,
},

"disk_encryption_key_sha256": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},

"initialize_params": &schema.Schema{
Type: schema.TypeList,
Optional: true,
ForceNew: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"size": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
if v.(int) < 1 {
errors = append(errors, fmt.Errorf(
"%q must be greater than 0", k))
}
return
},
},

"type": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{"pd-standard", "pd-ssd"}, false),
},

"image": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
},
},
},

"source": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
ConflictsWith: []string{"boot_disk.initialize_params"},
},
},
},
},

"disk": &schema.Schema{
Type: schema.TypeList,
Optional: true,
Expand Down Expand Up @@ -407,12 +488,23 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
}

// Build up the list of disks

disks := []*compute.AttachedDisk{}
var hasBootDisk bool
if _, hasBootDisk = d.GetOk("boot_disk"); hasBootDisk {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if _, hasBootDisk := means you don't need var hasBootDisk bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need it so I can refer to it outside of the if statement

bootDisk, err := expandBootDisk(d, config, zone, project)
if err != nil {
return err
}
disks = append(disks, bootDisk)
}

disksCount := d.Get("disk.#").(int)
attachedDisksCount := d.Get("attached_disk.#").(int)
if disksCount+attachedDisksCount == 0 {
return fmt.Errorf("At least one disk or attached_disk must be set")

if disksCount+attachedDisksCount == 0 && !hasBootDisk {
return fmt.Errorf("At least one disk, attached_disk, or boot_disk must be set")
}
disks := make([]*compute.AttachedDisk, 0, disksCount+attachedDisksCount)
for i := 0; i < disksCount; i++ {
prefix := fmt.Sprintf("disk.%d", i)

Expand All @@ -422,7 +514,7 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
var disk compute.AttachedDisk
disk.Type = "PERSISTENT"
disk.Mode = "READ_WRITE"
disk.Boot = i == 0
disk.Boot = i == 0 && !hasBootDisk
disk.AutoDelete = d.Get(prefix + ".auto_delete").(bool)

if _, ok := d.GetOk(prefix + ".disk"); ok {
Expand Down Expand Up @@ -513,7 +605,7 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
AutoDelete: false, // Don't allow autodelete; let terraform handle disk deletion
}

disk.Boot = i == 0 && disksCount == 0 // TODO(danawillow): This is super hacky, let's just add a boot field.
disk.Boot = i == 0 && disksCount == 0 && !hasBootDisk

if v, ok := d.GetOk(prefix + ".device_name"); ok {
disk.DeviceName = v.(string)
Expand Down Expand Up @@ -868,9 +960,10 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error

disksCount := d.Get("disk.#").(int)
attachedDisksCount := d.Get("attached_disk.#").(int)
disks := make([]map[string]interface{}, 0, disksCount)
attachedDisks := make([]map[string]interface{}, 0, attachedDisksCount)

if _, ok := d.GetOk("boot_disk"); ok {
disksCount++
}
if expectedDisks := disksCount + attachedDisksCount; len(instance.Disks) != expectedDisks {
return fmt.Errorf("Expected %d disks, API returned %d", expectedDisks, len(instance.Disks))
}
Expand All @@ -882,8 +975,14 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error

dIndex := 0
adIndex := 0
disks := make([]map[string]interface{}, 0, disksCount)
attachedDisks := make([]map[string]interface{}, 0, attachedDisksCount)
for _, disk := range instance.Disks {
if _, ok := attachedDiskSources[disk.Source]; !ok {
if _, ok := d.GetOk("boot_disk"); ok && disk.Boot {
// This disk is a boot disk and there is a boot disk set in the config, therefore
// this is the boot disk set in the config.
d.Set("boot_disk", flattenBootDisk(d, disk))
} else if _, ok := attachedDiskSources[disk.Source]; !ok {
di := map[string]interface{}{
"disk": d.Get(fmt.Sprintf("disk.%d.disk", dIndex)),
"image": d.Get(fmt.Sprintf("disk.%d.image", dIndex)),
Expand Down Expand Up @@ -1193,3 +1292,82 @@ func resourceInstanceTags(d *schema.ResourceData) *compute.Tags {

return tags
}

func expandBootDisk(d *schema.ResourceData, config *Config, zone *compute.Zone, project string) (*compute.AttachedDisk, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we only use schema elements that are properties of boot_disk.0; we can reduce the information this function needs by passing in a limited set of data, like BackendService's expandBackends()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with that approach is that it auto-populates all the map keys, and sets them to default values. So if you want to check the existence of something, you have to check against "", 0, false, and nil, rather than being able to use a consistent d.GetOk("whatever"); ok. It seems like you should be able to just do v, ok := data["whatever"]; ok, but that will return true for elements not set in the config, which is a problem when you're then trying to use that value in another call, like in line 1274. This way feels less bug-prone.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense, thanks!

disk := &compute.AttachedDisk{
AutoDelete: d.Get("boot_disk.0.auto_delete").(bool),
Boot: true,
}

if v, ok := d.GetOk("boot_disk.0.device_name"); ok {
disk.DeviceName = v.(string)
}

if v, ok := d.GetOk("boot_disk.0.disk_encryption_key_raw"); ok {
disk.DiskEncryptionKey = &compute.CustomerEncryptionKey{
RawKey: v.(string),
}
}

if v, ok := d.GetOk("boot_disk.0.source"); ok {
diskName := v.(string)
diskData, err := config.clientCompute.Disks.Get(
project, zone.Name, diskName).Do()
if err != nil {
return nil, fmt.Errorf("Error loading disk '%s': %s", diskName, err)
}
disk.Source = diskData.SelfLink
}

if _, ok := d.GetOk("boot_disk.0.initialize_params"); ok {
disk.InitializeParams = &compute.AttachedDiskInitializeParams{}

if v, ok := d.GetOk("boot_disk.0.initialize_params.0.size"); ok {
disk.InitializeParams.DiskSizeGb = int64(v.(int))
}

if v, ok := d.GetOk("boot_disk.0.initialize_params.0.type"); ok {
diskTypeName := v.(string)
diskType, err := readDiskType(config, zone, diskTypeName)
if err != nil {
return nil, fmt.Errorf("Error loading disk type '%s': %s", diskTypeName, err)
}
disk.InitializeParams.DiskType = diskType.Name
}

if v, ok := d.GetOk("boot_disk.0.initialize_params.0.image"); ok {
imageName := v.(string)
imageUrl, err := resolveImage(config, imageName)
if err != nil {
return nil, fmt.Errorf("Error resolving image name '%s': %s", imageName, err)
}

disk.InitializeParams.SourceImage = imageUrl
}
}

return disk, nil
}

func flattenBootDisk(d *schema.ResourceData, disk *compute.AttachedDisk) []map[string]interface{} {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as expandBootDisk - do we need access to all of schema.ResourceData here? Or just a subset?

sourceUrl := strings.Split(disk.Source, "/")
result := map[string]interface{}{
"auto_delete": disk.AutoDelete,
"device_name": disk.DeviceName,
"source": sourceUrl[len(sourceUrl)-1],
// disk_encryption_key_raw is not returned from the API, so don't store it in state.
// If necessary in the future, this can be copied from what the user originally specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in other places we do the fingerprint of the key. Is that returned from the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, added

}
if disk.DiskEncryptionKey != nil {
result["disk_encryption_key_sha256"] = disk.DiskEncryptionKey.Sha256
}
if v, ok := d.GetOk("boot_disk.0.initialize_params.#"); ok {
result["initialize_params.#"] = v.(int)
// initialize_params is not returned from the API, so don't store its values in state.
// If necessary in the future, this can be copied from what the user originally specified.
// However, because Terraform automatically sets `boot_disk.0.initialize_params.#` to 0 if
// nothing is set in state for it, set it to whatever it was set to before to avoid a perpetual diff.
}

return []map[string]interface{}{result}
}
110 changes: 108 additions & 2 deletions google/resource_compute_instance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,49 @@ func TestAccComputeInstance_attachedDisk(t *testing.T) {
})
}

func TestAccComputeInstance_bootDisk(t *testing.T) {
var instance compute.Instance
var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckComputeInstanceDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccComputeInstance_bootDisk(instanceName),
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeInstanceExists(
"google_compute_instance.foobar", &instance),
testAccCheckComputeInstanceBootDisk(&instance, instanceName),
),
},
},
})
}

func TestAccComputeInstance_bootDisk_source(t *testing.T) {
var instance compute.Instance
var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))
var diskName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckComputeInstanceDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccComputeInstance_bootDisk_source(diskName, instanceName),
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeInstanceExists(
"google_compute_instance.foobar", &instance),
testAccCheckComputeInstanceBootDisk(&instance, diskName),
),
},
},
})
}

func TestAccComputeInstance_noDisk(t *testing.T) {
var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10))

Expand All @@ -277,7 +320,7 @@ func TestAccComputeInstance_noDisk(t *testing.T) {
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccComputeInstance_noDisk(instanceName),
ExpectError: regexp.MustCompile("At least one disk or attached_disk must be set"),
ExpectError: regexp.MustCompile("At least one disk, attached_disk, or boot_disk must be set"),
},
},
})
Expand Down Expand Up @@ -751,7 +794,7 @@ func testAccCheckComputeInstanceDisk(instance *compute.Instance, source string,
}

for _, disk := range instance.Disks {
if strings.LastIndex(disk.Source, "/"+source) == len(disk.Source)-len(source)-1 && disk.AutoDelete == delete && disk.Boot == boot {
if strings.HasSuffix(disk.Source, source) && disk.AutoDelete == delete && disk.Boot == boot {
return nil
}
}
Expand All @@ -760,6 +803,24 @@ func testAccCheckComputeInstanceDisk(instance *compute.Instance, source string,
}
}

func testAccCheckComputeInstanceBootDisk(instance *compute.Instance, source string) resource.TestCheckFunc {
return func(s *terraform.State) error {
if instance.Disks == nil {
return fmt.Errorf("no disks")
}

for _, disk := range instance.Disks {
if disk.Boot == true {
if strings.HasSuffix(disk.Source, source) {
return nil
}
}
}

return fmt.Errorf("Boot disk not found with source %q", source)
}
}

func testAccCheckComputeInstanceDiskEncryptionKey(n string, instance *compute.Instance) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -1232,6 +1293,51 @@ resource "google_compute_instance" "foobar" {
`, disk, instance)
}

func testAccComputeInstance_bootDisk(instance string) string {
return fmt.Sprintf(`
resource "google_compute_instance" "foobar" {
name = "%s"
machine_type = "n1-standard-1"
zone = "us-central1-a"

boot_disk {
initialize_params {
image = "debian-8-jessie-v20160803"
}
disk_encryption_key_raw = "SGVsbG8gZnJvbSBHb29nbGUgQ2xvdWQgUGxhdGZvcm0="
}

network_interface {
network = "default"
}
}
`, instance)
}

func testAccComputeInstance_bootDisk_source(disk, instance string) string {
return fmt.Sprintf(`
resource "google_compute_disk" "foobar" {
name = "%s"
zone = "us-central1-a"
image = "debian-8-jessie-v20160803"
}

resource "google_compute_instance" "foobar" {
name = "%s"
machine_type = "n1-standard-1"
zone = "us-central1-a"

boot_disk {
source = "${google_compute_disk.foobar.name}"
}

network_interface {
network = "default"
}
}
`, disk, instance)
}

func testAccComputeInstance_noDisk(instance string) string {
return fmt.Sprintf(`
resource "google_compute_instance" "foobar" {
Expand Down
Loading