Normalize block values before passing to TF provider (#1971)

part of #1785 This change adds a normalisation step for collections when recovering cty values to pass to terraform. This ensures we represent them similarly to terraform. In practice this means that all block collections need to be passed to TF providers as an empty collection instead of nil. This should get rid of quite a few subtle discrepancies in the data we pass to the TF provider code. These sometimes result in panics since we pass unexpected nils. This gets rid of all known input discrepancies discovered so far through cross-testing. The full rules for what is a block are [here](https://github.com/hashicorp/terraform-plugin-sdk/blob/1f499688ebd9420768f501d4ed622a51b2135ced/helper/schema/core_schema.go#L60). It is essentially properties with schema: typeList or typeSet with a Resource Elem. fixes #1970 fixes #1915 fixes #1964 fixes #1767 fixes #1762 TODO: [DONE] remove the MaxItemsOne default hacks introduced in #1725 (opened #2049) --------- Co-authored-by: Anton Tayanovskyy <[email protected]> Co-authored-by: Ian Wahbe <[email protected]>
pulumi · Jun 5, 2024 · 98f15a1 · 98f15a1
1 parent a9d0c7c
commit 98f15a1
Show file tree

Hide file tree

Showing 5 changed files with 352 additions and 24 deletions.
diff --git a/pkg/tests/cross-tests/input_check.go b/pkg/tests/cross-tests/input_check.go
@@ -33,10 +33,6 @@ type inputTestCase struct {
 
 	Config     any
 	ObjectType *tftypes.Object
-
-	SkipCompareRawPlan   bool
-	SkipCompareRawConfig bool
-	SkipCompareRawState  bool
 }
 
 func FailNotEqual(t T, name string, tfVal, pulVal any) {
@@ -158,15 +154,7 @@ func runCreateInputCheck(t T, tc inputTestCase) {
 		assertValEqual(t, k+" Change New", tfChangeValNew, pulChangeValNew)
 	}
 
-	if !tc.SkipCompareRawConfig {
-		assertCtyValEqual(t, "RawConfig", tfResData.GetRawConfig(), pulResData.GetRawConfig())
-	}
-
-	if !tc.SkipCompareRawPlan {
-		assertCtyValEqual(t, "RawPlan", tfResData.GetRawPlan(), pulResData.GetRawPlan())
-	}
-
-	if !tc.SkipCompareRawState {
-		assertCtyValEqual(t, "RawState", tfResData.GetRawState(), pulResData.GetRawState())
-	}
+	assertCtyValEqual(t, "RawConfig", tfResData.GetRawConfig(), pulResData.GetRawConfig())
+	assertCtyValEqual(t, "RawPlan", tfResData.GetRawPlan(), pulResData.GetRawPlan())
+	assertCtyValEqual(t, "RawState", tfResData.GetRawState(), pulResData.GetRawState())
 }
diff --git a/pkg/tests/cross-tests/input_cross_test.go b/pkg/tests/cross-tests/input_cross_test.go
@@ -2,6 +2,7 @@ package crosstests
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -103,6 +104,7 @@ func TestInputsEqualObjectBasic(t *testing.T) {
 }
 
 func TestInputsConfigModeEqual(t *testing.T) {
+	// Regression test for [pulumi/pulumi-terraform-bridge#1762]
 	skipUnlessLinux(t)
 	t2 := tftypes.Object{AttributeTypes: map[string]tftypes.Type{
 		"x": tftypes.String,
@@ -183,8 +185,7 @@ func TestInputsConfigModeEqual(t *testing.T) {
 						},
 					},
 				},
-				Config:               tc.config,
-				SkipCompareRawConfig: true,
+				Config: tc.config,
 			})
 		})
 	}
@@ -215,7 +216,33 @@ func TestInputsEmptyString(t *testing.T) {
 	})
 }
 
