pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
192 stars 45 forks source link

Cannot set .Elem on a singly nested object block error when upgrading bridge #2185

Closed VenelinMartinov closed 3 weeks ago

VenelinMartinov commented 1 month ago

What happened?

The talos provider started getting an error when upgrading the bridge: https://github.com/pulumiverse/pulumi-talos/actions/runs/9934039926/job/27437766510

The error comes from here: https://github.com/pulumi/pulumi-terraform-bridge/blob/0e639f6a7353a1fa5d394886118bd19c6165aed4/pkg/tfbridge/info/validate.go#L121

@ringods tried to fix it by removing the .Elem nesting and the types overrides no longer work: https://github.com/pulumiverse/pulumi-talos/pull/99/files#diff-34c57e622183cb0d8dd0d3f9eaa0861b3340120e9b2ad811bac7ac7be4cea4b1

I believe there is something wrong with the code above as it checks for Map nested Resource Elems which is illegal in sdkv2.

Example

.

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

VenelinMartinov commented 1 month ago

Upstream schema: https://github.com/siderolabs/terraform-provider-talos/blob/d6c2a0bc939de41252d97f09b8e9dfcff3377450/pkg/talos/talos_machine_bootstrap_resource.go#L81

VenelinMartinov commented 1 month ago

Perhaps this is indeed legal in PF: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/attributes/map-nested

Not obvious where the failure is coming from then.

iwahbe commented 1 month ago

I'm marking this as impact/regression and p1 until we can provide guidance on the appropriate next steps for the talos provider.

t0yv0 commented 1 month ago

Would be curious on findings here, definitely need to tighten up testing around this.

I recently updated documentation on Elem() to give it some refinement https://github.com/pulumi/pulumi-terraform-bridge/pull/2187 and some worked out schema examples.

PF allows map-nested attributes indeed, that should project to Map<string, ObjectType>, and it's reasonable to have Elem customizing the ObjectType in this case. The validation code may not take this into account properly. In SDKV2 I don't think it's possible to express something like a map-nested attribute.

t0yv0 commented 1 month ago

One wild guess is that map-nested attributes and single-nested blocks are getting confused because they are indistinguishable in shim.Schema.

iwahbe commented 1 month ago

One wild guess is that map-nested attributes and single-nested blocks are getting confused because they are indistinguishable in shim.Schema.

That's very possible 😒, though I'm not sure how we would go about fixing it. The validation is at the level of shim.Schema, not the SDKv{1,2}/PF value.

t0yv0 commented 1 month ago

We'd need to extend shim.Schema to contain enough information to distinguish the cases if needed.

VenelinMartinov commented 1 month ago

Did some exploration of the shim layer mapping of pf types here: https://github.com/pulumi/pulumi-terraform-bridge/pull/2231

map-nested attributes and single-nested blocks

These are indeed identical in the shim layer.

Moreover I am fairly sure the CastToTypeObject function is wrong for PF too, not just for SDKv2: https://github.com/pulumi/pulumi-terraform-bridge/pull/2231#discussion_r1687740016

VenelinMartinov commented 1 month ago

The case being checked there is a PF-only concept - should this be moved out of the shim layer instead?