Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Terraform provider: make version mandatory and immutable #1007

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

hugoShaka
Copy link
Contributor

This is a breaking change, merge this once we know we won't do any new v14 release.

To mitigate gravitational/teleport#36773 we will enforce the resource version to be set starting with Teleport 15. This will ensure no surprise upgrade happens.

The documentation PR is here: gravitational/teleport#36774

Note: protos from the docs PR were used to generate the schema in order to have the nice version messages and render a proper reference.

so. If it's not specified, Terraform will pick the latest one by default. When
this happens, you will face unexpected changes when doing major Terraform
provider upgrades, potentially introducing new resource versions
and changing the behaviour of your existing TF code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning that when dealing with roles, it can impact access rules when upgrading the provider across major versions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to sound like a broken record but IMO we do also need some central reference for what role versions can be used, what is current, what the difference is between them, etc. We'll probably get a lot of questions about "what role version should I use" otherwise (more than we usually get)

Copy link
Contributor Author

@hugoShaka hugoShaka Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the tests, and I have bad news: we built a system with hysteresis 😭 .

A role upgraded to v6 from v5, and a role created directly with v6 with the same spec won't have the same rules.

Resources have their defaults set on initial creation, then never again. If you create a resource v5, you will have the v5 defaults, even after you upgrade to v6.

There's an interaction between Teleport resources without clear field ownership and how Terraform measures the existing state to do drift detection. Teleport edits the spec server-side, losing the original user intention. Then TF imports this amended resource back in its state, importing all the defaults and keeping them ad vitam.

The conclusion is that upgrading a resource between versions is not safe. As re-creating it from scratch will not lead to the same result. This also explains why very few people complained about their role being broken after upgrading to Teleport 13.

terraform/test/role_test.go Show resolved Hide resolved
Copy link
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add specific tests for this

We should also have a docs page or an anchor to a doc that explains the change that users must do

terraform/tfschema/devicetrust/v1/device_terraform.go Outdated Show resolved Hide resolved
@hugoShaka hugoShaka changed the title Terraform provider: make version mandatory on all resources Terraform provider: make version mandatory and immutable Jan 23, 2024
@hugoShaka
Copy link
Contributor Author

I updated the PR: now, the version cannot be changed without deleting/re-creating the resource. We also have more extensive tests for version updates and missing versions.

@tigrato @marcoandredinis can you take another look?

@hugoShaka hugoShaka force-pushed the hugo/make-version-mandatory branch from 9a529b5 to ab2705c Compare January 23, 2024 21:17
Copy link
Contributor

@tigrato tigrato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to somehow get a workaround for this otherwise for users that want to use Kubernetes resources other than pod, it will involve deleting several roles so the conversion can happen.
For now it will be fine but prepare for some support load since some kubernetes resources are a v7 only feature

@hugoShaka
Copy link
Contributor Author

We will need to somehow get a workaround for this otherwise for users that want to use Kubernetes resources other than pod, it will involve deleting several roles so the conversion can happen. For now it will be fine but prepare for some support load since some kubernetes resources are a v7 only feature

You're right. I will document how to upgrade a resource (create a new role v7, use it everywhere, remove rolev5). I also need to document best practices to dynamically use the correct role name and have Terraform correctly infer dependencies.

@hugoShaka hugoShaka enabled auto-merge (squash) January 30, 2024 23:08
@hugoShaka hugoShaka merged commit 94c0928 into master Jan 30, 2024
15 checks passed
@hugoShaka hugoShaka deleted the hugo/make-version-mandatory branch January 30, 2024 23:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants