resource/aws_rds_cluster_parameter_group and resource/aws_db_parameter_group: Restore ability to change parameter values

[camlow325]

Community Note

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

Closes #11846
Closes #13551

Release note for CHANGELOG:

* resource/aws_rds_cluster_parameter_group and resource/aws_db_parameter_group: Restore ability to change parameter values ([#11846](https://github.com/terraform-providers/terraform-provider-aws/issues/11846))

In #11540, support was added to the rds parameter group resources for resetting parameter values to AWS defaults when parameters are removed from config. A side-effect of these changes, however, was that parameters which remain in the config but whose values have been changed would also be reset to AWS defaults. This commit restores the ability for parameter values to be updated in the config while retaining the ability for parameters being removed from the config to be reset to AWS defaults.

Output from acceptance testing:

$  make testacc TEST=./aws TESTARGS='-run=TestAccAWSDBClusterParameterGroup_'
...
--- PASS: TestAccAWSDBClusterParameterGroup_disappears (18.01s)
--- PASS: TestAccAWSDBClusterParameterGroup_namePrefix (23.28s)
--- PASS: TestAccAWSDBClusterParameterGroup_only (23.40s)
--- PASS: TestAccAWSDBClusterParameterGroup_generatedName (23.42s)
--- PASS: TestAccAWSDBClusterParameterGroup_generatedName_Parameter (23.77s)
--- PASS: TestAccAWSDBClusterParameterGroup_namePrefix_Parameter (23.79s)
--- PASS: TestAccAWSDBClusterParameterGroup_withApplyMethod (23.89s)
--- PASS: TestAccAWSDBClusterParameterGroup_basic (86.64s)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDBParameterGroup_'
...
--- PASS: TestAccAWSDBParameterGroup_Disappears (18.18s)
--- PASS: TestAccAWSDBParameterGroup_generatedName (22.40s)
--- PASS: TestAccAWSDBParameterGroup_namePrefix (22.69s)
--- PASS: TestAccAWSDBParameterGroup_Only (23.40s)
--- PASS: TestAccAWSDBParameterGroup_MatchDefault (24.19s)
--- PASS: TestAccAWSDBParameterGroup_withApplyMethod (25.23s)
--- PASS: TestAccAWSDBParameterGroup_limit (45.08s)
--- PASS: TestAccAWSDBParameterGroup_basic (75.38s)

[deiga]

@lachlancooper Could this PR get a review? This is a very annoying bug

[lachlancooper]

@lachlancooper Could this PR get a review? This is a very annoying bug

Agreed, the bug has a rather high potential for disastrous impact on production systems. This fix looks sensible to me, but I'm not a maintainer. The contributing docs say a maintainer will review it, but not how to request a review.

@bflad: would you mind taking a look at this (since you reviewed the original PR)?

[mikkoc]

Any news on this? We're looking at adding parameter groups, but I'm very hesitant now given what I read.

[Olliepop]

Any news on this? We're looking at adding parameter groups, but I'm very hesitant now given what I read.

@mikkoc I recommend holding off on adding parameter groups until this is merged. Parameter values are reset to the default value on each apply, no matter what you specify. Until this PR is merged you must update the parameter value after applying a Terraform plan within the RDS console or with aws-cli.

The effect on production systems can be disastrous if you have a large number of parameter values which are not the RDS default.

[anGie44]

Hi @camlow325, thank you for providing this bug fix! I've left a couple comments related to the acceptance testing as that has changed in the provider recently. Overall, I would just recommend creating a new test with each of the new configs to test the new "update" behavior so we ensure we are not introducing a regression and we can attempt to keep the basic test as one that should only test for the minimum required fields, though understandably over time it has expanded quite a bit.

With regard to the code additions in both the db_parameter_group and rds_cluster_parameter_group resources, since the debug logs show that the ordering of the Update behavior transitions from Modifying then to Reset of the parameters, we could instead flip around that existing logic and that should allow a user to modify a param as intended without resetting soon after. This wouldn't require additional logic then what is already present in our codebase, just a shift in the ordering, but if you have any additional feedback or questions please let me know!

[camlow325]

Thanks for all of the review comments, @anGie44.

Overall, I would just recommend creating a new test with each of the new configs to test the new "update" behavior so we ensure we are not introducing a regression and we can attempt to keep the basic test as one that should only test for the minimum required fields, though understandably over time it has expanded quite a bit.

Sorry, just saw this comment after I reworked my previous test updates to use the TestCheckTypeSetElemNestedAttrs helper.Update had seemed like a part of Basic functionality for the resource but I agree that this test has a fair amount of other assertions that it's doing already. I can split this out into a new test.

With regard to the code additions in both the db_parameter_group and rds_cluster_parameter_group resources, since the debug logs show that the ordering of the Update behavior transitions from Modifying then to Reset of the parameters, we could instead flip around that existing logic and that should allow a user to modify a param as intended without resetting soon after. This wouldn't require additional logic then what is already present in our codebase, just a shift in the ordering, but if you have any additional feedback or questions please let me know!

This would be less turmoil than what I had added for the PR. @lachlancooper mentioned in #11846 (comment):

Assuming we want to keep the functionality of resetting parameters that are being removed, one extremely simple fix would be to swap the order of the modify and reset operations so that modify always "wins". That's still not ideal as the operations would happen non-atomically, meaning every changing parameter would get the wrong value for a brief period.

I agree with this comment and this is the reason why I had gone with the approach of ensuring that parameters that have changed are only updated once during an apply. I worry that by just flipping the logic that the initial reset of a parameter to its default value rather than the intended new value could have negative repercussions - especially for "apply immediately" parameters or if a network issue were to happen between the "reset" and "modify" that could indefinitely leave one or more parameters unintentionally reset to their default values.

[camlow325]

Overall, I would just recommend creating a new test with each of the new configs to test the new "update" behavior so we ensure we are not introducing a regression and we can attempt to keep the basic test as one that should only test for the minimum required fields, though understandably over time it has expanded quite a bit.

In 31e7b8f, I moved the new TestStep entries out into new test funcs - TestAccAWSDBParameterGroup_updateParameters TestAccAWSDBClusterParameterGroup_updateParameters - with separate configs for each.

[anGie44]

by just flipping the logic that the initial reset of a parameter to its default value rather than the intended new value could have negative repercussions

I think this is a great point @camlow325 ! and definitely well illustrated in @lachlancooper's comment. I'd go ahead with @dooferlad's #12112 (comment) . I would just nitpick on the use of *p.ParameterName , to avoid any unwanted panics (not very likely from the API response, but we have had cases in the past where struct fields can be nil) we can use the AWS SDK Go method aws.StringValue() to safely get the pointer's value.

[camlow325]

The changes from here on down in this file just rename the parameters variable to resetParameters in order to avoid conflicting with the previously defined parameters variable and make this code look more like the code in the resource_aws_db_parameter_group.go file.

[camlow325]

I'd go ahead with @dooferlad's #12112 (comment) . I would just nitpick on the use of *p.ParameterName , to avoid any unwanted panics (not very likely from the API response, but we have had cases in the past where struct fields can be nil) we can use the AWS SDK Go method aws.StringValue() to safely get the pointer's value.

I swapped in the new implementation in 1296450. For the p.ParameterName references, I just wrapped them with some nil checks to avoid adding an entry to or referencing an entry in the map for a nil value. That seemed a little better than just using a potential empty string key from aws.StringValue with the map. In any event, a nil value for p.ParameterValue would seem to be a sign of something bad/unintended.

[anGie44]

nice 👍 I forgot since technically we're expanding user input here (same in the rds cluster resource) and the "name" field within the parameter block is Required, we shouldn't ever come across a nil (as well i see the expandParameters actually skips for cases when "name" is an empty string) but i prefer erring on the side of caution when it comes to pointers.

[anGie44]

LGTM @camlow325 🚀 Thanks again!

Output of acceptance tests:

--- PASS: TestAccAWSDBParameterGroup_Disappears (38.53s)
--- PASS: TestAccAWSDBClusterParameterGroup_disappears (41.33s)
--- PASS: TestAccAWSDBClusterParameterGroup_generatedName (52.13s)
--- PASS: TestAccAWSDBParameterGroup_namePrefix (52.66s)
--- PASS: TestAccAWSDBClusterParameterGroup_namePrefix_Parameter (56.72s)
--- PASS: TestAccAWSDBClusterParameterGroup_only (57.32s)
--- PASS: TestAccAWSDBClusterParameterGroup_namePrefix (57.48s)
--- PASS: TestAccAWSDBParameterGroup_generatedName (58.02s)
--- PASS: TestAccAWSDBClusterParameterGroup_generatedName_Parameter (58.37s)
--- PASS: TestAccAWSDBParameterGroup_MatchDefault (58.84s)
--- PASS: TestAccAWSDBParameterGroup_withApplyMethod (60.38s)
--- PASS: TestAccAWSDBParameterGroup_Only (60.96s)
--- PASS: TestAccAWSDBClusterParameterGroup_withApplyMethod (71.76s)
--- PASS: TestAccAWSDBParameterGroup_updateParameters (78.54s)
--- PASS: TestAccAWSDBClusterParameterGroup_updateParameters (80.51s)
--- PASS: TestAccAWSDBParameterGroup_basic (80.41s)
--- PASS: TestAccAWSDBClusterParameterGroup_basic (88.65s)
--- PASS: TestAccAWSDBParameterGroup_limit (93.96s)

[ghost]

This has been released in version 3.3.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 for triage. Thanks!

[jamesmharvey]

@anGie44 I know it's a long shot, but is there any chance of getting this released as a pointfix in a v2.70.0? There are a significant number of breaking changes in v3.0.0, and we're still working through them.

[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!