enable changing encryption without creating new resource for redshift cluster

[sunilkumarmohanty]

Fixes #6857

Changes proposed in this pull request:

• Change Encryption without forcing for new resource

Output from acceptance testing:

make testacc TESTARGS='-run=TestAccAWSRedshiftCluster_changeEncryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSRedshiftCluster_changeEncryption -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSRedshiftCluster_changeEncryption
=== PAUSE TestAccAWSRedshiftCluster_changeEncryption
=== CONT  TestAccAWSRedshiftCluster_changeEncryption
--- PASS: TestAccAWSRedshiftCluster_changeEncryption (1555.59s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1555.623s

[bflad]

Hey @sunilkumarmohanty 👋 Thanks for submitting this! You found a fun false positive scenario that can occur in the acceptance testing framework. I'd be awesome if it was a one-liner in the resource code to remove ForceNew from an attribute. 😅

Presumably, the second TestStep of the new acceptance test is causing the Redshift cluster to be successfully recreated due to kms_key_id still being ForceNew. You can verify this by adding a TestCheck function that verifies that the cluster wasn't recreated:

func TestAccAWSRedshiftCluster_changeEncryption(t *testing.T) {
	var cluster1, cluster2 redshift.Cluster

	ri := acctest.RandInt()
	preConfig := testAccAWSRedshiftClusterConfig_basic(ri)
	postConfig := testAccAWSRedshiftClusterConfig_encrypted(ri)

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckAWSRedshiftClusterDestroy,
		Steps: []resource.TestStep{
			{
				Config: preConfig,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &cluster1),
					resource.TestCheckResourceAttr("aws_redshift_cluster.default", "encrypted", "false"),
				),
			},

			{
				Config: postConfig,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &cluster2),
					testAccCheckAWSRedshiftClusterNotRecreated(&cluster1, &cluster2),
					resource.TestCheckResourceAttr("aws_redshift_cluster.default", "encrypted", "true"),
				),
			},
		},
	})
}

func testAccCheckAWSRedshiftClusterNotRecreated(i, j *redshift.Cluster) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		if aws.TimeValue(i.ClusterCreateTime) != aws.TimeValue(j.ClusterCreateTime) {
			return errors.New("Redshift Cluster was recreated")
		}

		return nil
	}
}

To fix this and actually have the resource appropriately update Redshift clusters in place, you'll need to remove ForceNew: true from kms_key_id, then add the handling for both attributes in resourceAwsRedshiftClusterUpdate(). I would also setup the acceptance test to perform two update steps (one to encrypt, one to unencrypt) or preferably a just create a second acceptance test for testing the opposite of the first so the encrypt and unencrypt tests can run in parallel.

Hope this helps 🚀

[sunilkumarmohanty]

@bflad : As per https://docs.aws.amazon.com/redshift/latest/mgmt/changing-cluster-encryption.html, changing encryption migrates the cluster to a new cluster. Hence, checking of ClusterCreateTime will not yield correct result.

[bflad]

@sunilkumarmohanty this was a little tricky to figure out, but I found the only decent way for verifying that there was no data loss (e.g. Terraform calling destroy and create) with the Redshift API and its various identifying information was checking ClusterPublicKey, as this is always regenerated during Redshift cluster creation.

func testAccCheckAWSRedshiftClusterNotRecreated(i, j *redshift.Cluster) resource.TestCheckFunc {
  return func(s *terraform.State) error {
    // In lieu of some other uniquely identifying attribute from the API that always changes
    // when a cluster is destroyed and recreated with the same identifier, we use the SSH key
    // as it will get regenerated when a cluster is destroyed.
    // Certain update operations (e.g KMS encrypting a cluster) will change ClusterCreateTime.
    // Clusters with the same identifier can/will have an overlapping Endpoint.Address.
    if aws.StringValue(i.ClusterPublicKey) != aws.StringValue(j.ClusterPublicKey) {
      return errors.New("Redshift Cluster was recreated")
    }

    return nil
  }
}

With this in place, this check would now fail on the existing acceptance test since it would highlight the DESTROY/CREATE of the aws_redshift_cluster resource due to kms_key_id changing (being added) and kms_key_id being ForceNew: true.

