tailscale / terraform-provider-tailscale

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

tailscale_tailnet_key: only recreate reusable keys by default #310

Closed knyar closed 8 months ago

knyar commented 9 months ago

This change partially reverts the behaviour introduced in #287 that currently results in single-use keys being recreated, triggering unnecessary updates to downstream Terraform resources.

By default, the provider will now only recreate reusable keys, ignoring invalid single-use keys. This can also be changed now using a new recreate_if_invalid attribute.

Fixes #306

clstokes commented 8 months ago

I ran through a few scenarios with this branch and everything looked good except for one update-in-place scenario.

If I change recreate_if_invalid = "never" -> "always" on an expired and invalid key and then apply, the property gets changed on the first apply and the next subsequent apply replaces the key. I think the replace should happen in the first apply.

Steps to reproduce

  1. git clone git@gist.github.com:4adf7c5b90e60c2cfaa632f027b4132f.git (gist url)
  2. terraform apply -auto-approve
  3. terraform output key <--- Observe invalid = false
  4. sleep 30
  5. echo 'recreate_if_invalid = "always"' > terraform.tfvars
  6. terraform apply -auto-approve _<--- Observe recreate_if_invalid is updated, but key is not replaced._
  7. terraform output key <--- Observe invalid = true
  8. terraform apply -auto-approve <--- Observe Key is replaced.

Output

see gist

knyar commented 8 months ago

If I change recreate_if_invalid = "never" -> "always" on an expired and invalid key and then apply, the property gets changed on the first apply and the next subsequent apply replaces the key. I think the replace should happen in the first apply.

Nice, thanks a lot for testing and for sharing this. This is not trivial to fix: the stage at which Terraform shows a simple diff happens before we have an opportunity to read the resource.

I have addressed this by adding a custom diff function for the resource that now triggers recreation if a flag gets changed. It duplicates some logic of the Read function that similarly recreates the key when it expires, but I don't think there's an elegant single place we could do this in. PTAL?

clstokes commented 8 months ago

Nice, thanks a lot for testing and for sharing this. This is not trivial to fix: the stage at which Terraform shows a simple diff happens before we have an opportunity to read the resource.

I have addressed this by adding a custom diff function for the resource that now triggers recreation if a flag gets changed. It duplicates some logic of the Read function that similarly recreates the key when it expires, but I don't think there's an elegant single place we could do this in. PTAL?

I ran through the same scenario and others and this looks good to me!

michaelbeaumont commented 8 months ago

@knyar Great, thanks!