hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.79k stars 9.14k forks source link

[Bug]: Please reconsider fix introduced in 4.16.0 for disable_api_termination #27529

Open jpbuecken opened 1 year ago

jpbuecken commented 1 year ago

Terraform Core Version

1.2.4

AWS Provider Version

4.21.0

Affected Resource(s)

Expected Behavior

If you build your instance with disable_api_termination enabled, it should not be possible to destroy the instance via API (even if it is terraform.

The AWS API is used in pipelines or other automatic processes / workflows. E.g. terraform is one tool to use it that way. The idea of disable_api_protection to have an additional layer to protect your instance. For example it protects your instance if you have a a bug in your pipeline code or accidental/wrong usage of workflows.

If I try to destroy an instance with disable_api_termination enabled, it should fail.

Actual Behavior

If I try to destroy an instance with disable_api_termination enabled, it succeeds. The fix introduced in 4.16.0 enables api termination unconditionally, making the idea of option disable_api_termination meaningless.

Be aware, we use terraform modules, so we cannot use terraform prevent_destroy features as an additional layer. This is a long open issue in terraform (https://github.com/hashicorp/terraform/issues/18367).

So please consider if it would have been better to restore the behavior from version 3.37 as described in "Expected Behavior" in https://github.com/hashicorp/terraform-provider-aws/issues/19275

See also https://github.com/hashicorp/terraform-provider-aws/issues/19275#issuecomment-914677748

The problem people faced was that they enabled api termination via AWS console or other tools than terraform, they could not destroy the instance via terraform, because it disabled api termination again, and then tried to destroy the instance.

If I build an instance with disable_api_termination = true, then I do it on purpose to protect it for api / command line calls. Since we use terraform only to manage our instances, we wrote workflows that change the value via terraform, then apply, and then destroy the instance, but only in places needed, to avoid accidents.

So there is no need to fix it to enable api termination unconditionally. The old behavior allows people to decide if they modify there terraform code to disable api protection (e.g. make it a variable) or use AWS console or other ways before destroy. For me, it looks like https://github.com/hashicorp/terraform-provider-aws/pull/19276#issuecomment-946902292 has only been declined because the code of https://github.com/hashicorp/terraform-provider-aws/pull/19277 was more up to date , but I did not read any consideration if this is a good idea.

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

-

Steps to Reproduce

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

No response

Would you like to implement a fix?

No response

github-actions[bot] commented 1 year ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

mikeplem commented 1 year ago

If the provider functionality doesn't change would it be possible to add a note to the documentation to state that “Terraform will destroy the resource when this option is set to True.” That would give the reader more clarity about what to expect.

bgshacklett commented 12 months ago

Given the inflexibility of the prevent_destroy option in Terraform, this behavior [of forcibly destroying EC2 instances when the disable_api_termination value is set to true] is problematic. As has been mentioned elsewhere, if the aws_instance resource is within a sub-module, it makes it very difficult to manage termination protection via the lifecycle directive.

This certainly breaks the https://en.wikipedia.org/wiki/Principle_of_least_astonishment, and is a major change from the behavior that people have come to rely on.

jpbuecken commented 12 months ago

Given the inflexibility of the prevent_destroy option in Terraform, this behavior is problematic.

First I thought "this behaviour" refers to the one I requested in this issue. But you are supporting my request, aren't you?

After I read further, "this behaviour" refers to the current behaviour introduced with 4.16.0 and the disable_api_protection option provides a protection in terraform modules.

bgshacklett commented 12 months ago

Yes; my apologies for the ambiguity. I've edited my previous message to clarify.