The mentioned code changes above:

• Removing ForceNew: true from the kms_key_id attribute
• Adding Encrypted and KmsKeyId handling in resourceAwsRedshiftClusterUpdate:

  if d.HasChange("encrypted") {
    req.Encrypted = aws.Bool(d.Get("encrypted").(bool))
    requestUpdate = true
  }

  if d.Get("encrypted").(bool) && d.HasChange("kms_key_id") {
    req.KmsKeyId = aws.String(d.Get("kms_key_id").(string))
    requestUpdate = true
  }

This allows Terraform to successfully in-place KMS encrypt an unencrypted Redshift cluster in the acceptance testing without data loss (and theoretically unencrypt a KMS encrypted Redshift cluster as well although Redshift seems to have some strange internal bugs with this behavior from my testing).

Please let me know if you can finish the implementation given the above or if you would like me to submit a commit after yours. Thanks!

[sunilkumarmohanty]

@bflad : I am also facing issue with testing. I get messages from AWS saying the key is in pending deletion state. Terraform seems to be deleting the kms key before the redshift encryption is disabled. I am running the test now with your suggested changes. Let's see. It takes a while for it to get complete.

[bflad]

It was taking ~45 minutes with my runs. For now the support and testing for enabling encryption is more important (likely the more common use case) than removing encryption. We can leave a placeholder test with t.Skip() and a decent message explaining why the test is being skipped if necessary. 👍

[sunilkumarmohanty]

I want to try once more by not deleting the key from the config.

[sunilkumarmohanty]

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSRedshiftCluster_changeEncryption -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSRedshiftCluster_changeEncryption1
=== PAUSE TestAccAWSRedshiftCluster_changeEncryption1
=== RUN   TestAccAWSRedshiftCluster_changeEncryption2
=== PAUSE TestAccAWSRedshiftCluster_changeEncryption2
=== CONT  TestAccAWSRedshiftCluster_changeEncryption1
=== CONT  TestAccAWSRedshiftCluster_changeEncryption2
--- PASS: TestAccAWSRedshiftCluster_changeEncryption2 (2848.28s)
--- PASS: TestAccAWSRedshiftCluster_changeEncryption1 (2898.87s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       2898.905s

[sunilkumarmohanty]

@bflad: After long hours and sleepless night, hope this is ok now.

[bflad]

LGTM, thanks @sunilkumarmohanty 🚀

--- PASS: TestAccAWSRedshiftCluster_snapshotCopy (571.14s)
--- PASS: TestAccAWSRedshiftCluster_loggingEnabledDeprecated (573.64s)
--- PASS: TestAccAWSRedshiftCluster_tags (581.72s)
--- PASS: TestAccAWSRedshiftCluster_publiclyAccessible (583.37s)
--- PASS: TestAccAWSRedshiftCluster_importBasic (604.84s)
--- PASS: TestAccAWSRedshiftCluster_basic (628.01s)
--- PASS: TestAccAWSRedshiftCluster_loggingEnabled (631.82s)
--- PASS: TestAccAWSRedshiftCluster_kmsKey (645.41s)
--- PASS: TestAccAWSRedshiftCluster_iamRoles (726.23s)
--- PASS: TestAccAWSRedshiftCluster_enhancedVpcRoutingEnabled (748.42s)
--- PASS: TestAccAWSRedshiftCluster_withFinalSnapshot (763.32s)
--- PASS: TestAccAWSRedshiftCluster_changeAvailabilityZone (1159.66s)
--- PASS: TestAccAWSRedshiftCluster_forceNewUsername (1162.57s)
--- PASS: TestAccAWSRedshiftCluster_updateNodeType (2189.39s)
--- PASS: TestAccAWSRedshiftCluster_changeEncryption2 (2201.27s)
--- PASS: TestAccAWSRedshiftCluster_updateNodeCount (2412.41s)
--- PASS: TestAccAWSRedshiftCluster_changeEncryption1 (2727.70s)

[bflad]

This has been released in version 1.54.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!