+func TestInputsUnspecifiedMaxItemsOne(t *testing.T) {
+	// Regression test for [pulumi/pulumi-terraform-bridge#1767]
+	skipUnlessLinux(t)
+	runCreateInputCheck(t, inputTestCase{
+		Resource: &schema.Resource{
+			Schema: map[string]*schema.Schema{
+				"f0": {
+					Type:     schema.TypeList,
+					MaxItems: 1,
+					Optional: true,
+					Elem: &schema.Resource{
+						Schema: map[string]*schema.Schema{
+							"x": {Optional: true, Type: schema.TypeString},
+						},
+					},
+				},
+			},
+		},
+		Config: tftypes.NewValue(
+			tftypes.Object{},
+			map[string]tftypes.Value{},
+		),
+	})
+}
+
 func TestOptionalSetNotSpecified(t *testing.T) {
+	// Regression test for [pulumi/pulumi-terraform-bridge#1970] and [pulumi/pulumi-terraform-bridge#1964]
 	skipUnlessLinux(t)
 	runCreateInputCheck(t, inputTestCase{
 		Resource: &schema.Resource{
@@ -231,11 +258,48 @@ func TestOptionalSetNotSpecified(t *testing.T) {
 				},
 			},
 		},
-		Config:               tftypes.NewValue(tftypes.Object{}, map[string]tftypes.Value{}),
-		SkipCompareRawConfig: true,
+		Config: tftypes.NewValue(tftypes.Object{}, map[string]tftypes.Value{}),
 	})
 }
 
+func TestInputsEqualEmptyList(t *testing.T) {
+	// Regression test for [pulumi/pulumi-terraform-bridge#1915]
+	skipUnlessLinux(t)
+	for _, maxItems := range []int{0, 1} {
+		name := fmt.Sprintf("MaxItems: %v", maxItems)
+		t.Run(name, func(t *testing.T) {
+			t1 := tftypes.List{ElementType: tftypes.String}
+			t0 := tftypes.Object{
+				AttributeTypes: map[string]tftypes.Type{
+					"f0": t1,
+				},
+			}
+			runCreateInputCheck(t, inputTestCase{
+				Resource: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"f0": {
+							Optional: true,
+							Type:     schema.TypeList,
+							MaxItems: maxItems,
+							Elem: &schema.Resource{
+								Schema: map[string]*schema.Schema{
+									"x": {Optional: true, Type: schema.TypeString},
+								},
+							},
+						},
+					},
+				},
+				Config: tftypes.NewValue(
+					t0,
+					map[string]tftypes.Value{
+						"f0": tftypes.NewValue(t1, []tftypes.Value{}),
+					},
+				),
+			})
+		})
+	}
+}
+
 func TestExplicitNilList(t *testing.T) {
 	skipUnlessLinux(t)
 	t0 := tftypes.Map{ElementType: tftypes.Number}
@@ -323,8 +387,7 @@ func TestInputsEmptyCollections(t *testing.T) {
 						},
 					},
 				},
-				Config:               config,
-				SkipCompareRawConfig: true,
+				Config: config,
 			})
 		})
 	}
@@ -417,8 +480,7 @@ func TestInputsNestedBlocksEmpty(t *testing.T) {
 						},
 					},
 				},
-				Config:               tc.config,
-				SkipCompareRawConfig: true,
+				Config: tc.config,
 			})
 		})
 	}

diff --git a/pkg/tfbridge/provider_test.go b/pkg/tfbridge/provider_test.go
@@ -5525,7 +5525,7 @@ func TestPlanResourceChangeUnknowns(t *testing.T) {
 				"maxItemsOneProp":null,
 				"setBlockProps":[],
 				"listBlockProps":[],
-				"nestedListBlockProps":[{"nestedProps":null}],
+				"nestedListBlockProps":[{"nestedProps":[]}],
 				"maxItemsOneBlockProp":null
 			}
 		}

diff --git a/pkg/tfshim/sdk-v2/provider2.go b/pkg/tfshim/sdk-v2/provider2.go
@@ -53,6 +53,8 @@ func (r *v2Resource2) InstanceState(
 		original := schema.HCL2ValueFromConfigValue(object)
 		s = original
 	}
