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

Planned value does not match config value for number #34866

Closed
bendbennett opened this issue Mar 20, 2024 · 14 comments · Fixed by #35020
Closed

Planned value does not match config value for number #34866

bendbennett opened this issue Mar 20, 2024 · 14 comments · Fixed by #35020
Assignees
Labels
bug upstream v1.8 Issues (primarily bugs) reported against v1.8 releases

Comments

@bendbennett
Copy link

Terraform Version

Terraform v1.8.0-dev
on darwin_arm64

v1.8.0-beta1

Terraform Configuration Files

resource "playground_resource" "example" {
  number_attribute = 9223372036854775808
}

Debug Output

https://gist.github.com/bendbennett/694e16874249f8aaa1a9ab14da84129f

Expected Behavior

Expecting the plan displayed by the CLI to look as follows:

Terraform will perform the following actions:

  # playground_resource.example will be created
  + resource "playground_resource" "example" {
      + number_attribute = 9223372036854775808
    }

Actual Behavior

Plan displayed by the CLI looks as follows:

  # playground_resource.example will be created
  + resource "playground_resource" "example" {
      + number_attribute = 9223372036854776000
    }

The value stored in state matches that from the plan, but differs from that supplied in the configuration:

{
  "version": 4,
  "terraform_version": "1.8.0",
  "serial": 1,
  "lineage": "e3092a01-aadf-643b-a61c-f26cac8e0ca1",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "playground_resource",
      "name": "example",
      "provider": "provider[\"registry.terraform.io/bendbennett/playground\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "number_attribute": 9223372036854776000
          },
          "sensitive_attributes": []
        }
      ]
    }
  ],
  "check_results": null
}

Steps to Reproduce

terraform apply

Additional Context

This appears to be specific to the value of the integer supplied in the configuration (i.e., 9223372036854775808).

The structure of the provider is as follows:

package provider

import (
	"context"

	"github.com/hashicorp/terraform-plugin-framework/path"
	"github.com/hashicorp/terraform-plugin-framework/resource"
	"github.com/hashicorp/terraform-plugin-framework/resource/schema"
	"github.com/hashicorp/terraform-plugin-framework/types"
)

var _ resource.Resource = (*playgroundResource)(nil)
var _ resource.ResourceWithImportState = (*playgroundResource)(nil)

type playgroundResource struct {
}

func NewResource() resource.Resource {
	return &playgroundResource{}
}

func (e *playgroundResource) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) {
	resp.TypeName = req.ProviderTypeName + "_resource"
}

func (e *playgroundResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"number_attribute": schema.NumberAttribute{
				Optional: true,
				Computed: true,
			},
		},
	}
}

type playgroundResourceData struct {
	NumberAttribute types.Number `tfsdk:"number_attribute"`
}

func (e *playgroundResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
	var data playgroundResourceData

	diags := req.Plan.Get(ctx, &data)
	resp.Diagnostics.Append(diags...)

	if resp.Diagnostics.HasError() {
		return
	}

	diags = resp.State.Set(ctx, &data)
	resp.Diagnostics.Append(diags...)
}

func (e *playgroundResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
	var data playgroundResourceData

	diags := req.State.Get(ctx, &data)
	resp.Diagnostics.Append(diags...)

	if resp.Diagnostics.HasError() {
		return
	}

	diags = resp.State.Set(ctx, &data)
	resp.Diagnostics.Append(diags...)
}

func (e *playgroundResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
	var data playgroundResourceData

	diags := req.Plan.Get(ctx, &data)
	resp.Diagnostics.Append(diags...)

	if resp.Diagnostics.HasError() {
		return
	}

	diags = resp.State.Set(ctx, &data)
	resp.Diagnostics.Append(diags...)
}

func (e *playgroundResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
	var data playgroundResourceData

	diags := req.State.Get(ctx, &data)
	resp.Diagnostics.Append(diags...)

	if resp.Diagnostics.HasError() {
		return
	}
}

func (e *playgroundResource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
	resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp)
}

The v1.8.0-beta1 version of Terraform appears to be using github.com/zclconf/go-cty v1.14.3 which contains the fix for a previously reported, somewhat similar issue - #34589

