Skip to content

Commit

Permalink
fix set element order in state
Browse files Browse the repository at this point in the history
  • Loading branch information
VenelinMartinov committed Oct 11, 2024
1 parent 678d0fd commit eb6f421
Show file tree
Hide file tree
Showing 7 changed files with 264 additions and 15 deletions.
2 changes: 1 addition & 1 deletion pkg/tests/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,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.1.2
github.com/pulumi/providertest v0.1.3
github.com/pulumi/pulumi-terraform-bridge/v3 v3.80.0
github.com/pulumi/pulumi/sdk v1.14.1
github.com/stretchr/testify v1.9.0
Expand Down
4 changes: 2 additions & 2 deletions pkg/tests/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1980,8 +1980,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE=
github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c=
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.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng=
github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg=
github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0=
github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns=
github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM=
github.com/pulumi/pulumi-yaml v1.10.3 h1:j5cjPiE32ILmjrWnC1cfZ0MWdqCZ8fg9wlaWk7HOtM4=
Expand Down
107 changes: 107 additions & 0 deletions pkg/tests/schema_pulumi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ import (
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/pulcheck"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/tfcheck"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info"
shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2"
"github.com/pulumi/pulumi/sdk/v3/go/auto"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optpreview"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
Expand Down Expand Up @@ -4159,3 +4161,108 @@ func TestMakeTerraformResultNilVsEmptyMap(t *testing.T) {
assert.True(t, props["test"].DeepEquals(emptyMap))
})
}

func TestSetElementOrderInState(t *testing.T) {
sch := map[string]*schema.Schema{
"test": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
}

resMap := map[string]*schema.Resource{
"prov_test": {
Schema: sch,
},
}

tfp := &schema.Provider{ResourcesMap: resMap}
prov := pulcheck.BridgedProvider(t, "prov", tfp)

getStringOutputsFromResult := func(res auto.UpResult) []string {
outputs := res.Outputs["testOut"].Value.([]interface{})
stringOutputs := make([]string, len(outputs))
for i, v := range outputs {
stringOutputs[i] = v.(string)
}
return stringOutputs
}

// Test that the order of elements in the state is preserved when created and updated.
runTest := func(t *testing.T, prov info.Provider, props1, props2 []string) {
props1JSON, err := json.Marshal(props1)
require.NoError(t, err)
program := `
name: test
runtime: yaml
resources:
mainRes:
type: prov:index:Test
properties:
tests: %s
outputs:
testOut: ${mainRes.tests}`

program1 := fmt.Sprintf(program, props1JSON)

props2JSON, err := json.Marshal(props2)
require.NoError(t, err)
program2 := fmt.Sprintf(program, props2JSON)

pt := pulcheck.PulCheck(t, prov, program1)
res := pt.Up(t)

require.Equal(t, props1, getStringOutputsFromResult(res))

pt.WritePulumiYaml(t, program2)
res = pt.Up(t)
require.Equal(t, props2, getStringOutputsFromResult(res))
}

t.Run("ordered unchanged", func(t *testing.T) {
runTest(t, prov,
[]string{"val1", "val2", "val3"},
[]string{"val1", "val2", "val3"},
)
})

t.Run("unordered unchanged", func(t *testing.T) {
runTest(t, prov,
[]string{"val2", "val3", "val1"},
[]string{"val2", "val3", "val1"},
)
})

t.Run("ordered added", func(t *testing.T) {
runTest(t, prov,
[]string{"val1", "val2"},
[]string{"val1", "val2", "val3"},
)
})

t.Run("unordered added", func(t *testing.T) {
runTest(t, prov,
[]string{"val2", "val1"},
[]string{"val2", "val1", "val3"},
)
})

t.Run("ordered removed", func(t *testing.T) {
runTest(t, prov,
[]string{"val1", "val2", "val3"},
[]string{"val1", "val2"},
)
})

t.Run("unordered removed", func(t *testing.T) {
runTest(t, prov,
[]string{"val2", "val3", "val1"},
[]string{"val2", "val1"},
)
})
}

//TODO: Test unknown sets, unknowns elements in sets and nested unknowns in set elements.
8 changes: 6 additions & 2 deletions pkg/tfbridge/detailed_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,9 @@ func makeDetailedDiffV2(
if err != nil {
return nil, err
}
props, err := MakeTerraformResult(ctx, prov, proposedState, tfs, ps, assets, supportsSecrets)
props, err := makeTerraformResultWithOpts(
ctx, prov, proposedState, tfs, ps, assets, supportsSecrets,
makeTerraformResultOpts{originalInputs: newInputs})

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, ubuntu-latest, DEFAULT)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, ubuntu-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, macos-latest, DEFAULT)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, macos-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, ubuntu-latest, DEFAULT)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, ubuntu-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: newInputs

