r/aws_elasticache_replication_group: Add support for transit_encryption_mode and enabling transit encryption on existing groups

[stefansundin]

Description

I don't know if my fix for #30402 is good or not since there might be a better way to do it that I did not find. It is working in my simple test case though.

Relations

Closes #30402
Closes #30700
Closes #33906

References

Here's various errors you can get.

If you try to enable encryption on an older Redis version:

Error: updating ElastiCache Replication Group (tftest-redis): InvalidParameterCombination: Transit encryption mode is not supported for engine version 6.2.6. Please use engine version 7.0.5 or higher.

Have to set transit_encryption_mode when enabling encryption:

Error: updating ElastiCache Replication Group (tftest-redis): InvalidParameterCombination: To modify transit encryption, please specify transit-encryption-mode.

Can't go straight to transit_encryption_mode = "required":

Error: updating ElastiCache Replication Group (tftest-redis): InvalidParameterCombination: Direct transition from transit-encryption-disabled to transit-encryption-enabled is not allowed. Update the cluster to transit-encryption-mode preferred prior to enabling transit encryption.

Output from Acceptance Testing

I don't have time to work on updating the acceptance tests at the moment.

[github-actions]

Community Note

Voting for Prioritization

• Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
• Please see our prioritization guide for information on how we prioritize.
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

• Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
• For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
• Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

[stefansundin]

Rebased

[teddy-wahle]

Let's merge this!

I really need this.

[schammah]

when is it planned to be merged at?

[stefansundin]

@schammah Unfortunately HashiCorp seems to have some issues handling community contributions. In the past I have had multiple PRs open for 12+ months before they get any attention from HashiCorp employees.

I would call everyone to upvote the top comment in this PR by clicking the 👍 button. This PR is currently number 10 if you sort PRs by the number of upvotes. https://github.com/hashicorp/terraform-provider-aws/pulls?q=is%3Apr+is%3Aopen+sort%3Areactions-%2B1-desc

If anyone here works at a company that is paying money to HashiCorp then I would suggest that you try and send a link to this PR to your HashiCorp representative and ask them to prioritize merging this. I think this is the only surefire way of getting it merged faster.

We're currently at 232 days since this PR was opened.

[stefansundin]

We're now at 262 days since this PR was opened and it is now in the number 3 spot in the upvote ranking now. But evidently the upvotes don't actually matter since I see no extra attention paid to this PR. 🤷

I just opened the same PR to the opentofu fork. I'm not even sure if they accept contributions yet, but let's see if it moves faster. opentofu#36

Edit: well, that was disappointing.

[JTaylor-myenergi]

I'm guessing this is getting a low priority because the acceptance tests haven't been run, or updated. We could potentially help with this if you don't have time?

[stefansundin]

@JTaylor-myenergi I'm willing to pull updated tests into this branch. You can fork my branch and post a comment here with a link to it when you have updated the tests, and I'll cherry-pick your commits.

Or open a PR to the branch in my fork, whatever works best for you.

[JTaylor-myenergi]

@stefansundin I had a quick look at this but found a large number of pre-existing tests are failing here so this probably needs some further work.

[jar-b]

LGTM 🎉

% make testacc PKG=elasticache TESTS="TestAccElastiCacheReplicationGroup_" ACCTEST_PARALLELISM=8
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.21.8 test ./internal/service/elasticache/... -v -count 1 -parallel 8 -run='TestAccElastiCacheReplicationGroup_'  -timeout 360m

