From a544d5ef421635d5bb91666913b9396b27b581ba Mon Sep 17 00:00:00 2001 From: "grace.gao" Date: Mon, 24 Jul 2023 19:52:34 +1000 Subject: [PATCH 1/7] r/aws_elasticsearch_domain: omit iops/throughput when not supported --- internal/service/elasticsearch/domain.go | 29 ++++ internal/service/elasticsearch/domain_test.go | 144 +++++++++++++++++- internal/service/elasticsearch/flex.go | 17 ++- 3 files changed, 180 insertions(+), 10 deletions(-) diff --git a/internal/service/elasticsearch/domain.go b/internal/service/elasticsearch/domain.go index 3bd96669e00d..f16ffde3f8ab 100644 --- a/internal/service/elasticsearch/domain.go +++ b/internal/service/elasticsearch/domain.go @@ -12,6 +12,7 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/elasticsearchservice" elasticsearch "github.com/aws/aws-sdk-go/service/elasticsearchservice" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" awspolicy "github.com/hashicorp/awspolicyequivalence" @@ -1249,3 +1250,31 @@ func advancedOptionsIgnoreDefault(o map[string]interface{}, n map[string]interfa return n } + +// EBSVolumeTypePermitsIopsInput returns true if the volume type supports the Iops input +// +// This check prevents a ValidationException when updating EBS volume types from a value +// that supports IOPS (ex. gp3) to one that doesn't (ex. gp2). +func EBSVolumeTypePermitsIopsInput(volumeType string) bool { + permittedTypes := []string{elasticsearchservice.VolumeTypeGp3, elasticsearchservice.VolumeTypeIo1} + for _, t := range permittedTypes { + if volumeType == t { + return true + } + } + return false +} + +// EBSVolumeTypePermitsIopsInput returns true if the volume type supports the Throughput input +// +// This check prevents a ValidationException when updating EBS volume types from a value +// that supports Throughput (ex. gp3) to one that doesn't (ex. gp2). +func EBSVolumeTypePermitsThroughputInput(volumeType string) bool { + permittedTypes := []string{elasticsearchservice.VolumeTypeGp3} + for _, t := range permittedTypes { + if volumeType == t { + return true + } + } + return false +} diff --git a/internal/service/elasticsearch/domain_test.go b/internal/service/elasticsearch/domain_test.go index 872494da9968..9a0727050c46 100644 --- a/internal/service/elasticsearch/domain_test.go +++ b/internal/service/elasticsearch/domain_test.go @@ -12,7 +12,9 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/cognitoidentityprovider" + "github.com/aws/aws-sdk-go/service/elasticsearchservice" elasticsearch "github.com/aws/aws-sdk-go/service/elasticsearchservice" + "github.com/aws/aws-sdk-go/service/opensearchservice" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" @@ -22,6 +24,58 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) +func TestEBSVolumeTypePermitsIopsInput(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + volumeType string + want bool + }{ + {"empty", "", false}, + {"gp2", elasticsearchservice.VolumeTypeGp2, false}, + {"gp3", elasticsearchservice.VolumeTypeGp3, true}, + {"io1", elasticsearchservice.VolumeTypeIo1, true}, + {"standard", elasticsearchservice.VolumeTypeStandard, false}, + } + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + if got := tfelasticsearch.EBSVolumeTypePermitsIopsInput(testCase.volumeType); got != testCase.want { + t.Errorf("EBSVolumeTypePermitsIopsInput() = %v, want %v", got, testCase.want) + } + }) + } +} + +func TestEBSVolumeTypePermitsThroughputInput(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + volumeType string + want bool + }{ + {"empty", "", false}, + {"gp2", elasticsearchservice.VolumeTypeGp2, false}, + {"gp3", elasticsearchservice.VolumeTypeGp3, true}, + {"io1", elasticsearchservice.VolumeTypeIo1, false}, + {"standard", elasticsearchservice.VolumeTypeStandard, false}, + } + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + if got := tfelasticsearch.EBSVolumeTypePermitsThroughputInput(testCase.volumeType); got != testCase.want { + t.Errorf("EBSVolumeTypePermitsThroughputInput() = %v, want %v", got, testCase.want) + } + }) + } +} + func TestAccElasticsearchDomain_basic(t *testing.T) { ctx := acctest.Context(t) if testing.Short() { @@ -1429,6 +1483,55 @@ func TestAccElasticsearchDomain_VolumeType_update(t *testing.T) { }}) } +// Verifies that EBS volume_type can be changed from gp3 to a type which does not +// support the throughput and iops input values (ex. gp2) +// +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/32613 +func TestAccElasticsearchDomain_VolumeType_gp3ToGP2(t *testing.T) { + ctx := acctest.Context(t) + if testing.Short() { + t.Skip("skipping long-running test in short mode") + } + + var input elasticsearch.ElasticsearchDomainStatus + rName := testAccRandomDomainName() + resourceName := "aws_elasticsearch_domain.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheckIAMServiceLinkedRole(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, opensearchservice.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckDomainDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccDomainConfig_clusterEBSVolumeGP3DefaultIopsThroughput(rName, 10), + Check: resource.ComposeTestCheckFunc( + testAccCheckDomainExists(ctx, resourceName, &input), + resource.TestCheckResourceAttr(resourceName, "ebs_options.#", "1"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.ebs_enabled", "true"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.volume_size", "10"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.volume_type", "gp3"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateId: rName, + ImportStateVerify: true, + }, + { + Config: testAccDomainConfig_clusterEBSVolumeGP2(rName, 10), + Check: resource.ComposeTestCheckFunc( + testAccCheckDomainExists(ctx, resourceName, &input), + resource.TestCheckResourceAttr(resourceName, "ebs_options.#", "1"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.ebs_enabled", "true"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.volume_size", "10"), + resource.TestCheckResourceAttr(resourceName, "ebs_options.0.volume_type", "gp2"), + ), + }, + }}) +} + // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/13867 func TestAccElasticsearchDomain_VolumeType_missing(t *testing.T) { if testing.Short() { @@ -2117,6 +2220,41 @@ resource "aws_elasticsearch_domain" "test" { `, rName, volumeSize, volumeThroughput, volumeIops) } +func testAccDomainConfig_clusterEBSVolumeGP3DefaultIopsThroughput(rName string, volumeSize int) string { + return fmt.Sprintf(` +resource "aws_elasticsearch_domain" "test" { + domain_name = %[1]q + elasticsearch_version = "7.10" + ebs_options { + ebs_enabled = true + volume_size = %[2]d + volume_type = "gp3" + + } + cluster_config { + instance_type = "t3.small.elasticsearch" + } +} +`, rName, volumeSize) +} + +func testAccDomainConfig_clusterEBSVolumeGP2(rName string, volumeSize int) string { + return fmt.Sprintf(` +resource "aws_elasticsearch_domain" "test" { + domain_name = %[1]q + elasticsearch_version = "7.10" + ebs_options { + ebs_enabled = true + volume_size = %[2]d + volume_type = "gp2" + } + cluster_config { + instance_type = "t3.small.elasticsearch" + } +} +`, rName, volumeSize) +} + func testAccDomainConfig_clusterUpdateVersion(rName, version string) string { return fmt.Sprintf(` resource "aws_elasticsearch_domain" "test" { @@ -2915,16 +3053,16 @@ func testAccDomainConfig_logPublishingOptions(rName, logType string) string { master_user_password = "Barbarbarbar1!" } } - + domain_endpoint_options { enforce_https = true tls_security_policy = "Policy-Min-TLS-1-2-2019-07" } - + encrypt_at_rest { enabled = true } - + node_to_node_encryption { enabled = true }` diff --git a/internal/service/elasticsearch/flex.go b/internal/service/elasticsearch/flex.go index 85038af5b042..839f10564e8e 100644 --- a/internal/service/elasticsearch/flex.go +++ b/internal/service/elasticsearch/flex.go @@ -79,18 +79,21 @@ func expandEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions { options.EBSEnabled = aws.Bool(ebsEnabled.(bool)) if ebsEnabled.(bool) { - if v, ok := m["iops"]; ok && v.(int) > 0 { - options.Iops = aws.Int64(int64(v.(int))) - } - if v, ok := m["throughput"]; ok && v.(int) > 0 { - options.Throughput = aws.Int64(int64(v.(int))) - } if v, ok := m["volume_size"]; ok && v.(int) > 0 { options.VolumeSize = aws.Int64(int64(v.(int))) } + var volumeType string if v, ok := m["volume_type"]; ok && v.(string) != "" { - options.VolumeType = aws.String(v.(string)) + volumeType = v.(string) + options.VolumeType = aws.String(volumeType) } + if v, ok := m["iops"]; ok && v.(int) > 0 && EBSVolumeTypePermitsIopsInput(volumeType) { + options.Iops = aws.Int64(int64(v.(int))) + } + if v, ok := m["throughput"]; ok && v.(int) > 0 && EBSVolumeTypePermitsThroughputInput(volumeType) { + options.Throughput = aws.Int64(int64(v.(int))) + } + } } From af8a069050621ff362de395ffd5ea55d4bf9e010 Mon Sep 17 00:00:00 2001 From: "grace.gao" Date: Mon, 24 Jul 2023 20:06:52 +1000 Subject: [PATCH 2/7] add changelog --- .changelog/32659.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/32659.txt diff --git a/.changelog/32659.txt b/.changelog/32659.txt new file mode 100644 index 000000000000..cbf0c765a591 --- /dev/null +++ b/.changelog/32659.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_elasticsearch_domain: Omit `throughput` and `iops` for unsupported volume types +``` \ No newline at end of file From d799524fb5fb66047af94db9e5437c4235dbdde8 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 25 Jul 2023 08:44:59 -0400 Subject: [PATCH 3/7] Update 32659.txt --- .changelog/32659.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changelog/32659.txt b/.changelog/32659.txt index cbf0c765a591..bc6faaf19156 100644 --- a/.changelog/32659.txt +++ b/.changelog/32659.txt @@ -1,3 +1,3 @@ ```release-note:bug -resource/aws_elasticsearch_domain: Omit `throughput` and `iops` for unsupported volume types -``` \ No newline at end of file +resource/aws_elasticsearch_domain: Omit `ebs_options.throughput` and `ebs_options.iops` for unsupported volume types +``` From e11bfbe714976a4459401286bc3af78a746ee32e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 25 Jul 2023 08:59:23 -0400 Subject: [PATCH 4/7] Fix semgrep 'ci.semgrep.aws.multiple-service-imports'. --- internal/service/elasticsearch/domain.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/service/elasticsearch/domain.go b/internal/service/elasticsearch/domain.go index f16ffde3f8ab..fc9e5841ff77 100644 --- a/internal/service/elasticsearch/domain.go +++ b/internal/service/elasticsearch/domain.go @@ -12,7 +12,6 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/elasticsearchservice" elasticsearch "github.com/aws/aws-sdk-go/service/elasticsearchservice" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" awspolicy "github.com/hashicorp/awspolicyequivalence" @@ -1256,7 +1255,7 @@ func advancedOptionsIgnoreDefault(o map[string]interface{}, n map[string]interfa // This check prevents a ValidationException when updating EBS volume types from a value // that supports IOPS (ex. gp3) to one that doesn't (ex. gp2). func EBSVolumeTypePermitsIopsInput(volumeType string) bool { - permittedTypes := []string{elasticsearchservice.VolumeTypeGp3, elasticsearchservice.VolumeTypeIo1} + permittedTypes := []string{elasticsearch.VolumeTypeGp3, elasticsearch.VolumeTypeIo1} for _, t := range permittedTypes { if volumeType == t { return true @@ -1270,7 +1269,7 @@ func EBSVolumeTypePermitsIopsInput(volumeType string) bool { // This check prevents a ValidationException when updating EBS volume types from a value // that supports Throughput (ex. gp3) to one that doesn't (ex. gp2). func EBSVolumeTypePermitsThroughputInput(volumeType string) bool { - permittedTypes := []string{elasticsearchservice.VolumeTypeGp3} + permittedTypes := []string{elasticsearch.VolumeTypeGp3} for _, t := range permittedTypes { if volumeType == t { return true From bffbe8baaf47c73faed7f67f7133bb2336e97add Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 25 Jul 2023 09:01:02 -0400 Subject: [PATCH 5/7] Fix terrafmt errors. --- internal/service/elasticsearch/domain_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/service/elasticsearch/domain_test.go b/internal/service/elasticsearch/domain_test.go index 9a0727050c46..929698d32f13 100644 --- a/internal/service/elasticsearch/domain_test.go +++ b/internal/service/elasticsearch/domain_test.go @@ -2223,14 +2223,15 @@ resource "aws_elasticsearch_domain" "test" { func testAccDomainConfig_clusterEBSVolumeGP3DefaultIopsThroughput(rName string, volumeSize int) string { return fmt.Sprintf(` resource "aws_elasticsearch_domain" "test" { - domain_name = %[1]q + domain_name = %[1]q elasticsearch_version = "7.10" + ebs_options { ebs_enabled = true volume_size = %[2]d volume_type = "gp3" - } + cluster_config { instance_type = "t3.small.elasticsearch" } @@ -2241,13 +2242,15 @@ resource "aws_elasticsearch_domain" "test" { func testAccDomainConfig_clusterEBSVolumeGP2(rName string, volumeSize int) string { return fmt.Sprintf(` resource "aws_elasticsearch_domain" "test" { - domain_name = %[1]q + domain_name = %[1]q elasticsearch_version = "7.10" + ebs_options { ebs_enabled = true volume_size = %[2]d volume_type = "gp2" } + cluster_config { instance_type = "t3.small.elasticsearch" } From 3894bc33a39ac56b7a3db33aa43963c183e47c49 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 25 Jul 2023 09:02:39 -0400 Subject: [PATCH 6/7] Fix golangci-lint 'whitespace'. --- internal/service/elasticsearch/flex.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/service/elasticsearch/flex.go b/internal/service/elasticsearch/flex.go index 839f10564e8e..209a2b16f07f 100644 --- a/internal/service/elasticsearch/flex.go +++ b/internal/service/elasticsearch/flex.go @@ -93,7 +93,6 @@ func expandEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions { if v, ok := m["throughput"]; ok && v.(int) > 0 && EBSVolumeTypePermitsThroughputInput(volumeType) { options.Throughput = aws.Int64(int64(v.(int))) } - } } From 30c9ee3e13807b81039dcf585eca064cffd9ea34 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 25 Jul 2023 09:05:00 -0400 Subject: [PATCH 7/7] Fix golangci-lint 'stylecheck'. --- internal/service/elasticsearch/domain_test.go | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/internal/service/elasticsearch/domain_test.go b/internal/service/elasticsearch/domain_test.go index 929698d32f13..4f3e0170690c 100644 --- a/internal/service/elasticsearch/domain_test.go +++ b/internal/service/elasticsearch/domain_test.go @@ -12,9 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/cognitoidentityprovider" - "github.com/aws/aws-sdk-go/service/elasticsearchservice" elasticsearch "github.com/aws/aws-sdk-go/service/elasticsearchservice" - "github.com/aws/aws-sdk-go/service/opensearchservice" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" @@ -33,10 +31,10 @@ func TestEBSVolumeTypePermitsIopsInput(t *testing.T) { want bool }{ {"empty", "", false}, - {"gp2", elasticsearchservice.VolumeTypeGp2, false}, - {"gp3", elasticsearchservice.VolumeTypeGp3, true}, - {"io1", elasticsearchservice.VolumeTypeIo1, true}, - {"standard", elasticsearchservice.VolumeTypeStandard, false}, + {"gp2", elasticsearch.VolumeTypeGp2, false}, + {"gp3", elasticsearch.VolumeTypeGp3, true}, + {"io1", elasticsearch.VolumeTypeIo1, true}, + {"standard", elasticsearch.VolumeTypeStandard, false}, } for _, testCase := range testCases { testCase := testCase @@ -59,10 +57,10 @@ func TestEBSVolumeTypePermitsThroughputInput(t *testing.T) { want bool }{ {"empty", "", false}, - {"gp2", elasticsearchservice.VolumeTypeGp2, false}, - {"gp3", elasticsearchservice.VolumeTypeGp3, true}, - {"io1", elasticsearchservice.VolumeTypeIo1, false}, - {"standard", elasticsearchservice.VolumeTypeStandard, false}, + {"gp2", elasticsearch.VolumeTypeGp2, false}, + {"gp3", elasticsearch.VolumeTypeGp3, true}, + {"io1", elasticsearch.VolumeTypeIo1, false}, + {"standard", elasticsearch.VolumeTypeStandard, false}, } for _, testCase := range testCases { testCase := testCase @@ -1499,7 +1497,7 @@ func TestAccElasticsearchDomain_VolumeType_gp3ToGP2(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheckIAMServiceLinkedRole(ctx, t) }, - ErrorCheck: acctest.ErrorCheck(t, opensearchservice.EndpointsID), + ErrorCheck: acctest.ErrorCheck(t, elasticsearch.EndpointsID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, CheckDestroy: testAccCheckDomainDestroy(ctx), Steps: []resource.TestStep{