From 98f15a1486c63859eb4d52d18519feaeacfcba3a Mon Sep 17 00:00:00 2001 From: VenelinMartinov Date: Wed, 5 Jun 2024 15:46:37 +0100 Subject: [PATCH] Normalize block values before passing to TF provider (#1971) part of https://github.com/pulumi/pulumi-terraform-bridge/issues/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 https://github.com/pulumi/pulumi-terraform-bridge/issues/1970 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/1915 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/1964 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/1767 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/1762 TODO: [DONE] remove the MaxItemsOne default hacks introduced in https://github.com/pulumi/pulumi-terraform-bridge/pull/1725 (opened https://github.com/pulumi/pulumi-terraform-bridge/pull/2049) --------- Co-authored-by: Anton Tayanovskyy Co-authored-by: Ian Wahbe --- pkg/tests/cross-tests/input_check.go | 18 +-- pkg/tests/cross-tests/input_cross_test.go | 78 ++++++++- pkg/tfbridge/provider_test.go | 2 +- pkg/tfshim/sdk-v2/provider2.go | 90 +++++++++++ pkg/tfshim/sdk-v2/provider2_test.go | 188 ++++++++++++++++++++++ 5 files changed, 352 insertions(+), 24 deletions(-) diff --git a/pkg/tests/cross-tests/input_check.go b/pkg/tests/cross-tests/input_check.go index a34552fad..d32835263 100644 --- 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 index 359ee88c1..5e4ddb32a 100644 --- 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 index 528397714..e17627ea9 100644 --- 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 index 379ae0e4e..7836cc088 100644 --- 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 +} diff --git a/pkg/tfshim/sdk-v2/provider2_test.go b/pkg/tfshim/sdk-v2/provider2_test.go index c006769e2..bafbf392c 100644 --- a/pkg/tfshim/sdk-v2/provider2_test.go +++ b/pkg/tfshim/sdk-v2/provider2_test.go @@ -289,3 +289,191 @@ func TestProvider2UpgradeResourceState(t *testing.T) { }) } } + +func TestNormalizeBlockCollections(t *testing.T) { + for _, tc := range []struct { + name string + res *schema.Resource + input cty.Value + expected cty.Value + }{ + { + name: "basic", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.StringVal("val"), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": {Type: schema.TypeString, Optional: true}, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.StringVal("val"), + }, + ), + }, + { + name: "list attr", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.ListVal([]cty.Value{cty.StringVal("val")}), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.ListVal([]cty.Value{cty.StringVal("val")}), + }, + ), + }, + { + name: "list block with val", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.ListVal( + []cty.Value{ + cty.ObjectVal( + map[string]cty.Value{"field": cty.StringVal("val")}, + ), + }, + ), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": {Type: schema.TypeString, Optional: true}, + }, + }, + }, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.ListVal( + []cty.Value{ + cty.ObjectVal( + map[string]cty.Value{"field": cty.StringVal("val")}, + ), + }, + ), + }, + ), + }, + { + name: "list block no val", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.NullVal( + cty.List(cty.Object(map[string]cty.Type{"field": cty.String})), + ), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": {Type: schema.TypeString, Optional: true}, + }, + }, + }, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.ListValEmpty(cty.Object(map[string]cty.Type{"field": cty.String})), + }, + ), + }, + { + name: "list block no val config mode attr", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.NullVal( + cty.List(cty.Object(map[string]cty.Type{"field": cty.String})), + ), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeList, + Optional: true, + ConfigMode: schema.SchemaConfigModeAttr, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": {Type: schema.TypeString, Optional: true}, + }, + }, + }, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"field": cty.String}))), + }, + ), + }, + { + name: "set block no val", + input: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.NullVal( + cty.Set(cty.Object(map[string]cty.Type{"field": cty.String})), + ), + }, + ), + res: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": {Type: schema.TypeString, Optional: true}, + }, + }, + }, + }, + }, + expected: cty.ObjectVal( + map[string]cty.Value{ + "prop": cty.SetValEmpty(cty.Object(map[string]cty.Type{"field": cty.String})), + }, + ), + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.res.InternalValidate(nil, false) + require.NoError(t, err) + + res := normalizeBlockCollections( + tc.input, + tc.res, + ) + if !tc.expected.Equals(res).True() { + t.Logf("Expect: %s", tc.expected.GoString()) + t.Logf("Actual: %s", res.GoString()) + t.FailNow() + } + }) + } +}