r/aws_redshift_cluster: allow snapshot_copy modifications (#36655)

This change fixes an issue preventing clusters with the `snapshot_copy` block configured from updating any nested arguments.

Before:

```console
% make testacc PKG=redshift TESTS=TestAccRedshiftCluster_snapshotCopy
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.21.8 test ./internal/service/redshift/... -v -count 1 -parallel 20 -run='TestAccRedshiftCluster_snapshotCopy'  -timeout 360m
=== RUN   TestAccRedshiftCluster_snapshotCopy
=== PAUSE TestAccRedshiftCluster_snapshotCopy
=== CONT  TestAccRedshiftCluster_snapshotCopy
    cluster_test.go:311: Step 2/3 error: Error running apply: exit status 1

        Error: updating Redshift Cluster (tf-acc-test-3261690877562655687) snapshot_copy: enabling snapshot copy: SnapshotCopyAlreadyEnabledFault: Snapshot Copy is already enabled on Cluster tf-acc-test-3261690877562655687
                status code: 400, request id: cf5d914b-3146-4a66-8335-a44153800324

          with aws_redshift_cluster.test,
          on terraform_plugin_test.tf line 33, in resource aws_redshift_cluster test:
          33: resource aws_redshift_cluster test {

--- FAIL: TestAccRedshiftCluster_snapshotCopy (428.46s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-aws/internal/service/redshift   434.035s
FAIL
make: *** [testacc] Error 1
```

After:

```console
% make testacc PKG=redshift TESTS=TestAccRedshiftCluster_snapshotCopy
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.21.8 test ./internal/service/redshift/... -v -count 1 -parallel 20 -run='TestAccRedshiftCluster_snapshotCopy'  -timeout 360m
=== RUN   TestAccRedshiftCluster_snapshotCopy
=== PAUSE TestAccRedshiftCluster_snapshotCopy
=== CONT  TestAccRedshiftCluster_snapshotCopy
--- PASS: TestAccRedshiftCluster_snapshotCopy (431.00s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/redshift   436.505s
```

```console
% make testacc PKG=redshift TESTS=TestAccRedshiftCluster_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.21.8 test ./internal/service/redshift/... -v -count 1 -parallel 20 -run='TestAccRedshiftCluster_'  -timeout 360m

--- PASS: TestAccRedshiftCluster_availabilityZoneRelocation_publiclyAccessible (382.40s)
=== CONT  TestAccRedshiftCluster_disappears
--- PASS: TestAccRedshiftCluster_changeAvailabilityZone_availabilityZoneRelocationNotSet (382.71s)
=== CONT  TestAccRedshiftCluster_withFinalSnapshot
--- PASS: TestAccRedshiftCluster_basic (418.72s)
=== CONT  TestAccRedshiftCluster_aqua
--- PASS: TestAccRedshiftCluster_kmsKey (432.77s)
=== CONT  TestAccRedshiftCluster_loggingEnabled
--- PASS: TestAccRedshiftCluster_availabilityZoneRelocation (444.95s)
=== CONT  TestAccRedshiftCluster_enhancedVPCRoutingEnabled
--- PASS: TestAccRedshiftCluster_manageMasterPassword (450.88s)
--- PASS: TestAccRedshiftCluster_publiclyAccessible (452.54s)
--- PASS: TestAccRedshiftCluster_iamRoles (459.36s)
--- PASS: TestAccRedshiftCluster_snapshotCopy (460.15s)
--- PASS: TestAccRedshiftCluster_tags (551.91s)
--- PASS: TestAccRedshiftCluster_changeAvailabilityZoneAndSetAvailabilityZoneRelocation (766.95s)
--- PASS: TestAccRedshiftCluster_changeAvailabilityZone (767.07s)
--- PASS: TestAccRedshiftCluster_disappears (416.75s)
--- PASS: TestAccRedshiftCluster_withFinalSnapshot (463.58s)
--- PASS: TestAccRedshiftCluster_loggingEnabled (446.24s)
--- PASS: TestAccRedshiftCluster_forceNewUsername (902.68s)
--- PASS: TestAccRedshiftCluster_multiAZ (944.06s)
--- PASS: TestAccRedshiftCluster_aqua (529.97s)
--- PASS: TestAccRedshiftCluster_restoreFromSnapshotARN (1011.97s)
--- PASS: TestAccRedshiftCluster_restoreFromSnapshot (1044.51s)
--- PASS: TestAccRedshiftCluster_enhancedVPCRoutingEnabled (623.74s)
--- PASS: TestAccRedshiftCluster_updateNodeCount (1314.86s)
--- PASS: TestAccRedshiftCluster_changeEncryption2 (1351.20s)
--- PASS: TestAccRedshiftCluster_changeEncryption1 (1489.08s)
--- PASS: TestAccRedshiftCluster_updateNodeType (1531.73s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/redshift   1537.226s
```

.changelog/36655.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_redshift_cluster: Fix error preventing modification of a configured `snapshot_copy` block
+```

internal/service/redshift/cluster.go

@@ -917,11 +917,16 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, meta int
 	if d.HasChange("snapshot_copy") {
 		if v, ok := d.GetOk("snapshot_copy"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {
 			if err := enableSnapshotCopy(ctx, conn, d.Id(), v.([]interface{})[0].(map[string]interface{})); err != nil {
-				return sdkdiag.AppendErrorf(diags, "updating Redshift Cluster (%s): %s", d.Id(), err)
+				if !tfawserr.ErrCodeEquals(err, redshift.ErrCodeSnapshotCopyAlreadyEnabledFault) {
+					return sdkdiag.AppendErrorf(diags, "updating Redshift Cluster (%s) snapshot_copy: %s", d.Id(), err)
+				}
+				if err := toggleSnapshotCopy(ctx, conn, d.Id(), v.([]interface{})[0].(map[string]interface{})); err != nil {
+					return sdkdiag.AppendErrorf(diags, "updating Redshift Cluster (%s) snapshot_copy: %s", d.Id(), err)
+				}
 			}
 		} else {
 			if err := disableSnapshotCopy(ctx, conn, d.Id()); err != nil {
-				return sdkdiag.AppendErrorf(diags, "updating Redshift Cluster (%s): %s", d.Id(), err)
+				return sdkdiag.AppendErrorf(diags, "updating Redshift Cluster (%s) snapshot_copy: %s", d.Id(), err)
 			}
 		}
 	}
@@ -1121,6 +1126,23 @@ func disableSnapshotCopy(ctx context.Context, conn *redshift.Redshift, clusterID
 	return nil
 }
 
+// toggleSnapshotCopy calls disableSnapshotCopy followed by enableSnapshotCopy
+//
+// This workflow is necessary in cases where the existing snapshot copy configuration
+// needs to be updated. Once enabled, `EnableSnapshotCopy` cannot be called to update existing
+// settings. While the `ModifySnapshotCopyRetentionPeriod` API is available to update the
+// `retention_period` argument, there is no mechanism to update other arguments such
+// as `destination_region` or `snapshot_copy_grant_name` without disabling first.
+func toggleSnapshotCopy(ctx context.Context, conn *redshift.Redshift, clusterID string, tfMap map[string]interface{}) error {
+	if err := disableSnapshotCopy(ctx, conn, clusterID); err != nil {
+		return err
+	}
+	if err := enableSnapshotCopy(ctx, conn, clusterID, tfMap); err != nil {
+		return err
+	}
+	return nil
+}
+
 func flattenClusterNode(apiObject *redshift.ClusterNode) map[string]interface{} {
 	if apiObject == nil {
 		return nil

internal/service/redshift/cluster_test.go

@@ -318,13 +318,21 @@ func TestAccRedshiftCluster_snapshotCopy(t *testing.T) {
 		CheckDestroy:             testAccCheckClusterDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccClusterConfig_snapshotCopyEnabled(rName),
+				Config: testAccClusterConfig_snapshotCopyEnabled(rName, 1),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckClusterExists(ctx, resourceName, &v),
 					resource.TestCheckResourceAttrPair(resourceName, "snapshot_copy.0.destination_region", "data.aws_region.alternate", "name"),
 					resource.TestCheckResourceAttr(resourceName, "snapshot_copy.0.retention_period", "1"),
 				),
 			},
+			{
+				Config: testAccClusterConfig_snapshotCopyEnabled(rName, 3),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckClusterExists(ctx, resourceName, &v),
+					resource.TestCheckResourceAttrPair(resourceName, "snapshot_copy.0.destination_region", "data.aws_region.alternate", "name"),
+					resource.TestCheckResourceAttr(resourceName, "snapshot_copy.0.retention_period", "3"),
+				),
+			},
 			{
 				Config: testAccClusterConfig_snapshotCopyDisabled(rName),
 				Check: resource.ComposeTestCheckFunc(
@@ -1406,7 +1414,7 @@ resource "aws_redshift_cluster" "test" {
 `, rName))
 }
 
-func testAccClusterConfig_snapshotCopyEnabled(rName string) string {
+func testAccClusterConfig_snapshotCopyEnabled(rName string, retentionPeriod int) string {
 	return acctest.ConfigCompose(
 		acctest.ConfigMultipleRegionProvider(2),
 		acctest.ConfigAvailableAZsNoOptInExclude("usw2-az2"),
@@ -1427,12 +1435,12 @@ resource "aws_redshift_cluster" "test" {
 
   snapshot_copy {
     destination_region = data.aws_region.alternate.name
-    retention_period   = 1
+    retention_period   = %[2]d
   }
 
   skip_final_snapshot = true
 }
-`, rName))
+`, rName, retentionPeriod))
 }
 
 func testAccClusterConfig_tags1(rName, tagKey1, tagValue1 string) string {