provider/aws: DB Instance skip_final_snapshot

[stack72]

Fixes #3848

Adding skip_final_snapshop bool to th db_instance. This will allow us to specify whether a snapshot is
needed directly rather than checking for an empty string

Added as per the docs rather than checking for an empty string on final_snapshot_identifier

[jen20]

Unverified but I think you might be able to use GetOk here instead of Get and avoid the check.

[jen20]

@stack72 I think this is the correct approach versus special casing the final snapshot name, but I'll leave it up to @catsby or @phinze to have a final say on this as they're more likely to know of precedent that may have been set elsewhere in Terraform. I've made a few comments inline with respect to the code if this is the correct approach.

[catsby]

If the user has skip_final_snapshot = false in their config, but omits final_snapshot_identifier (valid config as we have things now), then no final snapshot will be taken. We should error in this case.

[stack72]

I think that makes sense :) Adding it now

[catsby]

Thanks for the PR @stack72! I have some questions though. As it stands I don't think this is a good move, but I'm open for discussing it.

Currently, if the user wants a final snapshot before deleting, the must specify the identifier in their config, or no snapshot will be taken.

The proposed change is an optional parameter with default of false. So, the situation hasn't changed, has it? If they want a backup they must supply a final snapshot identifier. They don't have to specify skip_final_snapshot, because false is the value you want if you want a final snapshot.

Specifying true to skip the final backup then seems unnecessary. In this situation you would also not specify a final snapshot identifier (I would suspect).

I pointed out above in a code comment that as it stands, specifying skip_final_snapshot without providing a final_snapshot_identifier will result in no final snapshot being taken. Even if that's addressed, I think there is a backwards compatibility change introduced. If I do not specify final_snapshot_identifier on v0.6.6, I can create and delete without interruption. With this proposed change, I cannot create in v0.6.6 and delete after upgrading to this change. Manual intervention is required, either to specify final_snapshot_identifier or specify skip_final_snapshot. That's not a deal breaker, or even a breaking change really, but I don't feel it's insignificant.

What are your thoughts on these?

[stack72]

@catsby I am just trying to solve the problem from the original issue - I haven't personally come up against the issue as we always take a snapshot. I think if you have a think about the original issue (linked) then I am happy to go whatever way you think is necessary

[catsby]

Reading up on #3848 and re-thinking, perhaps this is a thing we do need. I think skip_final_snapshot should default to true, since that's our behavior right now (albeit the AWS default is false, an instance where we go against the grain here).

If we default to true, we preserve the current behavior; omitting final_snapshot_identifier and skip_final_snapshot, you get the same behavior. Defaulting to true will make it easier to error if no identifier is given, since the user would need to specify false and an identifier to get the backup.

Does that sound reasonable? I think that would address #3848 by allowing that user to leave their final_snapshot_identifier as is, and then include skip_final_snapshot = ${var.skip_final} or similar, and toggle that variable based on environment. cc @phinze and @jen20 for sanity check

[stack72]

@catsby agree with this approach - this would mean no backwards functionality changes which rocks :)

[jen20]

👍 for preserving current behaviour here. This looks good to me. One thing - is it worth an acceptance test here to prevent regression since there is some (albeit simple) logic and the worst case is a snapshot not being made where one is expected?

[catsby]

Agree an acceptance test would be best here... I'm not sure how to do it though, thoughts? Create two instances, one without skip and the other skip = false, then query in a check to make sure there is a snapshot being made for the latter, and not the former? And then delete it to not leak?

[phinze]

That description sounds right, @catsby. It's a bit of a dance, but it would be nice to cover the snapshot behavior since it's really important to get that right for prod databases. 😀

[stack72]

I'll get this test added over the weekend :)

[stack72]

@jen20 / @catsby / @phinze finally got around to writing some tests for this. The Snapshot test cleans up after itself and deletes the snapshot in the CheckDestroy func

Creating a final Snapshot

TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSDBInstanceSnapshot' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSDBInstanceSnapshot -timeout 90m
=== RUN   TestAccAWSDBInstanceSnapshot
--- PASS: TestAccAWSDBInstanceSnapshot (951.31s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    951.323s

Skipping a final snapshot

TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSDBInstanceNoSnapshot' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSDBInstanceNoSnapshot -timeout 90m
=== RUN   TestAccAWSDBInstanceNoSnapshot
--- PASS: TestAccAWSDBInstanceNoSnapshot (939.14s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    939.160s

Build is currently running :)

[jen20]

LGTM. Thanks @stack72!

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