Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
VenelinMartinov opened this issue Jul 16, 2024 · 9 comments · Fixed by #2309
Closed

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

VenelinMartinov opened this issue Jul 16, 2024 · 9 comments · Fixed by #2309
Assignees
@iwahbe
Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer

Comments

@VenelinMartinov
Copy link
Contributor

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:

pulumi-terraform-bridge/pkg/tfbridge/info/validate.go

Line 121 in 0e639f6

c.error(path, errElemForObject)

@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.

  • Check if this is the case for PF too.

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 VenelinMartinov added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec labels Jul 16, 2024
@VenelinMartinov
Copy link
Contributor Author

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

@ringods ringods mentioned this issue Jul 16, 2024
Closed
@VenelinMartinov
Copy link
Contributor Author

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 iwahbe added p1 A bug severe enough to be the next item assigned to an engineer impact/regression Something that used to work, but is now broken and removed needs-triage Needs attention from the triage team labels Jul 18, 2024
@iwahbe
Copy link
Member

iwahbe commented Jul 18, 2024

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

@iwahbe iwahbe self-assigned this Jul 18, 2024
@t0yv0
Copy link
Member

t0yv0 commented Jul 19, 2024

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 #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
Copy link
Member

t0yv0 commented Jul 19, 2024

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

@iwahbe
Copy link
Member

iwahbe commented Jul 19, 2024

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
Copy link
Member

t0yv0 commented Jul 20, 2024

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

@VenelinMartinov
Copy link
Contributor Author

Did some exploration of the shim layer mapping of pf types here: #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: #2231 (comment)

@VenelinMartinov
Copy link
Contributor Author

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

@VenelinMartinov VenelinMartinov mentioned this issue Jul 23, 2024
Open
iwahbe added a commit that referenced this issue Aug 13, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
d5c0859 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.
```

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
@iwahbe iwahbe mentioned this issue Aug 13, 2024
Merged
iwahbe added a commit that referenced this issue Aug 13, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
44e39ea 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
iwahbe added a commit that referenced this issue Aug 14, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
2612044 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
iwahbe added a commit that referenced this issue Aug 14, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
c131662 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
iwahbe added a commit that referenced this issue Aug 15, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
01eac7f 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
iwahbe added a commit that referenced this issue Aug 15, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
b196311 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
iwahbe added a commit that referenced this issue Aug 15, 2024
@iwahbe
Enforce .Elem.Fields (instead of .Fields) for object types
f71e16b 
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with [schema
generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398).

We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because
of an ambiguity in `shim.Schema`. From a comment in the PR:

> To prevent confusion, users are barred from specifying
> information on ps directly, they must set .Fields on ps.Elem:
> ps.Elem.Fields.
>
> We need to make this choice (instead of having users set
> information on .Fields (and forbidding ps.Elem.Fields) because
> the [shim.Schema] representation is not capable of
> distinguishing between Map[Object] and Object. SDKv{1,2}
> providers are not able to express Map[Object], but Plugin
> Framework based providers are.

While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).

Fixes #2185
@iwahbe iwahbe closed this as completed in 823fd88 Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iwahbe iwahbe

Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Enforce .Elem.Fields (instead of .Fields) for object types pulumi/pulumi-terraform-bridge
3 participants
@t0yv0 @iwahbe @VenelinMartinov