Skip to content

Commit

Permalink
Fix nested computed read (#2181)
Browse files Browse the repository at this point in the history 
Fixes an issue with the SDKv2 and PF input extraction. During import we
wrongly return values for properties which are fully computed which
causes invalid programs to be generated.

~The object check in `CastToTypeObject` was wrong for SDKV2 objects:
https://github.com/pulumi/pulumi-terraform-bridge/blob/48fd41bd16e2f7f933bb9390ab7619d52faabb6d/pkg/tfshim/util/types.go#L36~

The `CastToTypeObject` object check is wrong for both SDKv2 and PF. For
the SDKV2 only sets and lists can have a `Resource` `.Elem`. For PF it
list-nested blocks are represented with a `Resource` `.Elem` in the shim
layer, so the check did not catch these. PF details here:
#2231
This fixes the check and adds regression tests for the broken imports
which resulted from 2180.

Test coverage added in
#2224 for sdkv2
and #2225 for pf.
fixes #2180
depends on pulumi/providertest#91
stacked on #2225
  • Loading branch information
@VenelinMartinov
VenelinMartinov authored Jul 23, 2024
1 parent bf90025 commit da22823
Show file tree
Hide file tree
Showing 13 changed files with 146 additions and 53 deletions.
4 changes: 2 additions & 2 deletions dynamic/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -760,8 +760,8 @@ github.com/pulumi/esc v0.9.1 h1:HH5eEv8sgyxSpY5a8yePyqFXzA8cvBvapfH8457+mIs=
github.com/pulumi/esc v0.9.1/go.mod h1:oEJ6bOsjYlQUpjf70GiX+CXn3VBmpwFDxUTlmtUN84c=
github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
github.com/pulumi/providertest v0.0.13 h1:9CAaoviOTuCVHDI15h3znXa5JsKYtXLYHIIdxOCzo3Y=
github.com/pulumi/providertest v0.0.13/go.mod h1:REAoaN+hGOtdWJGirfWYqcSjCejlbGfzyVTUuemJTuE=
github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2pHgxQ8=
github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.11.0 h1:Jw9gBvyfmfOMq/EkYDm9+zGPxsDAA8jfeMpHmtZ+1oA=
github.com/pulumi/pulumi-java/pkg v0.11.0/go.mod h1:sXAk25P47AQVQL6ilAbFmRNgZykC7og/+87ihnqzFTc=
github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.8 h1:mav2tSitA9BPJPLLahKgepHyYsMzwaTm4cvp0dcTMYw=
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ require (
github.com/mitchellh/reflectwalk v1.0.2
github.com/pkg/errors v0.9.1
github.com/pulumi/inflector v0.1.1
github.com/pulumi/providertest v0.0.13
github.com/pulumi/providertest v0.0.14
github.com/pulumi/pulumi-java/pkg v0.11.0
github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.8
github.com/pulumi/pulumi-yaml v1.9.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1902,8 +1902,8 @@ github.com/pulumi/esc v0.9.1 h1:HH5eEv8sgyxSpY5a8yePyqFXzA8cvBvapfH8457+mIs=
github.com/pulumi/esc v0.9.1/go.mod h1:oEJ6bOsjYlQUpjf70GiX+CXn3VBmpwFDxUTlmtUN84c=
github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
github.com/pulumi/providertest v0.0.13 h1:9CAaoviOTuCVHDI15h3znXa5JsKYtXLYHIIdxOCzo3Y=
github.com/pulumi/providertest v0.0.13/go.mod h1:REAoaN+hGOtdWJGirfWYqcSjCejlbGfzyVTUuemJTuE=
github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2pHgxQ8=
github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.11.0 h1:Jw9gBvyfmfOMq/EkYDm9+zGPxsDAA8jfeMpHmtZ+1oA=
github.com/pulumi/pulumi-java/pkg v0.11.0/go.mod h1:sXAk25P47AQVQL6ilAbFmRNgZykC7og/+87ihnqzFTc=
github.com/pulumi/pulumi-yaml v1.9.1 h1:JPeI80M23SPactxgnCFS1casZlSr7ZhAXwSx4H55QQ4=
Expand Down
4 changes: 2 additions & 2 deletions pf/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1901,8 +1901,8 @@ github.com/pulumi/esc v0.9.1 h1:HH5eEv8sgyxSpY5a8yePyqFXzA8cvBvapfH8457+mIs=
github.com/pulumi/esc v0.9.1/go.mod h1:oEJ6bOsjYlQUpjf70GiX+CXn3VBmpwFDxUTlmtUN84c=
github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
github.com/pulumi/providertest v0.0.13 h1:9CAaoviOTuCVHDI15h3znXa5JsKYtXLYHIIdxOCzo3Y=
github.com/pulumi/providertest v0.0.13/go.mod h1:REAoaN+hGOtdWJGirfWYqcSjCejlbGfzyVTUuemJTuE=
github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2pHgxQ8=
github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.11.0 h1:Jw9gBvyfmfOMq/EkYDm9+zGPxsDAA8jfeMpHmtZ+1oA=
github.com/pulumi/pulumi-java/pkg v0.11.0/go.mod h1:sXAk25P47AQVQL6ilAbFmRNgZykC7og/+87ihnqzFTc=
github.com/pulumi/pulumi-yaml v1.9.1 h1:JPeI80M23SPactxgnCFS1casZlSr7ZhAXwSx4H55QQ4=
Expand Down
20 changes: 6 additions & 14 deletions pf/tests/extract_inputs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,6 @@ func TestExtractInputsFromOutputsPF(t *testing.T) {
}}},
}),
},
// TODO[pulumi/pulumi-terraform-bridge#2180]: This should not yield values for computed properties
{
name: "list nested block computed not extracted",
props: resource.NewPropertyMapFromMap(map[string]interface{}{
Expand All @@ -727,12 +726,9 @@ func TestExtractInputsFromOutputsPF(t *testing.T) {
V: []resource.PropertyValue{},
},
resource.PropertyKey("block_field"): resource.PropertyValue{V: []resource.PropertyValue{{
V: resource.PropertyMap{
resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
},
resource.PropertyKey("nested_field"): resource.PropertyValue{V: "nested_value"}, // wrong
},
V: resource.PropertyMap{resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
}},
}}},
}),
},
Expand Down Expand Up @@ -801,7 +797,6 @@ func TestExtractInputsFromOutputsPF(t *testing.T) {
}}},
}),
},
// TODO[pulumi/pulumi-terraform-bridge#2180]: This should not yield values for computed properties
{
name: "set nested block computed not extracted",
props: resource.NewPropertyMapFromMap(map[string]interface{}{
Expand All @@ -825,12 +820,9 @@ func TestExtractInputsFromOutputsPF(t *testing.T) {
V: []resource.PropertyValue{},
},
resource.PropertyKey("block_field"): resource.PropertyValue{V: []resource.PropertyValue{{
V: resource.PropertyMap{
resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
},
resource.PropertyKey("nested_field"): resource.PropertyValue{V: "nested_value"}, //wrong
},
V: resource.PropertyMap{resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
}},
}}},
}),
},
Expand Down
2 changes: 1 addition & 1 deletion pf/tests/go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
github.com/hashicorp/terraform-provider-tls/shim v0.0.0-00010101000000-000000000000
github.com/hexops/autogold/v2 v2.2.1
github.com/pulumi/providertest v0.0.13
github.com/pulumi/providertest v0.0.14
github.com/pulumi/pulumi-terraform-bridge/pf v0.0.0
github.com/pulumi/pulumi-terraform-bridge/v3 v3.87.0
github.com/stretchr/testify v1.9.0
Expand Down
4 changes: 2 additions & 2 deletions pf/tests/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1928,8 +1928,8 @@ github.com/pulumi/esc v0.9.1 h1:HH5eEv8sgyxSpY5a8yePyqFXzA8cvBvapfH8457+mIs=
github.com/pulumi/esc v0.9.1/go.mod h1:oEJ6bOsjYlQUpjf70GiX+CXn3VBmpwFDxUTlmtUN84c=
github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
github.com/pulumi/providertest v0.0.13 h1:9CAaoviOTuCVHDI15h3znXa5JsKYtXLYHIIdxOCzo3Y=
github.com/pulumi/providertest v0.0.13/go.mod h1:REAoaN+hGOtdWJGirfWYqcSjCejlbGfzyVTUuemJTuE=
github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2pHgxQ8=
github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.11.0 h1:Jw9gBvyfmfOMq/EkYDm9+zGPxsDAA8jfeMpHmtZ+1oA=
github.com/pulumi/pulumi-java/pkg v0.11.0/go.mod h1:sXAk25P47AQVQL6ilAbFmRNgZykC7og/+87ihnqzFTc=
github.com/pulumi/pulumi-yaml v1.9.1 h1:JPeI80M23SPactxgnCFS1casZlSr7ZhAXwSx4H55QQ4=
Expand Down
2 changes: 1 addition & 1 deletion pkg/tests/go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0
github.com/hexops/autogold/v2 v2.2.1
github.com/hexops/valast v1.4.4
github.com/pulumi/providertest v0.0.13
github.com/pulumi/providertest v0.0.14
github.com/pulumi/pulumi-terraform-bridge/v3 v3.80.0
github.com/stretchr/testify v1.9.0
gotest.tools v2.2.0+incompatible
Expand Down
4 changes: 2 additions & 2 deletions pkg/tests/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1912,8 +1912,8 @@ github.com/pulumi/esc v0.9.1 h1:HH5eEv8sgyxSpY5a8yePyqFXzA8cvBvapfH8457+mIs=
github.com/pulumi/esc v0.9.1/go.mod h1:oEJ6bOsjYlQUpjf70GiX+CXn3VBmpwFDxUTlmtUN84c=
github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA=
github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY=
github.com/pulumi/providertest v0.0.13 h1:9CAaoviOTuCVHDI15h3znXa5JsKYtXLYHIIdxOCzo3Y=
github.com/pulumi/providertest v0.0.13/go.mod h1:REAoaN+hGOtdWJGirfWYqcSjCejlbGfzyVTUuemJTuE=
github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2pHgxQ8=
github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.11.0 h1:Jw9gBvyfmfOMq/EkYDm9+zGPxsDAA8jfeMpHmtZ+1oA=
github.com/pulumi/pulumi-java/pkg v0.11.0/go.mod h1:sXAk25P47AQVQL6ilAbFmRNgZykC7og/+87ihnqzFTc=
github.com/pulumi/pulumi-yaml v1.9.1 h1:JPeI80M23SPactxgnCFS1casZlSr7ZhAXwSx4H55QQ4=
Expand Down
78 changes: 78 additions & 0 deletions pkg/tests/schema_pulumi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1396,3 +1396,81 @@ Resources:
})
}
}

