Problematic makeTerraformInput array wrapping

[t0yv0]

There are some issues that are possible to hit with this block of code that is part of the makeTerraformInput conversion
from PropertyValue to a hybrid TF-shaped map representation.

		// If we have schema information that indicates that this value is being presented to a map-typed field whose
		// Elem is a shim.Resource, wrap the value in an array in order to work around a bug in Terraform.
		if tfs != nil && tfs.Type() == shim.TypeMap {
			if _, hasResourceElem := tfs.Elem().(shim.Resource); hasResourceElem {
				return []interface{}{input}, nil
			}
		}

Specifically this does not work correctly with PlanResourceChange. This trips up recoverCtyValue assumptions and can cause failures.

This issue was discovered as part of randomized conformance testing.

Minimal schema:

        &schema.Resource{
			Schema: map[string]*schema.Schema{
				"f0": {
					Required: true,
					Type:     schema.TypeMap,
					Elem: &schema.Resource{
						Schema: map[string]*schema.Schema{
							"x": {Optional: true, Type: schema.TypeString},
						},
					},
				},
			},
		},

Minimal value:

map[string]any{"f0": map[string]any{"x": "ok"}}

Creating an instance of this resource will fail under PlanResourceChange but succeeds ordinarily.

What we are trying to represent here is a simple Object-typed property. In schemav2 providers this naturally is
represented as TypeMap with an Elem that is a pseudo-resource listing the types of the object fields. In TF code these
properties are also known as single-nested blocks and expect to be populated by a block.

The problem is that cty.Value representation should be a cty.ObjectVal not a cty.ListVal([]cty.Value{cty.ObjectVal(..)}).

We need to figure out a path forward here. Not sure which bug in Terraform the code refers to, but I suspect this is not a bug but a misunderstanding. Yet removing this old code before we are fully under randomized test can cause problems.

I suspect we can make recoverCtyValue less strict in recognizing [x] as x when it needs to recover an ObjectVal.

A more intrusive way forward would be to re-code makeTerraformInputs and friends to target cty.Value instead of the hybrid interface{} representation. This would remove the need to have another cty.Value recovery pass.

CC @iwahbe @VenelinMartinov

[VenelinMartinov]

Interesting find, did you find any context around the terraform bug this works around in the commit which introduced this?

A more intrusive way forward would be to re-code makeTerraformInputs and friends to target cty.Value instead of the hybrid interface{} representation. This would remove the need to have another cty.Value recovery pass.

I'm not sure I follow this proposal - where do we do the cty.Value recovery pass?

[t0yv0]

https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pkg/tfshim/sdk-v2/cty.go#L83 see recoverAndCoerceCtyValue. There's some machinery that we integrate with from TF that most naturally accesses the data in cty.Value representation. So we need to map PropertyValue to cty.Value. It could be natural to do it directly but currently it's done indirectly by PropertyValue->interface{}->cty.Value, with some schema awareness, which is apparently not exactly right. Randomized testing is uncovering all these corner cases..

[t0yv0]

This is further complicated by #1828 unfortunately. I'm learning a lot from this.

[t0yv0]

It might actually not be an issue once 1828 is accounted for. We should definitely start there.