pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
192 stars 45 forks source link

Overflow causing incorrect values for large integers #1342

Open thomas11 opened 1 year ago

thomas11 commented 1 year ago

Reported in pulumi/pulumi-gcp#1036.

The upstream GCP provider defines this property:

"managed_zone_id": {
    Type:        schema.TypeInt,
    Computed:    true,
},

The pulumi-gcp user observes this:

From a pulumi up -v=9 --logtostderr run:

I0321 22:26:23.855003 2409260 log.go:75] Marshaling property for RPC[Provider[gcp, 0xc00042fd10].Diff(urn:pulumi:brinqa-prod::brinqa-net-private::gcp:dns/managedZone:ManagedZone::brinqa-net-private,projects/brinqa-prod/managedZones/brinqa-net-private).olds]: managedZoneId={2.9610687372243656e+18} Note: managedZoneId={2.9610687372243656e+18}

Then when it tries to perform the update:

I0321 22:26:41.900974 2409260 log.go:75] Updating ManagedZone "projects/xx-prod/managedZones/xx-net-private": map[string]interface {}{"cloudLoggingConfig":map[string]interface {}{}, "creationTime":"2023-03-21T17:05:13.497Z", "description":"xx-net-private", "dnsName":"xx.net.", "id":2961068737224365600, "name":"xx-net-private", [...]} Note: "id":2961068737224365600

This is an integer overflow as shown by the JS console:

> let x = 2961068737224365485;
undefined
> console.log(x);
2961068737224365600
Frassle commented 11 months ago

Related https://github.com/pulumi/pulumi/pull/13868

iwahbe commented 11 months ago

Since this an ID and not actually a number, we should consider changing the type to string (working around https://github.com/pulumi/pulumi/pull/13868).

t0yv0 commented 11 months ago

We might also need to consider proactively detecting overflow and making it visible.

iwahbe commented 11 months ago

We should do this for output values. We won't be able to do this for input values.

mikhailshilkov commented 10 months ago

Another large number issue: https://github.com/pulumi/pulumi-gcp/issues/877

t0yv0 commented 8 months ago

https://github.com/pulumi/pulumi-gcp/issues/1460 possibly..

guineveresaenger commented 5 months ago

Here's another instance of this - I closed this one as a duplicate of https://github.com/pulumi/pulumi-gcp/issues/1036 - https://github.com/pulumi/pulumi-gcp/issues/1836

VenelinMartinov commented 4 months ago

https://github.com/pulumi/pulumi-databricks/issues/55 is likely another instance of this.

iwahbe commented 4 months ago

We can't make progress here until https://github.com/pulumi/pulumi/issues/14242 is closed unless we unilaterally decide to encode all numbers as strings in the state and then re-parse.

t0yv0 commented 4 months ago

IN cases where these are identifiers, have we tried remapping them to strings at Pulumi level (and possibly fixing up any accidental parsing that truncates, though TF-level representations are usually good with big.Float internals). That'd be a breaking change but perhaps worth considering in some of these cases.

Frassle commented 4 months ago

I think that's what @lunaris was looking at doing for the new relic provider.

iwahbe commented 4 months ago

I think that's what @lunaris was looking at doing for the new relic provider.

Very similar. Basically, the problem is that int is not a good type for an id.

t0yv0 commented 1 month ago

Per conversation with @iwahbe, without Pulumi CLI support for large integers our best bet is to map fields of this type to the String type, which is currently supported and rolled out to GCP. What do we need to close this? Perhaps a quick playbook for provider authors explaining the string workaround?

iwahbe commented 1 month ago

I think we will need https://github.com/pulumi/pulumi/issues/14242 to genuinely close this, since users can still be bitten by this issue and provider authors don't have a universally applicable fix.

Right now, this is a tracking issue for https://github.com/pulumi/pulumi/issues/14242.