From 1b7a8b2384ddc469f098162ece4fc40899d9b514 Mon Sep 17 00:00:00 2001 From: dhruva-chandra Date: Thu, 7 May 2020 21:33:25 +1000 Subject: [PATCH 01/16] add `number_of_nodes` to `RestoreFromClusterSnapshotInput` --- aws/resource_aws_redshift_cluster.go | 1 + aws/resource_aws_redshift_cluster_test.go | 119 ++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/aws/resource_aws_redshift_cluster.go b/aws/resource_aws_redshift_cluster.go index 5631585f565..373ac94de3a 100644 --- a/aws/resource_aws_redshift_cluster.go +++ b/aws/resource_aws_redshift_cluster.go @@ -342,6 +342,7 @@ func resourceAwsRedshiftClusterCreate(d *schema.ResourceData, meta interface{}) Port: aws.Int64(int64(d.Get("port").(int))), AllowVersionUpgrade: aws.Bool(d.Get("allow_version_upgrade").(bool)), NodeType: aws.String(d.Get("node_type").(string)), + NumberOfNodes: aws.Int64(int64(d.Get("number_of_nodes").(int))), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), } diff --git a/aws/resource_aws_redshift_cluster_test.go b/aws/resource_aws_redshift_cluster_test.go index 370e5a9ae01..80f22fa1e1f 100644 --- a/aws/resource_aws_redshift_cluster_test.go +++ b/aws/resource_aws_redshift_cluster_test.go @@ -131,6 +131,42 @@ func TestAccAWSRedshiftCluster_basic(t *testing.T) { }) } +func TestAccAWSRedshiftCluster_restoreFromSnapshot(t *testing.T) { + var v redshift.Cluster + + ri := acctest.RandInt() + config := testAccAWSRedshiftClusterConfig_restoreFromSnapshot(ri) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSRedshiftClusterDestroy, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v), + resource.TestCheckResourceAttr( + "aws_redshift_cluster.default", "cluster_type", "multi-node"), + resource.TestCheckResourceAttr( + "aws_redshift_cluster.default", "publicly_accessible", "true"), + resource.TestMatchResourceAttr("aws_redshift_cluster.default", "dns_name", regexp.MustCompile(fmt.Sprintf("^tf-redshift-cluster-%d.*\\.redshift\\..*", ri))), + ), + }, + { + ResourceName: "aws_redshift_cluster.default", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "final_snapshot_identifier", + "master_password", + "skip_final_snapshot", + }, + }, + }, + }) +} + func TestAccAWSRedshiftCluster_withFinalSnapshot(t *testing.T) { var v redshift.Cluster @@ -956,6 +992,89 @@ resource "aws_redshift_cluster" "default" { `, rInt) } +func testAccAWSRedshiftClusterConfig_restoreFromSnapshot(rInt int) string { + return fmt.Sprintf(` +data "aws_availability_zones" "available" { + state = "available" + + filter { + name = "opt-in-status" + values = ["opt-in-not-required"] + } +} + +resource "aws_vpc" "foo" { + cidr_block = "10.0.0.0/16" + + tags = { + Name = "tf-acc-test-%d" + } +} + +resource "aws_internet_gateway" "foo" { + vpc_id = "${aws_vpc.foo.id}" + + tags = { + Name = "tf-acc-test-%d" + } +} + +resource "aws_subnet" "bar1" { + vpc_id = "${aws_vpc.foo.id}" + cidr_block = "10.0.0.0/24" + availability_zone = "${data.aws_availability_zones.available.names[0]}" + + tags = { + Name = "tf-acc-test-%d" + } +} + +resource "aws_subnet" "bar2" { + vpc_id = "${aws_vpc.foo.id}" + cidr_block = "10.0.1.0/24" + availability_zone = "${data.aws_availability_zones.available.names[1]}" + + tags = { + Name = "tf-acc-test-%d" + } +} + +resource "aws_redshift_subnet_group" "foo" { + name = "tf-acc-test-%d" + subnet_ids = ["${aws_subnet.bar1.id}","${aws_subnet.bar2.id}"] + + tags = { + Name = "tf-acc-test-%d" + } +} + +data "aws_security_group" "default" { + name = "default" + vpc_id = "${aws_vpc.foo.id}" +} + +resource "aws_redshift_cluster" "default" { + cluster_identifier = "tf-redshift-cluster-%d" + database_name = "dev" + node_type = "ra3.4xlarge" + automated_snapshot_retention_period = 2 + number_of_nodes = 2 +# snapshot_identifier = "xx" +# snapshot_cluster_identifier = "rs:xx" + skip_final_snapshot = true + preferred_maintenance_window = "sun:04:25-sun:04:55" + final_snapshot_identifier = "tf-redshift-cluster-%d" + publicly_accessible = true + cluster_subnet_group_name = "${aws_redshift_subnet_group.foo.id}" + vpc_security_group_ids = ["${data.aws_security_group.default.id}"] + + timeouts { + create = "30m" + } +} +`, rInt, rInt, rInt, rInt, rInt, rInt, rInt, rInt) +} + func testAccAWSRedshiftClusterConfig_encrypted(rInt int) string { return fmt.Sprintf(` resource "aws_kms_key" "foo" { From 8a818610983599bbe7e98b09b0e31af1a51288a0 Mon Sep 17 00:00:00 2001 From: Thomas Lacroix Date: Thu, 4 Jun 2020 17:16:33 +0200 Subject: [PATCH 02/16] Fixes redshift restore not using number_of_nodes --- aws/resource_aws_redshift_cluster.go | 1 + 1 file changed, 1 insertion(+) diff --git a/aws/resource_aws_redshift_cluster.go b/aws/resource_aws_redshift_cluster.go index 5455792ca51..d48ba3f6a9f 100644 --- a/aws/resource_aws_redshift_cluster.go +++ b/aws/resource_aws_redshift_cluster.go @@ -368,6 +368,7 @@ func resourceAwsRedshiftClusterCreate(d *schema.ResourceData, meta interface{}) NodeType: aws.String(d.Get("node_type").(string)), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), + NumberOfNodes: aws.Int64(int64(d.Get("number_of_nodes").(int))), } if v, ok := d.GetOk("owner_account"); ok { From 937f69ed15ebf2d95b2dfb033fba13937407a430 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 14:36:39 -0400 Subject: [PATCH 03/16] Revert "Fixes redshift restore not using number_of_nodes" This reverts commit 8a818610983599bbe7e98b09b0e31af1a51288a0. --- aws/resource_aws_redshift_cluster.go | 1 - 1 file changed, 1 deletion(-) diff --git a/aws/resource_aws_redshift_cluster.go b/aws/resource_aws_redshift_cluster.go index d48ba3f6a9f..5455792ca51 100644 --- a/aws/resource_aws_redshift_cluster.go +++ b/aws/resource_aws_redshift_cluster.go @@ -368,7 +368,6 @@ func resourceAwsRedshiftClusterCreate(d *schema.ResourceData, meta interface{}) NodeType: aws.String(d.Get("node_type").(string)), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), - NumberOfNodes: aws.Int64(int64(d.Get("number_of_nodes").(int))), } if v, ok := d.GetOk("owner_account"); ok { From a44ca051bfba5e62dbdb352df4fc6d66e0e26515 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 14:37:47 -0400 Subject: [PATCH 04/16] Revert "add `number_of_nodes` to `RestoreFromClusterSnapshotInput`" This reverts commit 1b7a8b2384ddc469f098162ece4fc40899d9b514. --- aws/resource_aws_redshift_cluster.go | 1 - aws/resource_aws_redshift_cluster_test.go | 119 ---------------------- 2 files changed, 120 deletions(-) diff --git a/aws/resource_aws_redshift_cluster.go b/aws/resource_aws_redshift_cluster.go index 373ac94de3a..5631585f565 100644 --- a/aws/resource_aws_redshift_cluster.go +++ b/aws/resource_aws_redshift_cluster.go @@ -342,7 +342,6 @@ func resourceAwsRedshiftClusterCreate(d *schema.ResourceData, meta interface{}) Port: aws.Int64(int64(d.Get("port").(int))), AllowVersionUpgrade: aws.Bool(d.Get("allow_version_upgrade").(bool)), NodeType: aws.String(d.Get("node_type").(string)), - NumberOfNodes: aws.Int64(int64(d.Get("number_of_nodes").(int))), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), } diff --git a/aws/resource_aws_redshift_cluster_test.go b/aws/resource_aws_redshift_cluster_test.go index 80f22fa1e1f..370e5a9ae01 100644 --- a/aws/resource_aws_redshift_cluster_test.go +++ b/aws/resource_aws_redshift_cluster_test.go @@ -131,42 +131,6 @@ func TestAccAWSRedshiftCluster_basic(t *testing.T) { }) } -func TestAccAWSRedshiftCluster_restoreFromSnapshot(t *testing.T) { - var v redshift.Cluster - - ri := acctest.RandInt() - config := testAccAWSRedshiftClusterConfig_restoreFromSnapshot(ri) - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - Providers: testAccProviders, - CheckDestroy: testAccCheckAWSRedshiftClusterDestroy, - Steps: []resource.TestStep{ - { - Config: config, - Check: resource.ComposeTestCheckFunc( - testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v), - resource.TestCheckResourceAttr( - "aws_redshift_cluster.default", "cluster_type", "multi-node"), - resource.TestCheckResourceAttr( - "aws_redshift_cluster.default", "publicly_accessible", "true"), - resource.TestMatchResourceAttr("aws_redshift_cluster.default", "dns_name", regexp.MustCompile(fmt.Sprintf("^tf-redshift-cluster-%d.*\\.redshift\\..*", ri))), - ), - }, - { - ResourceName: "aws_redshift_cluster.default", - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{ - "final_snapshot_identifier", - "master_password", - "skip_final_snapshot", - }, - }, - }, - }) -} - func TestAccAWSRedshiftCluster_withFinalSnapshot(t *testing.T) { var v redshift.Cluster @@ -992,89 +956,6 @@ resource "aws_redshift_cluster" "default" { `, rInt) } -func testAccAWSRedshiftClusterConfig_restoreFromSnapshot(rInt int) string { - return fmt.Sprintf(` -data "aws_availability_zones" "available" { - state = "available" - - filter { - name = "opt-in-status" - values = ["opt-in-not-required"] - } -} - -resource "aws_vpc" "foo" { - cidr_block = "10.0.0.0/16" - - tags = { - Name = "tf-acc-test-%d" - } -} - -resource "aws_internet_gateway" "foo" { - vpc_id = "${aws_vpc.foo.id}" - - tags = { - Name = "tf-acc-test-%d" - } -} - -resource "aws_subnet" "bar1" { - vpc_id = "${aws_vpc.foo.id}" - cidr_block = "10.0.0.0/24" - availability_zone = "${data.aws_availability_zones.available.names[0]}" - - tags = { - Name = "tf-acc-test-%d" - } -} - -resource "aws_subnet" "bar2" { - vpc_id = "${aws_vpc.foo.id}" - cidr_block = "10.0.1.0/24" - availability_zone = "${data.aws_availability_zones.available.names[1]}" - - tags = { - Name = "tf-acc-test-%d" - } -} - -resource "aws_redshift_subnet_group" "foo" { - name = "tf-acc-test-%d" - subnet_ids = ["${aws_subnet.bar1.id}","${aws_subnet.bar2.id}"] - - tags = { - Name = "tf-acc-test-%d" - } -} - -data "aws_security_group" "default" { - name = "default" - vpc_id = "${aws_vpc.foo.id}" -} - -resource "aws_redshift_cluster" "default" { - cluster_identifier = "tf-redshift-cluster-%d" - database_name = "dev" - node_type = "ra3.4xlarge" - automated_snapshot_retention_period = 2 - number_of_nodes = 2 -# snapshot_identifier = "xx" -# snapshot_cluster_identifier = "rs:xx" - skip_final_snapshot = true - preferred_maintenance_window = "sun:04:25-sun:04:55" - final_snapshot_identifier = "tf-redshift-cluster-%d" - publicly_accessible = true - cluster_subnet_group_name = "${aws_redshift_subnet_group.foo.id}" - vpc_security_group_ids = ["${data.aws_security_group.default.id}"] - - timeouts { - create = "30m" - } -} -`, rInt, rInt, rInt, rInt, rInt, rInt, rInt, rInt) -} - func testAccAWSRedshiftClusterConfig_encrypted(rInt int) string { return fmt.Sprintf(` resource "aws_kms_key" "foo" { From 7297a8ee0a89db0530832104efbb65d570bc7f57 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 14:45:40 -0400 Subject: [PATCH 05/16] r/aws_redshift_cluster: Alphabetize attributes. --- internal/service/redshift/cluster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index e850f15ade4..82914bd7b49 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -308,6 +308,8 @@ func ResourceCluster() *schema.Resource { Optional: true, ForceNew: true, }, + "tags": tftags.TagsSchema(), + "tags_all": tftags.TagsSchemaComputed(), "vpc_security_group_ids": { Type: schema.TypeSet, Optional: true, @@ -315,8 +317,6 @@ func ResourceCluster() *schema.Resource { Elem: &schema.Schema{Type: schema.TypeString}, Set: schema.HashString, }, - "tags": tftags.TagsSchema(), - "tags_all": tftags.TagsSchemaComputed(), }, CustomizeDiff: customdiff.All( From 04f84d7bd9257fe932fabbdb2b3325d984a7e7a9 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 14:55:46 -0400 Subject: [PATCH 06/16] r/aws_redshift_cluster: Tidy up resource Create (restore). --- internal/service/redshift/cluster.go | 109 ++++++++++++++------------- 1 file changed, 55 insertions(+), 54 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index 82914bd7b49..eed5466db09 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -359,77 +359,77 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { tags := defaultTagsConfig.MergeTags(tftags.New(d.Get("tags").(map[string]interface{}))) if v, ok := d.GetOk("snapshot_identifier"); ok { - restoreOpts := &redshift.RestoreFromClusterSnapshotInput{ - ClusterIdentifier: aws.String(d.Get("cluster_identifier").(string)), - SnapshotIdentifier: aws.String(v.(string)), - Port: aws.Int64(int64(d.Get("port").(int))), + clusterID := d.Get("cluster_identifier").(string) + input := &redshift.RestoreFromClusterSnapshotInput{ AllowVersionUpgrade: aws.Bool(d.Get("allow_version_upgrade").(bool)), + AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), + ClusterIdentifier: aws.String(clusterID), + Port: aws.Int64(int64(d.Get("port").(int))), NodeType: aws.String(d.Get("node_type").(string)), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), - AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), - } - - if v, ok := d.GetOk("owner_account"); ok { - restoreOpts.OwnerAccount = aws.String(v.(string)) - } - - if v, ok := d.GetOk("snapshot_cluster_identifier"); ok { - restoreOpts.SnapshotClusterIdentifier = aws.String(v.(string)) + SnapshotIdentifier: aws.String(v.(string)), } if v, ok := d.GetOk("availability_zone"); ok { - restoreOpts.AvailabilityZone = aws.String(v.(string)) + input.AvailabilityZone = aws.String(v.(string)) } if v, ok := d.GetOk("availability_zone_relocation_enabled"); ok { - restoreOpts.AvailabilityZoneRelocation = aws.Bool(v.(bool)) + input.AvailabilityZoneRelocation = aws.Bool(v.(bool)) } if v, ok := d.GetOk("cluster_subnet_group_name"); ok { - restoreOpts.ClusterSubnetGroupName = aws.String(v.(string)) + input.ClusterSubnetGroupName = aws.String(v.(string)) } if v, ok := d.GetOk("cluster_parameter_group_name"); ok { - restoreOpts.ClusterParameterGroupName = aws.String(v.(string)) + input.ClusterParameterGroupName = aws.String(v.(string)) } if v := d.Get("cluster_security_groups").(*schema.Set); v.Len() > 0 { - restoreOpts.ClusterSecurityGroups = flex.ExpandStringSet(v) + input.ClusterSecurityGroups = flex.ExpandStringSet(v) } - if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 { - restoreOpts.VpcSecurityGroupIds = flex.ExpandStringSet(v) + if v, ok := d.GetOk("elastic_ip"); ok { + input.ElasticIp = aws.String(v.(string)) } - if v, ok := d.GetOk("preferred_maintenance_window"); ok { - restoreOpts.PreferredMaintenanceWindow = aws.String(v.(string)) + if v, ok := d.GetOk("enhanced_vpc_routing"); ok { + input.EnhancedVpcRouting = aws.Bool(v.(bool)) + } + + if v, ok := d.GetOk("iam_roles"); ok { + input.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) } if v, ok := d.GetOk("kms_key_id"); ok { - restoreOpts.KmsKeyId = aws.String(v.(string)) + input.KmsKeyId = aws.String(v.(string)) } - if v, ok := d.GetOk("elastic_ip"); ok { - restoreOpts.ElasticIp = aws.String(v.(string)) + if v, ok := d.GetOk("owner_account"); ok { + input.OwnerAccount = aws.String(v.(string)) } - if v, ok := d.GetOk("enhanced_vpc_routing"); ok { - restoreOpts.EnhancedVpcRouting = aws.Bool(v.(bool)) + if v, ok := d.GetOk("preferred_maintenance_window"); ok { + input.PreferredMaintenanceWindow = aws.String(v.(string)) } - if v, ok := d.GetOk("iam_roles"); ok { - restoreOpts.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) + if v, ok := d.GetOk("snapshot_cluster_identifier"); ok { + input.SnapshotClusterIdentifier = aws.String(v.(string)) } - log.Printf("[DEBUG] Redshift Cluster restore cluster options: %s", restoreOpts) + if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 { + input.VpcSecurityGroupIds = flex.ExpandStringSet(v) + } + + log.Printf("[DEBUG] Restoring Redshift Cluster: %s", input) + output, err := conn.RestoreFromClusterSnapshot(input) - resp, err := conn.RestoreFromClusterSnapshot(restoreOpts) if err != nil { - return fmt.Errorf("error restoring Redshift Cluster from snapshot: %w", err) + return fmt.Errorf("error restoring Redshift Cluster (%s) from snapshot: %w", clusterID, err) } - d.SetId(aws.StringValue(resp.Cluster.ClusterIdentifier)) - + d.SetId(aws.StringValue(output.Cluster.ClusterIdentifier)) } else { if _, ok := d.GetOk("master_password"); !ok { return fmt.Errorf(`provider.aws: aws_redshift_cluster: %s: "master_password": required field is not set`, d.Get("cluster_identifier").(string)) @@ -439,7 +439,7 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf(`provider.aws: aws_redshift_cluster: %s: "master_username": required field is not set`, d.Get("cluster_identifier").(string)) } - createOpts := &redshift.CreateClusterInput{ + input := &redshift.CreateClusterInput{ ClusterIdentifier: aws.String(d.Get("cluster_identifier").(string)), Port: aws.Int64(int64(d.Get("port").(int))), MasterUserPassword: aws.String(d.Get("master_password").(string)), @@ -454,68 +454,69 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { } if v := d.Get("number_of_nodes").(int); v > 1 { - createOpts.ClusterType = aws.String("multi-node") - createOpts.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) + input.ClusterType = aws.String("multi-node") + input.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) } else { - createOpts.ClusterType = aws.String("single-node") + input.ClusterType = aws.String("single-node") } if v := d.Get("cluster_security_groups").(*schema.Set); v.Len() > 0 { - createOpts.ClusterSecurityGroups = flex.ExpandStringSet(v) + input.ClusterSecurityGroups = flex.ExpandStringSet(v) } if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 { - createOpts.VpcSecurityGroupIds = flex.ExpandStringSet(v) + input.VpcSecurityGroupIds = flex.ExpandStringSet(v) } if v, ok := d.GetOk("cluster_subnet_group_name"); ok { - createOpts.ClusterSubnetGroupName = aws.String(v.(string)) + input.ClusterSubnetGroupName = aws.String(v.(string)) } if v, ok := d.GetOk("availability_zone"); ok { - createOpts.AvailabilityZone = aws.String(v.(string)) + input.AvailabilityZone = aws.String(v.(string)) } if v, ok := d.GetOk("availability_zone_relocation_enabled"); ok { - createOpts.AvailabilityZoneRelocation = aws.Bool(v.(bool)) + input.AvailabilityZoneRelocation = aws.Bool(v.(bool)) } if v, ok := d.GetOk("preferred_maintenance_window"); ok { - createOpts.PreferredMaintenanceWindow = aws.String(v.(string)) + input.PreferredMaintenanceWindow = aws.String(v.(string)) } if v, ok := d.GetOk("cluster_parameter_group_name"); ok { - createOpts.ClusterParameterGroupName = aws.String(v.(string)) + input.ClusterParameterGroupName = aws.String(v.(string)) } if v, ok := d.GetOk("encrypted"); ok { - createOpts.Encrypted = aws.Bool(v.(bool)) + input.Encrypted = aws.Bool(v.(bool)) } if v, ok := d.GetOk("enhanced_vpc_routing"); ok { - createOpts.EnhancedVpcRouting = aws.Bool(v.(bool)) + input.EnhancedVpcRouting = aws.Bool(v.(bool)) } if v, ok := d.GetOk("kms_key_id"); ok { - createOpts.KmsKeyId = aws.String(v.(string)) + input.KmsKeyId = aws.String(v.(string)) } if v, ok := d.GetOk("elastic_ip"); ok { - createOpts.ElasticIp = aws.String(v.(string)) + input.ElasticIp = aws.String(v.(string)) } if v, ok := d.GetOk("iam_roles"); ok { - createOpts.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) + input.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) } - log.Printf("[DEBUG] Redshift Cluster create options: %s", createOpts) - resp, err := conn.CreateCluster(createOpts) + log.Printf("[DEBUG] Redshift Cluster create options: %s", input) + output, err := conn.CreateCluster(input) + if err != nil { return fmt.Errorf("error creating Redshift Cluster: %w", err) } - log.Printf("[DEBUG]: Cluster create response: %s", resp) - d.SetId(aws.StringValue(resp.Cluster.ClusterIdentifier)) + log.Printf("[DEBUG]: Cluster create response: %s", output) + d.SetId(aws.StringValue(output.Cluster.ClusterIdentifier)) } stateConf := &resource.StateChangeConf{ From c100de137e494c554fb77b08527375e3eaf8fbb5 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 16:29:01 -0400 Subject: [PATCH 07/16] r/aws_redshift_cluster: Tidy up resource Create. --- internal/service/redshift/cluster.go | 74 ++++++++++++++-------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index eed5466db09..35b26fb0e39 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -439,39 +439,21 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf(`provider.aws: aws_redshift_cluster: %s: "master_username": required field is not set`, d.Get("cluster_identifier").(string)) } + clusterID := d.Get("cluster_identifier").(string) input := &redshift.CreateClusterInput{ - ClusterIdentifier: aws.String(d.Get("cluster_identifier").(string)), - Port: aws.Int64(int64(d.Get("port").(int))), - MasterUserPassword: aws.String(d.Get("master_password").(string)), - MasterUsername: aws.String(d.Get("master_username").(string)), + AllowVersionUpgrade: aws.Bool(d.Get("allow_version_upgrade").(bool)), + AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), + ClusterIdentifier: aws.String(clusterID), ClusterVersion: aws.String(d.Get("cluster_version").(string)), - NodeType: aws.String(d.Get("node_type").(string)), DBName: aws.String(d.Get("database_name").(string)), - AllowVersionUpgrade: aws.Bool(d.Get("allow_version_upgrade").(bool)), + MasterUsername: aws.String(d.Get("master_username").(string)), + MasterUserPassword: aws.String(d.Get("master_password").(string)), + NodeType: aws.String(d.Get("node_type").(string)), + Port: aws.Int64(int64(d.Get("port").(int))), PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)), - AutomatedSnapshotRetentionPeriod: aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))), Tags: Tags(tags.IgnoreAWS()), } - if v := d.Get("number_of_nodes").(int); v > 1 { - input.ClusterType = aws.String("multi-node") - input.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) - } else { - input.ClusterType = aws.String("single-node") - } - - if v := d.Get("cluster_security_groups").(*schema.Set); v.Len() > 0 { - input.ClusterSecurityGroups = flex.ExpandStringSet(v) - } - - if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 { - input.VpcSecurityGroupIds = flex.ExpandStringSet(v) - } - - if v, ok := d.GetOk("cluster_subnet_group_name"); ok { - input.ClusterSubnetGroupName = aws.String(v.(string)) - } - if v, ok := d.GetOk("availability_zone"); ok { input.AvailabilityZone = aws.String(v.(string)) } @@ -480,14 +462,22 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { input.AvailabilityZoneRelocation = aws.Bool(v.(bool)) } - if v, ok := d.GetOk("preferred_maintenance_window"); ok { - input.PreferredMaintenanceWindow = aws.String(v.(string)) - } - if v, ok := d.GetOk("cluster_parameter_group_name"); ok { input.ClusterParameterGroupName = aws.String(v.(string)) } + if v := d.Get("cluster_security_groups").(*schema.Set); v.Len() > 0 { + input.ClusterSecurityGroups = flex.ExpandStringSet(v) + } + + if v, ok := d.GetOk("cluster_subnet_group_name"); ok { + input.ClusterSubnetGroupName = aws.String(v.(string)) + } + + if v, ok := d.GetOk("elastic_ip"); ok { + input.ElasticIp = aws.String(v.(string)) + } + if v, ok := d.GetOk("encrypted"); ok { input.Encrypted = aws.Bool(v.(bool)) } @@ -496,26 +486,36 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { input.EnhancedVpcRouting = aws.Bool(v.(bool)) } + if v, ok := d.GetOk("iam_roles"); ok { + input.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) + } + if v, ok := d.GetOk("kms_key_id"); ok { input.KmsKeyId = aws.String(v.(string)) } - if v, ok := d.GetOk("elastic_ip"); ok { - input.ElasticIp = aws.String(v.(string)) + if v := d.Get("number_of_nodes").(int); v > 1 { + input.ClusterType = aws.String(clusterTypeMultiNode) + input.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) + } else { + input.ClusterType = aws.String(clusterTypeSingleNode) } - if v, ok := d.GetOk("iam_roles"); ok { - input.IamRoles = flex.ExpandStringSet(v.(*schema.Set)) + if v, ok := d.GetOk("preferred_maintenance_window"); ok { + input.PreferredMaintenanceWindow = aws.String(v.(string)) + } + + if v := d.Get("vpc_security_group_ids").(*schema.Set); v.Len() > 0 { + input.VpcSecurityGroupIds = flex.ExpandStringSet(v) } - log.Printf("[DEBUG] Redshift Cluster create options: %s", input) + log.Printf("[DEBUG] Creating Redshift Cluster: %s", input) output, err := conn.CreateCluster(input) if err != nil { - return fmt.Errorf("error creating Redshift Cluster: %w", err) + return fmt.Errorf("error creating Redshift Cluster (%s): %w", clusterID, err) } - log.Printf("[DEBUG]: Cluster create response: %s", output) d.SetId(aws.StringValue(output.Cluster.ClusterIdentifier)) } From 4ac79cc440b8d490ae323c5607ccef3eb4ce8449 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 17:56:37 -0400 Subject: [PATCH 08/16] r/aws_redshift_cluster: Add and use 'waitClusterCreated'. --- internal/service/redshift/cluster.go | 17 ++++------------- internal/service/redshift/enum.go | 2 ++ internal/service/redshift/wait.go | 24 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index 35b26fb0e39..268a5763d48 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -519,21 +519,12 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { d.SetId(aws.StringValue(output.Cluster.ClusterIdentifier)) } - stateConf := &resource.StateChangeConf{ - Pending: []string{"creating", "backing-up", "modifying", "restoring", "available, prep-for-resize"}, - Target: []string{"available"}, - Refresh: resourceClusterStateRefreshFunc(d.Id(), conn), - Timeout: d.Timeout(schema.TimeoutCreate), - MinTimeout: 10 * time.Second, - } - _, err := stateConf.WaitForState() - if err != nil { - return fmt.Errorf("Error waiting for Redshift Cluster state to be \"available\": %w", err) + if _, err := waitClusterCreated(conn, d.Id(), d.Timeout(schema.TimeoutCreate)); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) create: %w", d.Id(), err) } - _, err = waitClusterRelocationStatusResolved(conn, d.Id()) - if err != nil { - return fmt.Errorf("error waiting for Redshift Cluster Availability Zone Relocation Status to resolve: %w", err) + if _, err := waitClusterRelocationStatusResolved(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) Availability Zone Relocation Status resolve: %w", d.Id(), err) } if v, ok := d.GetOk("snapshot_copy"); ok { diff --git a/internal/service/redshift/enum.go b/internal/service/redshift/enum.go index 4b49fa8c3d1..08360ae2c60 100644 --- a/internal/service/redshift/enum.go +++ b/internal/service/redshift/enum.go @@ -6,6 +6,7 @@ const ( clusterStatusAvailable = "available" clusterStatusAvailablePrepForResize = "available, prep-for-resize" clusterStatusAvailableResizeCleanup = "available, resize-cleanup" + clusterStatusBackingUp = "backing-up" clusterStatusCancellingResize = "cancelling-resize" clusterStatusCreating = "creating" clusterStatusDeleting = "deleting" @@ -20,6 +21,7 @@ const ( clusterStatusRebooting = "rebooting" clusterStatusRenaming = "renaming" clusterStatusResizing = "resizing" + clusterStatusRestoring = "restoring" clusterStatusRotatingKeys = "rotating-keys" clusterStatusStorageFull = "storage-full" clusterStatusUpdatingHSM = "updating-hsm" diff --git a/internal/service/redshift/wait.go b/internal/service/redshift/wait.go index 440a46d4c38..05026099f8f 100644 --- a/internal/service/redshift/wait.go +++ b/internal/service/redshift/wait.go @@ -13,6 +13,30 @@ const ( clusterRelocationStatusResolvedTimeout = 1 * time.Minute ) +func waitClusterCreated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{ + clusterStatusAvailablePrepForResize, + clusterStatusBackingUp, + clusterStatusCreating, + clusterStatusModifying, + clusterStatusRestoring, + }, + Target: []string{clusterStatusAvailable}, + Refresh: statusCluster(conn, id), + Timeout: timeout, + MinTimeout: 10 * time.Second, + } + + outputRaw, err := stateConf.WaitForState() + + if output, ok := outputRaw.(*redshift.Cluster); ok { + return output, err + } + + return nil, err +} + func waitClusterDeleted(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { stateConf := &resource.StateChangeConf{ Pending: []string{ From dba9fe48b40981b45b6c34aa9f5e835329e414c6 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 25 Mar 2022 18:30:50 -0400 Subject: [PATCH 09/16] r/aws_redshift_cluster: Tidy up resource Update. --- internal/service/redshift/cluster.go | 435 ++++++++++++--------------- internal/service/redshift/enum.go | 1 + internal/service/redshift/wait.go | 26 ++ 3 files changed, 217 insertions(+), 245 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index 268a5763d48..e8e2f4481db 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -14,7 +14,6 @@ import ( "github.com/aws/aws-sdk-go/service/redshift" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" @@ -321,9 +320,15 @@ func ResourceCluster() *schema.Resource { CustomizeDiff: customdiff.All( verify.SetTagsDiff, + func(_ context.Context, diff *schema.ResourceDiff, v interface{}) error { + if !diff.Get("skip_final_snapshot").(bool) && diff.Get("final_snapshot_identifier").(string) == "" { + return errors.New("`final_snapshot_identifier` must be set when `skip_final_snapshot` is false") + } + return nil + }, func(_ context.Context, diff *schema.ResourceDiff, v interface{}) error { if diff.Get("availability_zone_relocation_enabled").(bool) && diff.Get("publicly_accessible").(bool) { - return errors.New("availability_zone_relocation_enabled can not be true when publicly_accessible is true") + return errors.New("`availability_zone_relocation_enabled` cannot be true when `publicly_accessible` is true") } return nil }, @@ -336,7 +341,7 @@ func ResourceCluster() *schema.Resource { } o, n := diff.GetChange("availability_zone") if o.(string) != n.(string) { - return fmt.Errorf("cannot change availability_zone if availability_zone_relocation_enabled is not true") + return fmt.Errorf("cannot change `availability_zone` if `availability_zone_relocation_enabled` is not true") } return nil }, @@ -344,15 +349,6 @@ func ResourceCluster() *schema.Resource { } } -func resourceClusterImport( - d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { - // Neither skip_final_snapshot nor final_snapshot_identifier can be fetched - // from any API call, so we need to default skip_final_snapshot to true so - // that final_snapshot_identifier is not required - d.Set("skip_final_snapshot", true) - return []*schema.ResourceData{d}, nil -} - func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).RedshiftConn defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig @@ -524,19 +520,24 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { } if _, err := waitClusterRelocationStatusResolved(conn, d.Id()); err != nil { - return fmt.Errorf("error waiting for Redshift Cluster (%s) Availability Zone Relocation Status resolve: %w", d.Id(), err) + return fmt.Errorf("error waiting for Redshift Cluster (%s) Availability Zone Relocation Status resolution: %w", d.Id(), err) } - if v, ok := d.GetOk("snapshot_copy"); ok { - err := enableRedshiftSnapshotCopy(d.Id(), v.([]interface{}), conn) - if err != nil { + if v, ok := d.GetOk("snapshot_copy"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + if err := enableSnapshotCopy(conn, d.Id(), v.([]interface{})[0].(map[string]interface{})); err != nil { return err } } - if _, ok := d.GetOk("logging.0.enable"); ok { - if err := enableRedshiftClusterLogging(d, conn); err != nil { - return fmt.Errorf("error enabling Redshift Cluster (%s) logging: %w", d.Id(), err) + if v, ok := d.GetOk("logging"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + tfMap := v.([]interface{})[0].(map[string]interface{}) + + if v, ok := tfMap["enable"].(bool); ok && v { + err := enableLogging(conn, d.Id(), tfMap) + + if err != nil { + return err + } } } @@ -667,104 +668,89 @@ func resourceClusterRead(d *schema.ResourceData, meta interface{}) error { func resourceClusterUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).RedshiftConn - if d.HasChange("tags_all") { - o, n := d.GetChange("tags_all") + if d.HasChangesExcept("availability_zone", "iam_roles", "logging", "snapshot_copy", "tags", "tags_all") { + input := &redshift.ModifyClusterInput{ + ClusterIdentifier: aws.String(d.Id()), + } - if err := UpdateTags(conn, d.Get("arn").(string), o, n); err != nil { - return fmt.Errorf("error updating Redshift Cluster (%s) tags: %s", d.Get("arn").(string), err) + if d.HasChange("allow_version_upgrade") { + input.AllowVersionUpgrade = aws.Bool(d.Get("allow_version_upgrade").(bool)) } - } - requestUpdate := false - log.Printf("[INFO] Building Redshift Modify Cluster Options") - req := &redshift.ModifyClusterInput{ - ClusterIdentifier: aws.String(d.Id()), - } + if d.HasChange("automated_snapshot_retention_period") { + input.AutomatedSnapshotRetentionPeriod = aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))) + } - // If the cluster type, node type, or number of nodes changed, then the AWS API expects all three - // items to be sent over - if d.HasChanges("cluster_type", "node_type", "number_of_nodes") { - req.ClusterType = aws.String(d.Get("cluster_type").(string)) - req.NodeType = aws.String(d.Get("node_type").(string)) - if v := d.Get("number_of_nodes").(int); v > 1 { - req.ClusterType = aws.String("multi-node") - req.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) - } else { - req.ClusterType = aws.String("single-node") + if d.HasChange("availability_zone_relocation_enabled") { + input.AvailabilityZoneRelocation = aws.Bool(d.Get("availability_zone_relocation_enabled").(bool)) } - requestUpdate = true - } - if d.HasChange("availability_zone_relocation_enabled") { - req.AvailabilityZoneRelocation = aws.Bool(d.Get("availability_zone_relocation_enabled").(bool)) - requestUpdate = true - } + if d.HasChange("cluster_parameter_group_name") { + input.ClusterParameterGroupName = aws.String(d.Get("cluster_parameter_group_name").(string)) + } - if d.HasChange("cluster_security_groups") { - req.ClusterSecurityGroups = flex.ExpandStringSet(d.Get("cluster_security_groups").(*schema.Set)) - requestUpdate = true - } + if d.HasChange("cluster_security_groups") { + input.ClusterSecurityGroups = flex.ExpandStringSet(d.Get("cluster_security_groups").(*schema.Set)) + } - if d.HasChange("vpc_security_group_ids") { - req.VpcSecurityGroupIds = flex.ExpandStringSet(d.Get("vpc_security_group_ids").(*schema.Set)) - requestUpdate = true - } + // If the cluster type, node type, or number of nodes changed, then the AWS API expects all three + // items to be sent over. + if d.HasChanges("cluster_type", "node_type", "number_of_nodes") { + input.NodeType = aws.String(d.Get("node_type").(string)) - if d.HasChange("master_password") { - req.MasterUserPassword = aws.String(d.Get("master_password").(string)) - requestUpdate = true - } + if v := d.Get("number_of_nodes").(int); v > 1 { + input.ClusterType = aws.String(clusterTypeMultiNode) + input.NumberOfNodes = aws.Int64(int64(d.Get("number_of_nodes").(int))) + } else { + input.ClusterType = aws.String(clusterTypeSingleNode) + } + } - if d.HasChange("cluster_parameter_group_name") { - req.ClusterParameterGroupName = aws.String(d.Get("cluster_parameter_group_name").(string)) - requestUpdate = true - } + if d.HasChange("cluster_version") { + input.ClusterVersion = aws.String(d.Get("cluster_version").(string)) + } - if d.HasChange("automated_snapshot_retention_period") { - req.AutomatedSnapshotRetentionPeriod = aws.Int64(int64(d.Get("automated_snapshot_retention_period").(int))) - requestUpdate = true - } + if d.HasChange("encrypted") { + input.Encrypted = aws.Bool(d.Get("encrypted").(bool)) + } - if d.HasChange("preferred_maintenance_window") { - req.PreferredMaintenanceWindow = aws.String(d.Get("preferred_maintenance_window").(string)) - requestUpdate = true - } + if d.HasChange("enhanced_vpc_routing") { + input.EnhancedVpcRouting = aws.Bool(d.Get("enhanced_vpc_routing").(bool)) + } - if d.HasChange("cluster_version") { - req.ClusterVersion = aws.String(d.Get("cluster_version").(string)) - requestUpdate = true - } + if d.Get("encrypted").(bool) && d.HasChange("kms_key_id") { + input.KmsKeyId = aws.String(d.Get("kms_key_id").(string)) + } - if d.HasChange("allow_version_upgrade") { - req.AllowVersionUpgrade = aws.Bool(d.Get("allow_version_upgrade").(bool)) - requestUpdate = true - } + if d.HasChange("master_password") { + input.MasterUserPassword = aws.String(d.Get("master_password").(string)) + } - if d.HasChange("publicly_accessible") { - req.PubliclyAccessible = aws.Bool(d.Get("publicly_accessible").(bool)) - requestUpdate = true - } + if d.HasChange("preferred_maintenance_window") { + input.PreferredMaintenanceWindow = aws.String(d.Get("preferred_maintenance_window").(string)) + } - if d.HasChange("enhanced_vpc_routing") { - req.EnhancedVpcRouting = aws.Bool(d.Get("enhanced_vpc_routing").(bool)) - requestUpdate = true - } + if d.HasChange("publicly_accessible") { + input.PubliclyAccessible = aws.Bool(d.Get("publicly_accessible").(bool)) + } - if d.HasChange("encrypted") { - req.Encrypted = aws.Bool(d.Get("encrypted").(bool)) - requestUpdate = true - } + if d.HasChange("vpc_security_group_ids") { + input.VpcSecurityGroupIds = flex.ExpandStringSet(d.Get("vpc_security_group_ids").(*schema.Set)) + } - if d.Get("encrypted").(bool) && d.HasChange("kms_key_id") { - req.KmsKeyId = aws.String(d.Get("kms_key_id").(string)) - requestUpdate = true - } + log.Printf("[DEBUG] Modifying Redshift Cluster: %s", input) + _, err := conn.ModifyCluster(input) - if requestUpdate { - log.Printf("[DEBUG] Modifying Redshift Cluster: %s", d.Id()) - _, err := conn.ModifyCluster(req) if err != nil { - return fmt.Errorf("Error modifying Redshift Cluster (%s): %w", d.Id(), err) + return fmt.Errorf("error modifying Redshift Cluster (%s): %w", d.Id(), err) + } + + if _, err := waitClusterUpdated(conn, d.Id(), d.Timeout(schema.TimeoutUpdate)); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) update: %w", d.Id(), err) + } + + if _, err := waitClusterRelocationStatusResolved(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) Availability Zone Relocation Status resolution: %w", d.Id(), err) } } @@ -779,70 +765,50 @@ func resourceClusterUpdate(d *schema.ResourceData, meta interface{}) error { os := o.(*schema.Set) ns := n.(*schema.Set) + add := ns.Difference(os) + del := os.Difference(ns) - removeIams := os.Difference(ns) - addIams := ns.Difference(os) - - req := &redshift.ModifyClusterIamRolesInput{ + input := &redshift.ModifyClusterIamRolesInput{ + AddIamRoles: flex.ExpandStringSet(add), ClusterIdentifier: aws.String(d.Id()), - AddIamRoles: flex.ExpandStringSet(addIams), - RemoveIamRoles: flex.ExpandStringSet(removeIams), + RemoveIamRoles: flex.ExpandStringSet(del), } - log.Printf("[DEBUG] Modifying Redshift Cluster IAM Roles: %s", d.Id()) - _, err := conn.ModifyClusterIamRoles(req) - if err != nil { - return fmt.Errorf("Error modifying Redshift Cluster IAM Roles (%s): %w", d.Id(), err) - } - } + log.Printf("[DEBUG] Modifying Redshift Cluster IAM Roles: %s", input) + _, err := conn.ModifyClusterIamRoles(input) - if requestUpdate || d.HasChange("iam_roles") { - stateConf := &resource.StateChangeConf{ - Pending: []string{"creating", "deleting", "rebooting", "resizing", "renaming", "modifying", "available, prep-for-resize"}, - Target: []string{"available"}, - Refresh: resourceClusterStateRefreshFunc(d.Id(), conn), - Timeout: d.Timeout(schema.TimeoutUpdate), - MinTimeout: 10 * time.Second, - } - _, err := stateConf.WaitForState() if err != nil { - return fmt.Errorf("Error waiting for Redshift Cluster modification (%s): %w", d.Id(), err) + return fmt.Errorf("error modifying Redshift Cluster (%s) IAM roles: %w", d.Id(), err) } - _, err = waitClusterRelocationStatusResolved(conn, d.Id()) - if err != nil { - return fmt.Errorf("error waiting for Redshift Cluster Availability Zone Relocation Status to resolve: %w", err) + if _, err := waitClusterUpdated(conn, d.Id(), d.Timeout(schema.TimeoutUpdate)); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) update: %w", d.Id(), err) } } // Availability Zone cannot be changed at the same time as other settings if d.HasChange("availability_zone") { - req := &redshift.ModifyClusterInput{ - ClusterIdentifier: aws.String(d.Id()), + input := &redshift.ModifyClusterInput{ AvailabilityZone: aws.String(d.Get("availability_zone").(string)), + ClusterIdentifier: aws.String(d.Id()), } - log.Printf("[DEBUG] Relocating Redshift Cluster: %s", d.Id()) - _, err := conn.ModifyCluster(req) + + log.Printf("[DEBUG] Relocating Redshift Cluster: %s", input) + _, err := conn.ModifyCluster(input) + if err != nil { - return fmt.Errorf("Error relocating Redshift Cluster (%s): %w", d.Id(), err) + return fmt.Errorf("error relocating Redshift Cluster (%s): %w", d.Id(), err) } - stateConf := &resource.StateChangeConf{ - Pending: []string{"creating", "deleting", "rebooting", "resizing", "renaming", "modifying", "available, prep-for-resize", "recovering"}, - Target: []string{"available"}, - Refresh: resourceClusterStateRefreshFunc(d.Id(), conn), - Timeout: d.Timeout(schema.TimeoutUpdate), - MinTimeout: 10 * time.Second, - } - _, err = stateConf.WaitForState() - if err != nil { - return fmt.Errorf("Error waiting for Redshift Cluster relocation (%s): %w", d.Id(), err) + if _, err := waitClusterUpdated(conn, d.Id(), d.Timeout(schema.TimeoutUpdate)); err != nil { + return fmt.Errorf("error waiting for Redshift Cluster (%s) update: %w", d.Id(), err) } } if d.HasChange("snapshot_copy") { - if v, ok := d.GetOk("snapshot_copy"); ok { - err := enableRedshiftSnapshotCopy(d.Id(), v.([]interface{}), conn) + if v, ok := d.GetOk("snapshot_copy"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + err := enableSnapshotCopy(conn, d.Id(), v.([]interface{})[0].(map[string]interface{})) + if err != nil { return err } @@ -850,107 +816,58 @@ func resourceClusterUpdate(d *schema.ResourceData, meta interface{}) error { _, err := conn.DisableSnapshotCopy(&redshift.DisableSnapshotCopyInput{ ClusterIdentifier: aws.String(d.Id()), }) - if err != nil { - return fmt.Errorf("Failed to disable snapshot copy: %w", err) - } - } - } - - if d.HasChange("logging") { - if loggingEnabled, ok := d.GetOk("logging.0.enable"); ok && loggingEnabled.(bool) { - log.Printf("[INFO] Enabling Logging for Redshift Cluster %q", d.Id()) - err := enableRedshiftClusterLogging(d, conn) - if err != nil { - return err - } - } else { - log.Printf("[INFO] Disabling Logging for Redshift Cluster %q", d.Id()) - _, err := tfresource.RetryWhenAWSErrCodeEquals( - clusterInvalidClusterStateFaultTimeout, - func() (interface{}, error) { - return conn.DisableLogging(&redshift.DisableLoggingInput{ - ClusterIdentifier: aws.String(d.Id()), - }) - }, - redshift.ErrCodeInvalidClusterStateFault, - ) if err != nil { - return fmt.Errorf("error disabling Redshift Cluster (%s) logging: %w", d.Id(), err) + return fmt.Errorf("error disabling Redshift Cluster (%s) snapshot copy: %w", d.Id(), err) } } } - return resourceClusterRead(d, meta) -} - -func enableRedshiftClusterLogging(d *schema.ResourceData, conn *redshift.Redshift) error { - bucketNameRaw, ok := d.GetOk("logging.0.bucket_name") - - if !ok { - return fmt.Errorf("bucket_name must be set when enabling logging for Redshift Clusters") - } + if d.HasChange("logging") { + if v, ok := d.GetOk("logging"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + tfMap := v.([]interface{})[0].(map[string]interface{}) - params := &redshift.EnableLoggingInput{ - ClusterIdentifier: aws.String(d.Id()), - BucketName: aws.String(bucketNameRaw.(string)), - } + if v, ok := tfMap["enable"].(bool); ok && v { + err := enableLogging(conn, d.Id(), tfMap) - if v, ok := d.GetOk("logging.0.s3_key_prefix"); ok { - params.S3KeyPrefix = aws.String(v.(string)) - } - - _, err := tfresource.RetryWhenAWSErrCodeEquals( - clusterInvalidClusterStateFaultTimeout, - func() (interface{}, error) { - return conn.EnableLogging(params) - }, - redshift.ErrCodeInvalidClusterStateFault, - ) + if err != nil { + return err + } + } else { + _, err := tfresource.RetryWhenAWSErrCodeEquals( + clusterInvalidClusterStateFaultTimeout, + func() (interface{}, error) { + return conn.DisableLogging(&redshift.DisableLoggingInput{ + ClusterIdentifier: aws.String(d.Id()), + }) + }, + redshift.ErrCodeInvalidClusterStateFault, + ) - if err != nil { - return fmt.Errorf("error enabling Redshift Cluster (%s) logging: %w", d.Id(), err) + if err != nil { + return fmt.Errorf("error disabling Redshift Cluster (%s) logging: %w", d.Id(), err) + } + } + } } - return nil -} - -func enableRedshiftSnapshotCopy(id string, scList []interface{}, conn *redshift.Redshift) error { - sc := scList[0].(map[string]interface{}) + if d.HasChange("tags_all") { + o, n := d.GetChange("tags_all") - input := redshift.EnableSnapshotCopyInput{ - ClusterIdentifier: aws.String(id), - DestinationRegion: aws.String(sc["destination_region"].(string)), - } - if rp, ok := sc["retention_period"]; ok { - input.RetentionPeriod = aws.Int64(int64(rp.(int))) - } - if gn, ok := sc["grant_name"]; ok { - input.SnapshotCopyGrantName = aws.String(gn.(string)) + if err := UpdateTags(conn, d.Get("arn").(string), o, n); err != nil { + return fmt.Errorf("error updating Redshift Cluster (%s) tags: %s", d.Get("arn").(string), err) + } } - _, err := conn.EnableSnapshotCopy(&input) - if err != nil { - return fmt.Errorf("Failed to enable snapshot copy: %w", err) - } - return nil + return resourceClusterRead(d, meta) } func resourceClusterDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).RedshiftConn - skipFinalSnapshot := d.Get("skip_final_snapshot").(bool) input := &redshift.DeleteClusterInput{ ClusterIdentifier: aws.String(d.Id()), - SkipFinalClusterSnapshot: aws.Bool(skipFinalSnapshot), - } - - if !skipFinalSnapshot { - if v, ok := d.GetOk("final_snapshot_identifier"); ok { - input.FinalClusterSnapshotIdentifier = aws.String(v.(string)) - } else { - return fmt.Errorf("Redshift Cluster Instance FinalSnapshotIdentifier is required when a final snapshot is required") - } + SkipFinalClusterSnapshot: aws.Bool(d.Get("skip_final_snapshot").(bool)), } log.Printf("[DEBUG] Deleting Redshift Cluster: %s", d.Id()) @@ -979,39 +896,67 @@ func resourceClusterDelete(d *schema.ResourceData, meta interface{}) error { return nil } -func resourceClusterStateRefreshFunc(id string, conn *redshift.Redshift) resource.StateRefreshFunc { - return func() (interface{}, string, error) { - log.Printf("[INFO] Reading Redshift Cluster Information: %s", id) - resp, err := conn.DescribeClusters(&redshift.DescribeClustersInput{ - ClusterIdentifier: aws.String(id), - }) +func resourceClusterImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + // Neither skip_final_snapshot nor final_snapshot_identifier can be fetched + // from any API call, so we need to default skip_final_snapshot to true so + // that final_snapshot_identifier is not required. + d.Set("skip_final_snapshot", true) - if err != nil { - if tfawserr.ErrCodeEquals(err, redshift.ErrCodeClusterNotFoundFault) { - return 42, "destroyed", nil - } - log.Printf("[WARN] Error on retrieving Redshift Cluster (%s) when waiting: %s", id, err) - return nil, "", err - } + return []*schema.ResourceData{d}, nil +} - var rsc *redshift.Cluster +func enableLogging(conn *redshift.Redshift, clusterID string, tfMap map[string]interface{}) error { + bucketName, ok := tfMap["bucket_name"].(string) - for _, c := range resp.Clusters { - if *c.ClusterIdentifier == id { - rsc = c - } - } + if !ok || bucketName == "" { + return fmt.Errorf("`bucket_name` must be set when enabling logging for Redshift Clusters") + } - if rsc == nil { - return 42, "destroyed", nil - } + input := &redshift.EnableLoggingInput{ + BucketName: aws.String(bucketName), + ClusterIdentifier: aws.String(clusterID), + } - if rsc.ClusterStatus != nil { - log.Printf("[DEBUG] Redshift Cluster status (%s): %s", id, *rsc.ClusterStatus) - } + if v, ok := tfMap["s3_key_prefix"].(string); ok && v != "" { + input.S3KeyPrefix = aws.String(v) + } + + _, err := tfresource.RetryWhenAWSErrCodeEquals( + clusterInvalidClusterStateFaultTimeout, + func() (interface{}, error) { + return conn.EnableLogging(input) + }, + redshift.ErrCodeInvalidClusterStateFault, + ) + + if err != nil { + return fmt.Errorf("error enabling Redshift Cluster (%s) logging: %w", clusterID, err) + } + + return nil +} - return rsc, *rsc.ClusterStatus, nil +func enableSnapshotCopy(conn *redshift.Redshift, clusterID string, tfMap map[string]interface{}) error { + input := &redshift.EnableSnapshotCopyInput{ + ClusterIdentifier: aws.String(clusterID), + DestinationRegion: aws.String(tfMap["destination_region"].(string)), } + + if v, ok := tfMap["retention_period"]; ok { + input.RetentionPeriod = aws.Int64(int64(v.(int))) + } + + if v, ok := tfMap["grant_name"]; ok { + input.SnapshotCopyGrantName = aws.String(v.(string)) + } + + _, err := conn.EnableSnapshotCopy(input) + + if err != nil { + return fmt.Errorf("error enabling Redshift Cluster (%s) snapshot copy: %w", clusterID, err) + } + + return nil } func flattenRedshiftClusterNode(apiObject *redshift.ClusterNode) map[string]interface{} { diff --git a/internal/service/redshift/enum.go b/internal/service/redshift/enum.go index 08360ae2c60..1dc10a665b1 100644 --- a/internal/service/redshift/enum.go +++ b/internal/service/redshift/enum.go @@ -19,6 +19,7 @@ const ( clusterStatusModifying = "modifying" clusterStatusPaused = "paused" clusterStatusRebooting = "rebooting" + clusterStatusRecovering = "recovering" clusterStatusRenaming = "renaming" clusterStatusResizing = "resizing" clusterStatusRestoring = "restoring" diff --git a/internal/service/redshift/wait.go b/internal/service/redshift/wait.go index 05026099f8f..b9fc5c96a5d 100644 --- a/internal/service/redshift/wait.go +++ b/internal/service/redshift/wait.go @@ -62,6 +62,32 @@ func waitClusterDeleted(conn *redshift.Redshift, id string, timeout time.Duratio return nil, err } +func waitClusterUpdated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{ + clusterStatusAvailablePrepForResize, + clusterStatusCreating, + clusterStatusDeleting, + clusterStatusModifying, + clusterStatusRebooting, + clusterStatusRecovering, + clusterStatusRenaming, + clusterStatusResizing, + }, + Target: []string{clusterStatusAvailable}, + Refresh: statusCluster(conn, id), + Timeout: timeout, + } + + outputRaw, err := stateConf.WaitForState() + + if output, ok := outputRaw.(*redshift.Cluster); ok { + return output, err + } + + return nil, err +} + func waitClusterRelocationStatusResolved(conn *redshift.Redshift, id string) (*redshift.Cluster, error) { //nolint:unparam stateConf := &resource.StateChangeConf{ Pending: clusterAvailabilityZoneRelocationStatus_PendingValues(), From 26eb4dfc34605a995847038c6d3e8bd14df5858d Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 28 Mar 2022 09:34:42 -0400 Subject: [PATCH 10/16] Empty file. --- internal/service/redshift/cluster_snapshot_test.go | 1 - 1 file changed, 1 deletion(-) delete mode 100644 internal/service/redshift/cluster_snapshot_test.go diff --git a/internal/service/redshift/cluster_snapshot_test.go b/internal/service/redshift/cluster_snapshot_test.go deleted file mode 100644 index 86c1bb2273c..00000000000 --- a/internal/service/redshift/cluster_snapshot_test.go +++ /dev/null @@ -1 +0,0 @@ -package redshift_test From 36ad17a411223ab150b6ba250f2a14339298c5f6 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 28 Mar 2022 10:59:53 -0400 Subject: [PATCH 11/16] Don't use 'CustomizeDiff' for skip_final_snapshot/final_snapshot_identifier check. --- internal/service/redshift/cluster.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index e8e2f4481db..b86b388adb7 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -320,12 +320,6 @@ func ResourceCluster() *schema.Resource { CustomizeDiff: customdiff.All( verify.SetTagsDiff, - func(_ context.Context, diff *schema.ResourceDiff, v interface{}) error { - if !diff.Get("skip_final_snapshot").(bool) && diff.Get("final_snapshot_identifier").(string) == "" { - return errors.New("`final_snapshot_identifier` must be set when `skip_final_snapshot` is false") - } - return nil - }, func(_ context.Context, diff *schema.ResourceDiff, v interface{}) error { if diff.Get("availability_zone_relocation_enabled").(bool) && diff.Get("publicly_accessible").(bool) { return errors.New("`availability_zone_relocation_enabled` cannot be true when `publicly_accessible` is true") @@ -865,9 +859,18 @@ func resourceClusterUpdate(d *schema.ResourceData, meta interface{}) error { func resourceClusterDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).RedshiftConn + skipFinalSnapshot := d.Get("skip_final_snapshot").(bool) input := &redshift.DeleteClusterInput{ ClusterIdentifier: aws.String(d.Id()), - SkipFinalClusterSnapshot: aws.Bool(d.Get("skip_final_snapshot").(bool)), + SkipFinalClusterSnapshot: aws.Bool(skipFinalSnapshot), + } + + if !skipFinalSnapshot { + if v, ok := d.GetOk("final_snapshot_identifier"); ok { + input.FinalClusterSnapshotIdentifier = aws.String(v.(string)) + } else { + return fmt.Errorf("Redshift Cluster Instance FinalSnapshotIdentifier is required when a final snapshot is required") + } } log.Printf("[DEBUG] Deleting Redshift Cluster: %s", d.Id()) From 5263d7879671dbe9516c70becb6833f2b0a54b36 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 28 Mar 2022 11:02:37 -0400 Subject: [PATCH 12/16] r/aws_redshift_cluster: Use ClusterAvailabilityStatus when waiting for cluster state changes (#23638). Acceptance test output: % make testacc TESTS=TestAccRedshiftCluster_ PKG=redshift ACCTEST_PARALLELISM=3 ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./internal/service/redshift/... -v -count 1 -parallel 3 -run='TestAccRedshiftCluster_' -timeout 180m === RUN TestAccRedshiftCluster_basic === PAUSE TestAccRedshiftCluster_basic === RUN TestAccRedshiftCluster_disappears === PAUSE TestAccRedshiftCluster_disappears === RUN TestAccRedshiftCluster_withFinalSnapshot === PAUSE TestAccRedshiftCluster_withFinalSnapshot === RUN TestAccRedshiftCluster_kmsKey === PAUSE TestAccRedshiftCluster_kmsKey === RUN TestAccRedshiftCluster_enhancedVPCRoutingEnabled === PAUSE TestAccRedshiftCluster_enhancedVPCRoutingEnabled === RUN TestAccRedshiftCluster_loggingEnabled === PAUSE TestAccRedshiftCluster_loggingEnabled === RUN TestAccRedshiftCluster_snapshotCopy === PAUSE TestAccRedshiftCluster_snapshotCopy === RUN TestAccRedshiftCluster_iamRoles === PAUSE TestAccRedshiftCluster_iamRoles === RUN TestAccRedshiftCluster_publiclyAccessible === PAUSE TestAccRedshiftCluster_publiclyAccessible === RUN TestAccRedshiftCluster_updateNodeCount === PAUSE TestAccRedshiftCluster_updateNodeCount === RUN TestAccRedshiftCluster_updateNodeType === PAUSE TestAccRedshiftCluster_updateNodeType === RUN TestAccRedshiftCluster_tags === PAUSE TestAccRedshiftCluster_tags === RUN TestAccRedshiftCluster_forceNewUsername === PAUSE TestAccRedshiftCluster_forceNewUsername === RUN TestAccRedshiftCluster_changeAvailabilityZone === PAUSE TestAccRedshiftCluster_changeAvailabilityZone === RUN TestAccRedshiftCluster_changeAvailabilityZoneAndSetAvailabilityZoneRelocation === PAUSE TestAccRedshiftCluster_changeAvailabilityZoneAndSetAvailabilityZoneRelocation === RUN TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNotSet === PAUSE TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNotSet === RUN TestAccRedshiftCluster_changeEncryption1 === PAUSE TestAccRedshiftCluster_changeEncryption1 === RUN TestAccRedshiftCluster_changeEncryption2 === PAUSE TestAccRedshiftCluster_changeEncryption2 === RUN TestAccRedshiftCluster_availabilityZoneRelocation === PAUSE TestAccRedshiftCluster_availabilityZoneRelocation === RUN TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible === PAUSE TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible === CONT TestAccRedshiftCluster_basic === CONT TestAccRedshiftCluster_updateNodeType === CONT TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNotSet --- PASS: TestAccRedshiftCluster_basic (222.68s) === CONT TestAccRedshiftCluster_availabilityZoneRelocation --- PASS: TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNotSet (390.44s) === CONT TestAccRedshiftCluster_changeEncryption2 --- PASS: TestAccRedshiftCluster_availabilityZoneRelocation (456.19s) === CONT TestAccRedshiftCluster_changeEncryption1 --- PASS: TestAccRedshiftCluster_updateNodeType (1428.13s) === CONT TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible === CONT TestAccRedshiftCluster_changeAvailabilityZone --- PASS: TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible (2.04s) --- PASS: TestAccRedshiftCluster_changeAvailabilityZone (518.74s) === CONT TestAccRedshiftCluster_changeAvailabilityZoneAndSetAvailabilityZoneRelocation --- PASS: TestAccRedshiftCluster_changeEncryption2 (1651.28s) === CONT TestAccRedshiftCluster_loggingEnabled --- PASS: TestAccRedshiftCluster_changeEncryption1 (1640.72s) === CONT TestAccRedshiftCluster_iamRoles --- PASS: TestAccRedshiftCluster_loggingEnabled (336.16s) === CONT TestAccRedshiftCluster_snapshotCopy --- PASS: TestAccRedshiftCluster_changeAvailabilityZoneAndSetAvailabilityZoneRelocation (682.67s) === CONT TestAccRedshiftCluster_forceNewUsername --- PASS: TestAccRedshiftCluster_snapshotCopy (288.47s) === CONT TestAccRedshiftCluster_publiclyAccessible --- PASS: TestAccRedshiftCluster_iamRoles (402.34s) === CONT TestAccRedshiftCluster_updateNodeCount --- PASS: TestAccRedshiftCluster_publiclyAccessible (304.97s) === CONT TestAccRedshiftCluster_tags --- PASS: TestAccRedshiftCluster_forceNewUsername (503.93s) === CONT TestAccRedshiftCluster_kmsKey --- PASS: TestAccRedshiftCluster_tags (255.92s) === CONT TestAccRedshiftCluster_enhancedVPCRoutingEnabled --- PASS: TestAccRedshiftCluster_kmsKey (420.64s) === CONT TestAccRedshiftCluster_withFinalSnapshot --- PASS: TestAccRedshiftCluster_enhancedVPCRoutingEnabled (649.53s) === CONT TestAccRedshiftCluster_disappears --- PASS: TestAccRedshiftCluster_withFinalSnapshot (496.08s) --- PASS: TestAccRedshiftCluster_disappears (247.65s) --- PASS: TestAccRedshiftCluster_updateNodeCount (1669.39s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/redshift 4394.922s --- internal/service/redshift/cluster_test.go | 4 +- internal/service/redshift/enum.go | 8 ++++ internal/service/redshift/status.go | 16 ++++++++ internal/service/redshift/wait.go | 48 ++++++++--------------- 4 files changed, 43 insertions(+), 33 deletions(-) diff --git a/internal/service/redshift/cluster_test.go b/internal/service/redshift/cluster_test.go index 1586e975246..4f061156829 100644 --- a/internal/service/redshift/cluster_test.go +++ b/internal/service/redshift/cluster_test.go @@ -547,7 +547,7 @@ func TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNot }, { Config: testAccClusterConfig_updateAvailabilityZone_availabilityZoneRelocationNotSet(rName, 1), - ExpectError: regexp.MustCompile(`cannot change availability_zone if availability_zone_relocation_enabled is not true`), + ExpectError: regexp.MustCompile("cannot change `availability_zone` if `availability_zone_relocation_enabled` is not true"), }, }, }) @@ -663,7 +663,7 @@ func TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible(t *tes Steps: []resource.TestStep{ { Config: testAccClusterConfig_availabilityZoneRelocation_publiclyAccessible(rName), - ExpectError: regexp.MustCompile(`availability_zone_relocation_enabled can not be true when publicly_accessible is true`), + ExpectError: regexp.MustCompile("`availability_zone_relocation_enabled` cannot be true when `publicly_accessible` is true"), }, }, }) diff --git a/internal/service/redshift/enum.go b/internal/service/redshift/enum.go index 1dc10a665b1..2acaa786891 100644 --- a/internal/service/redshift/enum.go +++ b/internal/service/redshift/enum.go @@ -1,5 +1,13 @@ package redshift +const ( + clusterAvailabilityStatusAvailable = "Available" + clusterAvailabilityStatusFailed = "Failed" + clusterAvailabilityStatusMaintenance = "Maintenance" + clusterAvailabilityStatusModifying = "Modifying" + clusterAvailabilityStatusUnavailable = "Unavailable" +) + // https://docs.aws.amazon.com/redshift/latest/mgmt/working-with-clusters.html#rs-mgmt-cluster-status. //nolint:deadcode,varcheck // These constants are missing from the AWS SDK const ( diff --git a/internal/service/redshift/status.go b/internal/service/redshift/status.go index 63898f2afa6..6678b020fb5 100644 --- a/internal/service/redshift/status.go +++ b/internal/service/redshift/status.go @@ -23,6 +23,22 @@ func statusCluster(conn *redshift.Redshift, id string) resource.StateRefreshFunc } } +func statusClusterAvailability(conn *redshift.Redshift, id string) resource.StateRefreshFunc { + return func() (interface{}, string, error) { + output, err := FindClusterByID(conn, id) + + if tfresource.NotFound(err) { + return nil, "", nil + } + + if err != nil { + return nil, "", err + } + + return output, aws.StringValue(output.ClusterAvailabilityStatus), nil + } +} + func statusClusterAvailabilityZoneRelocation(conn *redshift.Redshift, id string) resource.StateRefreshFunc { return func() (interface{}, string, error) { output, err := FindClusterByID(conn, id) diff --git a/internal/service/redshift/wait.go b/internal/service/redshift/wait.go index b9fc5c96a5d..7132ef732c5 100644 --- a/internal/service/redshift/wait.go +++ b/internal/service/redshift/wait.go @@ -1,10 +1,13 @@ package redshift import ( + "errors" "time" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/redshift" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) const ( @@ -15,15 +18,9 @@ const ( func waitClusterCreated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { stateConf := &resource.StateChangeConf{ - Pending: []string{ - clusterStatusAvailablePrepForResize, - clusterStatusBackingUp, - clusterStatusCreating, - clusterStatusModifying, - clusterStatusRestoring, - }, - Target: []string{clusterStatusAvailable}, - Refresh: statusCluster(conn, id), + Pending: []string{clusterAvailabilityStatusModifying}, + Target: []string{clusterAvailabilityStatusAvailable}, + Refresh: statusClusterAvailability(conn, id), Timeout: timeout, MinTimeout: 10 * time.Second, } @@ -31,6 +28,8 @@ func waitClusterCreated(conn *redshift.Redshift, id string, timeout time.Duratio outputRaw, err := stateConf.WaitForState() if output, ok := outputRaw.(*redshift.Cluster); ok { + tfresource.SetLastError(err, errors.New(aws.StringValue(output.ClusterStatus))) + return output, err } @@ -39,23 +38,17 @@ func waitClusterCreated(conn *redshift.Redshift, id string, timeout time.Duratio func waitClusterDeleted(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { stateConf := &resource.StateChangeConf{ - Pending: []string{ - clusterStatusAvailable, - clusterStatusCreating, - clusterStatusDeleting, - clusterStatusFinalSnapshot, - clusterStatusRebooting, - clusterStatusRenaming, - clusterStatusResizing, - }, + Pending: []string{clusterAvailabilityStatusModifying}, Target: []string{}, - Refresh: statusCluster(conn, id), + Refresh: statusClusterAvailability(conn, id), Timeout: timeout, } outputRaw, err := stateConf.WaitForState() if output, ok := outputRaw.(*redshift.Cluster); ok { + tfresource.SetLastError(err, errors.New(aws.StringValue(output.ClusterStatus))) + return output, err } @@ -64,24 +57,17 @@ func waitClusterDeleted(conn *redshift.Redshift, id string, timeout time.Duratio func waitClusterUpdated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { stateConf := &resource.StateChangeConf{ - Pending: []string{ - clusterStatusAvailablePrepForResize, - clusterStatusCreating, - clusterStatusDeleting, - clusterStatusModifying, - clusterStatusRebooting, - clusterStatusRecovering, - clusterStatusRenaming, - clusterStatusResizing, - }, - Target: []string{clusterStatusAvailable}, - Refresh: statusCluster(conn, id), + Pending: []string{clusterAvailabilityStatusMaintenance, clusterAvailabilityStatusModifying, clusterAvailabilityStatusUnavailable}, + Target: []string{clusterAvailabilityStatusAvailable}, + Refresh: statusClusterAvailability(conn, id), Timeout: timeout, } outputRaw, err := stateConf.WaitForState() if output, ok := outputRaw.(*redshift.Cluster); ok { + tfresource.SetLastError(err, errors.New(aws.StringValue(output.ClusterStatus))) + return output, err } From 62b19b7b497e43f107739084933472cdd753ec22 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 28 Mar 2022 15:55:14 -0400 Subject: [PATCH 13/16] Add CHANGELOG entry. --- .changelog/13203.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/13203.txt diff --git a/.changelog/13203.txt b/.changelog/13203.txt new file mode 100644 index 00000000000..a7fc1f74261 --- /dev/null +++ b/.changelog/13203.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_redshift_cluster: Correctly use `number_of_nodes` argument value when restoring from snapshot +``` \ No newline at end of file From 163c727523086a1279cc6a91cb289a10ed239ef0 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 28 Mar 2022 17:40:55 -0400 Subject: [PATCH 14/16] r/aws_redshift_cluster: Correctly use 'number_of_nodes' when restoring from snapshot. Acceptance test output: % make testacc TESTS=TestAccRedshiftCluster_restoreFromSnapshot PKG=redshift ACCTEST_PARALLELISM=3 ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./internal/service/redshift/... -v -count 1 -parallel 3 -run='TestAccRedshiftCluster_restoreFromSnapshot' -timeout 180m === RUN TestAccRedshiftCluster_restoreFromSnapshot === PAUSE TestAccRedshiftCluster_restoreFromSnapshot === CONT TestAccRedshiftCluster_restoreFromSnapshot --- PASS: TestAccRedshiftCluster_restoreFromSnapshot (1408.34s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/redshift 1411.887s --- internal/service/redshift/cluster.go | 4 ++ internal/service/redshift/cluster_test.go | 79 +++++++++++++++++++++++ internal/service/redshift/wait.go | 2 +- 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index b86b388adb7..90da6e47e83 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -396,6 +396,10 @@ func resourceClusterCreate(d *schema.ResourceData, meta interface{}) error { input.KmsKeyId = aws.String(v.(string)) } + if v, ok := d.GetOk("number_of_nodes"); ok { + input.NumberOfNodes = aws.Int64(int64(v.(int))) + } + if v, ok := d.GetOk("owner_account"); ok { input.OwnerAccount = aws.String(v.(string)) } diff --git a/internal/service/redshift/cluster_test.go b/internal/service/redshift/cluster_test.go index 4f061156829..2b13c586860 100644 --- a/internal/service/redshift/cluster_test.go +++ b/internal/service/redshift/cluster_test.go @@ -669,6 +669,54 @@ func TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible(t *tes }) } +func TestAccRedshiftCluster_restoreFromSnapshot(t *testing.T) { + var v redshift.Cluster + resourceName := "aws_redshift_cluster.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, redshift.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckDestroyClusterSnapshot(rName), + Steps: []resource.TestStep{ + { + Config: testAccClusterCreateSnapshotConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckClusterExists(resourceName, &v), + resource.TestCheckResourceAttrPair(resourceName, "availability_zone", "data.aws_availability_zones.available", "names.0"), + resource.TestCheckResourceAttr(resourceName, "node_type", "dc2.8xlarge"), + resource.TestCheckResourceAttr(resourceName, "number_of_nodes", "2"), + ), + }, + // Apply a configuration without the source cluster to ensure final snapshot creation. + { + Config: acctest.ConfigAvailableAZsNoOptInExclude("usw2-az2"), + }, + { + Config: testAccClusterRestoreFromSnapshotConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckClusterExists(resourceName, &v), + resource.TestCheckResourceAttrPair(resourceName, "availability_zone", "data.aws_availability_zones.available", "names.1"), + resource.TestCheckResourceAttr(resourceName, "node_type", "dc2.large"), + resource.TestCheckResourceAttr(resourceName, "number_of_nodes", "8"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "final_snapshot_identifier", + "master_password", + "skip_final_snapshot", + "snapshot_identifier", + }, + }, + }, + }) +} + func testAccCheckClusterDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).RedshiftConn @@ -1488,3 +1536,34 @@ resource "aws_redshift_cluster" "test" { } `, rName)) } + +func testAccClusterCreateSnapshotConfig(rName string) string { + return acctest.ConfigCompose(acctest.ConfigAvailableAZsNoOptInExclude("usw2-az2"), fmt.Sprintf(` +resource "aws_redshift_cluster" "test" { + cluster_identifier = %[1]q + availability_zone = data.aws_availability_zones.available.names[0] + database_name = "mydb" + master_username = "foo_test" + master_password = "Mustbe8characters" + node_type = "dc2.8xlarge" + number_of_nodes = 2 + final_snapshot_identifier = %[1]q +} +`, rName)) +} + +func testAccClusterRestoreFromSnapshotConfig(rName string) string { + return acctest.ConfigCompose(acctest.ConfigAvailableAZsNoOptInExclude("usw2-az2"), fmt.Sprintf(` +resource "aws_redshift_cluster" "test" { + cluster_identifier = %[1]q + snapshot_identifier = %[1]q + availability_zone = data.aws_availability_zones.available.names[1] + database_name = "mydb" + master_username = "foo_test" + master_password = "Mustbe8characters" + node_type = "dc2.large" + number_of_nodes = 8 + skip_final_snapshot = true +} +`, rName)) +} diff --git a/internal/service/redshift/wait.go b/internal/service/redshift/wait.go index 7132ef732c5..3ebe522d095 100644 --- a/internal/service/redshift/wait.go +++ b/internal/service/redshift/wait.go @@ -18,7 +18,7 @@ const ( func waitClusterCreated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { stateConf := &resource.StateChangeConf{ - Pending: []string{clusterAvailabilityStatusModifying}, + Pending: []string{clusterAvailabilityStatusModifying, clusterAvailabilityStatusUnavailable}, Target: []string{clusterAvailabilityStatusAvailable}, Refresh: statusClusterAvailability(conn, id), Timeout: timeout, From 1c80646ca68fc7428ca94064ff259d8dc8b88818 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 29 Mar 2022 06:20:59 -0400 Subject: [PATCH 15/16] 'statusCluster' is unused. --- internal/service/redshift/status.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/internal/service/redshift/status.go b/internal/service/redshift/status.go index 6678b020fb5..0d58add3948 100644 --- a/internal/service/redshift/status.go +++ b/internal/service/redshift/status.go @@ -7,22 +7,6 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) -func statusCluster(conn *redshift.Redshift, id string) resource.StateRefreshFunc { - return func() (interface{}, string, error) { - output, err := FindClusterByID(conn, id) - - if tfresource.NotFound(err) { - return nil, "", nil - } - - if err != nil { - return nil, "", err - } - - return output, aws.StringValue(output.ClusterStatus), nil - } -} - func statusClusterAvailability(conn *redshift.Redshift, id string) resource.StateRefreshFunc { return func() (interface{}, string, error) { output, err := FindClusterByID(conn, id) From 9d2c01ffff360cf30a00aa17d23133a3b9f6323e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 29 Mar 2022 06:25:29 -0400 Subject: [PATCH 16/16] Fix golangci-lint errors. --- internal/service/redshift/cluster.go | 4 +--- internal/service/redshift/enum.go | 1 + internal/service/redshift/wait.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/service/redshift/cluster.go b/internal/service/redshift/cluster.go index 90da6e47e83..5ce188b60ab 100644 --- a/internal/service/redshift/cluster.go +++ b/internal/service/redshift/cluster.go @@ -894,9 +894,7 @@ func resourceClusterDelete(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf("error deleting Redshift Cluster (%s): %w", d.Id(), err) } - _, err = waitClusterDeleted(conn, d.Id(), d.Timeout(schema.TimeoutDelete)) - - if err != nil { + if _, err := waitClusterDeleted(conn, d.Id(), d.Timeout(schema.TimeoutDelete)); err != nil { return fmt.Errorf("error waiting for Redshift Cluster (%s) delete: %w", d.Id(), err) } diff --git a/internal/service/redshift/enum.go b/internal/service/redshift/enum.go index 2acaa786891..6003b755f21 100644 --- a/internal/service/redshift/enum.go +++ b/internal/service/redshift/enum.go @@ -1,5 +1,6 @@ package redshift +//nolint:deadcode,varcheck // These constants are missing from the AWS SDK const ( clusterAvailabilityStatusAvailable = "Available" clusterAvailabilityStatusFailed = "Failed" diff --git a/internal/service/redshift/wait.go b/internal/service/redshift/wait.go index 3ebe522d095..698b7ee34d0 100644 --- a/internal/service/redshift/wait.go +++ b/internal/service/redshift/wait.go @@ -55,7 +55,7 @@ func waitClusterDeleted(conn *redshift.Redshift, id string, timeout time.Duratio return nil, err } -func waitClusterUpdated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { +func waitClusterUpdated(conn *redshift.Redshift, id string, timeout time.Duration) (*redshift.Cluster, error) { //nolint:unparam stateConf := &resource.StateChangeConf{ Pending: []string{clusterAvailabilityStatusMaintenance, clusterAvailabilityStatusModifying, clusterAvailabilityStatusUnavailable}, Target: []string{clusterAvailabilityStatusAvailable},