Check failure on line 405 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, macos-latest, DEFAULT)

undefined: newInputs
if err != nil {
return nil, err
}
Expand All @@ -409,7 +411,9 @@ func makeDetailedDiffV2(
if err != nil {
return nil, err
}
priorProps, err := MakeTerraformResult(ctx, prov, prior, tfs, ps, assets, supportsSecrets)
priorProps, err := makeTerraformResultWithOpts(
ctx, prov, prior, tfs, ps, assets, supportsSecrets,
makeTerraformResultOpts{originalInputs: oldInputs})

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: oldInputs) (typecheck)

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: oldInputs) (typecheck)

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / lint

undefined: oldInputs) (typecheck)

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, ubuntu-latest, DEFAULT)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, ubuntu-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, macos-latest, DEFAULT)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.22.x, macos-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, ubuntu-latest, DEFAULT)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, ubuntu-latest, PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW)

undefined: oldInputs

Check failure on line 416 in pkg/tfbridge/detailed_diff.go

View workflow job for this annotation

GitHub Actions / Test and Lint / test (1.23.x, macos-latest, DEFAULT)

undefined: oldInputs
if err != nil {
return nil, err
}
Expand Down
20 changes: 16 additions & 4 deletions pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -1362,7 +1362,10 @@ func (p *Provider) Create(ctx context.Context, req *pulumirpc.CreateRequest) (*p
}

// Create the ID and property maps and return them.
props, err = MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
props, err = makeTerraformResultWithOpts(
ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
makeTerraformResultOpts{originalInputs: props},
)
if err != nil {
reasons = append(reasons, errors.Wrapf(err, "converting result for %s", urn).Error())
}
Expand Down Expand Up @@ -1465,7 +1468,10 @@ func (p *Provider) Read(ctx context.Context, req *pulumirpc.ReadRequest) (*pulum
// Store the ID and properties in the output. The ID *should* be the same as the input ID, but in the case
// that the resource no longer exists, we will simply return the empty string and an empty property map.
if newstate != nil {
props, err := MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
props, err := makeTerraformResultWithOpts(
ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
makeTerraformResultOpts{},
)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1707,7 +1713,10 @@ func (p *Provider) Update(ctx context.Context, req *pulumirpc.UpdateRequest) (*p
}
}

props, err := MakeTerraformResult(ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets)
props, err := makeTerraformResultWithOpts(
ctx, p.tf, newstate, res.TF.Schema(), res.Schema.Fields, assets, p.supportsSecrets,
makeTerraformResultOpts{originalInputs: news},
)
if err != nil {
reasons = append(reasons, errors.Wrapf(err, "converting result for %s", urn).Error())
}
Expand Down Expand Up @@ -1860,7 +1869,10 @@ func (p *Provider) Invoke(ctx context.Context, req *pulumirpc.InvokeRequest) (*p
}

// Add the special "id" attribute if it wasn't listed in the schema
props, err := MakeTerraformResult(ctx, p.tf, invoke, ds.TF.Schema(), ds.Schema.Fields, nil, p.supportsSecrets)
props, err := makeTerraformResultWithOpts(
ctx, p.tf, invoke, ds.TF.Schema(), ds.Schema.Fields, nil, p.supportsSecrets,
makeTerraformResultOpts{originalInputs: args},
)
if err != nil {
return nil, err
}
Expand Down
104 changes: 104 additions & 0 deletions pkg/tfbridge/recover_set_order.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package tfbridge

import (
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"

shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk"
)

// a variant of PropertyPath.Get which works on PropertyMaps
func getPathFromPropertyMap(
path resource.PropertyPath, propertyMap resource.PropertyMap,
) (resource.PropertyValue, bool) {
if len(path) == 0 {
return resource.NewNullProperty(), false
}

rootKeyStr, ok := path[0].(string)
contract.Assertf(ok && rootKeyStr != "", "root key must be a non-empty string")
rootKey := resource.PropertyKey(rootKeyStr)
restPath := path[1:]

if len(restPath) == 0 {
return propertyMap[rootKey], true
}

if !propertyMap.HasValue(rootKey) {
return resource.NewNullProperty(), false
}

return restPath.Get(propertyMap[rootKey])
}

// a variant of PropertyPath.Set which works on PropertyMaps
func setPathInPropertyMap(
path resource.PropertyPath, propertyMap resource.PropertyMap, value resource.PropertyValue,
) bool {
if len(path) == 0 {
return false
}

rootKeyStr, ok := path[0].(string)
contract.Assertf(ok && rootKeyStr != "", "root key must be a non-empty string")
rootKey := resource.PropertyKey(rootKeyStr)
restPath := path[1:]

if len(restPath) == 0 {
propertyMap[rootKey] = value
return true
}

if !propertyMap.HasValue(rootKey) {
return false
}

return restPath.Set(propertyMap[rootKey], value)
}

// Sets in Terraform are unordered but are represented as ordered lists in the Pulumi world, and detailed diff must
// return indices into this list to indicate the elements that have changed.
func recoverSetOrderSingle(
target resource.PropertyMap, origin resource.PropertyMap, setPath resource.PropertyPath,
) {
targetSet, ok := getPathFromPropertyMap(setPath, target)
if !ok || !targetSet.IsArray() || targetSet.IsComputed() {
return
}
originSet, ok := getPathFromPropertyMap(setPath, origin)
if !ok || !originSet.IsArray() {
return
}

// No need to recover the order if the set is empty or has only one element
if len(targetSet.ArrayValue()) == 0 || len(originSet.ArrayValue()) == 0 || len(targetSet.ArrayValue()) == 1 {
return
}

ok = setPathInPropertyMap(setPath, target, originSet)
contract.Assertf(ok, "failed to set the origin set but it should exist!")
}

// TODO: Should we cache this?
func getSetPaths(schema shim.SchemaMap) []SchemaPath {
paths := make([]SchemaPath, 0)

walk.VisitSchemaMap(schema, func(path SchemaPath, schema shim.Schema) {
if schema.Type() == shim.TypeSet {
paths = append(paths, path)
}
})

return paths
}

func recoverSetOrder(
target resource.PropertyMap, origin resource.PropertyMap, tfs shim.SchemaMap, ps map[string]*SchemaInfo,
) {
setPaths := getSetPaths(tfs)
for _, setPath := range setPaths {
propertyPath := SchemaPathToPropertyPath(setPath, tfs, ps)
recoverSetOrderSingle(target, origin, propertyPath)
}
}
34 changes: 28 additions & 6 deletions pkg/tfbridge/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -1002,21 +1002,21 @@ func makeTerraformUnknown(tfs shim.Schema, unknownCollectionsSupported bool) int
}
}

