tailscale / terraform-provider-tailscale

Terraform provider for Tailscale
https://registry.terraform.io/providers/tailscale/tailscale
MIT License
264 stars 47 forks source link

Destroying ACL configuration does not restore default configuration #426

Open colans opened 2 months ago

colans commented 2 months ago

Describe the bug

If managing ACLs via a tailscale_acl resource in Terraform that was created and destroyed at least once, terraform apply fails with:

You seem to be trying to overwrite a non-default ACL with a tailscale_acl resource. Before doing this, please import your existing ACL into Terraform state using: terraform import $(this_resource) acl

But the non-default configuration should have been removed when terraform destroy was last run so there shouldn't be any non-default configuration anymore. So there's no reason to stop the next build.

To Reproduce

Steps to reproduce the behaviour:

  1. Add some ACL configuration to a tailscale_acl resource.
  2. Run terraform apply
  3. Run terraform destroy
  4. Run terraform apply again.
  5. The build fails with the above error, forcing users to run the import command before continuing with it (unless they use the overwrite_existing_content workaround, which shouldn't be necessary).

Expected behaviour

The build does not fail with the above error.

To make this happen, terraform destroy should remove any custom configuration (this isn't happening now), and restore defaults. So the next time apply is run, it will only see defaults that can be overwritten (allowing the build to proceed).

Desktop (please complete the following information):

Additional context

The root cause of this problem is an incomplete resolution to https://github.com/tailscale/terraform-provider-tailscale/issues/182. It was a good idea to check for non-default configuration before apply, but non-default configuration must be destroyed (i.e. defaults restored) on destroy for this to work properly. Basically, the PR (https://github.com/tailscale/terraform-provider-tailscale/pull/186) should not have been merged without also doing this. Let's do it now to finally get this working properly.

Leaving this unresolved is causing other problems:

A "fix" for this was merged in https://github.com/tailscale/terraform-provider-tailscale/pull/303, but that's treating a symptom of breaking the spirit of how Terraform is supposed to work. It does not resolve the root cause. It shouldn't be necessary for such workarounds if destroy works properly.

CC @markwellis @knyar @DentonGentry @AaronFriel @timduhenchanter

mpminardi commented 2 months ago

Thanks for flagging this @colans !

Agree that this ends up being an awkward and unexpected flow when managing the ACL via Terraform.

I think in the short term we can potentially add an optional reset_acl_on_destroy (or something similar) property to reset to default on destroy. This is a bit of an awkward workaround, but would allow us to support this behaviour without unexpectedly breaking users who are now accustomed to the existing behaviour.

When we do a major version bump of the provider I think having the destroy behaviour reset the ACL to the default and removing reset_acl_on_destroy as an option would make sense.