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

Add support for 64-bit integers and/or "big" integers? #92

Closed
ewbankkit opened this issue Jan 10, 2018 · 8 comments
Closed

Add support for 64-bit integers and/or "big" integers? #92

ewbankkit opened this issue Jan 10, 2018 · 8 comments
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.

Comments

@ewbankkit
Copy link
Contributor

Motivated by support for Amazon side ASNs in hashicorp/terraform-provider-aws#1888 and hashicorp/terraform-provider-aws#2861 but more widely applicable.

Terraform uses the golang int type for values of TypeInt and the underlying width of the integer depends on the platform the binary is built for, so integer values greater than 2147483647 cannot safely be represented using TypeInt.

Adding support for TypeInt64 (golang int64) and/or TypeBigInt (golang big.Int) would touch more than just the schema code, affecting HIL, the interpolation functions etc.

@apparentlymart
Copy link
Contributor

Hi @ewbankkit!

The improved configuration language interpreter we're currently working on integrating (based on what is currently codenamed HCL2 removes the int vs. float distinction and instead makes all numbers arbitrary-precision floats, which are internally represented as big.Float.

As you noted, there are a lot of touchpoints to make this change across the whole of Terraform, and so our initial work is focused on making it work within Terraform Core, the interpolation language, functions, etc but will not include any changes to the helper/schema API. For the moment it will convert the big.Float representation to int automatically, and will thus unfortunately still be constrained by the platform-dependent int type in the initial release.

We plan to work on this helper/schema API change separately so we can take the time to find a suitable compromise that keeps the existing wealth of provider code working while allowing for the various new possibilities in the new type system. This might entail adding new type constants to helper/schema as you noted, or we may find some other strategy, but either way we need to complete the core-level integration first so that large integers (and more-precise floats) can safely pass through the rest of the system without error. Once Terraform Core is big-number-aware, a change to support additional Go native number types would be confined only to helper/schema, as convenience conversions from big.Float.

It will take some time for us to reach the point where this change is fully realized, so in the short term we've usually resorted to using TypeString attributes and then parsing them into integers of the appropriate range within the provider code itself. This does, of course, have some drawbacks for maintainability and error checking, but it allows progress to be made without blocking on the big configuration language project. The CustomizeDiff feature added in hashicorp/terraform#14887, in conjunction with the helper/customdiff library, provides a means to do range validation at plan time so that any error can be reported to the user.

@apparentlymart
Copy link
Contributor

I've labelled this with "schema-helper" because the Core type system work is already in progress and so I'd like to use this issue to represent the subsequent work to introduce new numeric types to helper/schema.

@ewbankkit
Copy link
Contributor Author

@apparentlymart Thanks for the thorough explanation.
I saw this code
https://github.com/hashicorp/terraform/blob/71e989ba3edb40e339adaf72e9499060c5216b16/config/hcl2shim/values.go#L48-L49
which made me think that a workaround for my particular case would be to use TypeFloat but I agree that use of TypeString for now is more consistent.

@apparentlymart
Copy link
Contributor

Using a TypeFloat could be reasonable in your case if the value you're looking for will fit within the significand of a float64, which gives you 53 bits. I'd suggest still doing some validation on that to verify that the user did indeed provide a whole number (rather than implicitly rounding), which means that from the user's standpoint it shouldn't make a big difference compared to TypeString.

This "hcl2shim" code is there to support the opt-in experimental version of the new language system that we're planning to release to gather feedback and do more testing before we switch wholesale to the new implementation, so this code is making a "best effort" attempt to map the new system onto the expectations of the old system so that the two can coexist for a few releases. This code will be removed once the new system is first-class, but indeed there will need to be some similar code in helper/schema at that point to retain compatibility with that abstraction.

(Note that the shim is intentionally allowing loss of precision for integers that require more than 53 bits on a 32-bit system, since that compatibility shim's goal is to make sure we can represent all possible values that HCL and helper/schema understand today, rather than to enable any new representations.)

@kilokahn
Copy link

Hey @apparentlymart - thanks for conveying the workaround of using TypeString in the interim. If we have an existing resource argument that is currently represented as a TypeInt and we change its schema to TypeString, the corresponding .tf config doesn't need to explicitly quote the rvalue, right? I am assuming that the framework takes care of converting it.

Also, conversely, once a resource argument has been made as a TypeString and we switch it to TypeInt (or TypeInt64 in the future), the conversion to/from the state file will be seamlessly handled by the framework, right?

In my limited testing, I do see this being done by the framework, but was not sure if this was an intended behavior that the users of the framework can depend on.

Thanks!

@apparentlymart
Copy link
Contributor

Yes, today the provider SDK is doing that automatic conversion.

From 0.12 onwards it will be Terraform Core that does the conversion, since it will then know the expected types of all arguments and the configuration language interpreter itself will do type checking and automatic conversions where possible. From the perspective of the provider SDK API there should be no discernible difference, though... just the same work being done in a different place.

@hashibot hashibot transferred this issue from hashicorp/terraform Sep 26, 2019
@hashibot hashibot added the enhancement New feature or request label Oct 2, 2019
@paddycarver paddycarver added the subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it. label Jan 6, 2021
@paddycarver
Copy link
Contributor

I believe #662 resolved this.

@github-actions
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 Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/types Issues and feature requests related to the type system of Terraform and our shims around it.
Projects
None yet
Development

No branches or pull requests

6 participants