From bc170b9b040232bf33a410befa628de2ad55f736 Mon Sep 17 00:00:00 2001 From: Matthias Rampke Date: Mon, 1 Feb 2021 17:02:05 +0000 Subject: [PATCH 1/4] aws_instance: acceptance test for gp3 with iops and throughput I am suspecting a bug when both iops and throughput are specified on an inline EBS block device. Test the expected behavior. Signed-off-by: Matthias Rampke --- aws/resource_aws_instance_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_instance_test.go b/aws/resource_aws_instance_test.go index 4fc3687923c..020f546b2ef 100644 --- a/aws/resource_aws_instance_test.go +++ b/aws/resource_aws_instance_test.go @@ -513,6 +513,10 @@ func TestAccAWSInstance_blockDevices(t *testing.T) { return fmt.Errorf("block device doesn't exist: /dev/sdf") } + if _, ok := blockDevices["/dev/sdg"]; !ok { + return fmt.Errorf("block device doesn't exist: /dev/sdg") + } + return nil } } @@ -534,7 +538,7 @@ func TestAccAWSInstance_blockDevices(t *testing.T) { resource.TestMatchResourceAttr(resourceName, "root_block_device.0.volume_id", regexp.MustCompile("vol-[a-z0-9]+")), resource.TestCheckResourceAttr(resourceName, "root_block_device.0.volume_size", rootVolumeSize), resource.TestCheckResourceAttr(resourceName, "root_block_device.0.volume_type", "gp2"), - resource.TestCheckResourceAttr(resourceName, "ebs_block_device.#", "4"), + resource.TestCheckResourceAttr(resourceName, "ebs_block_device.#", "5"), resource.TestCheckTypeSetElemNestedAttrs(resourceName, "ebs_block_device.*", map[string]string{ "device_name": "/dev/sdb", "volume_size": "9", @@ -555,6 +559,13 @@ func TestAccAWSInstance_blockDevices(t *testing.T) { "volume_type": "gp3", "throughput": "300", }), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "ebs_block_device.*", map[string]string{ + "device_name": "/dev/sdg", + "volume_size": "10", + "volume_type": "gp3", + "throughput": "300", + "iops": "4000", + }), resource.TestMatchTypeSetElemNestedAttrs(resourceName, "ebs_block_device.*", map[string]*regexp.Regexp{ "volume_id": regexp.MustCompile("vol-[a-z0-9]+"), }), @@ -3871,6 +3882,14 @@ resource "aws_instance" "test" { throughput = 300 } + ebs_block_device { + device_name = "/dev/sdg" + volume_size = 10 + volume_type = "gp3" + throughput = 300 + iops = 4000 + } + } `, size, delete)) } From 74b2e96b8c3cfe98978d0faa87e0b44a05abfe56 Mon Sep 17 00:00:00 2001 From: Matthias Rampke Date: Tue, 2 Feb 2021 15:07:04 +0000 Subject: [PATCH 2/4] aws_instance: acceptance test for gp3 root w/ iops and throughput as requested by @ewbankkit: https://github.com/hashicorp/terraform-provider-aws/pull/17380#issuecomment-771696906 Signed-off-by: Matthias Rampke --- aws/resource_aws_instance_test.go | 65 +++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/aws/resource_aws_instance_test.go b/aws/resource_aws_instance_test.go index 020f546b2ef..6a7f04d24b1 100644 --- a/aws/resource_aws_instance_test.go +++ b/aws/resource_aws_instance_test.go @@ -3807,6 +3807,71 @@ resource "aws_instance" "test" { `, size, delete, volumeType, throughput)) } +func TestAccAWSInstance_GP3RootBlockDevice(t *testing.T) { + var v ec2.Instance + resourceName := "aws_instance.test" + + testCheck := func() resource.TestCheckFunc { + return func(*terraform.State) error { + // Map out the block devices by name, which should be unique. + blockDevices := make(map[string]*ec2.InstanceBlockDeviceMapping) + for _, blockDevice := range v.BlockDeviceMappings { + blockDevices[*blockDevice.DeviceName] = blockDevice + } + + // Check if the root block device exists. + if _, ok := blockDevices["/dev/xvda"]; !ok { + return fmt.Errorf("block device doesn't exist: /dev/xvda") + } + + return nil + } + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IDRefreshName: resourceName, + IDRefreshIgnore: []string{"ephemeral_block_device", "user_data"}, + Providers: testAccProviders, + CheckDestroy: testAccCheckInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccInstanceConfigGP3RootBlockDevice(), + Check: resource.ComposeTestCheckFunc( + testAccCheckInstanceExists(resourceName, &v), + resource.TestCheckResourceAttr(resourceName, "root_block_device.#", "1"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.volume_size", "10"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.volume_type", "gp3"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.iops", "4000"), + resource.TestCheckResourceAttr(resourceName, "root_block_device.0.throughput", "300"), + testCheck(), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testAccInstanceConfigGP3RootBlockDevice() string { + return composeConfig(testAccLatestAmazonLinuxHvmEbsAmiConfig(), ` +resource "aws_instance" "test" { + ami = data.aws_ami.amzn-ami-minimal-hvm-ebs.id + instance_type = "t2.medium" + + root_block_device { + volume_size = 10 + volume_type = "gp3" + throughput = 300 + iops = 4000 + } +} +`) +} + const testAccAwsEc2InstanceAmiWithEbsRootVolume = ` data "aws_ami" "ami" { owners = ["amazon"] From 9321d5d7a31b424bc6f7e50a6d51a14931c2fd33 Mon Sep 17 00:00:00 2001 From: Matthias Rampke Date: Mon, 1 Feb 2021 17:08:56 +0000 Subject: [PATCH 3/4] aws_instance: fix for setting iops+throughput at the same time When both `iops` and `throughput` are specified on an inline block device (`root_block_device` or `ebs_block_device`), both attributes should be honored. With the `else if`, specifying `iops` would make the `throughput` attribute a NOOP during instance creation. The desired throughput shows up correctly in the plan, but after apply, it is actually the default (125) in the state and in EC2. Signed-off-by: Matthias Rampke --- aws/resource_aws_instance.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/aws/resource_aws_instance.go b/aws/resource_aws_instance.go index 7e69c85f6c3..1a649184a94 100644 --- a/aws/resource_aws_instance.go +++ b/aws/resource_aws_instance.go @@ -1987,7 +1987,8 @@ func readBlockDeviceMappingsFromConfig(d *schema.ResourceData, conn *ec2.EC2) ([ // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/12667 return nil, fmt.Errorf("error creating resource: iops attribute not supported for ebs_block_device with volume_type %s", v) } - } else if throughput, ok := bd["throughput"].(int); ok && throughput > 0 { + } + if throughput, ok := bd["throughput"].(int); ok && throughput > 0 { // `throughput` is only valid for gp3 if ec2.VolumeTypeGp3 == strings.ToLower(v) { ebs.Throughput = aws.Int64(int64(throughput)) @@ -2061,7 +2062,8 @@ func readBlockDeviceMappingsFromConfig(d *schema.ResourceData, conn *ec2.EC2) ([ // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/12667 return nil, fmt.Errorf("error creating resource: iops attribute not supported for root_block_device with volume_type %s", v) } - } else if throughput, ok := bd["throughput"].(int); ok && throughput > 0 { + } + if throughput, ok := bd["throughput"].(int); ok && throughput > 0 { // throughput is only valid for gp3 if ec2.VolumeTypeGp3 == strings.ToLower(v) { ebs.Throughput = aws.Int64(int64(throughput)) From 9b9f23a90a9ca2e18acf43690bfe4e246c900e60 Mon Sep 17 00:00:00 2001 From: Matthias Rampke Date: Tue, 2 Feb 2021 07:39:48 +0000 Subject: [PATCH 4/4] Add changelog for #17380 Signed-off-by: Matthias Rampke --- .changelog/17380.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/17380.txt diff --git a/.changelog/17380.txt b/.changelog/17380.txt new file mode 100644 index 00000000000..dd744dfeb3c --- /dev/null +++ b/.changelog/17380.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_instance: Fix use of `throughput` and `iops` for `gp3` volumes at the same time +```