Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Add support for Skipping Final Snapshot in RDS Cluster #6795

Merged
merged 2 commits into from
May 20, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented May 20, 2016

Fixes #6786

This works exactly the same as in DB Instance. We cannot use an empty
string to skip the final snapshot or we get the following error:

only alphanumeric characters and hyphens allowed in
"final_snapshot_identifier"

Therefore, we need to wrap this with another parameter. The we DO NOT
skip final snapshots by default as we currently create them right now
and this would change the user functionality

@catsby
Copy link
Contributor

catsby commented May 20, 2016

This isn't quite right. I ran TestAccAWSRDSClusterInstance_basic and it timed out. Turns out we do not send the DELETE to the cluster correctly. Raw logs:

2016/05/20 10:58:55 [DEBUG] RDS Cluster delete options: {
  DBClusterIdentifier: "tf-aurora-cluster-test-123",
  FinalDBSnapshotIdentifier: "",
  SkipFinalSnapshot: false
}
2016/05/20 10:58:55 [DEBUG] [aws-sdk-go] DEBUG: Request rds/DeleteDBCluster Details:
---[ REQUEST POST-SIGN ]-----------------------------
POST / HTTP/1.1
Host: rds.us-west-2.amazonaws.com
User-Agent: terraform/0.7.0 (dev) aws-sdk-go/1.1.23 (go1.6.1; darwin; amd64)
Content-Length: 139
Authorization: AWS4-HMAC-SHA256 Credential=AKIAJHPR3RGMNB6CCFWQ/20160520/us-west-2/rds/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date, Signature=1279a1c1c52618b7225cb64a57df7d5e1fcdf03020d65bdaa5038a4612f9b6fc
Content-Type: application/x-www-form-urlencoded; charset=utf-8
X-Amz-Content-Sha256: 7634703ffe31256d3384502eb253f6f2444080840cce2e905ca21c22fe3c3b41
X-Amz-Date: 20160520T155855Z
Accept-Encoding: gzip

Action=DeleteDBCluster&DBClusterIdentifier=tf-aurora-cluster-test-123&FinalDBSnapshotIdentifier=&SkipFinalSnapshot=false&Version=2014-10-31
-----------------------------------------------------
2016/05/20 10:58:56 [DEBUG] [aws-sdk-go] DEBUG: Response rds/DeleteDBCluster Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 400 Bad Request
Connection: close
Content-Length: 317
Content-Type: text/xml
Date: Fri, 20 May 2016 15:58:55 GMT
X-Amzn-Requestid: c1e1c595-1ea3-11e6-82e4-870ed3096801

<ErrorResponse xmlns="http://rds.amazonaws.com/doc/2014-10-31/">
  <Error>
    <Type>Sender</Type>
    <Code>InvalidParameterCombination</Code>
    <Message>Please specify FinalSnapshotIdentifier or SkipFinalSnapshot</Message>
  </Error>
  <RequestId>c1e1c595-1ea3-11e6-82e4-870ed3096801</RequestId>
</ErrorResponse>

Re:

The we DO NOT
skip final snapshots by default as we currently create them right now
and this would change the user functionality

We do skip final snapshots now. From the docs:

final_snapshot_identifier - (Optional) The name of your final DB snapshot when this DB cluster is deleted. If omitted, no final snapshot will be made.

When no final_snapshot_identifier, no snapshot is taken. In the example config of #6786, it's failing a validation because the user is specifying an empty string, which fails the validation. Omitting the attribute does not take a snapshot.

finalSnapshot := d.Get("final_snapshot_identifier").(string)
if finalSnapshot == "" {
if skipFinalSnapshot == true {
Copy link
Contributor

@catsby catsby May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making skip_final_snapshot optional and default false indicates they do not want to skip it, and then assumes they'll supply a final_snapshot_identifier, except that attribute is optional as well.

This works exactly the same as in DB Instance. We cannot use an empty
string to skip the final snapshot or we get the following error:

```
only alphanumeric characters and hyphens allowed in
"final_snapshot_identifier"
```

Therefore, we need to wrap this with another parameter. The we skip final snapshots by default as we do this  currently and this would change the user functionality
@stack72 stack72 force-pushed the rds-disable-snapshop branch from fe0d169 to 5a3a061 Compare May 20, 2016 16:31
@stack72
Copy link
Contributor Author

stack72 commented May 20, 2016

@catsby you are indeed correct. This should take care of it now - i was just able to replicate everything you saw and it's now fixed

@stack72
Copy link
Contributor Author

stack72 commented May 20, 2016

When you skip_final_snapshop = true but no final snapshot identifier is specified:

aws_rds_cluster.default: Destroying...
Error applying plan:

1 error(s) occurred:

* aws_rds_cluster.default: RDS Cluster FinalSnapshotIdentifier is required when a final snapshot is required

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

@catsby
Copy link
Contributor

catsby commented May 20, 2016

Better, thanks!

@catsby catsby merged commit ec8c242 into master May 20, 2016
@catsby catsby deleted the rds-disable-snapshop branch May 20, 2016 17:52
cristicalin pushed a commit to cristicalin/terraform that referenced this pull request May 24, 2016
…ashicorp#6795)

* provider/aws: Add support for Skipping Final Snapshot in RDS Cluster
@ghost
Copy link

ghost commented Apr 25, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot disable final snapshot in aws_rds_cluster resource
2 participants