+	s = normalizeBlockCollections(s, r.tf)
+
 	return &v2InstanceState2{
 		stateValue:   s,
 		resourceType: r.resourceType,
@@ -170,6 +172,8 @@ func (p *planResourceChangeImpl) Diff(
 	if err != nil {
 		return nil, fmt.Errorf("Resource %q: %w", t, err)
 	}
+	cfg = normalizeBlockCollections(cfg, res)
+
 	prop, err := proposedNew(res, state.stateValue, cfg)
 	if err != nil {
 		return nil, err
@@ -820,3 +824,89 @@ func recoverDiagnostic(d tfprotov5.Diagnostic) diag.Diagnostic {
 	}
 	return dd
 }
+
+func normalizeBlockCollections(val cty.Value, res *schema.Resource) cty.Value {
+	// Full rules about block vs attr
+	//nolint:lll
+	// https://github.com/hashicorp/terraform-plugin-sdk/blob/1f499688ebd9420768f501d4ed622a51b2135ced/helper/schema/core_schema.go#L60
+	sch := res.CoreConfigSchema()
+	if !val.Type().IsObjectType() {
+		contract.Failf("normalizeBlockCollections: Expected object type, got %v", val.Type().GoString())
+	}
+
+	if val.IsNull() || !val.IsKnown() {
+		return val
+	}
+
+	valMap := val.AsValueMap()
+
+	for fieldName := range sch.BlockTypes {
+		if !val.Type().HasAttribute(fieldName) {
+			continue
+		}
+		subVal := val.GetAttr(fieldName)
+		if subVal.IsNull() {
+			fieldType := val.Type().AttributeType(fieldName)
+			// Only lists and sets can be blocks and pass InternalValidate
+			// Ignore other types.
+			if fieldType.IsListType() {
+				glog.V(10).Info(
+					"normalizeBlockCollections: replacing a nil list with an empty list because the underlying "+
+						"TF property is a block %s, %s",
+					fieldName, fieldType.ElementType())
+				valMap[fieldName] = cty.ListValEmpty(fieldType.ElementType())
+			} else if fieldType.IsSetType() {
+				glog.V(10).Info(
+					"normalizeBlockCollections: replacing a nil set with an empty set because the underlying "+
+						"TF property is a block %s, %s",
+					fieldName, fieldType.ElementType())
+				valMap[fieldName] = cty.SetValEmpty(fieldType.ElementType())
+			}
+		} else {
+			subBlockSchema := res.SchemaMap()[fieldName]
+			if subBlockSchema == nil {
+				glog.V(5).Info("normalizeBlockCollections: Unexpected nil subBlockSchema for %s", fieldName)
+				continue
+			}
+			subBlockRes, ok := subBlockSchema.Elem.(*schema.Resource)
+			if !ok {
+				glog.V(5).Info("normalizeBlockCollections: Unexpected schema type %s", fieldName)
+				continue
+			}
+			normalizedVal := normalizeSubBlock(subVal, subBlockRes)
+			valMap[fieldName] = normalizedVal
+		}
+	}
+	return cty.ObjectVal(valMap)
+}
+
+func normalizeSubBlock(val cty.Value, subBlockRes *schema.Resource) cty.Value {
+	if !val.IsKnown() || val.IsNull() {
+		// Blocks shouldn't be unknown, but if they are, we can't do anything with them.
+		// val should also not be nil here as that case is handled separately above.
+		return val
+	}
+
+	if val.Type().IsListType() {
+		blockValSlice := val.AsValueSlice()
+		newSlice := make([]cty.Value, len(blockValSlice))
+		for i, v := range blockValSlice {
+			newSlice[i] = normalizeBlockCollections(v, subBlockRes)
+		}
+		if len(newSlice) != 0 {
+			return cty.ListVal(newSlice)
+		}
+		return cty.ListValEmpty(val.Type().ElementType())
+	} else if val.Type().IsSetType() {
+		blockValSlice := val.AsValueSlice()
+		newSlice := make([]cty.Value, len(blockValSlice))
+		for i, v := range blockValSlice {
+			newSlice[i] = normalizeBlockCollections(v, subBlockRes)
+		}
+		if len(newSlice) != 0 {
+			return cty.SetVal(newSlice)
+		}
+		return cty.SetValEmpty(val.Type().ElementType())
+	}
+	return val
+}