allow some aws_db_instance changes on "storage-full" state

[jordane]

Currently, terraform will error if the following are true:

• The user is increasing the allocated_storage of an aws_db_instance
• The instance is in the storage-full state
• apply_immediately is set to false

However, this is a perfectly valid operation to perform for most db engines.

See https://aws.amazon.com/premiumsupport/knowledge-center/rds-out-of-storage/

This change allows allocated_storage and iops to be changed (and
not applied immediately).

---

For reference, here is the change and error produced (resource name and ID are changed):

  ~ aws_db_instance.db-name
      allocated_storage: "100" => "120"

aws_db_instance.db-name: Modifying... (ID: <removed>)
  allocated_storage: "100" => "120"
...
1 error(s) occurred:

* aws_db_instance.db-name: 1 error(s) occurred:

* aws_db_instance.db-name: unexpected state 'storage-full', wanted target 'available, storage-optimization'. last error: %!s(<nil>)

There are currently no tests in this PR. I don't have a good idea of how to write an Acceptance test for this, and I didn't see any tests for the storage-optimization state that was added previously.

[bflad]

Hi @jordane! 👋 Thanks for submitting this. I think we'll want to understand the RDS API behavior here a little more before making this adjustment.

Some questions I have:

• Does RDS return immediate errors if you try to make modifications not relating to increasing the disk? This could influence whether it should be a pending or target state.
• Does RDS return immediate error if you try to make disk modification for SQL Server? Most likely we actually just want to return this error rather than masking it with the cryptic unexpected state 'storage-full' Terraform error, meaning removing the custom sqlserver logic. AWS may update their behavior to allow said change.

Regardless of the answers above, we probably want to:

• Add storage-full to resourceAwsDbInstanceDeletePendingStates - to cover the case of trying to delete a full database instance

[jordane]

Hi @bflad,

Yes, attempting to modify non-storage attributes produces the following error:

The specified database instance is currently in storage-full state. Please allocate more storage by modifying the DB instance. (Service: AmazonRDS; Status Code: 400; Error Code: InvalidDBInstanceState; Request ID: <removed>)

aws rds describe-valid-db-instance-modifications shows only StorageType, StorageSize, IopsToStorageRatio, and ProvisionedIops.

---

It turns out that the RDS documentation was out-of-date for sqlserver, and resizing is now a valid operation there as well. I've removed the guard on that piece of code.

---

I added storage-full to resourceAwsDbInstanceDeletePendingStates. I noticed there are other valid states not listed there. I know at least incompatible-parameters is missing, not sure on the others [1]. I can file a separate issue for the rest if you'd like.

[1] https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Overview.DBInstance.Status.html

[bflad]

💯 Thanks again for all your work here.

Yes, attempting to modify non-storage attributes produces the following error
It turns out that the RDS documentation was out-of-date for sqlserver, and resizing is now a valid operation there as well.

Fantastic on both accounts! Given the behavior I think in this case we can just add storage-full to resourceAwsDbInstanceUpdatePendingStates as well instead of the custom logic around the target state and letting AWS return its error instead of the Terraform one (e.g. making this PR two lines difference from master from both PendingStates additions, less is more!). I'd ✅ that unless you think that's a bad idea. 😉

I noticed there are other valid states not listed there. I know at least incompatible-parameters is missing, not sure on the others [1]. I can file a separate issue for the rest if you'd like.

This is certainly a cat and mouse game for us -- I would definitely create a separate issue for them if they make sense. Is there a reason we should let an instance sit waiting for incompatible-parameters status to change rather than just failing immediately like it would today? 🤔

[jordane]

Fantastic on both accounts! Given the behavior I think in this case we can just add storage-full to resourceAwsDbInstanceUpdatePendingStates as well instead of the custom logic around the target state and letting AWS return its error instead of the Terraform one (e.g. making this PR two lines difference from master from both PendingStates additions, less is more!). I'd  that unless you think that's a bad idea. 

That makes sense to me. I've made the suggested change, and squashed things down.

This is certainly a cat and mouse game for us -- I would definitely create a separate issue for them if they make sense. Is there a reason we should let an instance sit waiting for incompatible-parameters status to change rather than just failing immediately like it would today? 

What I meant was that an instance in the incompatible-parameters state can still be deleted (delete is a valid operation on most states). Wouldn't that mean it also should be allowed in resourceAwsDbInstanceDeletePendingStates?

[bflad]

What I meant was that an instance in the incompatible-parameters state can still be deleted (delete is a valid operation on most states). Wouldn't that mean it also should be allowed in resourceAwsDbInstanceDeletePendingStates?

Ah yes, of course! That would be great. Feel free to also include it here since its a trivial change.

[jordane]

Ah yes, of course! That would be great. Feel free to also include it here since its a trivial change.

I have done so. I'll go through some of the other states and file a future issue/pr for those. Thanks!

[bflad]

LGTM once TravisCI agrees 🚀

[bflad]

This has been released in version 1.12.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!