From e71c1a6dafa243907e0b67fa12f13ec8fd8ca76c Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Mon, 22 Apr 2024 18:03:24 -0700 Subject: [PATCH] Support overriding `int` with `string` (#1896) - **Refactor TestOverridingTFSchema for clarity.** This commit is a pure refactor of the tests, and does not add or remove any tests. - **Support override int with string** This conversion was previously handled by by terraform as a best-effort approach. We should handle it in-house for 2 reasons: 1. To provider errors at the pulumi level. 2. To ensure that providers receive the same inputs as from TF, instead of relying on TF to convert on the wrong type. Related to https://github.com/pulumi/pulumi-terraform-bridge/issues/1342 This is a pre-requisite for https://github.com/pulumi/pulumi-databricks/issues/55. --- pkg/tfbridge/schema.go | 44 ++++++-- pkg/tfbridge/schema_test.go | 220 ++++++++++++++++++++++++------------ 2 files changed, 180 insertions(+), 84 deletions(-) diff --git a/pkg/tfbridge/schema.go b/pkg/tfbridge/schema.go index 27a937ff0..760e4a78e 100644 --- a/pkg/tfbridge/schema.go +++ b/pkg/tfbridge/schema.go @@ -384,32 +384,42 @@ func (ctx *conversionContext) makeTerraformInput( v = wrap(v) } + wrapError := func(v any, err error) (any, error) { + if err == nil { + return v, nil + } + + return v, fmt.Errorf("%s: %w", name, err) + } + // If there is a custom transform for this value, run it before processing the value. if ps != nil && ps.Transform != nil { nv, err := ps.Transform(v) if err != nil { - return nil, err + return wrapError(nv, err) } v = nv } + if tfs == nil { + tfs = (&schema.Schema{}).Shim() + } + switch { case v.IsNull(): return nil, nil case v.IsBool(): - if tfs != nil && tfs.Type() == shim.TypeString { + switch tfs.Type() { + case shim.TypeString: if v.BoolValue() { return "true", nil } return "false", nil + default: + return v.BoolValue(), nil } - return v.BoolValue(), nil case v.IsNumber(): - var typ shim.ValueType - if tfs != nil { - typ = tfs.Type() - } - switch typ { + switch tfs.Type() { case shim.TypeFloat: return v.NumberValue(), nil case shim.TypeString: @@ -418,7 +428,12 @@ func (ctx *conversionContext) makeTerraformInput( return int(v.NumberValue()), nil } case v.IsString(): - return v.StringValue(), nil + switch tfs.Type() { + case shim.TypeInt: + return wrapError(strconv.ParseInt(v.StringValue(), 10, 64)) + default: + return v.StringValue(), nil + } case v.IsArray(): var oldArr []resource.PropertyValue if old.IsArray() { @@ -1100,6 +1115,10 @@ func MakeTerraformOutput( v = list } + if ps == nil { + ps = &SchemaInfo{} + } + // We use reflection instead of a type switch so that we can support mapping values whose underlying type is // supported into a Pulumi value, even if they stored as a wrapper type (such as a strongly-typed enum). // @@ -1111,7 +1130,12 @@ func MakeTerraformOutput( case reflect.Bool: return resource.NewBoolProperty(val.Bool()) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return resource.NewNumberProperty(float64(val.Int())) + switch ps.Type { + case "string": + return resource.NewProperty(strconv.FormatInt(val.Int(), 10)) + default: + return resource.NewNumberProperty(float64(val.Int())) + } case reflect.Float32, reflect.Float64: return resource.NewNumberProperty(val.Float()) case reflect.String: diff --git a/pkg/tfbridge/schema_test.go b/pkg/tfbridge/schema_test.go index df632fb4a..321ba7956 100644 --- a/pkg/tfbridge/schema_test.go +++ b/pkg/tfbridge/schema_test.go @@ -1241,92 +1241,164 @@ func TestInvalidAsset(t *testing.T) { } func TestOverridingTFSchema(t *testing.T) { - ctx := context.Background() + t.Parallel() - tfInputs := map[string]interface{}{ - "pulumi_override_tf_string_to_boolean": MyString("true"), - "pulumi_override_tf_string_to_bool": MyString("true"), - "pulumi_empty_tf_override": MyString("true"), - "pulumi_override_tf_string_to_int": MyString("1"), - "pulumi_override_tf_string_to_integer": MyString("1"), - "tf_empty_string_to_pulumi_bool_override": MyString(""), - } - - tfSchema := shimv1.NewSchemaMap(map[string]*schemav1.Schema{ - "pulumi_override_tf_string_to_boolean": {Type: schemav1.TypeString}, - "pulumi_override_tf_string_to_bool": {Type: schemav1.TypeString}, - "pulumi_empty_tf_override": {Type: schemav1.TypeString}, - "pulumi_override_tf_string_to_int": {Type: schemav1.TypeString}, - "pulumi_override_tf_string_to_integer": {Type: schemav1.TypeString}, - "tf_empty_string_to_pulumi_bool_override": {Type: schemav1.TypeString}, + const largeNumber int64 = 1<<62 + 1 + + // We need to assert that when both the Pulumi type (String) and the Terraform + // type (Int) are large enough to hold a large number, we never round trip it + // through a smaller type like a float64. + // + // We assert this requirement by checking that the number we use *does not* round + // trip through float64. + t.Run("number_is_large", func(t *testing.T) { + t.Parallel() + assert.NotEqual(t, largeNumber, int64(float64(largeNumber))) }) - typeOverrides := map[string]*SchemaInfo{ - "pulumi_override_tf_string_to_boolean": { - Type: "boolean", + tests := []struct { + name string + + tfSchema *schemav1.Schema + info *SchemaInfo + + tfInput any + tfOutput resource.PropertyValue + }{ + { + name: "pulumi_override_tf_string_to_boolean", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{Type: "boolean"}, + + tfInput: MyString("true"), + tfOutput: resource.NewProperty(true), }, - "pulumi_override_tf_string_to_bool": { - Type: "bool", + { + name: "pulumi_override_tf_string_to_bool", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{Type: "bool"}, + + tfInput: MyString("true"), + tfOutput: resource.NewProperty(true), }, - "pulumi_empty_tf_override": { - Type: "", + { + name: "pulumi_empty_tf_override", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{Type: ""}, + + tfInput: MyString("true"), + tfOutput: resource.NewProperty("true"), }, - "pulumi_override_tf_string_to_int": { - Type: "int", + { + name: "pulumi_override_tf_string_to_int", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{Type: "int"}, + + tfInput: MyString("1"), + tfOutput: resource.NewProperty(1.0), }, - "pulumi_override_tf_string_to_integer": { - Type: "integer", + { + name: "pulumi_override_tf_string_to_integer", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{Type: "integer"}, + + tfInput: MyString("1"), + tfOutput: resource.NewProperty(1.0), }, - "tf_empty_string_to_pulumi_bool_override": { - Type: "boolean", - MarkAsOptional: boolPointer(true), + { + name: "tf_empty_string_to_pulumi_bool_override", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeString}, + info: &SchemaInfo{ + Type: "boolean", + MarkAsOptional: True(), + }, + + tfInput: MyString(""), + tfOutput: resource.NewNullProperty(), + }, + { + name: "tf_int_to_pulumi_string", + + tfSchema: &schemav1.Schema{Type: schemav1.TypeInt}, + info: &SchemaInfo{Type: "string"}, + + tfInput: largeNumber, + tfOutput: resource.NewProperty(strconv.FormatInt(largeNumber, 10)), }, } - tfOutputs := resource.NewPropertyMapFromMap(map[string]interface{}{ - "pulumiOverrideTfStringToBoolean": true, - "pulumiOverrideTfStringToBool": true, - "pulumiEmptyTfOverride": "true", - "pulumiOverrideTfStringToInt": 1, - "pulumiOverrideTfStringToInteger": 1, - "tfEmptyStringToPulumiBoolOverride": nil, - }) + const testProp = "prop" - t.Run("MakeTerraformOutputs", func(t *testing.T) { - result := MakeTerraformOutputs( - ctx, - shimv1.NewProvider(testTFProvider), - tfInputs, - tfSchema, - typeOverrides, - nil, /* assets */ - true, - ) - assert.Equal(t, tfOutputs, result) - }) - t.Run("MakeTerraformInputs", func(t *testing.T) { - result, _, err := makeTerraformInputsForConfig( - nil, - tfOutputs, - tfSchema, - typeOverrides, - ) - require.NoError(t, err) - expected := map[string]interface{}{ - // SDKv2 Providers have __defaults included. - "__defaults": []interface{}{}, - } - for k, v := range tfInputs { - // We don't transform nil values because terraform distinguished - // between nil and "" values. - if s := string(v.(MyString)); s == "" { - expected[k] = nil - } else { - expected[k] = s - } - } - assert.Equal(t, expected, result) - }) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + t.Run("MakeTerraformOutputs", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + result := MakeTerraformOutputs( + ctx, + shimv1.NewProvider(testTFProvider), + map[string]any{ + testProp: tt.tfInput, + }, + shimv1.NewSchemaMap(map[string]*schemav1.Schema{ + testProp: tt.tfSchema, + }), + map[string]*SchemaInfo{ + testProp: tt.info, + }, + nil, /* assets */ + true, + ) + assert.Equal(t, resource.PropertyMap{ + testProp: tt.tfOutput, + }, result) + }) + t.Run("MakeTerraformInputs", func(t *testing.T) { + t.Parallel() + + result, _, err := makeTerraformInputsForConfig( + nil, + resource.PropertyMap{ + testProp: tt.tfOutput, + }, + shimv1.NewSchemaMap(map[string]*schemav1.Schema{ + testProp: tt.tfSchema, + }), + map[string]*SchemaInfo{ + testProp: tt.info, + }, + ) + require.NoError(t, err) + expected := map[string]any{ + // SDKv2 Providers have __defaults included. + "__defaults": []any{}, + } + + switch tfInput := tt.tfInput.(type) { + // We don't transform nil values because terraform distinguished + // between nil and "" values. + case MyString: + if tfInput == "" { + expected[testProp] = nil + } else { + expected[testProp] = string(tfInput) + } + default: + expected[testProp] = tfInput + } + assert.Equal(t, expected, result) + }) + }) + } } func TestArchiveAsAsset(t *testing.T) {