References

@jbardin
Copy link
Member

jbardin commented Mar 20, 2024

Thanks @bendbennett!

This is quite peculiar, because while the plan appears to have stored the truncated value, at least within Terraform alone the final state still ends up with the correct 9223372036854775808, meaning the problem is entirely within the rendering of the plan to both the CLI and JSON.

If your example (which I haven't tried yet) ends up with the incorrect value in state, then there is a separate bug to track down somewhere in the protocol/framework.

@liamcervante
Copy link
Member

liamcervante commented Mar 20, 2024

This is also potentially related to #34686, which I thought had been fixed. There might have been another rendering path somewhere that has been missed.

@liamcervante
Copy link
Member

Just as little addition here:

{
  "format_version": "1.2",
  "terraform_version": "1.9.0-dev",
  "planned_values": {
    "root_module": {
      "resources": [
        {
          "address": "tfcoremock_simple_resource.resource",
          "mode": "managed",
          "type": "tfcoremock_simple_resource",
          "name": "resource",
          "provider_name": "registry.terraform.io/hashicorp/tfcoremock",
          "schema_version": 0,
          "values": {
            "bool": null,
            "float": null,
            "integer": null,
            "number": 9223372036854776000,
            "string": null
          },
          "sensitive_values": {}
        }
      ]
    }
  },
  "resource_changes": [
    {
      "address": "tfcoremock_simple_resource.resource",
      "mode": "managed",
      "type": "tfcoremock_simple_resource",
      "name": "resource",
      "provider_name": "registry.terraform.io/hashicorp/tfcoremock",
      "change": {
        "actions": [
          "create"
        ],
        "before": null,
        "after": {
          "bool": null,
          "float": null,
          "integer": null,
          "number": 9223372036854776000,
          "string": null
        },
        "after_unknown": {
          "id": true
        },
        "before_sensitive": false,
        "after_sensitive": {}
      }
    }
  ],
  "configuration": {
    "provider_config": {
      "tfcoremock": {
        "name": "tfcoremock",
        "full_name": "registry.terraform.io/hashicorp/tfcoremock"
      }
    },
    "root_module": {
      "resources": [
        {
          "address": "tfcoremock_simple_resource.resource",
          "mode": "managed",
          "type": "tfcoremock_simple_resource",
          "name": "resource",
          "provider_config_key": "tfcoremock",
          "expressions": {
            "number": {
              "constant_value": 9223372036854775808
            }
          },
          "schema_version": 0
        }
      ]
    }
  },
  "timestamp": "2024-03-20T14:44:45Z",
  "applyable": true,
  "complete": true,
  "errored": false
}

The correct value is written within the configuration block in the JSON output, but the truncated value within the resource_changes.

@liamcervante
Copy link
Member

liamcervante commented Mar 20, 2024

The problematic number raised here is 2^63. (2^63)-1 and (2^63)+1 are not affected and other numbers close by seem to be unaffected (at least I couldn't find any). But the following are also affected:

  • 2^64
  • 2^65
  • 2^66
  • 2^67
  • 2^68

And the same pattern seems to continue, with other numbers around those numbers being unaffected and only the exact whole numbers being truncated. I don't have an explanation for this, and I don't think this is in the rendering code of the human-readable plan as this is being printed by the JSON output as well. Very strange.

@liamcervante
Copy link
Member

Also, no number satisfying x < 2^63 is affected so 2^62 and 2^61 and so on are fine.

@apparentlymart
Copy link
Member

apparentlymart commented Mar 20, 2024

These seem like the "breakpoints" where the floating point exponent would increase, making the mantissa the same (1) for all of these examples, right?

So it seems like what they have in common is that the exponent is >= 63 and the exponent is 1. I don't know what to conclude from that yet, but it is at least an interesting pattern!

@liamcervante
Copy link
Member

There's still something in go-cty or msgpack causing this, I've added some test cases to go-cty that demonstrate this. Might be helpful as a starting point for whoever ends up picking this up.

@apparentlymart
Copy link
Member

apparentlymart commented Mar 20, 2024

One way we've seen symptoms like this before was accidentally creating a big.Float with only 52-bit precision instead of full precision. I think that mainly arose in converting from float64 to big.Float, where unless you override it the precision of the source float is preserved. But I wonder if there's something similar going on when parsing big.Float from a string on certain codepaths.

@apparentlymart
Copy link
Member

apparentlymart commented Mar 20, 2024

I've debugged enough to at least describe the circumstances that cause the bug:

If the given number is a whole number that would fit in an int64 then cty/msgpack encodes as the smallest possible MessagePack integer format. MessagePack does not have integer formats greater than signed 64-bit.

If the number is too big for an int64 then cty/msgpack then tries to take the big/float as a float64 and pass that to MessagePack. It's that entry point that causes this bug to happen. It seems that big.Float.Float64 is indicating that the result has "exact" accuracy in that case, which I think makes sense because mantissa 1 and exponent >=63, <2048 can fit into a float64 without loss. The number one less than that would have a mantissa that doesn't fit in a float64, and so cty/msgpack would encode it as a string.

Therefore it seems that the problem is in the round-tripping of some values when they get encoded as float64. What I've not determined yet, but intend to determine next, is exactly where the precision is being lost.

@apparentlymart
Copy link
Member

It does indeed seem to be a repeat of the "big.Float with only float64 precision" problem. Specifically, cty.NumberFloat64 constructs a big.Float based on a float64 and doesn't override the precision, so the result has precision 52.

Explicitly setting the precision to 512 (which is the convention elsewhere in cty) makes the string representation the same before and after. Using a smaller precision apparently causes the formatter to do some rounding.

I'm going to fix this upstream by explicitly setting the precision to 512 before populating the big.Float from the float64. That should then fix anything that uses cty.NumberFloatVal, which includes the MessagePack unmarshaler.

@apparentlymart
Copy link
Member

Actually, I've changed my mind: unilaterally deciding to expand the precision of all numbers when converting to float64 has some annoying effects on the string representations of fractional numbers that can't be exactly represented in floating point, which I now remember is why this is written the way it is.

Instead of pretending incoming values are higher precision than they are, we instead expand the precision "just in time" during the operations that would benefit from it, so that we're doing calculations with higher precision when needed, but avoid implying in string representations that there's more precision available than was present in the source value.

I think the most narrow fix here is to avoid using the float64 encoding for whole numbers even if they can technically fit there, and instead have the encoder use the string representation of any integer that's too big to represent as one of the MessagePack integer encodings. That'll ensure that they get decoded at full precision, because we do always use precision 512 for numbers parsed from strings.

@apparentlymart
Copy link
Member

apparentlymart commented Mar 20, 2024

I've fixed this upstream in cty v1.14.4. The specific commit is zclconf/go-cty@f41ae52 .

However, it's important to note that I fixed this by changing the MessagePack encoder to use a different encoding for large integers. terraform-plugin-go has its own MessagePack implementation that has the same behavior, so upgrading cty here should fix it in the core-to-provider direction, but will not fix it in the provider-to-core direction.

I'm not sure which was causing the problem here -- perhaps it's both, since we round-trip through MessagePack in both directions after all -- but either way I would suggest making a similar change to terraform-plugin-go so that the treatment of numbers will be the same whichever way they are travelling.

I'm not going to propose any upgrade change in Terraform yet, because I expect we'll probably want to coordinate the cty change and the terraform-plugin-go change to release at a similar time so we can minimize the window during which things are inconsistent. If someone else wants to open a PR to upgrade to newer cty once we're feeling ready to do it, please go ahead!

@bendbennett
Copy link
Author

Thank you all for the comprehensive investigation and feedback everyone. Much appreciated.
Thanks also for the fix in cty v1.14.4.
I've opened an issue on terraform-plugin-go to track coordination of the change in core and the change that needs to be made to terraform-plugin-go.

@jbardin jbardin self-assigned this Apr 4, 2024
@jbardin jbardin added upstream and removed new new issue not yet triaged labels Apr 4, 2024
@apparentlymart apparentlymart added the v1.8 Issues (primarily bugs) reported against v1.8 releases label Apr 15, 2024
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug upstream v1.8 Issues (primarily bugs) reported against v1.8 releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants