prevent_destroy does not prevent destruction when removing a resource from the configuration

[samjgalbraith]

The documentation for prevent_destroy says

This flag provides extra protection against the destruction of a given resource. When this is set to true, any plan that includes a destroy of this resource will return an error message

However, removing or commenting out a resource seems to still destroy it with no warning, even though technically it was a plan that includes the destruction of the resource. What I expected was that the state file would remember that it was created with prevent_destroy, and prevent its destruction even if it was no longer part of the parsed configuration. It would be reasonable to expect that the user would be forced to remove the prevent_destroy flag, terraform apply, then remove it from the configuration if they were absolutely sure they wanted to go ahead.

Terraform Version

Terraform v0.11.3
+ provider.aws v1.11.0

Terraform Configuration Files

locals {
  dynamo_db_mutext_attribute = "LockID"  # The name must be exactly this
}

provider "aws" {
  profile = "test"
  region = "ap-southeast-2"
}

resource "aws_s3_bucket" "test_environment_terraform_backend_storage" {
  region = "ap-southeast-2"
  bucket_prefix = "SOME-BUCKET-NAME_PREFIX-"
  versioning {
    enabled = true
  }
  tags {
    Environment = "test"
  }
  # We explicitly prevent destruction using terraform. Remove this only if you really know what you're doing.
  lifecycle {
    prevent_destroy = true
  }
}

resource "aws_dynamodb_table" "test_environment_terraform_backend_mutex" {
  "attribute" {
    name = "${local.dynamo_db_mutext_attribute}"
    type = "S"
  }
  hash_key = "${local.dynamo_db_mutext_attribute}"
  name = "test_environment_terraform_backend_mutex"
  read_capacity = 5
  write_capacity = 5
  # We explicitly prevent destruction using terraform. Remove this only if you really know what you're doing.
  lifecycle {
    prevent_destroy = true
  }
}

Expected Behavior

Resource is prevented from being destroyed because it was created with lifecycle { prevent_destroy = true }

Actual Behavior

Resource with prevent_destroy set to true is destroyed when commenting out or removing from configuration.

Steps to Reproduce

1. Enter the above Terraform configuration into a new module.
2. terraform apply and say yes.
3. Now, comment out the s3 bucket resource.
4. terraform apply again, and say yes

Additional Context

References

[apparentlymart]

Hi @theflyingnerd! Thanks for pointing this out.

Currently removing a resource from configuration is treated as intent to remove the resource, since that's an explicit action on your part rather than it happening as a part of some other action, such as resource replacement or a whole-configuration destroy.

However, your idea of retaining this in the state and requiring multiple steps to actually destroy is an interesting one:

1. Leave resource in config but remove prevent_destroy
2. Run terraform apply to update the state to no longer have that flag set.
3. Remove the resource from configuration entirely.
4. Run terraform apply again to actually destroy it.

I'd like to think about this some more because while this does seem safer it also feels like this flow could be quite frustrating if e.g. you've already committed some reorganization to your version control that includes removing a resource and then Terraform refuses to apply it until you revert and do steps 1 and 2 from the above.

In the mean time, we should at least update the documentation to be more explicit about how prevent_destroy behaves in this scenario.

[samjgalbraith]

Great, thanks for your quick and thorough reply @apparentlymart . I agree that a documentation update would be sufficient for now and the proposed functionality could be left open for debate. My comments about this process were a mixture of how I might expect it to work if I didn't read any more documentation as well as a proposal. It's influenced by the idea of AWS instance termination protection. This protection must be explicitly removed first before the instance may be terminated, even if the instance is explicitly terminated by id.

[a-nldisr]

Please see: #16392

[dguisinger]

Any further thoughts on this?
I used Terraform to setup build pipelines including AWS CodeCommit for storage of code.
A mistake in terraform will wipe out the entire contents of the Git repo. I too want a way to protect resources in case they get removed from a script.

[tdmalone]

Potential duplicate of #3468

[roidelapluie]

Strange that 17599 closes 3468 and not the opposite but that is still a very wanted feature.

