Skip to content

Commit

Permalink
Fix unknown collections of obj (#2061)
Browse files Browse the repository at this point in the history
This changes the bridge to correctly return unknowns for objects and
collections in sdkv2.

fixes #1885
fixes #2032

stacked on #2158

Confirmed that TF sdkv2 supports both unknown blocks and unknown
collections of blocks, so we should be fine to pass these into TF
providers.

The TF sdkv1 does not support unknowns for blocks and collections so we
keep the old behaviour of passing empty/ collection of unknown.

<details>

```hcl
provider "google" {
  region       = "us-central1"
}

resource "google_storage_bucket" "bucket" {
  name     = "example-bucket12312322312"
  location = "US"
}

resource "google_storage_bucket" "bucket1" {
  name     = "example-bucket123123223"
  location = "US"
  dynamic "lifecycle_rule" {
    for_each = google_storage_bucket.bucket.effective_labels
    content {
      action {
        type = lifecycle_rule.value
      }
      condition {
        age = 1
      }
    }
  }
}
```

</details>


This returns `"lifecycle_rule":cty.UnknownVal(cty.List(cty.Object))`


Our handling of collections containing unknowns and unknown collections
is significantly better:

Unknown collections:
before:

```
+ prov:index/test:Test: (create)
  [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
  tests     : [
      [0]: {}
  ]
```

after:

```
+ prov:index/test:Test: (create)
  [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
  tests     : output<string>
```

Note that the array being output as an `output<string>` is an engine
limitation.

Nested unknown collections:
before:

```
+ prov:index/nestedTest:NestedTest: (create)
  [urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
  tests     : [
      [0]: {
          nestedProps: [
              [0]: {
                  testProps : [
                      [0]: output<string>
                  ]
              }
          ]
      }
  ]
```

after:

```
+ prov:index/nestedTest:NestedTest: (create)
    [urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
    tests     : [
        [0]: {
            nestedProps: [
                [0]: {
                    testProps : output<string>
                }
            ]
        }
    ]
```

The unknown was being put one level lower than it should be.

Quite a few other very wrong outputs in
#2140, including
diff duplications, missing diffs etc.
  • Loading branch information
VenelinMartinov authored Jul 17, 2024
1 parent 97c9899 commit 4c5493a
Show file tree
Hide file tree
Showing 17 changed files with 320 additions and 118 deletions.
4 changes: 4 additions & 0 deletions pf/internal/schemashim/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,7 @@ func (p *SchemaOnlyProvider) NewResourceConfig(context.Context, map[string]inter
func (p *SchemaOnlyProvider) IsSet(context.Context, interface{}) ([]interface{}, bool) {
panic("schemaOnlyProvider does not implement runtime operation IsSet")
}

func (p *SchemaOnlyProvider) SupportsUnknownCollections() bool {
return true
}
4 changes: 4 additions & 0 deletions pf/proto/unsupported.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,7 @@ func (Provider) NewResourceConfig(ctx context.Context, object map[string]interfa
func (Provider) IsSet(ctx context.Context, v interface{}) ([]interface{}, bool) {
panic("Unimplemented")
}

func (Provider) SupportsUnknownCollections() bool {
panic("Unimplemented")
}
30 changes: 26 additions & 4 deletions pkg/tests/internal/pulcheck/pulcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,26 @@ import (
"gotest.tools/assert"
)

func propNeedsUpdate(prop *schema.Schema) bool {
if prop.Computed && !prop.Optional {
return false
}
if prop.ForceNew {
return false
}
return true
}

func resourceNeedsUpdate(res *schema.Resource) bool {
// If any of the properties need an update, then the resource needs an update.
for _, s := range res.Schema {
if propNeedsUpdate(s) {
return true
}
}
return false
}

// This is an experimental API.
func EnsureProviderValid(t T, tfp *schema.Provider) {
for _, r := range tfp.ResourcesMap {
Expand All @@ -54,10 +74,12 @@ func EnsureProviderValid(t T, tfp *schema.Provider) {
}
}

r.UpdateContext = func(
ctx context.Context, rd *schema.ResourceData, i interface{},
) diag.Diagnostics {
return diag.Diagnostics{}
if resourceNeedsUpdate(r) && r.UpdateContext == nil {
r.UpdateContext = func(
ctx context.Context, rd *schema.ResourceData, i interface{},
) diag.Diagnostics {
return diag.Diagnostics{}
}
}
}
require.NoError(t, tfp.InternalValidate())
Expand Down
74 changes: 30 additions & 44 deletions pkg/tests/schema_pulumi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -852,9 +852,7 @@ resources:
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+ prov:index/test:Test: (create)
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
tests : [
[0]: {}
]
tests : output<string>
Resources:
+ 3 to create
`),
Expand All @@ -866,11 +864,12 @@ Resources:
~ prov:index/test:Test: (update)
[id=newid]
[urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
~ tests: [
~ [0]: {
- testProp: "known_val"
}
- tests: [
- [0]: {
- testProp: "known_val"
}
]
+ tests: output<string>
Resources:
+ 1 to create
~ 1 to update
Expand Down Expand Up @@ -1012,9 +1011,7 @@ resources:
[urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes]
+ prov:index/nestedTest:NestedTest: (create)
[urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
tests : [
[0]: {}
]
tests : output<string>
Resources:
+ 3 to create
`),
Expand All @@ -1026,24 +1023,18 @@ Resources:
~ prov:index/nestedTest:NestedTest: (update)
[id=newid]
[urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
~ tests: [
~ [0]: {
- nestedProps: [
- [0]: {
- testProps: [
- [0]: "known_val"
]
}
]
- nestedProps: [
- [0]: {
- testProps: [
- [0]: "known_val"
]
}
]
}
- tests: [
- [0]: {
- nestedProps: [
- [0]: {
- testProps: [
- [0]: "known_val"
]
}
]
}
]
+ tests: output<string>
Resources:
+ 1 to create
~ 1 to update
Expand Down Expand Up @@ -1134,9 +1125,7 @@ resources:
[urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
tests : [
[0]: {
nestedProps: [
[0]: {}
]
nestedProps: output<string>
}
]
Resources:
Expand All @@ -1152,16 +1141,14 @@ Resources:
[urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
~ tests: [
~ [0]: {
~ nestedProps: [
~ [0]: {
- testProps: [
- [0]: "known_val"
]
- testProps: [
- [0]: "known_val"
]
}
- nestedProps: [
- [0]: {
- testProps: [
- [0]: "known_val"
]
}
]
+ nestedProps: output<string>
}
]
Resources:
Expand Down Expand Up @@ -1262,9 +1249,7 @@ resources:
[0]: {
nestedProps: [
[0]: {
testProps : [
[0]: output<string>
]
testProps : output<string>
}
]
}
Expand All @@ -1284,9 +1269,10 @@ Resources:
~ [0]: {
~ nestedProps: [
~ [0]: {
~ testProps: [
~ [0]: "known_val" => output<string>
- testProps: [
- [0]: "known_val"
]
+ testProps: output<string>
}
]
}
Expand Down
14 changes: 8 additions & 6 deletions pkg/tfbridge/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestCustomizeDiff(t *testing.T) {
Schema: &ResourceInfo{Fields: info},
}
tfState, err := makeTerraformStateWithOpts(ctx, r, "id", stateMap,
makeTerraformStateOptions{defaultZeroSchemaVersion: true})
makeTerraformStateOptions{defaultZeroSchemaVersion: true, unknownCollectionsSupported: true})
assert.NoError(t, err)

config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
Expand Down Expand Up @@ -130,7 +130,7 @@ func TestCustomizeDiff(t *testing.T) {
Schema: &ResourceInfo{Fields: info},
}
tfState, err := makeTerraformStateWithOpts(ctx, r, "id", stateMap,
makeTerraformStateOptions{defaultZeroSchemaVersion: true})
makeTerraformStateOptions{defaultZeroSchemaVersion: true, unknownCollectionsSupported: true})
assert.NoError(t, err)

config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
Expand Down Expand Up @@ -184,7 +184,7 @@ func TestCustomizeDiff(t *testing.T) {
Schema: &ResourceInfo{Fields: info},
}
tfState, err := makeTerraformStateWithOpts(ctx, r, "id", stateMap,
makeTerraformStateOptions{defaultZeroSchemaVersion: true})
makeTerraformStateOptions{defaultZeroSchemaVersion: true, unknownCollectionsSupported: true})
assert.NoError(t, err)

config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
Expand Down Expand Up @@ -289,7 +289,8 @@ func diffTest(t *testing.T, tfs map[string]*v2Schema.Schema, inputs,
sch, r, provider, info := s.setup(tfs)

tfState, err := makeTerraformStateWithOpts(ctx, r, "id", stateMap,
makeTerraformStateOptions{defaultZeroSchemaVersion: true})
makeTerraformStateOptions{
defaultZeroSchemaVersion: true, unknownCollectionsSupported: provider.SupportsUnknownCollections()})
assert.NoError(t, err)

config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
Expand Down Expand Up @@ -318,7 +319,8 @@ func diffTest(t *testing.T, tfs map[string]*v2Schema.Schema, inputs,
t.Run("withIgnoreAllExpected", func(t *testing.T) {
sch, r, provider, info := s.setup(tfs)
tfState, err := makeTerraformStateWithOpts(ctx, r, "id", stateMap,
makeTerraformStateOptions{defaultZeroSchemaVersion: true})
makeTerraformStateOptions{
defaultZeroSchemaVersion: true, unknownCollectionsSupported: provider.SupportsUnknownCollections()})
assert.NoError(t, err)

config, _, err := MakeTerraformConfig(ctx, &Provider{tf: provider}, inputsMap, sch, info)
Expand Down Expand Up @@ -1361,7 +1363,7 @@ func TestComputedListUpdate(t *testing.T) {
"prop": []interface{}{"foo"},
"outp": "bar",
},
map[string]DiffKind{
map[string]pulumirpc.PropertyDiff_Kind{
"prop": U,
},
pulumirpc.DiffResponse_DIFF_SOME)
Expand Down
134 changes: 134 additions & 0 deletions pkg/tfbridge/internal/schemaconvert/schemaconvert.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package schemaconvert

import (
v1Schema "github.com/hashicorp/terraform-plugin-sdk/helper/schema"
v2Schema "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)

func Sdkv2ToV1Type(t v2Schema.ValueType) v1Schema.ValueType {
return v1Schema.ValueType(t)
}

func Sdkv2ToV1SchemaOrResource(elem interface{}) interface{} {
switch elem := elem.(type) {
case nil:
return nil
case *v2Schema.Schema:
return Sdkv2ToV1Schema(elem)
case *v2Schema.Resource:
return Sdkv2ToV1Resource(elem)
default:
contract.Failf("unexpected type %T", elem)
return nil
}
}

func Sdkv2ToV1Resource(sch *v2Schema.Resource) *v1Schema.Resource {
//nolint:staticcheck
if sch.MigrateState != nil {
contract.Failf("MigrateState is not supported in conversion")
}
if sch.StateUpgraders != nil {
contract.Failf("StateUpgraders is not supported in conversion")
}
//nolint:staticcheck
if sch.Create != nil || sch.Read != nil || sch.Update != nil || sch.Delete != nil || sch.Exists != nil ||
sch.CreateContext != nil || sch.ReadContext != nil || sch.UpdateContext != nil ||
sch.DeleteContext != nil || sch.Importer != nil {
contract.Failf("runtime methods not supported in conversion")
}

if sch.CustomizeDiff != nil {
contract.Failf("CustomizeDiff is not supported in conversion")
}

timeouts := v1Schema.ResourceTimeout{}
if sch.Timeouts != nil {
timeouts = v1Schema.ResourceTimeout{
Create: sch.Timeouts.Create,
Read: sch.Timeouts.Read,
Update: sch.Timeouts.Update,
Delete: sch.Timeouts.Delete,
Default: sch.Timeouts.Default,
}
}
timoutsPtr := &timeouts
if sch.Timeouts == nil {
timoutsPtr = nil
}

return &v1Schema.Resource{
Schema: Sdkv2ToV1SchemaMap(sch.Schema),
SchemaVersion: sch.SchemaVersion,
DeprecationMessage: sch.DeprecationMessage,
Timeouts: timoutsPtr,
}
}

func Sdkv2ToV1Schema(sch *v2Schema.Schema) *v1Schema.Schema {
if sch.DiffSuppressFunc != nil {
contract.Failf("DiffSuppressFunc is not supported in conversion")
}

defaultFunc := v1Schema.SchemaDefaultFunc(nil)
if sch.DefaultFunc != nil {
defaultFunc = func() (interface{}, error) {
return sch.DefaultFunc()
}
}

stateFunc := v1Schema.SchemaStateFunc(nil)
if sch.StateFunc != nil {
stateFunc = func(i interface{}) string {
return sch.StateFunc(i)
}
}

set := v1Schema.SchemaSetFunc(nil)
if sch.Set != nil {
set = func(i interface{}) int {
return sch.Set(i)
}
}

validateFunc := v1Schema.SchemaValidateFunc(nil)
if sch.ValidateFunc != nil {
validateFunc = func(i interface{}, s string) ([]string, []error) {
return sch.ValidateFunc(i, s)
}
}

return &v1Schema.Schema{
Type: Sdkv2ToV1Type(sch.Type),
Optional: sch.Optional,
Required: sch.Required,
Default: sch.Default,
DefaultFunc: defaultFunc,
Description: sch.Description,
InputDefault: sch.InputDefault,
Computed: sch.Computed,
ForceNew: sch.ForceNew,
StateFunc: stateFunc,
Elem: Sdkv2ToV1SchemaOrResource(sch.Elem),
MaxItems: sch.MaxItems,
MinItems: sch.MinItems,
Set: set,
//nolint:staticcheck
ComputedWhen: sch.ComputedWhen,
ConflictsWith: sch.ConflictsWith,
ExactlyOneOf: sch.ExactlyOneOf,
AtLeastOneOf: sch.AtLeastOneOf,
Deprecated: sch.Deprecated,
ValidateFunc: validateFunc,
Sensitive: sch.Sensitive,
}
}

func Sdkv2ToV1SchemaMap(sch map[string]*v2Schema.Schema) map[string]*v1Schema.Schema {
res := make(map[string]*v1Schema.Schema)
for k, v := range sch {
res[k] = Sdkv2ToV1Schema(v)
}
return res
}
Loading

0 comments on commit 4c5493a

Please sign in to comment.