func TestFullyComputedNestedAttribute(t *testing.T) {
resMap := map[string]*schema.Resource{
"prov_test": {
Schema: map[string]*schema.Schema{
"attached_disks": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"name": {
Optional: true,
Type: schema.TypeString,
},
"key256": {
Computed: true,
Type: schema.TypeString,
},
},
},
},
"top_level_computed": {
Type: schema.TypeString,
Computed: true,
},
},
},
}

importer := func(val any) func(context.Context, *schema.ResourceData, interface{}) ([]*schema.ResourceData, error) {
return func(ctx context.Context, rd *schema.ResourceData, i interface{}) ([]*schema.ResourceData, error) {
elMap := map[string]any{
"name": "disk1",
"key256": val,
}
err := rd.Set("attached_disks", []map[string]any{elMap})
require.NoError(t, err)

err = rd.Set("top_level_computed", "computed_val")
require.NoError(t, err)

return []*schema.ResourceData{rd}, nil
}
}
bridgedProvider := pulcheck.BridgedProvider(t, "prov", resMap)

program := `
name: test
runtime: yaml
`
for _, tc := range []struct {
name string
importVal any
}{
{
"non-nil",
"val1",
},
{
"nil",
nil,
},
} {
t.Run(tc.name, func(t *testing.T) {
resMap["prov_test"].Importer = &schema.ResourceImporter{
StateContext: importer(tc.importVal),
}

pt := pulcheck.PulCheck(t, bridgedProvider, program)

res := pt.Import("prov:index/test:Test", "res1", "id1", "")

t.Logf(res.Stdout)

require.NotContains(t, res.Stdout, "One or more imported inputs failed to validate")
})
}
}
7 changes: 2 additions & 5 deletions pkg/tfbridge/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (

shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/schema"
shimutil "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/util"
)