[JustinGrote]

This was (is) a major risk factor of accidental deletion and was unacceptable for us using terraform in production.

This is our workaround for this that runs in our Azure DevOps pipeline on every commit before application, I'll see if I can get the code (Powershell Pester Test) approved to be shared.

1. Run Terraform plan and get list of resources to-be-destroyed
2. Review the previous state (retrieved from previous commit) for any to-be-destroyed resources that had prevent_destroy set and if so, fail the test, and stop the pipeline.
3. To fix, two commits are required, one to remove prevent_destroy (which doesn't trigger the destroy resource and thus doesn't trigger failing the test), and then one to remove the resource from the config.

In addition to this all production changes have an approval step that formats the terraform apply state very clearly and has to be approved by all the team members.

[Zeal0us]

I currently don't intend to manage certain things with Terraform due to this. As part of deployment of new code (i.e. lambda, ECS containers, api gateway, etc...) my Terraform files are built dynamically, so although it seems like removing it from the .tf files is explicit approval, it could just as easily be a coding error.

As such, managing any type of Dynamo table or RDS or something is out of the question, and those will just have to be managed manually. While I'm kind of ok with this, it certainly would be nice to just manage the entire application and still be able to sleep.

[simlu]

Any update on this? It seems like quite an important feature...

[JustinGrote]

I would imagine all their energies are on getting 0.12 out the door right now.

…

On Mon, Mar 4, 2019 at 7:37 PM simlu ***@***.***> wrote: Any update on this? It seems like quite an important feature... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17599 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOjVUmLqlNPxCZNSo6y4R-PqzZVILSSXks5vTeZcgaJpZM4StAww> .

[evmin02]

+2

[CodeSwimBikeRun]

omg. I'm looking at how to just prevent helm provider from destroying a chart every time I deploy. I came here to see this amazing prevent_destroy trait that I want to try out, but judging from this, it doesnt even work?

You are willing to wipe out a whole database created with prevent_destroy flag on? What the heck?

[tdmalone]

@CodeSwimBikeRun Your comment before the edit was more correct - it works, but not if you remove the resource from your .tf file, because by doing so you're then also no longer setting prevent_destroy. This issue is about potentially storing that in the state so that it the instruction can potentially still remain around (I'm not sure if this is particularly what you were after or not).

You can, however, set flags on some resources (such as AWS EC2 and RDS instances) to prevent deletion at the AWS API level, which will help you no matter what.

Not sure the language is warranted.

[nbering]

@CodeSwimBikeRun This issue is about a specific case, in which the configuration block for the resource no longer exists. In which case, Terraform is following instructions in the strict sense that the resource is no longer configured with prevent_destroy.

Terraform interprets the configuration files as your desired state, and the state file (and/or refreshed in-memory state) as the current state. Destruction becomes a complex edge-case in this way because the absence of a resource implicitly means that you desired it to be destroyed, but in practice this may not always be true. There are other cases to watch out for which are similar, but are consequences of the basic design. For example, #13549.

The feature works - in the general sense - and is worth using. But it's still pretty important to examine the plan before applying changes. Declarative configuration systems in general are very powerful, but also come with an inherent risk of accidentally automating failure (see: Automation: Enabling Failure at Scale for an example outside Terraform). I personally find that Terraform's plan feature is actually superior to some other declarative configuration tools I've used in that regard. It will tell you, before changing anything at all, that it wants to destroy a resource.

[rusik69]

