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

cloudflare_account_member status changes after user accepts invite #2187

Closed
2 tasks done
glenwinters opened this issue Jan 25, 2023 · 13 comments · Fixed by #2217
Closed
2 tasks done

cloudflare_account_member status changes after user accepts invite #2187

glenwinters opened this issue Jan 25, 2023 · 13 comments · Fixed by #2217
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.
Milestone

Comments

@glenwinters
Copy link

Confirmation

  • My issue isn't already found on the issue tracker.
  • I have replicated my issue using the latest version of the provider and it is still present.

Terraform and Cloudflare provider version

Terraform 1.3.6
Cloudflare provider 3.32.0 (not latest, but no mention of cloudflare_account_member in last couple releases)

Affected resource(s)

cloudflare_account_member

Terraform configuration files

resource "cloudflare_account_member" "test" {
  account_id    = var.cloudflare_account_id
  email_address = "email@example.com"
  role_ids = [
    "SOME_ROLE_ID"
  ]
}

Link to debug output

None

Panic output

No response

Expected output

No changes

Actual output

The status field shows as a change in the plan. It's not clear if this is just clearing it from state or actually modifying the status.

  ~ resource "cloudflare_account_member" "test" {
        id            = "SNIPPED"
      - status        = "accepted" -> null
        # (3 unchanged attributes hidden)
    }

Steps to reproduce

  1. Run terraform apply to create the account member
  2. Have the user accept the invite
  3. Run a terraform plan

Additional factoids

Sorry this isn't a full bug report, but it seems a fairly straightforward bug with the status field. Just 2 weeks ago, this PR added status to the state: #2156

References

No response

@glenwinters glenwinters added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2023
@github-actions
Copy link
Contributor

Thank you for reporting this issue! For maintainers to dig into issues it is required that all issues include the entirety of TF_LOG=DEBUG output to be provided. The only parts that should be redacted are your user credentials in the X-Auth-Key, X-Auth-Email and Authorization HTTP headers. Details such as zone or account identifiers are not considered sensitive but can be redacted if you are very cautious. This log file provides additional context from Terraform, the provider and the Cloudflare API that helps in debugging issues. Without it, maintainers are very limited in what they can do and may hamper diagnosis efforts.

This issue has been marked with triage/needs-information and is unlikely to receive maintainer attention until the log file is provided making this a complete bug report.

@github-actions github-actions bot added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2023
@lra
Copy link

lra commented Jan 25, 2023

Also happens in 3.33.1.

Contrary to what the doc states at https://registry.terraform.io/providers/cloudflare/cloudflare/latest/docs/resources/account_member#status the status property is not optional as if you don't declare it, you get this drift forever:

  ~ resource "cloudflare_account_member" "test" {
        id            = "SNIPPED"
      - status        = "accepted" -> null
        # (3 unchanged attributes hidden)
    }

and I don't understand why status is not read-only as there is no point modifying this.

@jacobbednarz
Copy link
Member

the status field isn't required but recommended now that we sync the state. I suspect in the near-ish future this will be required from both the API and provider to address sync issues like this.

and I don't understand why status is not read-only as there is no point modifying this.

there are accounts that are entitled to do "direct add" which uses this.

@glenwinters
Copy link
Author

Considering a user has to accept the invite in email, wouldn't the state change always happen outside of Terraform? If so, why require updating Terraform after it's accepted to avoid drift?

@jacobbednarz
Copy link
Member

please see above; not all cases requiring a user to accept the invite.

@george-angel
Copy link

@jacobbednarz that makes absolutely no sense.

cloudflare_account_member resource: https://registry.terraform.io/providers/cloudflare/cloudflare/latest/docs/resources/account_member is not a "direct add" resource, at least it was not in the past.

Meaning a newly created resource will always have status pending on creation. It always required a user to accept an invitation from their mailbox, changing status to accepted outside of Terraform.

there are accounts that are entitled to do "direct add" which uses this.

Then create a new resource that follows this behavior, don't break functionality for everyone else.

@jacobbednarz
Copy link
Member

jacobbednarz commented Jan 26, 2023

@george-angel thanks for letting me how the APIs that i interact with on a daily basis work 😂 direct add is what the feature is called in the UI for those who have it entitled (and the user already has an account), which by the sound of things, is not an account you have access to. the functionality has existed for the better part of 2-3 years but it is gated as it's not behaviour we want for all users out of the box.

unfortunately, bugs can happen. effort is made to avoid it but sometimes these things happen. if you don't like it, you're welcome not to use the software. simple as that.

@george-angel
Copy link

george-angel commented Jan 26, 2023

I have to use your software because our company has a 6 figure contract with your company.

And we have had a Terraform module since 2019-10 defining users, and now you are introducing breaking changes to it.

@lra
Copy link

lra commented Jan 26, 2023

the status field isn't required but recommended now that we sync the state. I suspect in the near-ish future this will be required from both the API and provider to address sync issues like this.

It is effectively required, because if you don't specify it, you get a terraform drift forever even after applying.

unfortunately, bugs can happen. effort is made to avoid it but sometimes these things happen. if you don't like it, you're welcome not to use the software. simple as that.

We usually don't have a choice like stated above, and out of the 30+ providers we have to use, cloudflare is the one that requires the most maintenance by not playing by the terraform provider rules. Example it's still is the only provider that has a deprecated warning with no workaround for 7 months now: #1716 (comment)

The problem in this issue is not that you added a new status property, is that if it's not set, it will drift forever even after applying, instead of setting it to null or whatever the default value of your API should be.

@scluff
Copy link

scluff commented Jan 31, 2023

Adding

lifecycle {
    ignore_changes = [
      status,
    ]
  }

to the resource works around this issue for our use case. I do agree with the sentiment of the other comments, and feel this could be handled better. It seems the only way to avoid drift entirely is the direct add scenario (which is many cases is not even possible due to the member not previously existing). Given that the API default is pending, would having a second "accepter" resource for those opting in (and entitled) to perform a direct add for a cloudflare_account_member not allow this capability while retaining compatibility?

@LanceSandino
Copy link

following

@lra
Copy link

lra commented Feb 6, 2023

Thanks for switching status to computed, and congrats on v4 RC1!

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

This functionality has been released in v3.34.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
6 participants