Open VenelinMartinov opened 1 month ago
Investigated in https://github.com/pulumi/pulumi-terraform-bridge/pull/2054:
a few interesting cases:
TestPlanResourceChangeUnknowns/unknown_for_set_block_prop
- here the whole set block is unknown and is returned back as a nil object.
TestPlanResourceChangeUnknowns/unknown_for_set_block_prop_collection
- the whole collection here is unknown and is returned as a set with a nil object element, similar to above.
TestPlanResourceChangeUnknowns/unknown_for_set_prop
- here the value is a set attribute with an unknown element but the value is returned as as an unknown set.
For the first two cases it looks like the mistranslation happens in https://github.com/pulumi/pulumi-terraform-bridge/blob/26a293813b6274f73a7cdb36390961346b20877c/pkg/tfshim/sdk-v2/proposed_new.go#L34
Proposed new returns a nil object instead of an unknown - I hacked it and the test now correctly returns an unknown for the test.
digging deeper it looks like it's objchange.ProposedNew
which gets it wrong - it returns a "set_block_prop":cty.SetVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)})})
instead of a "set_block_prop":cty.SetVal([]cty.Value{cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String}))}),
I tried to repro TestPlanResourceChangeUnknowns/unknown_for_set_block_prop_collection
in a program, as that seems to be a different root case than _prop.
Program:
GRPC logs:
We seem to discard the unknown collection of objects in check, so I couldn't repro the test case - it fails much earlier.
I'm investigating a fix for the _prop case in https://github.com/pulumi/pulumi-terraform-bridge/pull/2060/files
I've also added a repro for Check discarding the unknown collection of objects.
Ok, the "unknown array of obj" -> array of single empty obj is happening in makeTerraformUnknown
:
It sets unknown for any required properties and misses the rest.
I think we can actually do better here - TF supports dynamic blocks, which AFAIK support unknown values, hence TF supports unknowns inside collections.
This seems to then get promoted to an unknown collection as evidenced by the _block_prop tests.
So in the case of unknown collections we could generate a collection of unknown instead of collection of empty.
Testing the last hypothesis in https://github.com/pulumi/pulumi-terraform-bridge/pull/2061
ProposedNew is copied directly from TF though maybe an older version compared to opentofu so I strongly suspect editing it is going to take us further away from TF behavior, at least worth checking to x-compare.
makeTerraformUnknown is interesting it looks like a heuristic to deal with exactly this case and indeed maybe it can be refined. Of course for cases where makeTerraformUnknown kicks in we can't -compare because it's not expressible in TF.
The original hypothesis here that in TF blocks can't be unknown is false, since TF supports dynamic blocks, which are indeed unknown during plan.
We did find some valuable improvements in our handling of unknowns in https://github.com/pulumi/pulumi-terraform-bridge/pull/2061 and this issue was repurposed for that.
What happened?
In TF, blocks can't be nil or unknown, as there is no way to specify that in HCL.
On the pulumi side we map blocks to either flat or collection parameters, depending on MaxItemsOne. These can then be specified as unknown in pulumi.
This is "unsupported" on the TF side so we should investigate if it works correctly and if there is a way to prevent that.
I suspect this isn't too big of a problem in the wild as nothing should output the block type but there might be uses with apply which output an unknown block
Example
the block parameter of myres can not be specified as an unknown value here.
In pulumi this maps to either:
or
both of which might be unknown, perhaps with
out.apply(lambda x: MyResBlockArgs(val=x)
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).