resource/cloudflare_account: initialize AccountSettings

[jfchevrette]

Attempt to fix a panic issue with uninitialized account.Settings in resourceCloudflareAccountUpdate

Fixes #2031

[github-actions]

changelog detected ✅

[jacobbednarz]

thanks for this @jfchevrette. appreciate it!

instead of unconditionally initialising the Settings i opted to only initialise it when 2FA is present. this avoids unnecessary allocations only to be ignored by the struct. added a test as well to confirm the functionality works as intended.

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareAccount_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareAccount_Basic
=== PAUSE TestAccCloudflareAccount_Basic
=== RUN   TestAccCloudflareAccount_2FAEnforced
=== PAUSE TestAccCloudflareAccount_2FAEnforced
=== CONT  TestAccCloudflareAccount_Basic
--- PASS: TestAccCloudflareAccount_Basic (18.17s)
=== CONT  TestAccCloudflareAccount_2FAEnforced
--- PASS: TestAccCloudflareAccount_2FAEnforced (18.58s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/internal/provider	37.356s

[kwilczynski]

@jacobbednarz, thank you for accepting this change! Much appreciated.

However, with the changes you added atop of the original changeset, you won't ever be able to disable the 2-factor requirements via the enforce_twofactor attribute (the test added wouldn't catch this). This is because of how schema.ResourceData.GetOk() and Go zero-values work.

Have a look at the following. I've added GetOk(), GetOkExists() and printed the variable that holds the Account type, as per:

• Enabling enforce_twofactor:

2022-11-18T21:49:01.484+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout="(string) (len=7"
2022-11-18T21:49:01.484+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout=") "GetOk()""
2022-11-18T21:49:01.484+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout="(bool) true
(bool) true
(string) (len=13) "GetOkExists()"
(bool) true
(bool) true
(string) (len=10) "updatedAcc"
(cloudflare.Account) {
 ID: (string) "",
 Name: (string) (len=11) "kwilczynski",
 Type: (string) "",
 CreatedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC,
 Settings: (*cloudflare.AccountSettings)(0xc000044c20)({
  EnforceTwoFactor: (bool) true
 })
}"

• Disabling enforce_twofactor:

2022-11-18T23:08:03.778+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout=""GetOk()"
(bool) false
(bool) false
(string) (len=13) "GetOkExists()"
(bool) false
(bool) true
(string) (len=10) "updatedAcc"
(cloudflare.Account) {
 ID: (string) "",
 Name: (string) (len=11) "kwilczynski",
 Type: (string) "",
 CreatedOn: (time.Time)"
2022-11-18T23:08:03.778+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout="0001-01-01 00:00:00 +0000 UTC,"
2022-11-18T23:08:03.778+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout=Settings:
2022-11-18T23:08:03.778+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout=(*cloudflare.AccountSettings)(<nil>)
2022-11-18T23:08:03.778+0900 [WARN]  unexpected data: registry.terraform.io/cloudflare/cloudflare:stdout=}

Even though the logging in Terraform can be a bit unwieldy, can you see where the problem manifests itself?

Update:

To add more colour. The change @jfchevrette proposed fixed the problem and also made the other path - the disabling 2-factor authentication one - work, as the Account type has Settings initialised before the request was made, and it so happens that AccountSettings type holds enforce_twofactor as a normal bool type value, which in Go has it's zero-value as false, thus when you set the attribute to false it would just work (from the users point of view) as the default turns out to also be false.

[jacobbednarz]

the zero value issue will need to be solved by updating the struct in cloudflare-go to be a boolean pointer. regardless of where the settings struct would have been initialised, we still would have had this issue due to zero values in Terraform, not only Go (this is something I've raised with the core team a couple of times).

[jacobbednarz]

actually disregard; this is slightly different to the underlying issue at hashicorp/terraform-plugin-sdk#817.

as the schema has false as the default, the update shouldn't be conditionally setting this at all. the payload should always include the value regardless.

you're welcome to send a PR removing the conditional or I can do it next week.

[kwilczynski]

Hi @jacobbednarz,

actually disregard; this is slightly different to the underlying issue at hashicorp/terraform-plugin-sdk#817.

I am glad you raised this with them. I was worried that it would be a hill I, and many other people, would die on alone, so to speak. This has been an issue in Terraform for almost half a decade now.

as the schema has false as the default, the update shouldn't be conditionally setting this at all. the payload should always include the value regardless.

you're welcome to send a PR removing the conditional or I can do it next week.

You can do it at your convenience as long as it makes it into the next release. No rush otherwise. Also, thank you in advance for your help! Much appreciated.

[jacobbednarz]

i've raised #2036 which covers the removal use case however, in testing this, i also found a bug that prevents creating a new account with the settings from the start. the only way to apply 2FA is with a second run to update the resource. thanks for helping uncover these two issues!

[kwilczynski]

Hi @jacobbednarz,

i've raised #2036 which covers the removal use case however, in testing this, i also found a bug that prevents creating a new account with the settings from the start. the only way to apply 2FA is with a second run to update the resource.

This amazing. Thank you so much!

thanks for helping uncover these two issues!

My pleasure! This is some amazing work you and your team are doing with this provider, so I am always happy to help as its user. :)

[github-actions]

This functionality has been released in v3.29.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!