--- PASS: TestAccElastiCacheReplicationGroup_clusteringAndCacheNodesCausesError (1.73s)
=== CONT  TestAccElastiCacheReplicationGroup_Validation_globalReplicationGroupIdAndNodeType
--- PASS: TestAccElastiCacheReplicationGroup_Validation_noNodeType (6.56s)
=== CONT  TestAccElastiCacheReplicationGroup_finalSnapshot
--- PASS: TestAccElastiCacheReplicationGroup_basic (765.44s)
=== CONT  TestAccElastiCacheReplicationGroup_autoMinorVersionUpgrade
--- PASS: TestAccElastiCacheReplicationGroup_multiAzNotInVPC (787.55s)
=== CONT  TestAccElastiCacheReplicationGroup_updateMaintenanceWindow
--- PASS: TestAccElastiCacheReplicationGroup_Validation_globalReplicationGroupIdAndNodeType (888.98s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappears_addMemberCluster
--- PASS: TestAccElastiCacheReplicationGroup_tags (935.00s)
=== CONT  TestAccElastiCacheReplicationGroup_vpc
--- PASS: TestAccElastiCacheReplicationGroup_finalSnapshot (959.36s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappearsRemoveMemberCluster_scaleDown
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersFailover_autoFailoverDisabled (1298.62s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappearsRemoveMemberCluster_atTargetSize
=== CONT  TestAccElastiCacheReplicationGroup_stateUpgrade5270
--- PASS: TestAccElastiCacheReplicationGroup_updateMaintenanceWindow (761.09s)
--- PASS: TestAccElastiCacheReplicationGroup_autoMinorVersionUpgrade (824.35s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClusters_multiAZEnabled
--- PASS: TestAccElastiCacheReplicationGroup_vpc (991.71s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappears_noChange
--- PASS: TestAccElastiCacheReplicationGroup_GlobalReplicationGroupID_basic (2246.99s)
=== CONT  TestAccElastiCacheReplicationGroup_authToken
--- PASS: TestAccElastiCacheReplicationGroup_stateUpgrade5270 (850.05s)
=== CONT  TestAccElastiCacheReplicationGroup_updateParameterGroup
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappears_addMemberCluster (1509.31s)
=== CONT  TestAccElastiCacheReplicationGroup_dataTiering
=== CONT  TestAccElastiCacheReplicationGroup_updateNodeSize
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappearsRemoveMemberCluster_atTargetSize (1105.17s)
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappearsRemoveMemberCluster_scaleDown (1460.95s)
=== CONT  TestAccElastiCacheReplicationGroup_Engine_Redis_LogDeliveryConfigurations_ClusterMode_Enabled
--- PASS: TestAccElastiCacheReplicationGroup_GlobalReplicationGroupID_full (2863.52s)
=== CONT  TestAccElastiCacheReplicationGroup_updateUserGroups
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClusters_multiAZEnabled (1502.14s)
=== CONT  TestAccElastiCacheReplicationGroup_Engine_Redis_LogDeliveryConfigurations_ClusterMode_Disabled
--- PASS: TestAccElastiCacheReplicationGroup_dataTiering (732.11s)
=== CONT  TestAccElastiCacheReplicationGroup_GlobalReplicationGroupIDClusterMode_basic
--- PASS: TestAccElastiCacheReplicationGroup_updateParameterGroup (922.30s)
=== CONT  TestAccElastiCacheReplicationGroup_GlobalReplicationGroupIDClusterModeValidation_numNodeGroupsOnSecondary
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersMemberClusterDisappears_noChange (1498.39s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClustersFailover_autoFailoverEnabled
--- PASS: TestAccElastiCacheReplicationGroup_Engine_Redis_LogDeliveryConfigurations_ClusterMode_Enabled (1026.32s)
=== CONT  TestAccElastiCacheReplicationGroup_EngineVersion_update
--- PASS: TestAccElastiCacheReplicationGroup_updateNodeSize (1368.18s)
=== CONT  TestAccElastiCacheReplicationGroup_disappears
--- PASS: TestAccElastiCacheReplicationGroup_authToken (1533.32s)
=== CONT  TestAccElastiCacheReplicationGroup_EngineVersion_6xToRealVersion
--- PASS: TestAccElastiCacheReplicationGroup_Engine_Redis_LogDeliveryConfigurations_ClusterMode_Disabled (721.71s)
=== CONT  TestAccElastiCacheReplicationGroup_updateDescription
--- PASS: TestAccElastiCacheReplicationGroup_updateUserGroups (975.68s)
=== CONT  TestAccElastiCacheReplicationGroup_GlobalReplicationGroupID_disappears
--- PASS: TestAccElastiCacheReplicationGroup_disappears (144.42s)
=== CONT  TestAccElastiCacheReplicationGroup_uppercase
--- PASS: TestAccElastiCacheReplicationGroup_updateDescription (115.92s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterMode_nonClusteredParameterGroup
--- PASS: TestAccElastiCacheReplicationGroup_GlobalReplicationGroupIDClusterModeValidation_numNodeGroupsOnSecondary (613.00s)
=== CONT  TestAccElastiCacheReplicationGroup_EngineVersion_v7
--- PASS: TestAccElastiCacheReplicationGroup_EngineVersion_6xToRealVersion (154.05s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterMode_singleNode
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClustersFailover_autoFailoverEnabled (667.28s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterMode_updateReplicasPerNodeGroup
--- PASS: TestAccElastiCacheReplicationGroup_uppercase (198.63s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroups_scaleDown
--- PASS: TestAccElastiCacheReplicationGroup_ClusterMode_singleNode (547.05s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroupsAndReplicasPerNodeGroup_scaleDown
--- PASS: TestAccElastiCacheReplicationGroup_EngineVersion_v7 (589.39s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroups_scaleUp
--- PASS: TestAccElastiCacheReplicationGroup_ClusterMode_nonClusteredParameterGroup (598.99s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroupsAndReplicasPerNodeGroup_scaleUp
--- PASS: TestAccElastiCacheReplicationGroup_GlobalReplicationGroupIDClusterMode_basic (1679.37s)
=== CONT  TestAccElastiCacheReplicationGroup_basic_v5
--- PASS: TestAccElastiCacheReplicationGroup_GlobalReplicationGroupID_disappears (1512.96s)
=== CONT  TestAccElastiCacheReplicationGroup_tagWithOtherModification
--- PASS: TestAccElastiCacheReplicationGroup_basic_v5 (796.13s)
=== CONT  TestAccElastiCacheReplicationGroup_transitEncryption7x
--- PASS: TestAccElastiCacheReplicationGroup_ClusterMode_updateReplicasPerNodeGroup (1690.47s)
=== CONT  TestAccElastiCacheReplicationGroup_ValidationMultiAz_noAutomaticFailover
--- PASS: TestAccElastiCacheReplicationGroup_ValidationMultiAz_noAutomaticFailover (1.18s)
=== CONT  TestAccElastiCacheReplicationGroup_NumberCacheClusters_basic
--- PASS: TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroups_scaleDown (1792.18s)
=== CONT  TestAccElastiCacheReplicationGroup_ClusterMode_basic
--- PASS: TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroups_scaleUp (1707.04s)
=== CONT  TestAccElastiCacheReplicationGroup_useCMKKMSKeyID
--- PASS: TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroupsAndReplicasPerNodeGroup_scaleDown (2216.28s)
=== CONT  TestAccElastiCacheReplicationGroup_networkType
--- PASS: TestAccElastiCacheReplicationGroup_ClusterModeUpdateNumNodeGroupsAndReplicasPerNodeGroup_scaleUp (2237.06s)
=== CONT  TestAccElastiCacheReplicationGroup_enableAtRestEncryption
--- PASS: TestAccElastiCacheReplicationGroup_ClusterMode_basic (879.29s)
=== CONT  TestAccElastiCacheReplicationGroup_ipDiscovery
--- PASS: TestAccElastiCacheReplicationGroup_useCMKKMSKeyID (727.13s)
=== CONT  TestAccElastiCacheReplicationGroup_transitEncryptionWithAuthToken
--- PASS: TestAccElastiCacheReplicationGroup_EngineVersion_update (3748.93s)
=== CONT  TestAccElastiCacheReplicationGroup_multiAzInVPC
--- PASS: TestAccElastiCacheReplicationGroup_NumberCacheClusters_basic (1520.00s)
=== CONT  TestAccElastiCacheReplicationGroup_transitEncryption5x
--- PASS: TestAccElastiCacheReplicationGroup_tagWithOtherModification (2014.84s)
=== CONT  TestAccElastiCacheReplicationGroup_deprecatedAvailabilityZones_multiAzInVPC
--- PASS: TestAccElastiCacheReplicationGroup_enableAtRestEncryption (731.15s)
=== CONT  TestAccElastiCacheReplicationGroup_multiAzNotInVPC_repeated
--- PASS: TestAccElastiCacheReplicationGroup_ipDiscovery (898.31s)
=== CONT  TestAccElastiCacheReplicationGroup_enableSnapshotting
--- PASS: TestAccElastiCacheReplicationGroup_networkType (990.69s)
--- PASS: TestAccElastiCacheReplicationGroup_transitEncryptionWithAuthToken (730.96s)
--- PASS: TestAccElastiCacheReplicationGroup_multiAzNotInVPC_repeated (234.11s)
--- PASS: TestAccElastiCacheReplicationGroup_transitEncryption7x (2125.64s)
--- PASS: TestAccElastiCacheReplicationGroup_deprecatedAvailabilityZones_multiAzInVPC (366.87s)
--- PASS: TestAccElastiCacheReplicationGroup_multiAzInVPC (533.45s)
--- PASS: TestAccElastiCacheReplicationGroup_transitEncryption5x (525.81s)
--- PASS: TestAccElastiCacheReplicationGroup_enableSnapshotting (196.81s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/elasticache        7887.350s

[jar-b]

Thanks for your contribution, @stefansundin! 👍

We also appreciate your patience in getting this reviewed. We do our best to keep track and prioritize the most popular community items, but recognize we don't always get it right. I made a handful of small changes to your initial submission:

• Instead of explicitly checking for an available status in the read operation, I changed the waiter to accept a delay argument. This allows us to skip the initial delay in cases we expect the replication group to already be available (such as the read operation or checking for tagging operations to be completed), but continue to delay polling in cases we know it will take time (such as creation or modification).
• I added some context to the transit_encryption_enabled documentation to indicate a new resource is still forced when the Redis engine version is < 7.0.5.
• I added acceptance tests to cover workflows where the transit_encryption_enabled and transit_encryption_mode arguments are modified for both 5.x and 7.x engine versions, ensuring the replication group is re-created in the former case, but updated in-place for the latter.

We appreciate all the detail around potential error cases in the PR summary and related issues - it was helpful to ensure we got the test cases right. Thanks again!

[github-actions]

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!