Db param group case sensitivity

[kevincormier-toast]

aws_db_parameter_group.parameter.value has a number of weird edge cases around capitalization/casing. The long standing fix using lower() is causing problems with Parameter Formula Variables, which are case sensitive and use capital letters.

Each formula variable returns integer or a Boolean value. The names of the variables are case-sensitive.
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_WorkingWithParamGroups.html

This PR is a quick attempt to make the values case sensitive.

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

Relates OR Closes #0000

Release note for CHANGELOG:

resource_aws_db_parameter_group: parameter values are now case sensitive

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDBParameterGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDBParameterGroup_ -timeout 120m
=== RUN   TestAccAWSDBParameterGroup_basic
=== PAUSE TestAccAWSDBParameterGroup_basic
=== RUN   TestAccAWSDBParameterGroup_limit
=== PAUSE TestAccAWSDBParameterGroup_limit
=== RUN   TestAccAWSDBParameterGroup_Disappears
=== PAUSE TestAccAWSDBParameterGroup_Disappears
=== RUN   TestAccAWSDBParameterGroup_namePrefix
=== PAUSE TestAccAWSDBParameterGroup_namePrefix
=== RUN   TestAccAWSDBParameterGroup_generatedName
=== PAUSE TestAccAWSDBParameterGroup_generatedName
=== RUN   TestAccAWSDBParameterGroup_withApplyMethod
=== PAUSE TestAccAWSDBParameterGroup_withApplyMethod
=== RUN   TestAccAWSDBParameterGroup_Only
=== PAUSE TestAccAWSDBParameterGroup_Only
=== RUN   TestAccAWSDBParameterGroup_MatchDefault
=== PAUSE TestAccAWSDBParameterGroup_MatchDefault
=== CONT  TestAccAWSDBParameterGroup_basic
=== CONT  TestAccAWSDBParameterGroup_Only
=== CONT  TestAccAWSDBParameterGroup_Disappears
=== CONT  TestAccAWSDBParameterGroup_withApplyMethod
=== CONT  TestAccAWSDBParameterGroup_MatchDefault
=== CONT  TestAccAWSDBParameterGroup_namePrefix
=== CONT  TestAccAWSDBParameterGroup_generatedName
=== CONT  TestAccAWSDBParameterGroup_limit
--- PASS: TestAccAWSDBParameterGroup_Disappears (17.48s)
--- PASS: TestAccAWSDBParameterGroup_Only (22.46s)
--- PASS: TestAccAWSDBParameterGroup_namePrefix (23.85s)
--- PASS: TestAccAWSDBParameterGroup_generatedName (24.29s)
--- PASS: TestAccAWSDBParameterGroup_MatchDefault (25.54s)
--- PASS: TestAccAWSDBParameterGroup_withApplyMethod (28.40s)
--- PASS: TestAccAWSDBParameterGroup_limit (53.85s)
--- PASS: TestAccAWSDBParameterGroup_basic (67.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	69.285s

[kevincormier-toast]

Can anyone review this? Am I missing anything to get a review?

[kevincormier-toast]

@apparentlymart @bflad This was from a bit ago. Can I get some input here?

[bflad]

Hi @kevincormier-toast 👋 Thank you for submitting this.

I'm not directly familiar with the decision to lowercase these values to begin with, so for a future reviewer (not sure if it will be me or another maintainer) it may be super helpful to lookup and provide references to why the original code was written the way it was. Generally speaking, these lowercase value workarounds are for when the API is doing this type of canonicalization automatically from operator provided values on create/update. Removing that lowercasing may cause unexpected differences to show up when operators update their provider version if there are test cases we are missing.

From a more tactical perspective though some general comments:

• It would be very helpful to find or create a bug report issue outlining what this code change is solving (the PR description is likely a good start if creating one).
• We generally recommend adding tests and test configurations, rather than adjusting existing ones, to prevent any potential regressions.

Thanks again.

[kevincormier-toast]

@bflad looking over the history, I came to the same conclusion you did that this was in response to the API being case insensitive and always returning lowered results. If that was one the case, it is no longer true. I found some older issues from before this provider was split out of core terraform that hint at that being true, though nothing definitive. Either way, the api is now case preserving and for SOME values case sensitive. As far as existing issues, I don't know if the case sensitive variables used in the parameters are widely used yet. I haven't seen other complaints about the issue yet.

I don't believe making this case sensitive would actually affect operators in a negative way. These are the scenarios I've been able to think through. In each case, I think the change introduced is less impactful than the side effects of leaving the existing behavior.

Case Sensitive Var Used	Started Upper	Currently Upper	Description
❌	✅	✅	My experimentation show the plan will show no changes normally. When any value changes, anything with an upper suddenly shows as a change. This can cause unexpected behavior as terraform makes API calls to reset and update various parameters. If any of these parameters require reboot, it can cause any db using the parameter group to unnecessarily show that it needs a reboot to load pg changes. With this change, those values will stop erroneously showing as diffs.
❌	✅	❌	These folks are in a situation where the terraform code does not match what's actually in AWS. Despite the fact that they don't match, current state you'll never see a diff. With this code change, the diff will show in the next plan. If applied, this will result in a no-op with the same side effects as the above scenario, though only the first time it is applied.
❌	❌	❌	There's no change in behavior here
✅	✅	✅	This is identical to the first scenario
✅	✅	❌	This scenario is the most concerning. Not only does terraform not match AWS, but the terraform code would actually fail if the resource was tainted or copied with the current version. People can be sitting on bad terraform and not know for a long time until something forces a new resource to be created. This is the worst scenario where the error can be introduced but appear to plan and apply correctly, only to cause a problem months later. The good news is that AWS detects the incorrect case in every example I've seen and fails explicitly, making it a bit easier to track down. It is true that people will see their plan/apply start failing with the version bump if this code change is included, but I think that's OK considering they're already sitting on bad code.
✅	❌	❌	This is broken in every scenario, as it should be

I'm happy to create a new test case rather than update existing ones. Can you give me any pointers on how the numeric portion of the key is generated? For example, in resource.TestCheckResourceAttr(resourceName, "parameter.1937131004.name", "event_scheduler"), where does the 1937131004 come from?

[bflad]

@kevincormier-toast we just merged #13556 yesterday, which provides functionality for testing TypeSet attributes without needing to worry about the hash indexes. 👍 Hopefully its more intuitive than the (now) old way.

[kevincormier-toast]

If I'd only looked at the branch conflicts... Looking at the test cases that were updated, those tests won't pass with the existing change. They are all simulating the second scenario in my table above.

Code issues aside, do you think we are likely to get this change merged? Since this will change existing behavior, I don't want to go down that path if you disagree with my assessment of the situation in my last comment! I know as a rule we try not to change existing behavior.

[kevincormier-toast]

@bflad I realized I forgot to @ you in my previous comment

[handlerbot]

My organization has observed live problems with case insensitivity re: RDS parameter groups, which I suspect (dearly hope!) this PR would resolve. I'll see if we can get the simplest possible isolated repro case documented and added here.

[cwoerner]

We also need this functionality. Given that the DB Parameter Group Formula syntax is parsed by something other than terraform it seems reasonable to pass it straight through.

[handlerbot]

OK, after some testing, I have to retract my prior comment! With Terraform 0.13.1 and AWS provider 3.4.0, I can't manifest the problems I was seeing before, so maybe #12112 resolved it?

With this simplest-possible config:

resource "aws_db_parameter_group" "x" {
  name        = "x"
  family      = "mysql5.7"

  parameter {
    name         = "innodb_buffer_pool_size"
    value        = "{DBInstanceClassMemory*1/2}"
    apply_method = "pending-reboot"
  }
}

I can apply it, and I don't get any diffs on plan, with or without -refresh.

What I did find, related to lowercase variables, is that if you put a variable calculation where you don't match AWS expected case for a variable, AWS will reject the parameter group change with Error modifying DB Parameter Group: InvalidParameterValue: value does not match pattern, but Terraform will write the invalid value to the state file, so plan -refresh=false will show nothing, but plan with refresh will produce a change that Terraform wants to roll out. I'll file that as its own issue.

There's definitely an annoyance here where using all lowercase for AWS RDS parameter groups variable calculations used to work fine, and at some point AWS started enforcing case matching, but I'm not sure if Terraform can do anything here other than attempting to pre-discover the proper case for all valid variables for RDS parameter group calculations, which seems like an operational nightmare. 😐

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