From d1bba3095b8bac5e0d32b53024fb8e6a656de078 Mon Sep 17 00:00:00 2001 From: clint shryock Date: Wed, 2 Dec 2015 14:19:19 -0600 Subject: [PATCH 1/3] providers/aws: Validate IOPs for EBS Volumes --- .../providers/aws/resource_aws_ebs_volume.go | 29 ++++++++++++++----- .../docs/providers/aws/r/ebs_volume.html.md | 2 +- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/builtin/providers/aws/resource_aws_ebs_volume.go b/builtin/providers/aws/resource_aws_ebs_volume.go index 1680b4f533e0..0e016ecaac98 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume.go +++ b/builtin/providers/aws/resource_aws_ebs_volume.go @@ -37,6 +37,14 @@ func resourceAwsEbsVolume() *schema.Resource { Optional: true, Computed: true, ForceNew: true, + ValidateFunc: func(v interface{}, k string) (ws []string, es []error) { + value := v.(int) + if value < 100 { + es = append(es, fmt.Errorf( + "%q must be an integer, minimum value 100", k)) + } + return + }, }, "kms_key_id": &schema.Schema{ Type: schema.TypeString, @@ -76,9 +84,6 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error if value, ok := d.GetOk("encrypted"); ok { request.Encrypted = aws.Bool(value.(bool)) } - if value, ok := d.GetOk("iops"); ok { - request.Iops = aws.Int64(int64(value.(int))) - } if value, ok := d.GetOk("kms_key_id"); ok { request.KmsKeyId = aws.String(value.(string)) } @@ -88,18 +93,28 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error if value, ok := d.GetOk("snapshot_id"); ok { request.SnapshotId = aws.String(value.(string)) } + var t string if value, ok := d.GetOk("type"); ok { - request.VolumeType = aws.String(value.(string)) + t = value.(string) + request.VolumeType = aws.String(t) + } + if value, ok := d.GetOk("iops"); ok { + if t == "io1" { + request.Iops = aws.Int64(int64(value.(int))) + } else { + return fmt.Errorf("iops is only valid for EBS Volume of type io1") + } } + log.Printf( + "[DEBUG] EBS Volume create opts: %s", request) result, err := conn.CreateVolume(request) if err != nil { return fmt.Errorf("Error creating EC2 volume: %s", err) } - log.Printf( - "[DEBUG] Waiting for Volume (%s) to become available", - d.Id()) + log.Println( + "[DEBUG] Waiting for Volume to become available") stateConf := &resource.StateChangeConf{ Pending: []string{"creating"}, diff --git a/website/source/docs/providers/aws/r/ebs_volume.html.md b/website/source/docs/providers/aws/r/ebs_volume.html.md index 00bb639a6a77..8a41ea26b5ac 100644 --- a/website/source/docs/providers/aws/r/ebs_volume.html.md +++ b/website/source/docs/providers/aws/r/ebs_volume.html.md @@ -14,7 +14,7 @@ Manages a single EBS volume. ``` resource "aws_ebs_volume" "example" { - availability_zone = "us-west-1a" + availability_zone = "us-west-2a" size = 40 tags { Name = "HelloWorld" From 5e54bcc6ffa0cc1ff684ded0698dbf1cd639f505 Mon Sep 17 00:00:00 2001 From: clint shryock Date: Mon, 7 Dec 2015 11:16:29 -0600 Subject: [PATCH 2/3] Add test for iops with gp2, remove strict validation --- .../providers/aws/resource_aws_ebs_volume.go | 8 ------ .../aws/resource_aws_ebs_volume_test.go | 28 +++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/builtin/providers/aws/resource_aws_ebs_volume.go b/builtin/providers/aws/resource_aws_ebs_volume.go index 0e016ecaac98..856c94871e5b 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume.go +++ b/builtin/providers/aws/resource_aws_ebs_volume.go @@ -37,14 +37,6 @@ func resourceAwsEbsVolume() *schema.Resource { Optional: true, Computed: true, ForceNew: true, - ValidateFunc: func(v interface{}, k string) (ws []string, es []error) { - value := v.(int) - if value < 100 { - es = append(es, fmt.Errorf( - "%q must be an integer, minimum value 100", k)) - } - return - }, }, "kms_key_id": &schema.Schema{ Type: schema.TypeString, diff --git a/builtin/providers/aws/resource_aws_ebs_volume_test.go b/builtin/providers/aws/resource_aws_ebs_volume_test.go index aab92eb01122..fabcdb1a1109 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume_test.go +++ b/builtin/providers/aws/resource_aws_ebs_volume_test.go @@ -26,6 +26,22 @@ func TestAccAWSEBSVolume_basic(t *testing.T) { }) } +func TestAccAWSEBSVolume_Iops(t *testing.T) { + var v ec2.Volume + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAwsEbsVolumeConfigWithIops, + Check: resource.ComposeTestCheckFunc( + testAccCheckVolumeExists("aws_ebs_volume.iops_test", &v), + ), + }, + }, + }) +} + func TestAccAWSEBSVolume_withTags(t *testing.T) { var v ec2.Volume resource.Test(t, resource.TestCase{ @@ -86,3 +102,15 @@ resource "aws_ebs_volume" "tags_test" { } } ` + +const testAccAwsEbsVolumeConfigWithIops = ` +resource "aws_ebs_volume" "iops_test" { + availability_zone = "us-west-2a" + size = 10 + type = "gp2" + iops = 0 + tags { + Name = "TerraformTest" + } +} +` From 7bf404619c0ffaa756f22e58769b1f62327802ed Mon Sep 17 00:00:00 2001 From: clint shryock Date: Mon, 7 Dec 2015 14:49:44 -0600 Subject: [PATCH 3/3] adjust the ebs validation to not error, only log, and only set iops for io1 --- .../providers/aws/resource_aws_ebs_volume.go | 33 ++++++++++++++----- .../aws/resource_aws_ebs_volume_test.go | 6 ++-- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/builtin/providers/aws/resource_aws_ebs_volume.go b/builtin/providers/aws/resource_aws_ebs_volume.go index 856c94871e5b..3046ac46c6d8 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume.go +++ b/builtin/providers/aws/resource_aws_ebs_volume.go @@ -85,17 +85,24 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error if value, ok := d.GetOk("snapshot_id"); ok { request.SnapshotId = aws.String(value.(string)) } + + // IOPs are only valid, and required for, storage type io1. The current minimu + // is 100. Instead of a hard validation we we only apply the IOPs to the + // request if the type is io1, and log a warning otherwise. This allows users + // to "disable" iops. See https://github.com/hashicorp/terraform/pull/4146 var t string if value, ok := d.GetOk("type"); ok { t = value.(string) request.VolumeType = aws.String(t) } - if value, ok := d.GetOk("iops"); ok { - if t == "io1" { - request.Iops = aws.Int64(int64(value.(int))) - } else { - return fmt.Errorf("iops is only valid for EBS Volume of type io1") - } + + iops := d.Get("iops").(int) + if t != "io1" && iops > 0 { + log.Printf("[WARN] IOPs is only valid for storate type io1 for EBS Volumes") + } else if t == "io1" { + // We add the iops value without validating it's size, to allow AWS to + // enforce a size requirement (currently 100) + request.Iops = aws.Int64(int64(iops)) } log.Printf( @@ -206,9 +213,6 @@ func readVolume(d *schema.ResourceData, volume *ec2.Volume) error { if volume.Encrypted != nil { d.Set("encrypted", *volume.Encrypted) } - if volume.Iops != nil { - d.Set("iops", *volume.Iops) - } if volume.KmsKeyId != nil { d.Set("kms_key_id", *volume.KmsKeyId) } @@ -221,6 +225,17 @@ func readVolume(d *schema.ResourceData, volume *ec2.Volume) error { if volume.VolumeType != nil { d.Set("type", *volume.VolumeType) } + + if volume.VolumeType != nil && *volume.VolumeType == "io1" { + // Only set the iops attribute if the volume type is io1. Setting otherwise + // can trigger a refresh/plan loop based on the computed value that is given + // from AWS, and prevent us from specifying 0 as a valid iops. + // See https://github.com/hashicorp/terraform/pull/4146 + if volume.Iops != nil { + d.Set("iops", *volume.Iops) + } + } + if volume.Tags != nil { d.Set("tags", tagsToMap(volume.Tags)) } diff --git a/builtin/providers/aws/resource_aws_ebs_volume_test.go b/builtin/providers/aws/resource_aws_ebs_volume_test.go index fabcdb1a1109..940c8157cabf 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume_test.go +++ b/builtin/providers/aws/resource_aws_ebs_volume_test.go @@ -26,14 +26,14 @@ func TestAccAWSEBSVolume_basic(t *testing.T) { }) } -func TestAccAWSEBSVolume_Iops(t *testing.T) { +func TestAccAWSEBSVolume_NoIops(t *testing.T) { var v ec2.Volume resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, Steps: []resource.TestStep{ resource.TestStep{ - Config: testAccAwsEbsVolumeConfigWithIops, + Config: testAccAwsEbsVolumeConfigWithNoIops, Check: resource.ComposeTestCheckFunc( testAccCheckVolumeExists("aws_ebs_volume.iops_test", &v), ), @@ -103,7 +103,7 @@ resource "aws_ebs_volume" "tags_test" { } ` -const testAccAwsEbsVolumeConfigWithIops = ` +const testAccAwsEbsVolumeConfigWithNoIops = ` resource "aws_ebs_volume" "iops_test" { availability_zone = "us-west-2a" size = 10