Open prashantv opened 2 years ago
Hi @prashantv,
Thanks for filing the issue, and the extensive references you've collected! As you've mentioned, the only solution presented by Terraform to obtain this order of operations is create_before_destroy
. The additional enhancements proposed here are not new ideas, but have not been implemented for a couple of primary reasons:
tftest_notifier.n2.id(destroy)
and tftest_policy.p1(update)
only works in isolation, in a larger graph it introduces cycles (see https://github.com/hashicorp/terraform/blob/main/docs/destroying.md for some notes on replacement and destroy ordering). If tftest_notifier.n2.id
were being replaced rather than destroyed the order of operations would always cause a cycle even in the simple case of 2 resources. If we ignore the cycles in the simple case of replacement, then tftest_policy.p1(update)
would need to happen twice in a single apply, once to remove the old tftest_notifier.n2.id
, then again to add the new tftest_notifier.n2.id
, which isn't possible within the current architecture of Terraform.create_before_destroy
inverts the edges between the create and destroy subgraphs of resource dependencies, that inversion must be cascaded upstream to prevent cycles during replacement. We've reserved this ability to only come from the user creating the configuration, because only that user has the context to understand the changes as a whole.Unfortunately this means as of now the problem falls mostly on the providers. Any resource which requires this sort of "registration pattern" should be designed to allow create_before_destroy
, at least optionally, by using auto-generated identifiers. The need to put create_before_destroy
in the configuration is up to the provider to document, but also the user to discover, which I also agree is not optimal here.
Even in the case that a usable provider-forced option for a new ordering were possible, it would still be up to the providers to implement. Seeing the number of cases where create_before_destroy
wasn't documented or allowed by resources which require it, it might not meaningfully reduce the number of overall bugs encountered.
While we're not opposed to the possibility of such a feature, there haven't yet been any proposals which could meat the two fundamental requirements of; not imposing changes on resources outside of that provider's control, not causing cycles in arbitrarily large or complex graphs. Since Terraform is working as designed here, I'm going to re-label this as an enhancement proposal. This can more or less serve as the core equivalent of https://github.com/hashicorp/terraform-plugin-sdk/issues/585, which is meant to cover the same usability aspects.
Thanks!
Terraform Version
Terraform Configuration Files
This configuration relies on a dummy provider, see github/prashantv/terraform-dep-order-repro
ref
After applying the above, remove "n2" and the reference to it,
Applying this will fail as n2 is destroyed first, while there is still a policy referencing it.
Debug Output
https://gist.github.com/prashantv/45859f8607e690ff22990c310381f8c8
Expected Behavior
When the config removing "n2" is applied, the reference is removed first, and then n2 is removed successfully.
Actual Behavior
Terraform tries to remove "n2" before updating the policy reference, and the remove fails since the notifier is still referenced by a policy.
Steps to Reproduce
Full steps for reproduction are in the repo.
Summary is:
Additional Context
There is a workaround which causes the destroy to happen after the update: setting the lifecycle meta-argument
create_before_destroy
. This has to be set on the resoruce and applied before the delete operation. However, this workaround has some issues:create_before_destroy
.create_before_destroy
is not appropriate for some resources. If a resource has a field that must be unique (e.g.,name
), and a separate attribute that hasForceNew
, thencreate_before_destroy
is not compatible with these resources, so the workaround does not work. We acutally have this combination -- the notifier resource has a name that must be unique, and a user-selected immutable ID field that usesForceNew
, socreate_before_destroy
shifts the problem to users being unable to change names.Ideally the solution would be:
ForceNew
fields).References
There are a few existing issues which are almost all closed:
create_before_destroy
is recommended in many of the above, but as mentioned in "Additional Context", it's a workaround with drawbacks rather than a solution to this problem.