// metaKey is the key in a TF bridge result that is used to store a resource's meta-attributes.
const metaKey = "__meta"
type makeTerraformResultOpts struct {
// If specified these will be used to recover the original order of elements in arrays representing TF sets.
originalInputs resource.PropertyMap
}

// MakeTerraformResult expands a Terraform state into an expanded Pulumi resource property map. This respects
// the property maps so that results end up with their correct Pulumi names when shipping back to the engine.
func MakeTerraformResult(
func makeTerraformResultWithOpts(
ctx context.Context,
p shim.Provider,
state shim.InstanceState,
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
assets AssetTable,
supportsSecrets bool,
opts makeTerraformResultOpts,
) (resource.PropertyMap, error) {

var outs map[string]interface{}
if state != nil {
obj, err := state.Object(tfs)
Expand All @@ -1035,9 +1035,31 @@ func MakeTerraformResult(
outMap[metaKey] = resource.NewStringProperty(string(metaJSON))
}

if opts.originalInputs != nil {
recoverSetOrder(outMap, opts.originalInputs, tfs, ps)
}

return outMap, nil
}

// metaKey is the key in a TF bridge result that is used to store a resource's meta-attributes.
const metaKey = "__meta"

// MakeTerraformResult expands a Terraform state into an expanded Pulumi resource property map. This respects
// the property maps so that results end up with their correct Pulumi names when shipping back to the engine.
// Deprecated: Use makeTerraformResultWithOpts instead.
func MakeTerraformResult(
ctx context.Context,
p shim.Provider,
state shim.InstanceState,
tfs shim.SchemaMap,
ps map[string]*SchemaInfo,
assets AssetTable,
supportsSecrets bool,
) (resource.PropertyMap, error) {
return makeTerraformResultWithOpts(ctx, p, state, tfs, ps, assets, supportsSecrets, makeTerraformResultOpts{})
}

// MakeTerraformOutputs takes an expanded Terraform property map and returns a Pulumi equivalent. This respects
// the property maps so that results end up with their correct Pulumi names when shipping back to the engine.
func MakeTerraformOutputs(
Expand Down

0 comments on commit eb6f421

Please sign in to comment.