Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tailscale_tailnet_key: only recreate reusable keys by default #310

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

knyar
Copy link
Collaborator

@knyar knyar commented Dec 8, 2023

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

@knyar knyar requested review from clstokes and zofrex December 8, 2023 14:25
@clstokes
Copy link
Collaborator

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 [email protected]: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
Copy link
Collaborator Author

knyar commented Dec 11, 2023

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
Copy link
Collaborator

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!

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

Signed-off-by: Anton Tolchanov <[email protected]>
@knyar knyar merged commit f65978c into main Dec 12, 2023
3 checks passed
@knyar knyar deleted the knyar/recreate branch December 12, 2023 15:24
@michaelbeaumont
Copy link
Contributor

@knyar Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't delete tailscale_tailnet_key after the key is used.
3 participants