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

Relax Terraform provider version re-creation condition #1042

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

hugoShaka
Copy link
Contributor

Fixes gravitational/teleport#39638

This PR keeps the mandatory version attribute but does not trigger re-creation when it changes like the v14 provider was doing. We might enable re-creation back in a future version.

Relaxing the re-creation is safe on any non-computed field. So basically every resource but the role with kubernetes_resource set.

This change is required because it's blocking people from upgrading to v15. I will investigate other fixes for the roles with Kubernetes resources attrs.

@wadells
Copy link
Contributor

wadells commented Mar 20, 2024

For context, I was encountering the error where upgrading an accesslist to v15 required adding version but that required addition was triggering a recreate. For resources like access lists that may be only partially terraformed this recreate is painful.

│ Error: Incorrect attribute value type
│ 
│   on access-lists.tf line 56, in resource "teleport_access_list" "access_list_reviewers":
│   56:   header = {
│   57:     metadata = {
│   58:       name = "${each.value.name}-reviewer"
│   59:     }
│   60:   }
│ 
│ Inappropriate value for attribute "header": attribute "version" is
│ required.

However, when I added the version (v1, pulled from tctl), I saw the access list s getting recreated.

# teleport_access_list.access_list["XYZ"] must be replaced
-/+ resource "teleport_access_list" "access_list" {
      ~ header = {
          ~ metadata = {
                name      = "XYZ"
              ~ namespace = "" -> (known after apply)
            }
          + version  = "v1" # forces replacement
        }
      ~ id     = "XYZ" -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Copy link
Contributor

@wadells wadells left a comment

Choose a reason for hiding this comment

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

I don't know the code well enough to provide relevant feedback, but this has my approval in spirit.

@hugoShaka
Copy link
Contributor Author

The tests are broken because of the relaxed condition. As this PR is already approved, I wrote a fix in another PR: #1043

Copy link
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

Just a few wording changes, but otherwise LGTM.


const (
DefaultRoleKubeModifierErrSummary = "DefaultRoleKubeResources modifier failed"
DefaultRoleKubeModifierDescription = `This modifier re-render the role.spec.allow.kubernetes_resources from the user provided config instead of using the state.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/re-render/re-renders/ or s/re-render/will re-render/

s/user provided/user-provided/

}
allowRaw, ok := spec.Attrs["allow"]
if !ok {
resp.Diagnostics.AddError(DefaultRoleKubeModifierErrSummary, "Failed to get 'spec' from TF object.")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/spec/allow/ in error message

@hugoShaka hugoShaka force-pushed the hugo/relax-version-condition branch from bc4fb04 to 81c656c Compare March 27, 2024 20:56
@hugoShaka
Copy link
Contributor Author

I spoke with @r0mant and we don't want to fix the role bug in v15 as this might be breaking for a small amount of users who already had roles upgraded from previous versions before reaching v15).

I will push the plan modifier in v16 and skip the broken test for now.

@hugoShaka hugoShaka force-pushed the hugo/relax-version-condition branch from 81c656c to 5ac9b3c Compare March 27, 2024 21:00
@hugoShaka hugoShaka enabled auto-merge (squash) March 27, 2024 21:02
@hugoShaka hugoShaka merged commit 38eeff1 into master Mar 27, 2024
14 checks passed
@hugoShaka hugoShaka deleted the hugo/relax-version-condition branch March 27, 2024 21:07
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.

Terraform provider v15 attmepts to re-create resource when specifying a version
5 participants