tailscale / terraform-provider-tailscale

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

Don't delete `tailscale_tailnet_key` after the key is used. #306

Closed michaelbeaumont closed 8 months ago

michaelbeaumont commented 9 months ago

Describe the bug It doesn't seem possible to use tailscale_tailnet_key in practice, because as soon as it's used once, the key is "deleted" and Terraform wants to replace the resource, which causes a cascade of updates/replacements, when actually the key may not be needed anymore/it's saved in the state file.

lifecycle {
  ignore_changes = all
}

doesn't seem to work. Is there a feature of this resource/Terraform that I might be missing?

To Reproduce Use the tailscale_tailnet_key resource to trigger creating a new machine and then run terraform apply again.

Expected behaviour I should be able to use tailscale_tailnet_key and not have it trigger a recreation.

knyar commented 9 months ago

I think what you are seeing is a recent change introduced in #287 (#144). What I believe that change aimed to achieve is making sure that each tailscale_tailnet_key actually points to a currently valid key. I can see how this could result in Terraform recreating the key unnecessarily, especially if a key is single-use.

Single-use or expiring resources don't fit particularly well into the Terraform data model, but I think a reasonable way forward might be to:

This will make the resource behave like it did before #287, but will provide a way to make sure that the key is recreated for folks who need it.

If anyone has any alternative ideas, let's discuss!

clstokes commented 9 months ago

@michaelbeaumont you can use ignore_changes on the downstream resource that uses the key to prevent updates or re-creation. Will that work for you?

For example:

resource "tailscale_tailnet_key" "main" {
  # ...
}

resource "aws_instance" "tailscale_instance" {
  user_data = "script that does something with ${tailscale_tailnet_key.main.key}"

  lifecycle {
    ignore_changes = [ user_data ]
  }
}

In my opinion the current behavior provides the most predictable behavior and I believe is consistent with how "expiring" resources from other providers behave as well. The previous behavior (ignoring deleted or invalid keys) leads to incorrect Terraform state where Terraform thinks a resource exists when it doesn't and subsequent terraform operations will fail in unexpected and difficult-to-troubleshoot ways - e.g. tailscale up --auth-key ... failing when the user has no indication why the key would be invalid. If we do make a change here, I think the current behavior should be the default and ignore keys should be opt-in.

knyar commented 9 months ago

If we do make a change here, I think the current behavior should be the default and ignore keys should be opt-in.

What do you think about current behaviour being the default only if reusable is set to true? Single-use keys are typically required only for initial machine provisioning, and re-creating them by default would most likely result in unnecessary keys being created and written into Terraform state, but never used.

michaelbeaumont commented 9 months ago

@michaelbeaumont you can use ignore_changes on the downstream resource that uses the key to prevent updates or re-creation. Will that work for you?

Seems suboptimal to have to leave a key laying around never to be used.

I don't mind of course which behavior is the default. I know this is not something provider-specific, it's really a missing feature of terraform itself. But it would be great to give the possibility for users who know what they're doing/accept the risks to allow behavior like this.