// This file deals with translating between the Pulumi representations of a resource's configuration and state and the
Expand Down Expand Up @@ -497,8 +496,6 @@ func (ctx *conversionContext) makeTerraformInput(

var tfflds shim.SchemaMap

// We cannot use [shimutil.CastToTypeObject] because we have machinery that constructs invalid
// resource objects, such as [elemSchemas].
if tfs != nil {
if r, ok := tfs.Elem().(shim.Resource); ok {
tfflds = r.Schema()
Expand Down Expand Up @@ -1719,9 +1716,9 @@ func extractSchemaInputs(
return resource.NewArrayProperty(v)
case state.IsObject():
obj := state.ObjectValue()
if tfflds, ok := shimutil.CastToTypeObject(tfs); ok {
if tfflds, ok := tfs.Elem().(shim.Resource); ok {
return resource.NewProperty(
extractSchemaInputsObject(obj, tfflds, ps.Fields),
extractSchemaInputsObject(obj, tfflds.Schema(), ps.Fields),
)
}

Expand Down
67 changes: 47 additions & 20 deletions pkg/tfbridge/schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2179,6 +2179,46 @@ func TestRefreshExtractInputsFromOutputsMaxItemsOne(t *testing.T) {
assert.NoError(t, err)
}

func TestRefreshExtractInputsFromOutputsListOfObjects(t *testing.T) {
t.Parallel()

ruleSetProps := resource.PropertyMap{
"attachedDisks": resource.NewArrayProperty([]resource.PropertyValue{
resource.NewObjectProperty(resource.PropertyMap{
"name": resource.NewStringProperty("name1"),
"key256": resource.NewNullProperty(),
}),
}),
}

ruleSetSchema := func() shim.SchemaMap {
blockList := func(elem schema.SchemaMap) shim.Schema {
s := schema.Schema{
Type: shim.TypeList,
Optional: true,
Elem: (&schema.Resource{
Schema: elem,
}).Shim(),
}
return s.Shim()
}

return schema.SchemaMap{
"attachedDisks": blockList(schema.SchemaMap{
"name": (&schema.Schema{Type: shim.TypeString, Optional: true}).Shim(),
"key256": (&schema.Schema{Type: shim.TypeString, Computed: true}).Shim(),
}),
}
}

out, err := ExtractInputsFromOutputs(nil, ruleSetProps, ruleSetSchema(), nil, false)
assert.NoError(t, err)
t.Logf("out: %v", out)
attachedDiskVal := out["attachedDisks"].ArrayValue()[0].ObjectValue()
_, ok := attachedDiskVal["key256"]
assert.False(t, ok)
}

func TestFailureReasonForMissingRequiredFields(t *testing.T) {
// Define two required inputs
tfProvider := makeTestTFProviderV1(
Expand Down Expand Up @@ -2742,7 +2782,6 @@ func TestExtractSchemaInputsNestedMaxItemsOne(t *testing.T) {
"listObjects": resource.NewProperty([]resource.PropertyValue{
resource.NewProperty(resource.PropertyMap{
"__defaults": resource.NewProperty([]resource.PropertyValue{}),
"field1": resource.NewProperty(false),
"listScalar": resource.NewProperty(1.0),
}),
}),
Expand Down Expand Up @@ -3665,7 +3704,6 @@ func TestExtractInputsFromOutputsSdkv2(t *testing.T) {
// },
// expected: autogold.Expect(),
// },
// TODO[pulumi/pulumi-terraform-bridge#2180]: This is wrong as an input should not be produced for computed values.
{
name: "list block with computed element not extracted",
props: resource.NewPropertyMapFromMap(map[string]interface{}{
Expand All @@ -3687,20 +3725,16 @@ func TestExtractInputsFromOutputsSdkv2(t *testing.T) {
V: []resource.PropertyValue{},
},
resource.PropertyKey("foo"): resource.PropertyValue{V: []resource.PropertyValue{{
V: resource.PropertyMap{
resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
},
resource.PropertyKey("bar"): resource.PropertyValue{V: "baz"},
},
V: resource.PropertyMap{resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
}},
}}},
}),
},
// TODO[pulumi/pulumi-terraform-bridge#2180]: This is wrong as an input should not be produced for computed values.
{
name: "list block max items one with computed element not extracted",
props: resource.NewPropertyMapFromMap(map[string]interface{}{
"foo": []interface{}{map[string]string{"bar": "baz"}},
"foo": map[string]string{"bar": "baz"},
}),
schemaMap: map[string]*schemav2.Schema{
"foo": {
Expand All @@ -3714,16 +3748,9 @@ func TestExtractInputsFromOutputsSdkv2(t *testing.T) {
},
},
},
expected: autogold.Expect(resource.PropertyMap{
resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
},
resource.PropertyKey("foo"): resource.PropertyValue{V: []resource.PropertyValue{{
V: resource.PropertyMap{resource.PropertyKey("bar"): resource.PropertyValue{
V: "baz",
}},
}}},
}),
expected: autogold.Expect(resource.PropertyMap{resource.PropertyKey("__defaults"): resource.PropertyValue{
V: []resource.PropertyValue{},
}}),
},
}

Expand Down
1 change: 0 additions & 1 deletion pkg/tfshim/util/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ func IsOfTypeMap(tfs shim.Schema) bool {

// CastToTypeObject performs a checked cast from shim.Schema to a TF object (a collection
// of fields).
//
// See [shim.Schema.Elem()] comment for all the details of the encoding.
func CastToTypeObject(tfs shim.Schema) (shim.SchemaMap, bool) {
if tfs == nil {
Expand Down

0 comments on commit da22823

Please sign in to comment.