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

hclsyntax: Template expression produces too large value refinements #616

Closed
wata727 opened this issue Jun 30, 2023 · 3 comments · Fixed by #617
Closed

hclsyntax: Template expression produces too large value refinements #616

wata727 opened this issue Jun 30, 2023 · 3 comments · Fixed by #617

Comments

@wata727
Copy link
Contributor

wata727 commented Jun 30, 2023

See #590
See zclconf/go-cty#153

HCL v2.17 introduced value refinements. However, this can result in template expressions that produce too large refinements.

module example.com/m

go 1.20

require (
        github.com/hashicorp/hcl/v2 v2.17.0
        github.com/zclconf/go-cty v1.13.2
)

require (
        github.com/agext/levenshtein v1.2.1 // indirect
        github.com/apparentlymart/go-textseg/v13 v13.0.0 // indirect
        github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 // indirect
        github.com/vmihailenco/msgpack/v5 v5.3.5 // indirect
        github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect
        golang.org/x/text v0.3.8 // indirect
)
package main

import (
        "fmt"
        "strings"

        "github.com/hashicorp/hcl/v2"
        "github.com/hashicorp/hcl/v2/hclsyntax"
        "github.com/zclconf/go-cty/cty"
        "github.com/zclconf/go-cty/cty/msgpack"
)

func main() {
        src := strings.Repeat("a", 1024) + "${unknown}"

        expr, diags := hclsyntax.ParseTemplate([]byte(src), "", hcl.InitialPos)
        if diags.HasErrors() {
                panic(diags)
        }

        val, diags := expr.Value(&hcl.EvalContext{Variables: map[string]cty.Value{"unknown": cty.UnknownVal(cty.String)}})
        if diags.HasErrors() {
                panic(diags)
        }
        fmt.Println(val.GoString())

        bytes, err := msgpack.Marshal(val, cty.String)
        if err != nil {
                panic(err)
        }
        restored, err := msgpack.Unmarshal(bytes, cty.String)
        if err != nil {
                panic(err)
        }
        fmt.Println(restored.GoString())

}
$ go run main.go
cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").NewValue()
panic: oversize unknown value refinement

goroutine 1 [running]:
main.main()
        /workspaces/tflint/work/refine/main.go:33 +0x305
exit status 2

There are two issues with too large refinements. One, as shown above, is hitting the upper limit of go-cty's MessagePack serialization. The other is that it can be nonsense for user feedback purposes.

Such issues mainly occur in Terraform's templatefile function. Imagine an example with an unknown value at the end of a huge JSON file. terraform-linters/tflint#1791 is an actual reported issue in TFLint.

To be honest, I struggled with whether to report this issue to HCL or go-cty. I believe the solutions to this issue are:

  • Set a reasonable upper limit on the size of refinements in HCL
  • Remove the upper limit of go-cty's MessagePack serialization
  • Add API to remove refinements from the value
    • As far as I can see, there doesn't seem to be a way to discard refinements that have already been set. It probably looks like it can be discarded converted to JSON format, but it would be nice to have an intuitive API like value.WithoutRefinements.
@apparentlymart
Copy link
Contributor

Hi @wata727! Thanks for reporting this.

I think for the moment it's probably most pragmatic to just set a reasonable upper limit in HCL's template implementation where after a certain size it truncates the known prefix. That should still produce a valid refinement because it will broaden rather than tighten the range of the result.

The question of whether and how to have limits in cty is also valid but will require some more consideration. Changing the HCL template logic is less risky for an immediate fix because it only affects one use-case where we know that a problem exists; large strings can potentially appear in other places too, but templates seem like the most likely origin.

@apparentlymart
Copy link
Contributor

Oh and on the subject of removing refinements: today we have some code in Terraform doing that to work around some historical implementation mistakes in Terraform's rules for validating plans and we're doing it by using cty.Transform with a transformer that checks if the value is unknown and if so returns cty.UnknownVal(v.Type()), therefore constructing a new unknown value of the same type without any refinements.

That could potentially be a helper function in cty but I'd like to see if we can avoid the need for it by fixing these rough edges in the main refinements handling.

@wata727
Copy link
Contributor Author

wata727 commented Jul 1, 2023

Thank you for answering my question.

I think for the moment it's probably most pragmatic to just set a reasonable upper limit in HCL's template implementation where after a certain size it truncates the known prefix.

Agreed. I've opened a PR for this issue #617
I struggled with what a "reasonable upper limit" is, but decided on 128 B here. Please let me know if there is a better upper limit. Also, if you have a proper implementation other than this, you can ignore this PR.

we're doing it by using cty.Transform with a transformer that checks if the value is unknown and if so returns cty.UnknownVal(v.Type()), therefore constructing a new unknown value of the same type without any refinements.

Thank you for telling me how to do this. The unrefineValue is indeed what I was looking for. For the time being, I will try #617 to see if the issue is mitigated. If it is difficult to fix, consider implementing a function equivalent to unrefinedValue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants