From 8be809f3cb4ed3c396266db53090dd8f3d08b092 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 25 Feb 2020 12:25:37 -0500 Subject: [PATCH 1/3] convert: Fix converting to nested dynamic map When converting an object tree to a nested dynamic map (for example, `cty.Map(cty.Map(cty.DynamicPseudoType))` or deeper), it is necessary to unify the types of the child elements of the map. Previously, we would only apply this process to the innermost leaf object's attributes, which could result in local unification but globally incompatible types. This caused a panic when converting the single top-level object into a map with concrete type, as the types of the sub-trees were incompatible. This commit addresses this class of bugs in two ways: adding type unification for multi-level map and object structures, and fallback error handling to prevent panics for this class of failure. The additional type unification process has several steps: - First, we add a second unify & convert step at the end of the process, immediately before constructing the final map. This second unification ensures that not only sibling elements but also cousins are of the same type. - We also ensure that generated type information is preserved for empty collections, by returning an empty map of the detected type, instead of the dynamic type. This prevents a multi-step unification bouncing back and forth between concrete and dynamic element types. - Finally, we add a specific unification procedure for map-to-map conversions. This inspects and unifies the element types for the maps which are being converted. For example, a populated map(string) and an empty map(any) will be unified to a map(string) type. Tests are extended to cover reduced configurations from the three source Terraform bugs which prompted this work. --- cty/convert/conversion_collection.go | 132 ++++++++++++++++++++++----- cty/convert/public_test.go | 68 ++++++++++++++ cty/convert/unify.go | 43 +++++++++ 3 files changed, 221 insertions(+), 22 deletions(-) diff --git a/cty/convert/conversion_collection.go b/cty/convert/conversion_collection.go index 1b0678bf..ea23bf61 100644 --- a/cty/convert/conversion_collection.go +++ b/cty/convert/conversion_collection.go @@ -15,18 +15,18 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, val.LengthInt()) i := int64(0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -37,6 +37,9 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.ListValEmpty(ety), nil } @@ -55,18 +58,18 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, val.LengthInt()) i := int64(0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -77,6 +80,11 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + // Prefer a concrete type over a dynamic type when returning an + // empty set + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.SetValEmpty(ety), nil } @@ -93,13 +101,13 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make(map[string]cty.Value, 0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { key, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: key, } @@ -107,11 +115,11 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { if err != nil { // Should never happen, because keys can only be numbers or // strings and both can convert to string. - return cty.DynamicVal, path.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName()) + return cty.DynamicVal, elemPath.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName()) } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -121,9 +129,25 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + // Prefer a concrete type over a dynamic type when returning an + // empty map + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.MapValEmpty(ety), nil } + if ety.IsCollectionType() || ety.IsObjectType() { + var err error + if elems, err = conversionUnifyCollectionElements(elems, path, false); err != nil { + return cty.NilVal, err + } + } + + if err := conversionCheckMapElementTypes(elems, path); err != nil { + return cty.NilVal, err + } + return cty.MapVal(elems), nil } } @@ -171,20 +195,20 @@ func conversionTupleToSet(tupleType cty.Type, listEty cty.Type, unsafe bool) con // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) i := int64(0) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } conv := elemConvs[i] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -241,20 +265,20 @@ func conversionTupleToList(tupleType cty.Type, listEty cty.Type, unsafe bool) co // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) i := int64(0) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } conv := elemConvs[i] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -315,19 +339,19 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make(map[string]cty.Value, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { name, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: name, } conv := elemConvs[name.AsString()] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -335,6 +359,17 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co elems[name.AsString()] = val } + if mapEty.IsCollectionType() || mapEty.IsObjectType() { + var err error + if elems, err = conversionUnifyCollectionElements(elems, path, unsafe); err != nil { + return cty.NilVal, err + } + } + + if err := conversionCheckMapElementTypes(elems, path); err != nil { + return cty.NilVal, err + } + return cty.MapVal(elems), nil } } @@ -368,7 +403,7 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make(map[string]cty.Value, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { name, val := it.Element() @@ -380,13 +415,13 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: name, } conv := elemConvs[name.AsString()] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -398,3 +433,56 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv return cty.ObjectVal(elems), nil } } + +func conversionUnifyCollectionElements(elems map[string]cty.Value, path cty.Path, unsafe bool) (map[string]cty.Value, error) { + elemTypes := make([]cty.Type, 0, len(elems)) + for _, elem := range elems { + elemTypes = append(elemTypes, elem.Type()) + } + unifiedType, _ := unify(elemTypes, unsafe) + if unifiedType == cty.NilType { + } + + unifiedElems := make(map[string]cty.Value) + elemPath := append(path.Copy(), nil) + + for name, elem := range elems { + if elem.Type().Equals(unifiedType) { + unifiedElems[name] = elem + continue + } + conv := getConversion(elem.Type(), unifiedType, unsafe) + if conv == nil { + } + elemPath[len(elemPath)-1] = cty.IndexStep{ + Key: cty.StringVal(name), + } + val, err := conv(elem, elemPath) + if err != nil { + return nil, err + } + unifiedElems[name] = val + } + + return unifiedElems, nil +} + +func conversionCheckMapElementTypes(elems map[string]cty.Value, path cty.Path) error { + elementType := cty.NilType + elemPath := append(path.Copy(), nil) + + for name, elem := range elems { + if elementType == cty.NilType { + elementType = elem.Type() + continue + } + if !elementType.Equals(elem.Type()) { + elemPath[len(elemPath)-1] = cty.IndexStep{ + Key: cty.StringVal(name), + } + return elemPath.NewErrorf("%s is required", elementType.FriendlyName()) + } + } + + return nil +} diff --git a/cty/convert/public_test.go b/cty/convert/public_test.go index 72ab9830..3e46d5b4 100644 --- a/cty/convert/public_test.go +++ b/cty/convert/public_test.go @@ -249,6 +249,16 @@ func TestConvert(t *testing.T) { cty.StringVal("hello"), }), }, + { + Value: cty.ListValEmpty(cty.String), + Type: cty.Set(cty.DynamicPseudoType), + Want: cty.SetValEmpty(cty.String), + }, + { + Value: cty.SetValEmpty(cty.String), + Type: cty.List(cty.DynamicPseudoType), + Want: cty.ListValEmpty(cty.String), + }, { Value: cty.ObjectVal(map[string]cty.Value{ "num": cty.NumberIntVal(5), @@ -581,6 +591,64 @@ func TestConvert(t *testing.T) { Type: cty.Object(map[string]cty.Type{"foo": cty.String}), Want: cty.ObjectVal(map[string]cty.Value{"foo": cty.StringVal("hello")}), }, + // reduction of https://github.com/hashicorp/terraform/issues/23804 + { + Value: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "x": cty.TupleVal([]cty.Value{cty.StringVal("foo")}), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "x": cty.TupleVal([]cty.Value{cty.StringVal("bar")}), + }), + "c": cty.ObjectVal(map[string]cty.Value{ + "x": cty.TupleVal([]cty.Value{cty.StringVal("foo"), cty.StringVal("bar")}), + }), + }), + Type: cty.Map(cty.Map(cty.DynamicPseudoType)), + Want: cty.MapVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "x": cty.ListVal([]cty.Value{cty.StringVal("foo")}), + }), + "b": cty.MapVal(map[string]cty.Value{ + "x": cty.ListVal([]cty.Value{cty.StringVal("bar")}), + }), + "c": cty.MapVal(map[string]cty.Value{ + "x": cty.ListVal([]cty.Value{cty.StringVal("foo"), cty.StringVal("bar")}), + }), + }), + }, + // reduction of https://github.com/hashicorp/issues/24167 + { + Value: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "x": cty.NullVal(cty.DynamicPseudoType), + }), + "b": cty.ObjectVal(map[string]cty.Value{ + "x": cty.ObjectVal(map[string]cty.Value{ + "c": cty.NumberIntVal(1), + "d": cty.NumberIntVal(2), + }), + }), + }), + Type: cty.Map(cty.Map(cty.Object(map[string]cty.Type{"x": cty.Map(cty.DynamicPseudoType)}))), + WantError: true, + }, + // reduction of https://github.com/hashicorp/terraform/issues/23431 + { + Value: cty.ObjectVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "x": cty.StringVal("foo"), + }), + "b": cty.MapValEmpty(cty.DynamicPseudoType), + }), + Type: cty.Map(cty.Map(cty.DynamicPseudoType)), + Want: cty.MapVal(map[string]cty.Value{ + "a": cty.MapVal(map[string]cty.Value{ + "x": cty.StringVal("foo"), + }), + "b": cty.MapValEmpty(cty.String), + }), + }, } for _, test := range tests { diff --git a/cty/convert/unify.go b/cty/convert/unify.go index 53ebbfe0..4b4c3540 100644 --- a/cty/convert/unify.go +++ b/cty/convert/unify.go @@ -28,11 +28,14 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) { // a subset of that type, which would be a much less useful conversion for // unification purposes. { + mapCt := 0 objectCt := 0 tupleCt := 0 dynamicCt := 0 for _, ty := range types { switch { + case ty.IsMapType(): + mapCt++ case ty.IsObjectType(): objectCt++ case ty.IsTupleType(): @@ -44,6 +47,8 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) { } } switch { + case mapCt > 0 && (mapCt+dynamicCt) == len(types): + return unifyMapTypes(types, unsafe, dynamicCt > 0) case objectCt > 0 && (objectCt+dynamicCt) == len(types): return unifyObjectTypes(types, unsafe, dynamicCt > 0) case tupleCt > 0 && (tupleCt+dynamicCt) == len(types): @@ -95,6 +100,44 @@ Preferences: return cty.NilType, nil } +func unifyMapTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) { + // If we had any dynamic types in the input here then we can't predict + // what path we'll take through here once these become known types, so + // we'll conservatively produce DynamicVal for these. + if hasDynamic { + return unifyAllAsDynamic(types) + } + + elemTypes := make([]cty.Type, 0, len(types)) + for _, ty := range types { + elemTypes = append(elemTypes, ty.ElementType()) + } + retElemType, _ := unify(elemTypes, unsafe) + if retElemType == cty.NilType { + return cty.NilType, nil + } + + retTy := cty.Map(retElemType) + + conversions := make([]Conversion, len(types)) + for i, ty := range types { + if ty.Equals(retTy) { + continue + } + if unsafe { + conversions[i] = GetConversionUnsafe(ty, retTy) + } else { + conversions[i] = GetConversion(ty, retTy) + } + if conversions[i] == nil { + // Shouldn't be reachable, since we were able to unify + return unifyObjectTypesToMap(types, unsafe) + } + } + + return retTy, conversions +} + func unifyObjectTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) { // If we had any dynamic types in the input here then we can't predict // what path we'll take through here once these become known types, so From 65f6e09615592fe04513a0e07c9507f536709b84 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 3 Mar 2020 09:49:14 -0500 Subject: [PATCH 2/3] Update cty/convert/public_test.go Co-Authored-By: Kristin Laemmert --- cty/convert/public_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cty/convert/public_test.go b/cty/convert/public_test.go index 3e46d5b4..4300aefa 100644 --- a/cty/convert/public_test.go +++ b/cty/convert/public_test.go @@ -617,7 +617,7 @@ func TestConvert(t *testing.T) { }), }), }, - // reduction of https://github.com/hashicorp/issues/24167 + // reduction of https://github.com/hashicorp/terraform/issues/24167 { Value: cty.ObjectVal(map[string]cty.Value{ "a": cty.ObjectVal(map[string]cty.Value{ From e1048fba80481074d5001fd7f696b93e47392695 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 3 Mar 2020 10:27:39 -0500 Subject: [PATCH 3/3] Fix potential panic in unreachable code --- cty/convert/unify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cty/convert/unify.go b/cty/convert/unify.go index 4b4c3540..ee171d1e 100644 --- a/cty/convert/unify.go +++ b/cty/convert/unify.go @@ -131,7 +131,7 @@ func unifyMapTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, [] } if conversions[i] == nil { // Shouldn't be reachable, since we were able to unify - return unifyObjectTypesToMap(types, unsafe) + return cty.NilType, nil } }