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

resource/aws_rds_cluster: Add deletion_protection argument #6010

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Sep 27, 2018

Closes #6007

Changes proposed in this pull request:

  • Add deletion_protection argument to aws_rds_cluster resource

Output from acceptance testing:

--- PASS: TestAccAWSRDSCluster_DeletionProtection (158.31s)
--- PASS: TestAccAWSRDSCluster_SnapshotIdentifier_DeletionProtection (402.09s)

```
--- PASS: TestAccAWSRDSCluster_DeletionProtection (158.31s)
--- PASS: TestAccAWSRDSCluster_SnapshotIdentifier_DeletionProtection (402.09s)
```
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/rds Issues and PRs that pertain to the rds service. labels Sep 27, 2018
@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/rds Issues and PRs that pertain to the rds service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Sep 27, 2018
@bflad bflad requested a review from a team September 27, 2018 03:39
@ozbillwang
Copy link

ozbillwang commented Oct 3, 2018

Nice job, I am waiting for the PR to be merged.

But @bflad, I have questions:

prevent_destroy is higher level of lifecycle hook, what should we deal with it after add this new argument deletion_protection

What if someone set prevent_destroy to true and deletion_protection to false, what will happen?

Or we need add comments in README, such as, with new feature of deletion_protection, prevent_destroy with lifecycle hook for this resource is not required any more to avoid conflicts.

@bflad
Copy link
Contributor Author

bflad commented Oct 3, 2018

Hi @ozbillwang 👋 Good question.

There are two similar, but different goals achieved by the lifecycle prevent_destroy meta-parameter for resources and service API implementations designed to prevent resource destruction.

prevent_destroy is a Terraform-only construct that applies to any resource in your configuration, in which you would prefer Terraform to produce an error if a terraform plan or terraform apply would attempt to remove it. Essentially this configuration is preventing the Delete function of a Terraform resource from being called by Terraform. Lifecycle meta-parameters like these cannot be removed from a Terraform resource and do not have any relationship with the remote API. You can still use the AWS web console, CLI, etc. to remove an AWS resource in Terraform defined with prevent_destroy = true

For the few service APIs that implement their own "prevent destroy" behavior like deletion_protection here, the removal protection is at the API layer. This protection generates an API error regardless of Terraform deletion or manual deletion requests (e.g. AWS web console, CLI, etc) if you attempt to delete it while the protection is enabled. Service API implementations are "safer" because there is no workaround to the protection mechanism other than disabling it first in the API.

To specifically answer your questions:

prevent_destroy is higher level of lifecycle hook, what should we deal with it after add this new argument deletion_protection

I'm not sure (personally) if anything specifically needs to be done or documented here, but we would be open to suggestions in a new issue or pull request. Understanding how prevent_destroy works is outside the scope of a single Terraform resource (as it applies to all Terraform resources), regardless of whether or not the API supports other deletion protection mechanisms.

What if someone set prevent_destroy to true and deletion_protection to false, what will happen?

Terraform will return an error during plan or apply, preventing the database from being removed via Terraform. The database can still be removed outside Terraform. We do not have any code hook back into the prevent_destroy meta-parameter to control its behavior from the provider to somehow consolidate its behavior into the common prevent_destroy argument. That sort of enhancement would need to be requested/added upstream in Terraform core first before anything could be implemented here in this provider.

Or we need add comments in README, such as, with new feature of deletion_protection, prevent_destroy with lifecycle hook for this resource is not required any more to avoid conflicts.

All of these are valid use cases:

  • Enabling both mechanisms: I want the API to guarantee (via any deletion method) no one accidentally deletes this database without explicitly disabling the protection first and I want Terraform to guarantee no one accidentally removes this from the Terraform configuration (triggering a database deletion)
  • Enabling only prevent_destroy: I want Terraform to guarantee no one accidentally removes this from the Terraform configuration (triggering a database deletion)
  • Enabling only deletion_protection in this resource: I want the API to guarantee (via any deletion method) no one accidentally deletes this database without explicitly disabling the protection first
  • Enabling none of them at all

If we were to add documentation about this somewhere, I would recommend it would be in a shared location that is linked to by multiple resources, rather than duplicating the content with every resource that supports an API-level deletion protection mechanism. I'm not sure if that makes sense within the provider though as its not provider specific.

@bflad bflad merged commit 27ebaa6 into master Oct 3, 2018
@bflad bflad deleted the f-aws_rds_cluster-deletion_protection branch October 3, 2018 16:43
bflad added a commit that referenced this pull request Oct 3, 2018
@ghost
Copy link

ghost commented Oct 3, 2018

This has been released in version 1.39.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "aws" {
	version = "~> 1.39.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 3, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/rds Issues and PRs that pertain to the rds service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDS Deletion Protection API Support (aws_rds_cluster)
3 participants