Plan crashes with provider 2.19.1 #988
Comments
warwick-mitchell1
added
kind/bug
|
I can confirm, exact same stack trace. 2.19.0 is fine here. only 2.19.1 has the bug. 97b4439 looks like the culprit |
|
Yep, 2.19.1 only had a single commit 🙂 I started looking into this earlier today via #984 but still don't have a test case that replicates. Is it particular record types? Existing state? Any pointers how you've hit this one? |
|
Hitting the same issue. Looks to me that record.Priority is not set and then then the assignment errors out on line 442. I think this happens for me for pretty much all dns records. |
|
I don't know whether it is as simple as that because we have other test cases exercising various code paths (types, values, etc) that are not triggering a panic. I'll continue digging when I'm in front of a computer next. |
|
Inspected some records states and I can see not all records have the priority set in the state file: resource "cloudflare_record" "XXXXXX" { |
|
I'm getting a bunch of following errors: And part of crash report: Started to happen today with newest provider 2.19.1. Works ok with 2.19.0. |
|
Should priority be set in the terraform code? Its failing when it tries to dereference the pointer. https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/cloudflare/resource_cloudflare_record.go#L442 This function does not check for the zero value(Priority not present) https://github.com/hashicorp/terraform/blob/6697245c18dad6848e7647598dda1ab04c2680ca/internal/legacy/helper/schema/resource_diff.go#L409 |
Pin the CloudFlare provider version to overcome: cloudflare/terraform-provider-cloudflare#988 Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
|
This also broke all of our workspaces. This should probably be reverted. |
|
Our config that hits the error: provider "cloudflare" { The error: Error: Error refreshing state: 1 error occurred: Pinning the provider at 2.19.0 avoids it for us. |
|
@usrlocalang no - priority is only valid for some record types and for this block in particular, the
@tedivm apologies for the disruption, I'd recommend pinning to v2.19.0 in the meantime while this is being addressed. |
|
The potential fix here looks like this: @@ -437,7 +437,7 @@ func resourceCloudflareRecordRead(d *schema.ResourceData, meta interface{}) erro
}
d.Set("proxiable", record.Proxiable)
- if _, ok := d.GetOkExists("priority"); ok {
+ if record.Priority != nil {
priority := record.Priority
p := *priority
d.Set("priority", fmt.Sprintf("%d", int(p)))Which then moves the onus onto the service API to always provide the priority as needed. The issue, however, is that I don't yet have a test case that fails in the broken code causing others to crash which means we don't have a good understanding of the conditions that cause this scenario. |
jacobbednarz
added
kind/crash
Following on from the changes in 2.19.0 (and 2.19.1 to actually ship the change), a regression was introduced that is inadvertantly causing a panic due to the `d.GetOkExists` failing to accurately determine when to apply the `record.Priority` value to the state. `d.GetOkExists` is documented as: > GetOkExists can check if TypeBool attributes that are Optional with > no Default value have been set. While the value isn't a bool, it acts similarly as 0 in this context is both a valid value and a zero value for the field. To combat the panic, I've swapped the check to instead compare the value provided by the API to determine whether or not we want to set it. This approach wasn't initially taken as the API used to return 0 in all cases where the value wasn't explicitly set and the underlying library would ignore it. Hopefully, with MX records allowing 0, this is no longer an issue going forward. Closes #988
|
I've pushed up #992 with a fix for this but seeing how I don't have a reproducible case, I'll need a few people here to checkout the branch locally and build the provider to confirm it fixes the issue before we release it. Please report your findings in #992, not here. |
Following on from the changes in 2.19.0 (and 2.19.1 to actually ship the change), a regression was introduced that is inadvertantly causing a panic due to the `d.GetOkExists` failing to accurately determine when to apply the `record.Priority` value to the state. `d.GetOkExists` is documented as: > GetOkExists can check if TypeBool attributes that are Optional with > no Default value have been set. While the value isn't a bool, it acts similarly as 0 in this context is both a valid value and a zero value for the field. To combat the panic, I've swapped the check to instead compare the value provided by the API to determine whether or not we want to set it. This approach wasn't initially taken as the API used to return 0 in all cases where the value wasn't explicitly set and the underlying library would ignore it. Hopefully, with MX records allowing 0, this is no longer an issue going forward. Closes #988
Terraform version
Terraform v0.14.6
Affected resource(s)
cloudflare_record
Terraform configuration files
Debug output
Panic output
Expected output
Plan to succeed
Actual output
Crash, see above
Steps to reproduce
Additional factoids
Work around was to pin at last known good version 2.18.0
References
No response
The text was updated successfully, but these errors were encountered: