From 1ef550e59a5f2b894186fa5086fa59e037dfae61 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 22 May 2023 08:21:02 -0700 Subject: [PATCH 1/8] go.mod: go get github.com/zclconf/go-cty@v1.3.2 --- go.mod | 6 +++--- go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index caaeddb8717a..40e6186e1f10 100644 --- a/go.mod +++ b/go.mod @@ -76,7 +76,7 @@ require ( github.com/tombuildsstuff/giovanni v0.15.1 github.com/xanzy/ssh-agent v0.3.1 github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557 - github.com/zclconf/go-cty v1.12.2 + github.com/zclconf/go-cty v1.13.2 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b github.com/zclconf/go-cty-yaml v1.0.3 golang.org/x/crypto v0.1.0 @@ -172,8 +172,8 @@ require ( github.com/spf13/pflag v1.0.5 // indirect github.com/stretchr/objx v0.5.0 // indirect github.com/ulikunitz/xz v0.5.10 // indirect - github.com/vmihailenco/msgpack/v4 v4.3.12 // indirect - github.com/vmihailenco/tagparser v0.1.1 // indirect + github.com/vmihailenco/msgpack/v5 v5.3.5 // indirect + github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect go.opencensus.io v0.23.0 // indirect golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect golang.org/x/time v0.3.0 // indirect diff --git a/go.sum b/go.sum index de4368b5b7f2..8bee50a7284d 100644 --- a/go.sum +++ b/go.sum @@ -778,10 +778,10 @@ github.com/tombuildsstuff/giovanni v0.15.1/go.mod h1:0TZugJPEtqzPlMpuJHYfXY6Dq2u github.com/ulikunitz/xz v0.5.10 h1:t92gobL9l3HE202wg3rlk19F6X+JOxl9BBrCCMYEYd8= github.com/ulikunitz/xz v0.5.10/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oWt/14= github.com/vmihailenco/msgpack v3.3.3+incompatible/go.mod h1:fy3FlTQTDXWkZ7Bh6AcGMlsjHatGryHQYUTf1ShIgkk= -github.com/vmihailenco/msgpack/v4 v4.3.12 h1:07s4sz9IReOgdikxLTKNbBdqDMLsjPKXwvCazn8G65U= -github.com/vmihailenco/msgpack/v4 v4.3.12/go.mod h1:gborTTJjAo/GWTqqRjrLCn9pgNN+NXzzngzBKDPIqw4= -github.com/vmihailenco/tagparser v0.1.1 h1:quXMXlA39OCbd2wAdTsGDlK9RkOk6Wuw+x37wVyIuWY= -github.com/vmihailenco/tagparser v0.1.1/go.mod h1:OeAg3pn3UbLjkWt+rN9oFYB6u/cQgqMEUPoW2WPyhdI= +github.com/vmihailenco/msgpack/v5 v5.3.5 h1:5gO0H1iULLWGhs2H5tbAHIZTV8/cYafcFOr9znI5mJU= +github.com/vmihailenco/msgpack/v5 v5.3.5/go.mod h1:7xyJ9e+0+9SaZT0Wt1RGleJXzli6Q/V5KbhBonMG9jc= +github.com/vmihailenco/tagparser/v2 v2.0.0 h1:y09buUbR+b5aycVFQs/g70pqKVZNBmxwAhO7/IwNM9g= +github.com/vmihailenco/tagparser/v2 v2.0.0/go.mod h1:Wri+At7QHww0WTrCBeu4J6bNtoV6mEfg5OIWRZA9qds= github.com/xanzy/ssh-agent v0.3.1 h1:AmzO1SSWxw73zxFZPRwaMN1MohDw8UyHnmuxyceTEGo= github.com/xanzy/ssh-agent v0.3.1/go.mod h1:QIE4lCeL7nkC25x+yA3LBIYfwCc1TFziCtG7cBAac6w= github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557 h1:Jpn2j6wHkC9wJv5iMfJhKqrZJx3TahFx+7sbZ7zQdxs= @@ -795,8 +795,8 @@ github.com/yuin/goldmark v1.4.0/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= github.com/zclconf/go-cty v1.1.0/go.mod h1:xnAOWiHeOqg2nWS62VtQ7pbOu17FtxJNW8RLEih+O3s= github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= -github.com/zclconf/go-cty v1.12.2 h1:h4VH6eKXHTw60DiEJEVjh6pqVPDcoe3DuAkH/Ejs+4g= -github.com/zclconf/go-cty v1.12.2/go.mod h1:s9IfD1LK5ccNMSWCVFCE2rJfHiZgi7JijgeWIMfhLvA= +github.com/zclconf/go-cty v1.13.2 h1:4GvrUxe/QUDYuJKAav4EYqdM47/kZa672LwmXFmEKT0= +github.com/zclconf/go-cty v1.13.2/go.mod h1:YKQzy/7pZ7iq2jNFzy5go57xdxdWoLLpaEp4u238AE0= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8= github.com/zclconf/go-cty-yaml v1.0.3 h1:og/eOQ7lvA/WWhHGFETVWNduJM7Rjsv2RRpx1sdFMLc= From c91297015344a183268d3664f0618a97b4a44963 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 7 Feb 2023 14:07:25 -0800 Subject: [PATCH 2/8] lang/funcs: Non-null refinements for various functions cty's new "refinements" concept allows us to reduce the range of unknown values from our functions. This initial changeset focuses only on declaring which functions are guaranteed to return a non-null result, which is a helpful baseline refinement because it allows "== null" and "!= null" tests to produce known results even when the given value is otherwise unknown. This commit also includes some updates to test results that are now refined based on cty's own built-in refinement behaviors, just as a result of us having updated cty in the previous commit. --- internal/lang/funcs/cidr.go | 12 ++++--- internal/lang/funcs/collection.go | 18 +++++++--- internal/lang/funcs/collection_test.go | 49 ++++++++++++++------------ internal/lang/funcs/crypto.go | 20 +++++++---- internal/lang/funcs/datetime.go | 11 +++--- internal/lang/funcs/datetime_test.go | 14 ++++---- internal/lang/funcs/encoding.go | 18 ++++++---- internal/lang/funcs/encoding_test.go | 16 ++++----- internal/lang/funcs/filesystem.go | 21 +++++++---- internal/lang/funcs/number.go | 10 ++++-- internal/lang/funcs/refinements.go | 9 +++++ internal/lang/funcs/string.go | 9 +++-- 12 files changed, 130 insertions(+), 77 deletions(-) create mode 100644 internal/lang/funcs/refinements.go diff --git a/internal/lang/funcs/cidr.go b/internal/lang/funcs/cidr.go index d183e7ac648c..12f27943178a 100644 --- a/internal/lang/funcs/cidr.go +++ b/internal/lang/funcs/cidr.go @@ -27,7 +27,8 @@ var CidrHostFunc = function.New(&function.Spec{ Type: cty.Number, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var hostNum *big.Int if err := gocty.FromCtyValue(args[1], &hostNum); err != nil { @@ -56,7 +57,8 @@ var CidrNetmaskFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { _, network, err := ipaddr.ParseCIDR(args[0].AsString()) if err != nil { @@ -88,7 +90,8 @@ var CidrSubnetFunc = function.New(&function.Spec{ Type: cty.Number, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var newbits int if err := gocty.FromCtyValue(args[1], &newbits); err != nil { @@ -126,7 +129,8 @@ var CidrSubnetsFunc = function.New(&function.Spec{ Name: "newbits", Type: cty.Number, }, - Type: function.StaticReturnType(cty.List(cty.String)), + Type: function.StaticReturnType(cty.List(cty.String)), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { _, network, err := ipaddr.ParseCIDR(args[0].AsString()) if err != nil { diff --git a/internal/lang/funcs/collection.go b/internal/lang/funcs/collection.go index 03b87a40c4e9..88dc7c941285 100644 --- a/internal/lang/funcs/collection.go +++ b/internal/lang/funcs/collection.go @@ -35,6 +35,7 @@ var LengthFunc = function.New(&function.Spec{ return cty.Number, errors.New("argument must be a string, a collection type, or a structural type") } }, + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { coll := args[0] collTy := args[0].Type() @@ -71,7 +72,8 @@ var AllTrueFunc = function.New(&function.Spec{ Type: cty.List(cty.Bool), }, }, - Type: function.StaticReturnType(cty.Bool), + Type: function.StaticReturnType(cty.Bool), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { result := cty.True for it := args[0].ElementIterator(); it.Next(); { @@ -100,7 +102,8 @@ var AnyTrueFunc = function.New(&function.Spec{ Type: cty.List(cty.Bool), }, }, - Type: function.StaticReturnType(cty.Bool), + Type: function.StaticReturnType(cty.Bool), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { result := cty.False var hasUnknown bool @@ -149,6 +152,7 @@ var CoalesceFunc = function.New(&function.Spec{ } return retType, nil }, + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { for _, argVal := range args { // We already know this will succeed because of the checks in our Type func above @@ -181,7 +185,8 @@ var IndexFunc = function.New(&function.Spec{ Type: cty.DynamicPseudoType, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { if !(args[0].Type().IsListType() || args[0].Type().IsTupleType()) { return cty.NilVal, errors.New("argument must be a list or tuple") @@ -346,6 +351,7 @@ var MatchkeysFunc = function.New(&function.Spec{ // the return type is based on args[0] (values) return args[0].Type(), nil }, + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { if !args[0].IsKnown() { return cty.UnknownVal(cty.List(retType.ElementType())), nil @@ -489,7 +495,8 @@ var SumFunc = function.New(&function.Spec{ Type: cty.DynamicPseudoType, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { if !args[0].CanIterateElements() { @@ -558,7 +565,8 @@ var TransposeFunc = function.New(&function.Spec{ Type: cty.Map(cty.List(cty.String)), }, }, - Type: function.StaticReturnType(cty.Map(cty.List(cty.String))), + Type: function.StaticReturnType(cty.Map(cty.List(cty.String))), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { inputMap := args[0] if !inputMap.IsWhollyKnown() { diff --git a/internal/lang/funcs/collection_test.go b/internal/lang/funcs/collection_test.go index d0459411cb98..e3749d2efd67 100644 --- a/internal/lang/funcs/collection_test.go +++ b/internal/lang/funcs/collection_test.go @@ -71,11 +71,15 @@ func TestLength(t *testing.T) { }, { cty.UnknownVal(cty.List(cty.Bool)), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).Refine(). + NotNull(). + NumberRangeLowerBound(cty.Zero, true). + NumberRangeUpperBound(cty.NumberIntVal(math.MaxInt), true). + NewValue(), }, { cty.DynamicVal, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), }, { cty.StringVal("hello"), @@ -120,11 +124,10 @@ func TestLength(t *testing.T) { }, { cty.UnknownVal(cty.String), - cty.UnknownVal(cty.Number), - }, - { - cty.DynamicVal, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).Refine(). + NotNull(). + NumberRangeLowerBound(cty.Zero, true). + NewValue(), }, { // Marked collections return a marked length cty.ListVal([]cty.Value{ @@ -229,7 +232,7 @@ func TestAllTrue(t *testing.T) { }, { cty.ListVal([]cty.Value{cty.UnknownVal(cty.Bool)}), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -237,12 +240,12 @@ func TestAllTrue(t *testing.T) { cty.UnknownVal(cty.Bool), cty.UnknownVal(cty.Bool), }), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { cty.UnknownVal(cty.List(cty.Bool)), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -310,7 +313,7 @@ func TestAnyTrue(t *testing.T) { }, { cty.ListVal([]cty.Value{cty.UnknownVal(cty.Bool)}), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -318,7 +321,7 @@ func TestAnyTrue(t *testing.T) { cty.UnknownVal(cty.Bool), cty.False, }), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -331,7 +334,7 @@ func TestAnyTrue(t *testing.T) { }, { cty.UnknownVal(cty.List(cty.Bool)), - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -409,17 +412,17 @@ func TestCoalesce(t *testing.T) { }, { []cty.Value{cty.UnknownVal(cty.Bool), cty.True}, - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { []cty.Value{cty.UnknownVal(cty.Bool), cty.StringVal("hello")}, - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), false, }, { []cty.Value{cty.DynamicVal, cty.True}, - cty.UnknownVal(cty.Bool), + cty.UnknownVal(cty.Bool).RefineNotNull(), false, }, { @@ -1065,7 +1068,7 @@ func TestMatchkeys(t *testing.T) { cty.ListVal([]cty.Value{ cty.StringVal("ref1"), }), - cty.UnknownVal(cty.List(cty.String)), + cty.UnknownVal(cty.List(cty.String)).RefineNotNull(), false, }, { // different types that can be unified @@ -1529,7 +1532,7 @@ func TestSum(t *testing.T) { cty.StringVal("b"), cty.StringVal("c"), }), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), "argument must be list, set, or tuple of number values", }, { @@ -1583,7 +1586,7 @@ func TestSum(t *testing.T) { cty.StringVal("a"), cty.NumberIntVal(38), }), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), "argument must be list, set, or tuple of number values", }, { @@ -1603,17 +1606,17 @@ func TestSum(t *testing.T) { }, { cty.UnknownVal(cty.Number), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), "", }, { cty.UnknownVal(cty.List(cty.Number)), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), "", }, { // known list containing unknown values cty.ListVal([]cty.Value{cty.UnknownVal(cty.Number)}), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), "", }, { // numbers too large to represent as float64 @@ -1707,7 +1710,7 @@ func TestTranspose(t *testing.T) { cty.MapVal(map[string]cty.Value{ "key1": cty.UnknownVal(cty.List(cty.String)), }), - cty.UnknownVal(cty.Map(cty.List(cty.String))), + cty.UnknownVal(cty.Map(cty.List(cty.String))).RefineNotNull(), false, }, { // bad map - empty value diff --git a/internal/lang/funcs/crypto.go b/internal/lang/funcs/crypto.go index e5f33bfd6e63..0b7bef984f31 100644 --- a/internal/lang/funcs/crypto.go +++ b/internal/lang/funcs/crypto.go @@ -27,8 +27,9 @@ import ( ) var UUIDFunc = function.New(&function.Spec{ - Params: []function.Parameter{}, - Type: function.StaticReturnType(cty.String), + Params: []function.Parameter{}, + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { result, err := uuid.GenerateUUID() if err != nil { @@ -49,7 +50,8 @@ var UUIDV5Func = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var namespace uuidv5.UUID switch { @@ -103,7 +105,8 @@ var BcryptFunc = function.New(&function.Spec{ Name: "cost", Type: cty.Number, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { defaultCost := 10 @@ -150,7 +153,8 @@ var RsaDecryptFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { s := args[0].AsString() key := args[1].AsString() @@ -225,7 +229,8 @@ func makeStringHashFunction(hf func() hash.Hash, enc func([]byte) string) functi Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { s := args[0].AsString() h := hf() @@ -244,7 +249,8 @@ func makeFileHashFunction(baseDir string, hf func() hash.Hash, enc func([]byte) Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { path := args[0].AsString() f, err := openFile(baseDir, path) diff --git a/internal/lang/funcs/datetime.go b/internal/lang/funcs/datetime.go index ecf24ad1a907..29896b4e81be 100644 --- a/internal/lang/funcs/datetime.go +++ b/internal/lang/funcs/datetime.go @@ -13,8 +13,9 @@ import ( // TimestampFunc constructs a function that returns a string representation of the current date and time. var TimestampFunc = function.New(&function.Spec{ - Params: []function.Parameter{}, - Type: function.StaticReturnType(cty.String), + Params: []function.Parameter{}, + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { return cty.StringVal(time.Now().UTC().Format(time.RFC3339)), nil }, @@ -44,7 +45,8 @@ var TimeAddFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { ts, err := parseTimestamp(args[0].AsString()) if err != nil { @@ -71,7 +73,8 @@ var TimeCmpFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { tsA, err := parseTimestamp(args[0].AsString()) if err != nil { diff --git a/internal/lang/funcs/datetime_test.go b/internal/lang/funcs/datetime_test.go index e0387e80188f..c792aa5fc943 100644 --- a/internal/lang/funcs/datetime_test.go +++ b/internal/lang/funcs/datetime_test.go @@ -56,13 +56,13 @@ func TestTimeadd(t *testing.T) { { // Invalid format timestamp cty.StringVal("2017-11-22"), cty.StringVal("-1h"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), true, }, { // Invalid format duration (day is not supported by ParseDuration) cty.StringVal("2017-11-22T00:00:00Z"), cty.StringVal("1d"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), true, }, } @@ -132,31 +132,31 @@ func TestTimeCmp(t *testing.T) { { cty.StringVal("2017-11-22T00:00:00Z"), cty.StringVal("bloop"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `not a valid RFC3339 timestamp: cannot use "bloop" as year`, }, { cty.StringVal("2017-11-22 00:00:00Z"), cty.StringVal("2017-11-22T00:00:00Z"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `not a valid RFC3339 timestamp: missing required time introducer 'T'`, }, { cty.StringVal("2017-11-22T00:00:00Z"), cty.UnknownVal(cty.String), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), ``, }, { cty.UnknownVal(cty.String), cty.StringVal("2017-11-22T00:00:00Z"), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), ``, }, { cty.UnknownVal(cty.String), cty.UnknownVal(cty.String), - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), ``, }, } diff --git a/internal/lang/funcs/encoding.go b/internal/lang/funcs/encoding.go index c00d416a50ab..8001fe97dbbf 100644 --- a/internal/lang/funcs/encoding.go +++ b/internal/lang/funcs/encoding.go @@ -26,7 +26,8 @@ var Base64DecodeFunc = function.New(&function.Spec{ AllowMarked: true, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { str, strMarks := args[0].Unmark() s := str.AsString() @@ -50,7 +51,8 @@ var Base64EncodeFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { return cty.StringVal(base64.StdEncoding.EncodeToString([]byte(args[0].AsString()))), nil }, @@ -68,7 +70,8 @@ var TextEncodeBase64Func = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { encoding, err := ianaindex.IANA.Encoding(args[1].AsString()) if err != nil || encoding == nil { @@ -111,7 +114,8 @@ var TextDecodeBase64Func = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { encoding, err := ianaindex.IANA.Encoding(args[1].AsString()) if err != nil || encoding == nil { @@ -154,7 +158,8 @@ var Base64GzipFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { s := args[0].AsString() @@ -181,7 +186,8 @@ var URLEncodeFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { return cty.StringVal(url.QueryEscape(args[0].AsString())), nil }, diff --git a/internal/lang/funcs/encoding_test.go b/internal/lang/funcs/encoding_test.go index 4c556f8c9477..c1ba3fa6507d 100644 --- a/internal/lang/funcs/encoding_test.go +++ b/internal/lang/funcs/encoding_test.go @@ -235,25 +235,25 @@ func TestBase64TextEncode(t *testing.T) { { cty.StringVal("abc123!?$*&()'-=@~"), cty.StringVal("NOT-EXISTS"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `"NOT-EXISTS" is not a supported IANA encoding name or alias in this Terraform version`, }, { cty.StringVal("🤔"), cty.StringVal("cp437"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `the given string contains characters that cannot be represented in IBM437`, }, { cty.UnknownVal(cty.String), cty.StringVal("windows-1250"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), ``, }, { cty.StringVal("hello world"), cty.UnknownVal(cty.String), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), ``, }, } @@ -309,13 +309,13 @@ func TestBase64TextDecode(t *testing.T) { { cty.StringVal("doesn't matter"), cty.StringVal("NOT-EXISTS"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `"NOT-EXISTS" is not a supported IANA encoding name or alias in this Terraform version`, }, { cty.StringVal(""), cty.StringVal("cp437"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), `the given value is has an invalid base64 symbol at offset 0`, }, { @@ -327,13 +327,13 @@ func TestBase64TextDecode(t *testing.T) { { cty.UnknownVal(cty.String), cty.StringVal("windows-1250"), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), ``, }, { cty.StringVal("YQBiAGMAMQAyADMAIQA/ACQAKgAmACgAKQAnAC0APQBAAH4A"), cty.UnknownVal(cty.String), - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), ``, }, } diff --git a/internal/lang/funcs/filesystem.go b/internal/lang/funcs/filesystem.go index 2399fc71167e..8363980fcd7b 100644 --- a/internal/lang/funcs/filesystem.go +++ b/internal/lang/funcs/filesystem.go @@ -31,7 +31,8 @@ func MakeFileFunc(baseDir string, encBase64 bool) function.Function { AllowMarked: true, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { pathArg, pathMarks := args[0].Unmark() path := pathArg.AsString() @@ -201,7 +202,8 @@ func MakeFileExistsFunc(baseDir string) function.Function { AllowMarked: true, }, }, - Type: function.StaticReturnType(cty.Bool), + Type: function.StaticReturnType(cty.Bool), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { pathArg, pathMarks := args[0].Unmark() path := pathArg.AsString() @@ -273,7 +275,8 @@ func MakeFileSetFunc(baseDir string) function.Function { AllowMarked: true, }, }, - Type: function.StaticReturnType(cty.Set(cty.String)), + Type: function.StaticReturnType(cty.Set(cty.String)), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { pathArg, pathMarks := args[0].Unmark() path := pathArg.AsString() @@ -340,7 +343,8 @@ var BasenameFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { return cty.StringVal(filepath.Base(args[0].AsString())), nil }, @@ -355,7 +359,8 @@ var DirnameFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { return cty.StringVal(filepath.Dir(args[0].AsString())), nil }, @@ -369,7 +374,8 @@ var AbsPathFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { absPath, err := filepath.Abs(args[0].AsString()) return cty.StringVal(filepath.ToSlash(absPath)), err @@ -384,7 +390,8 @@ var PathExpandFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { homePath, err := homedir.Expand(args[0].AsString()) diff --git a/internal/lang/funcs/number.go b/internal/lang/funcs/number.go index ed36424baa63..8a8f1410ff74 100644 --- a/internal/lang/funcs/number.go +++ b/internal/lang/funcs/number.go @@ -24,7 +24,8 @@ var LogFunc = function.New(&function.Spec{ Type: cty.Number, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var num float64 if err := gocty.FromCtyValue(args[0], &num); err != nil { @@ -52,7 +53,8 @@ var PowFunc = function.New(&function.Spec{ Type: cty.Number, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var num float64 if err := gocty.FromCtyValue(args[0], &num); err != nil { @@ -77,7 +79,8 @@ var SignumFunc = function.New(&function.Spec{ Type: cty.Number, }, }, - Type: function.StaticReturnType(cty.Number), + Type: function.StaticReturnType(cty.Number), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { var num int if err := gocty.FromCtyValue(args[0], &num); err != nil { @@ -115,6 +118,7 @@ var ParseIntFunc = function.New(&function.Spec{ } return cty.Number, nil }, + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { var numstr string diff --git a/internal/lang/funcs/refinements.go b/internal/lang/funcs/refinements.go new file mode 100644 index 000000000000..1ae17e007023 --- /dev/null +++ b/internal/lang/funcs/refinements.go @@ -0,0 +1,9 @@ +package funcs + +import ( + "github.com/zclconf/go-cty/cty" +) + +func refineNotNull(b *cty.RefinementBuilder) *cty.RefinementBuilder { + return b.NotNull() +} diff --git a/internal/lang/funcs/string.go b/internal/lang/funcs/string.go index ea7ada1be09a..56459b2f9725 100644 --- a/internal/lang/funcs/string.go +++ b/internal/lang/funcs/string.go @@ -24,7 +24,8 @@ var StartsWithFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.Bool), + Type: function.StaticReturnType(cty.Bool), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { str := args[0].AsString() prefix := args[1].AsString() @@ -50,7 +51,8 @@ var EndsWithFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.Bool), + Type: function.StaticReturnType(cty.Bool), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { str := args[0].AsString() suffix := args[1].AsString() @@ -80,7 +82,8 @@ var ReplaceFunc = function.New(&function.Spec{ Type: cty.String, }, }, - Type: function.StaticReturnType(cty.String), + Type: function.StaticReturnType(cty.String), + RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { str := args[0].AsString() substr := args[1].AsString() From 696cd68913058baec9ce5e8d26eda3c488d8b1b7 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 3 Feb 2023 17:42:59 -0800 Subject: [PATCH 3/8] command/views: Describe unknown collection bounds in diagnostics --- internal/command/views/json/diagnostic.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/command/views/json/diagnostic.go b/internal/command/views/json/diagnostic.go index ea7a409d66fb..9e2e7a546479 100644 --- a/internal/command/views/json/diagnostic.go +++ b/internal/command/views/json/diagnostic.go @@ -318,7 +318,27 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost // unknown value even when it isn't. if ty := val.Type(); ty != cty.DynamicPseudoType { if includeUnknown { - value.Statement = fmt.Sprintf("is a %s, known only after apply", ty.FriendlyName()) + switch { + case ty.IsCollectionType(): + valRng := val.Range() + minLen := valRng.LengthLowerBound() + maxLen := valRng.LengthUpperBound() + const maxLimit = 1024 // (upper limit is just an arbitrary value to avoid showing distracting large numbers in the UI) + switch { + case minLen == maxLen: + value.Statement = fmt.Sprintf("is a %s of length %d, known only after apply", ty.FriendlyName(), minLen) + case minLen != 0 && maxLen <= maxLimit: + value.Statement = fmt.Sprintf("is a %s with between %d and %d elements, known only after apply", ty.FriendlyName(), minLen, maxLen) + case minLen != 0: + value.Statement = fmt.Sprintf("is a %s with at least %d elements, known only after apply", ty.FriendlyName(), minLen) + case maxLen <= maxLimit: + value.Statement = fmt.Sprintf("is a %s with up to %d elements, known only after apply", ty.FriendlyName(), maxLen) + default: + value.Statement = fmt.Sprintf("is a %s, known only after apply", ty.FriendlyName()) + } + default: + value.Statement = fmt.Sprintf("is a %s, known only after apply", ty.FriendlyName()) + } } else { value.Statement = fmt.Sprintf("is a %s", ty.FriendlyName()) } From 691018dd00a8b08255126ed823239ff8778aaa20 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 7 Feb 2023 16:36:13 -0800 Subject: [PATCH 4/8] builtin/providers/terraform: terraform_data "id" is guaranteed non-null The "id" attribute of this resource type is generated by the provider itself and can never be null, so we'll refine the range of its unknown result in case that helps downstream expressions to produce known results even when the exact value hasn't yet been planned. --- internal/builtin/providers/terraform/resource_data.go | 4 ++-- .../builtin/providers/terraform/resource_data_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/builtin/providers/terraform/resource_data.go b/internal/builtin/providers/terraform/resource_data.go index 6fdba7ea5cbe..a17c60fc0b49 100644 --- a/internal/builtin/providers/terraform/resource_data.go +++ b/internal/builtin/providers/terraform/resource_data.go @@ -75,7 +75,7 @@ func planDataStoreResourceChange(req providers.PlanResourceChangeRequest) (resp case req.PriorState.IsNull(): // Create // Set the id value to unknown. - planned["id"] = cty.UnknownVal(cty.String) + planned["id"] = cty.UnknownVal(cty.String).RefineNotNull() // Output type must always match the input, even when it's null. if input.IsNull() { @@ -90,7 +90,7 @@ func planDataStoreResourceChange(req providers.PlanResourceChangeRequest) (resp case !req.PriorState.GetAttr("triggers_replace").RawEquals(trigger): // trigger changed, so we need to replace the entire instance resp.RequiresReplace = append(resp.RequiresReplace, cty.GetAttrPath("triggers_replace")) - planned["id"] = cty.UnknownVal(cty.String) + planned["id"] = cty.UnknownVal(cty.String).RefineNotNull() // We need to check the input for the replacement instance to compute a // new output. diff --git a/internal/builtin/providers/terraform/resource_data_test.go b/internal/builtin/providers/terraform/resource_data_test.go index 28a038333fce..3adb4484d12e 100644 --- a/internal/builtin/providers/terraform/resource_data_test.go +++ b/internal/builtin/providers/terraform/resource_data_test.go @@ -124,7 +124,7 @@ func TestManagedDataPlan(t *testing.T) { "input": cty.NullVal(cty.DynamicPseudoType), "output": cty.NullVal(cty.DynamicPseudoType), "triggers_replace": cty.NullVal(cty.DynamicPseudoType), - "id": cty.UnknownVal(cty.String), + "id": cty.UnknownVal(cty.String).RefineNotNull(), }), }, @@ -140,7 +140,7 @@ func TestManagedDataPlan(t *testing.T) { "input": cty.NullVal(cty.String), "output": cty.NullVal(cty.String), "triggers_replace": cty.NullVal(cty.DynamicPseudoType), - "id": cty.UnknownVal(cty.String), + "id": cty.UnknownVal(cty.String).RefineNotNull(), }), }, @@ -156,7 +156,7 @@ func TestManagedDataPlan(t *testing.T) { "input": cty.StringVal("input"), "output": cty.UnknownVal(cty.String), "triggers_replace": cty.NullVal(cty.DynamicPseudoType), - "id": cty.UnknownVal(cty.String), + "id": cty.UnknownVal(cty.String).RefineNotNull(), }), }, @@ -198,7 +198,7 @@ func TestManagedDataPlan(t *testing.T) { "input": cty.StringVal("input"), "output": cty.UnknownVal(cty.String), "triggers_replace": cty.StringVal("new-value"), - "id": cty.UnknownVal(cty.String), + "id": cty.UnknownVal(cty.String).RefineNotNull(), }), }, @@ -225,7 +225,7 @@ func TestManagedDataPlan(t *testing.T) { "triggers_replace": cty.MapVal(map[string]cty.Value{ "key": cty.StringVal("new value"), }), - "id": cty.UnknownVal(cty.String), + "id": cty.UnknownVal(cty.String).RefineNotNull(), }), }, } { From 81c15f987eaa42b492dfb5fd36588cc7b9b28653 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 8 Feb 2023 14:26:36 -0800 Subject: [PATCH 5/8] lang/funcs: startswith considers string prefix refinement If the string to be tested is an unknown value that's been refined with a prefix and the prefix we're being asked to test is in turn a prefix of that known prefix then we can return a known answer despite the inputs not being fully known. There are also some other similar deductions we can make about other combinations of inputs. This extra analysis could be useful in a custom condition check that requires a string with a particular prefix, since it can allow the condition to fail even on partially-unknown input, thereby giving earlier feedback about a problem. --- internal/lang/funcs/string.go | 45 ++++++++--- internal/lang/funcs/string_test.go | 120 ++++++++++++++++++++++++++++- 2 files changed, 154 insertions(+), 11 deletions(-) diff --git a/internal/lang/funcs/string.go b/internal/lang/funcs/string.go index 56459b2f9725..454c118a4a70 100644 --- a/internal/lang/funcs/string.go +++ b/internal/lang/funcs/string.go @@ -16,8 +16,9 @@ import ( var StartsWithFunc = function.New(&function.Spec{ Params: []function.Parameter{ { - Name: "str", - Type: cty.String, + Name: "str", + Type: cty.String, + AllowUnknown: true, }, { Name: "prefix", @@ -27,9 +28,31 @@ var StartsWithFunc = function.New(&function.Spec{ Type: function.StaticReturnType(cty.Bool), RefineResult: refineNotNull, Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { - str := args[0].AsString() prefix := args[1].AsString() + if !args[0].IsKnown() { + // If the unknown value has a known prefix then we might be + // able to still produce a known result. + if prefix == "" { + // The empty string is a prefix of any string. + return cty.True, nil + } + if knownPrefix := args[0].Range().StringPrefix(); knownPrefix != "" { + if strings.HasPrefix(knownPrefix, prefix) { + return cty.True, nil + } + if len(knownPrefix) >= len(prefix) { + // If the prefix we're testing is no longer than the known + // prefix and it didn't match then the full string with + // that same prefix can't match either. + return cty.False, nil + } + } + return cty.UnknownVal(cty.Bool), nil + } + + str := args[0].AsString() + if strings.HasPrefix(str, prefix) { return cty.True, nil } @@ -104,12 +127,6 @@ var ReplaceFunc = function.New(&function.Spec{ }, }) -// Replace searches a given string for another given substring, -// and replaces all occurences with a given replacement string. -func Replace(str, substr, replace cty.Value) (cty.Value, error) { - return ReplaceFunc.Call([]cty.Value{str, substr, replace}) -} - // StrContainsFunc searches a given string for another given substring, // if found the function returns true, otherwise returns false. var StrContainsFunc = function.New(&function.Spec{ @@ -135,3 +152,13 @@ var StrContainsFunc = function.New(&function.Spec{ return cty.False, nil }, }) + +// Replace searches a given string for another given substring, +// and replaces all occurences with a given replacement string. +func Replace(str, substr, replace cty.Value) (cty.Value, error) { + return ReplaceFunc.Call([]cty.Value{str, substr, replace}) +} + +func StrContains(str, substr cty.Value) (cty.Value, error) { + return StrContainsFunc.Call([]cty.Value{str, substr}) +} diff --git a/internal/lang/funcs/string_test.go b/internal/lang/funcs/string_test.go index d5c5996d98bf..c89d17a67cab 100644 --- a/internal/lang/funcs/string_test.go +++ b/internal/lang/funcs/string_test.go @@ -134,6 +134,122 @@ func TestStrContains(t *testing.T) { } } -func StrContains(str, substr cty.Value) (cty.Value, error) { - return StrContainsFunc.Call([]cty.Value{str, substr}) +func TestStartsWith(t *testing.T) { + tests := []struct { + String, Prefix cty.Value + Want cty.Value + WantError string + }{ + { + cty.StringVal("hello world"), + cty.StringVal("hello"), + cty.True, + ``, + }, + { + cty.StringVal("hey world"), + cty.StringVal("hello"), + cty.False, + ``, + }, + { + cty.StringVal(""), + cty.StringVal(""), + cty.True, + ``, + }, + { + cty.StringVal("a"), + cty.StringVal(""), + cty.True, + ``, + }, + { + cty.StringVal(""), + cty.StringVal("a"), + cty.False, + ``, + }, + { + cty.UnknownVal(cty.String), + cty.StringVal("a"), + cty.UnknownVal(cty.Bool).RefineNotNull(), + ``, + }, + { + cty.UnknownVal(cty.String), + cty.StringVal(""), + cty.True, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal(""), + cty.True, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal("a"), + cty.False, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal("ht"), + cty.True, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal("https:"), + cty.True, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal("https-"), + cty.False, + ``, + }, + { + cty.UnknownVal(cty.String).Refine().StringPrefix("https:").NewValue(), + cty.StringVal("https://"), + cty.UnknownVal(cty.Bool).RefineNotNull(), + ``, + }, + { + // Unicode combining characters edge-case: we match the prefix + // in terms of unicode code units rather than grapheme clusters, + // which is inconsistent with our string processing elsewhere but + // would be a breaking change to fix that bug now. + cty.StringVal("\U0001f937\u200d\u2642"), // "Man Shrugging" is encoded as "Person Shrugging" followed by zero-width joiner and then the masculine gender presentation modifier + cty.StringVal("\U0001f937"), // Just the "Person Shrugging" character without any modifiers + cty.True, + ``, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("StartsWith(%#v, %#v)", test.String, test.Prefix), func(t *testing.T) { + got, err := StartsWithFunc.Call([]cty.Value{test.String, test.Prefix}) + + if test.WantError != "" { + gotErr := fmt.Sprintf("%s", err) + if gotErr != test.WantError { + t.Errorf("wrong error\ngot: %s\nwant: %s", gotErr, test.WantError) + } + return + } else if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if !got.RawEquals(test.Want) { + t.Errorf( + "wrong result\nstring: %#v\nprefix: %#v\ngot: %#v\nwant: %#v", + test.String, test.Prefix, got, test.Want, + ) + } + }) + } } From dfe5e1ddc4d5970e79fea8d45c07b50c4f23715b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 8 Feb 2023 17:08:25 -0800 Subject: [PATCH 6/8] plans/objchange: Providers must honor their unknown value refinements If the original value was unknown but its range was refined then the provider must return a value that is within the refined range, because otherwise downstream planning decisions could be invalidated. This relies on cty's definition of whether a value is in a refined range, which has pretty good coverage for the "false" case and so should give a pretty good signal, but it'll probably improve over time and so providers must not rely on any loopholes in the current implementation and must keep their promises even if Terraform can't currently check them. --- internal/plans/objchange/compatible.go | 9 +++- internal/plans/objchange/compatible_test.go | 59 +++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/internal/plans/objchange/compatible.go b/internal/plans/objchange/compatible.go index c6f0c2bc50fc..65609ef15457 100644 --- a/internal/plans/objchange/compatible.go +++ b/internal/plans/objchange/compatible.go @@ -34,7 +34,7 @@ func assertObjectCompatible(schema *configschema.Block, planned, actual cty.Valu var errs []error var atRoot string if len(path) == 0 { - atRoot = "Root resource " + atRoot = "Root object " } if planned.IsNull() && !actual.IsNull() { @@ -216,7 +216,12 @@ func assertValueCompatible(planned, actual cty.Value, path cty.Path) []error { if !planned.IsKnown() { // We didn't know what were going to end up with during plan, so - // anything goes during apply. + // the final value needs only to match the type and refinements of + // the unknown value placeholder. + plannedRng := planned.Range() + if ok := plannedRng.Includes(actual); ok.IsKnown() && ok.False() { + errs = append(errs, path.NewErrorf("final value %#v does not conform to planning placeholder %#v", actual, planned)) + } return errs } diff --git a/internal/plans/objchange/compatible_test.go b/internal/plans/objchange/compatible_test.go index 60c9876702ee..eab1e35844e5 100644 --- a/internal/plans/objchange/compatible_test.go +++ b/internal/plans/objchange/compatible_test.go @@ -118,6 +118,65 @@ func TestAssertObjectCompatible(t *testing.T) { `.name: was cty.StringVal("wotsit"), but now cty.StringVal("thingy")`, }, }, + { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "name": cty.UnknownVal(cty.String), + }), + cty.ObjectVal(map[string]cty.Value{ + "name": cty.Zero, + }), + []string{ + `.name: wrong final value type: string required`, + }, + }, + { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "name": cty.UnknownVal(cty.String).RefineNotNull(), + }), + cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + []string{ + `.name: final value cty.NullVal(cty.String) does not conform to planning placeholder cty.UnknownVal(cty.String).RefineNotNull()`, + }, + }, + { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "name": cty.UnknownVal(cty.String).Refine(). + StringPrefix("boop:"). + NewValue(), + }), + cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal("thingy"), + }), + []string{ + `.name: final value cty.StringVal("thingy") does not conform to planning placeholder cty.UnknownVal(cty.String).Refine().StringPrefixFull("boop:").NewValue()`, + }, + }, { &configschema.Block{ Attributes: map[string]*configschema.Attribute{ From 4c439b099f0ca886255f063c4cbcf0613a298e08 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 15 Mar 2023 16:55:26 -0700 Subject: [PATCH 7/8] plans/objchange: Don't consider refinements when validating plans Providers that existed prior to refinements (all of them, at the time of writing) cannot preserve refinements sent in unknown values in the configuration, and even if one day providers _are_ aware of refinements there we might add new ones that existing providers don't know how to handle. For that reason we'll absolve providers of the responsibility of preserving refinements from config into plan by fixing some cases where we were incorrectly using RawEquals to compare values; that function isn't appropriate for comparing values that might be unknown. However, to avoid a disruptive change right now this initial fix just strips off the refinements before comparing. Ideally this should be using Value.Equals and handling unknown values more explicitly, but we'll save that for a possible later improvement. This does not include a similar exception for validating whether a final value conforms to a plan because the plan value and the final value are both produced by the same provider and so providers ought to be able to be consistent with their _own_ treatment of refinements, if any. Configuration is special because Terraform itself generates that, and so it can potentially contain refinements that a particular provider has no awareness of. --- internal/plans/objchange/objchange.go | 4 +-- internal/plans/objchange/plan_valid.go | 13 ++++++-- internal/plans/objchange/plan_valid_test.go | 34 +++++++++++++++++++-- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index f806174f0cee..f21cd9b6b2bb 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -429,7 +429,7 @@ func optionalValueNotComputable(schema *configschema.Attribute, val cty.Value) b // values have been added. This function is only used to correlated // configuration with possible valid prior values within sets. func validPriorFromConfig(schema nestedSchema, prior, config cty.Value) bool { - if config.RawEquals(prior) { + if unrefinedValue(config).RawEquals(unrefinedValue(prior)) { return true } @@ -446,7 +446,7 @@ func validPriorFromConfig(schema nestedSchema, prior, config cty.Value) bool { } // we don't need to know the schema if both are equal - if configV.RawEquals(priorV) { + if unrefinedValue(configV).RawEquals(unrefinedValue(priorV)) { // we know they are equal, so no need to descend further return false, nil } diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index 6e8941fa0270..ad7c051783bb 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -270,11 +270,11 @@ func assertPlannedAttrValid(name string, attrS *configschema.Attribute, priorSta func assertPlannedValueValid(attrS *configschema.Attribute, priorV, configV, plannedV cty.Value, path cty.Path) []error { var errs []error - if plannedV.RawEquals(configV) { + if unrefinedValue(plannedV).RawEquals(unrefinedValue(configV)) { // This is the easy path: provider didn't change anything at all. return errs } - if plannedV.RawEquals(priorV) && !priorV.IsNull() && !configV.IsNull() { + if unrefinedValue(plannedV).RawEquals(unrefinedValue(priorV)) && !priorV.IsNull() && !configV.IsNull() { // Also pretty easy: there is a prior value and the provider has // returned it unchanged. This indicates that configV and plannedV // are functionally equivalent and so the provider wishes to disregard @@ -463,3 +463,12 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne return errs } + +// unrefinedValue returns the given value with any unknown value refinements +// stripped away, making it a basic unknown value with only a type constraint. +func unrefinedValue(v cty.Value) cty.Value { + if !v.IsKnown() { + return cty.UnknownVal(v.Type()) + } + return v +} diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index 316bf56133c3..655c6e22f7bf 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1796,11 +1796,39 @@ func TestAssertPlanValid(t *testing.T) { )), }), []string{ - `.set: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, - `.list: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, - `.map: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, + `.set: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, + `.list: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, + `.map: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, }, }, + + "refined unknown values can become less refined": { + // Providers often can't preserve refinements through the provider + // wire protocol: although we do have a defined serialization for + // it, most providers were written before there was any such + // thing as refinements, and in future there might be new + // refinements that even refinement-aware providers don't know + // how to preserve, so we allow them to get dropped here as + // a concession to backward-compatibility. + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "a": { + Type: cty.String, + Required: true, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "a": cty.StringVal("old"), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String).RefineNotNull(), + }), + cty.ObjectVal(map[string]cty.Value{ + "a": cty.UnknownVal(cty.String), + }), + nil, + }, } for name, test := range tests { From e0ef2748e6b33884fe02e2447ff9526f5a675cce Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 8 Feb 2023 14:47:57 -0800 Subject: [PATCH 8/8] docs: Describe the plugin protocol encoding of refined unknown values This is actually a description of the "cty" library's encoding of refined values, but from the perspective of the plugin protocol it's an implementation detail that Terraform Core outsources that to a third-party library, and current server-side implementations of the protocol use an independent implementation of this format which will need to be compatible with what cty does. --- docs/plugin-protocol/object-wire-format.md | 61 +++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/docs/plugin-protocol/object-wire-format.md b/docs/plugin-protocol/object-wire-format.md index 5e1809c819fe..9552f35023ff 100644 --- a/docs/plugin-protocol/object-wire-format.md +++ b/docs/plugin-protocol/object-wire-format.md @@ -76,8 +76,7 @@ in the table below, regardless of type: * An unknown value (that is, a placeholder for a value that will be decided only during the apply operation) is represented as a [MessagePack extension](https://github.com/msgpack/msgpack/blob/master/spec.md#extension-types) - value whose type identifier is zero and whose value is unspecified and - meaningless. + value, described in more detail below. | `type` Pattern | MessagePack Representation | |---|---| @@ -91,6 +90,64 @@ in the table below, regardless of type: | `["tuple",TYPES]` | A MessagePack array with one element per element described by the `TYPES` array. The element values are constructed by applying these same mapping rules to the corresponding element of `TYPES`. | | `"dynamic"` | A MessagePack array with exactly two elements. The first element is a MessagePack binary value containing a JSON-serialized type constraint in the same format described in this table. The second element is the result of applying these same mapping rules to the value with the type given in the first element. This special type constraint represents values whose types will be decided only at runtime. | +Unknown values have two possible representations, both using +[MessagePack extension](https://github.com/msgpack/msgpack/blob/master/spec.md#extension-types) +values. + +The older encoding is for unrefined unknown values and uses an extension +code of zero, with the extension value payload completely ignored. + +Newer Terraform versions can produce "refined" unknown values which carry some +additional information that constrains the possible range of the final value/ +Refined unknown values have extension code 12 and then the extension object's +payload is a MessagePack-encoded map using integer keys to represent different +kinds of refinement: + +* `1` represents "nullness", and the value of that key will be a boolean + value that is true if the value is definitely null or false if it is + definitely not null. If this key isn't present at all then the value may or + may not be null. It's not actually useful to encode that an unknown value + is null; use a known null value instead in that case, because there is only + one null value of each type. +* `2` represents string prefix, and the value is a string that the final + value is known to begin with. This is valid only for unknown values of string + type. +* `3` and `4` represent the lower and upper bounds respectively of a number + value, and the value of both is a two-element msgpack array whose + first element is a valid encoding of a number (as in the table above) + and whose second element is a boolean value that is true for an inclusive + bound and false for an exclusive bound. This is valid only for unknown values + of number type. +* `5` and `6` represent the lower and upper bounds respectively of the length + of a collection value. The value of both is an integer representing an + inclusive bound. This is valid only for unknown values of the three kinds of + collection types: list, set, and map. + +Unknown value refinements are an optional way to reduce the range of possible +values for situations where that makes it possible to produce a known result +for unknown inputs or where it allows detecting an error during the planning +phase that would otherwise be detected only during the apply phase. It's always +safe to ignore refinements and just treat an unknown value as wholly unknown, +but considering refinements may allow a more precise answer. A provider that +produces refined values in its planned new state (from `PlanResourceChange`) +_must_ honor those refinements in the final state (from `ApplyResourceChange`). + +Unmarshalling code should ignore refinement map keys that they don't know about, +because future versions of the protocol might define additional refinements. + +When encoding an unknown value without any refinements, always use the older +format with extension code zero instead of using extension code 12 with an +empty refinement map. Any refined unknown value _must_ have at least one +refinement map entry. This rule ensures backward compatibility with older +implementations that predate the value refinements concept. + +A server implementation of the protocol should treat _any_ MessagePack extension +code as representing an unknown value, but should ignore the payload of that +extension value entirely unless the extension code is 12 to indicate that +the body represents refinements. Future versions of this protocol may define +specific formats for other extension codes, but they will always represent +unknown values. + ### `Schema.NestedBlock` Mapping Rules for MessagePack The MessagePack serialization of a collection of blocks of a particular type