aws_db_instance: Downcase "engine" for RDS

[ctiwald]

If we don't, then mixed case engines (e.g. "MySQL") will force a rebuild
of the db_instance on every apply.

Unfortunately, we can't simply write lowercase to the state file. The
engine property forces a new instance when a diff is detected. This
solution, while not perfect, protects the user from themselves.

[ctiwald]

Learned this one the hard way on my own stuff. Figured I'd submit a quick patch once I was able to isolate the bug :-).

[catsby]

Thanks for submitting this @ctiwald!

I'm curious, you mention being unable to simply write lowercase to the state file, were you doing this manually, or using a StateFunc?

• https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go#L88-L94

[ctiwald]

@catsby: manually. Wasn't aware of the state function but I'm certain that'll do it. I'll resubmit tomorrow.

[ctiwald]

@catsby -- updated to use the StateFunc. Also fixed a bug in the acc tests, and injected a mixed-case config into the acc tests to ensure the new state func works. Runs green for me now.

[radeksimko]

I agree this is a problem, but I'm not overly happy with the proposed solution. AWS APIs sometimes are magic (in a negative way) - especially when changing uppercase to lowercase. I don't think that Terraform should behave the same way.

Why not use the ValidateFunc and prevent people from entering uppercase in the first place?
https://github.com/TimeIncOSS/terraform/blob/3a9852568ae2787881b9f3e9961b2e074da9e431/builtin/providers/aws/resource_aws_db_instance.go#L77-L96

[ctiwald]

@radeksimko -- In the end, I'm somewhat indifferent about the implementation here. I started by throwing an error (without the ValidateFunc, notably), then moved to StateFunc, and I could move it back.

For what it's worth, though, I completely agree: The AWS APIs are magical, inconsistent, and in many ways incoherent. The rules that apply to one service don't apply to another, and it's completely obvious they were designed in isolation, by teams with different rules, guidelines, and product managers. As a user of terraform, one of the things I like about TF is that it keeps it simple.

I'd personally rather Terraform hide AWS's terrible design than proxy it forward, forcing me, the TF user, to memorize a slew of inconsistent and odd validations. I'm totally on board with validating truly invalid input, but this isn't invalid input, it's just input that creates an inconsistent state on <resource>Read. This exact API is actually a perfect example of the problem. Check out the docs for this API call. The bit about the engine actually instructs users to use a mixed-case value:

Engine
The name of the database engine to be used for this instance.

Valid Values: MySQL | oracle-se1 | oracle-se | oracle-ee | sqlserver-ee | sqlserver-se | sqlserver-ex | sqlserver-web | postgres

I'm totally willing to defer to you and @catsby here. In the end, I'm pretty indifferent about this particular case. But for what it's worth, I think it'd be great if TF allowed me to reduce my mental model of "random crap I have to know to use AWS" by silently taking care of stuff like this.

[radeksimko]

I'm totally on board with validating truly invalid input, but this isn't invalid input

Well, it may be valid based on the docs, but in reality you cannot create an RDS instance with uppercase engine because it will always be lowercased, so effectively it's invalid - the API just makes invalid input valid again. :)

I'm letting @catsby to decide here. I just think that there are benefits of keeping attribute values inside Terraform code close to the reality.

[ctiwald]

Sure. I'm fine with whatever you guys decide. Just let me know and I'll modify the PR accordingly.

[catsby]

Valid points on both sides.

Why not use the ValidateFunc and prevent people from entering uppercase in the first place?

Unfortunately, engine is a ForceNew attribute. If we add validation to enforce lowercasing, and someone has a preexisting uppercase named engine, they'll be forced to destroy and re-create their database 😱

That said, and assuming that adding a SetFunc to lower case doesn't cause unnecessary diffs, I'd prefer that route (SetFunc).

I don't want Terraform to do a lot of magic for users either, but I think we're in a corner here and need to compromise.

Does that sound reasonable?

[ctiwald]

@catsby -- I'm totally up for the compromise. Do note however that with the current bug, if they're using an uppercase plan, they are, in fact, going to get a new database with each apply. That's what caused me to investigate in the first place, because I saw that in a plan and said to myself, "Oh no. That's really really not right."

The StateFunc solution does allow existing, mixed caps engine declarations to work without change.

I'm happy to implement whichever solution you all like.

[catsby]

I think the StateFunc is the way to go here, would you mind? Thanks!

[ctiwald]

@catsby -- in that case you can merge this right now pending review. That is what this PR implements in its most recent round.

[catsby]

Are you running this in a Classic env? I get an error when trying to create in a VPC. If possible, please remove this.

[ctiwald]

Ah yeah, except I'm not specifically opting into it. AWS just must default me there. One moment.

[ctiwald]

Updated, @catsby. I'm running an environment that defaults to EC2 Classic, which caused the tests to fail for me.

[catsby]

Thanks @ctiwald !

[catsby]

Thanks @radeksimko too, for the discussion / suggestions

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.