we had part of our production deleted because of this :(

[beanaroo]

Storing last set prevent_destroy in state would be beneficial. Especially when generating tf plans programmatically.

[bionicles]

seems like a no-brainer safety-critical feature ... prevent destroy should work

[aaronmell]

Would be nice to have a feature added like Cloudformations Retain. If the resource is removed from the file, CF doesn't try to remove the resource, and just removes it from the state file.

[penfold]

I'm looking at configuring Azure FrontDoor which means that I'll need to reference Azure DNS zone. As I currently use the same domain (different sub-domains) for live and test environments, then I run the risk of destroying live the DNS zone when I clean down a test environment.

Is there any thought on implementing the change that @apparentlymart suggested at the start of the thread?

[nbering]

@penfold Your case sounds like terraform_remote_state should be a suitable workaround. Or possibly a better implementation all-around depending on your constraints.

https://www.terraform.io/language/state/remote-state-data

You could put your shared resources in a separate state project to protect the dependant Terraform runs from clobbering it's resources, but still benefit from referencing values from it's resources across environments.

[HariSekhon]

This is still such a huge problem for everything containing state/data - from Databases to GitHub repos.

Terraform is simply not production-grade for these use cases until it records the prevent_destroy in the state file.

[aton-elvisb]

Hi @apparentlymart, as an alternative to the following workflow to remove a protected resource:

1. Leave resource in config but remove prevent_destroy
2. Run terraform apply to update the state to no longer have that flag set.
3. Remove the resource from configuration entirely.
4. Run terraform apply again to actually destroy it.

we could also think about an alternative workflow:

1. Remove the resource from configuration entirely.
2. Run terraform state allow-destroy <resoure name> (a new command)
3. Run terraform apply to actually destroy it.

The order of steps 1 and 2 is not relevant. The step 2 removes the destroy prevention from the state.

If the the step 1 is not executed, so that the resource remains in the configuration entirely, but the step 2 is executed, then the terraform apply at step 3 simply reintroduce the protection.

[m98]

I ended up resolving this by just protecting my resources with iam on aws. For me I wanted to prevent our user pool from being deleted.

resource "aws_iam_policy" "prevent_user_pool_deletion" {
  name = "prevent_user_pool_deletion"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
        "Effect": "Deny",
        "Action": [
          "cognito-idp:DeleteUserPool",
          "cognito-idp:DeleteUserPoolClient",
          "cognito-idp:DeleteUserPoolDomain"
        ],
        "Resource": [
            "arn:aws:cognito-idp:${var.region}:${local.account_number}:userpool/eu-west-3_xxxx"
        ]
    }
  ]
}
EOF
}

resource "aws_iam_group_policy_attachment" "attach" {
  group      = "admin"
  policy_arn = aws_iam_policy.prevent_user_pool_deletion.arn
}

With this in place I have less fear of accidentally deleting our user pool.

I think the problem with this solution is that if this policy attachment is accidentally getting removed from the .tf config, then there is a chance of destroying the Cognito User Pool resource. It's very similar to the prevent_destory lifecycle, but with more code!

I think another way is to limit the IAM user that Terraforms itself is using. You can add this policy to the user:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "NotAction": [
        "cognito-idp:AdminDeleteUser",
        "cognito-idp:DeleteGroup",
        "cognito-idp:AdminDeleteUserAttributes",
        "cognito-idp:DeleteUserPool",
        "cognito-idp:DeleteIdentityProvider",
        "cognito-idp:DeleteResourceServer",
        "cognito-idp:DeleteUserPoolClient",
        "cognito-identity:UnlinkDeveloperIdentity",
        "cognito-idp:DeleteUserPoolDomain",
        "cognito-identity:DeleteIdentityPool",
        "cognito-idp:DeleteUserAttributes",
        "cognito-identity:DeleteIdentities",
        "cognito-identity:UnlinkIdentity",
        "cognito-idp:DeleteUser"
      ],
      "Resource": ["*"]
    }
  ]
}

But anyway, I think Terraform itself should not delete any user pool that has at least one user in it. It does the same for the S3 bucket that can't get destroyed if there is an object in it.

[lpil]

Hi gang. We got bit by this yesterday. Is there a workaround yet?

[koreyGambill]

I'm trying to automate a CI/CD solution for terraform, but am deeply concerned with auto-applying changes on merges to main because of this issue. If someone doesn't look over a plan properly, they could cause a pretty drastic outage, and we could lose a lot of data. I suppose my real concern is managing anything with terraform that contains persistent data, and doesn't have a prevent deletion flag available to be set.

The best solution (workaround) I see in these comments is checking previous terraform state before doing a deploy as #17599 (comment) suggested, or perhaps not being very permissive with terraform's permissions through IAM. But I'd much rather a first class solution.

There are multiple good (and seemingly easy) solutions that could be implemented here. I liked the Tombstone idea, or doing something like a Cloudformation Retain, or ensuring that the lifecycle is negated in the previous apply, or another terraform apply flag like terraform apply --allow-destroy that can destroy resources with that lifecycle set. I think we'd all love to see a native, intuitive way to prevent an accident from wiping out data.

Is HashiCorp not worried about this issue?

[HariSekhon]

@koreyGambill surprisingly dangerous, isn't it?

I've automated Terraform CI/CD for 2 companies in 2 different ways for you to consider:

1. auto-applies on merge to main using my GitHub Actions reusable workflow, the plan is pasted into the Pull Request ticket, but once you merge that PR, that's it, boom. This is probably the one that scares you more.
2. Jenkins runs the Plan from the main branch and then uses an interactive Approval before doing the Apply. This uses a saved plan so you know exactly what Terraform is going to apply if you approve the Jenkins pipeline final stage and it only applies that.

Even with this 2nd stronger option, there is still always the possibility of human error that somebody will approve without checking properly and it'll destroy something critical.

For such critical data resources, you might have to keep them outside of Terraform until this issue is solved.

[Harrisonbro]

Just adding a +1 to this. I'm really struggling to recommend Terraform to our clients for this reason.

[simonweil]

@Harrisonbro terraform is not perfect, but what is the better alternative that has no downsides of it's own?

[Harrisonbro]

@Harrisonbro terraform is not perfect, but what is the better alternative that has no downsides of it's own?

Nothing has no downsides, of course. We do recommend terraform for stateless resources that can be recreated, but for databases and such we have to recommend clients use other approaches (often just manual config) because they can't accept the risk of a Git commit wiping out data.

[whiskeysierra]

The way we deal with this in our company is twofold:

1. Locks - depends on your cloud provider. Azure has management locks which we will put on databases and database servers preventing any delete, not just via terraform.
2. Lack of permissions - we restrict the identity that terraform assumes to not have the necessary permissions to delete.

[maxb]

There are undoubtedly many possibly workarounds. Indeed, where I work, we've resorted to patching terraform-provider-github to embed the feature at the provider level specifically for github_repo resources.

However given the outpouring of supportive sentiment, @apparentlymart could you give us an update on your comment from 2018?

[simonweil]

I use delete protection and final snapshots, this prevents the DB from being deleted by mistake...

[ben-z]

There are undoubtedly many possibly workarounds. Indeed, where I work, we've resorted to patching terraform-provider-github to embed the feature at the provider level specifically for github_repo resources.

@maxb any chance the patch to terraform-provider-github is open source? My org has the same use case and we don't want to reinvent the wheel.

[maxb]

It is not (and I no longer work at that employer). However, the modern github_repository resource has a setting archive_on_destroy which can be turned on, which provides a different safety net to accidental repository deletion.

[ben-z]

I made a proof-of-concept (#33229) to implement storing the flag in the state. You can try it using:

git clone https://github.com/WATonomous/terraform.git
git checkout prevent_removal
go install .

and using the prevent_removal lifecycle flag instead of the prevent_destroy lifecycle flag.

[thedewi]

This is very surprising behaviour IMO. I'd have expected the policy to be stored in state and applied in all destroy contexts. Does anyone actually want it to apply only part of the time? Having a second similar policy will add confusion; maybe then the original should be deprecated?

[HariSekhon]

I made a proof-of-concept (#33229) to implement the behaviour of the change requested in this issue. You can test it out by:

git clone https://github.com/WATonomous/terraform.git
git checkout prevent_removal
go install .

and using the prevent_removal lifecycle flag instead of the prevent_destroy lifecycle flag.

It would be better for Hashicorp to just fix the behaviour of prevent_destroy to be stored in the terraform state.