TF Importer method not called during .get resource method with partial state

VenelinMartinov commented 3 months ago

What happened?

When resource .get methods are called an the program specifies at least one resource argument, the bridge incorrectly misses the TF Importer method call. This is due to the assumption that during /Read if there are any resource properties specified then we are doing a Refresh, not an Import. This happens here: https://github.com/pulumi/pulumi-terraform-bridge/blob/f18ff2f593cb3b8aab4237d4b37fca0b677db3d2/pkg/tfbridge/provider.go#L1366

Not calling the Importer method, in turn, means that the TF Refresh method is called with partial resource state, which it is NOT meant to handle, as TF has no such use. This can lead to provider panics and other unexpected behaviour, as in https://github.com/pulumi/pulumi-gcp/issues/2285

Example

https://github.com/pulumi/pulumi-gcp/issues/2285

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 3 months ago

Had a chat with @lunaris and @Frassle about .get and they gave me a bunch of much needed context here.

From our discussion:

No obvious reason why the provider receives the resource arguments during .get. Indeed when using the import resource option the provider does not receive the partial state. It doesn't look as if the bridge is equipped to use the partial state in any way - we only assume non-empty props means Refresh, which leads to the above problem.

Not clear how the provider is actually meant to infer that it is doing a Read for a .get and not a Refresh. It turns out the assumption above is wrong.

VenelinMartinov commented 3 months ago

Note that some resources do work without Importer as some resources' Refresh can handle id-only Refresh. For example: https://github.com/hashicorp/terraform-provider-aws/blob/281a78fe9957a79b0125c2fbf64362e9de27d93a/internal/service/s3/bucket.go#L67-L69

Looks like quite a few resources in AWS are configured that way.

Others, however, do not. The GCP example above is one, other resources in GCP too, there's also quite a few in AWS and very likely other providers.

VenelinMartinov commented 3 months ago

Seems like we need to either:

Stop sending properties with .get. The import resource option already does that, we could make .get work similarly, where the engine sends a Read and then matches the returned state to the specified state in the program.
Add some way for the provider to distinguish between .get and Refresh in the Read call

VenelinMartinov commented 3 months ago

very unfortunate: https://github.com/pulumi/pulumi/pull/2531/files#diff-891673360e9ecc312ee0f67f69b7a4455d9c1e04d00ff8056bbcb9ef7007b411R570 The order of the parameters there is wrong and conflicts with the documented behaviour: https://github.com/pulumi/pulumi/pull/2531/files#diff-cd7facc0851674f0fb0156f6944fd9f0edf2a60069c9a8c80718f537a535cffdR140

This looks as if it has been broken from the beginning, as the bridge checks for the documented behaviour and not the actual behaviour.

This PR which @Frassle found is also relevant: https://github.com/pulumi/pulumi/pull/14683 - this changed the properties to match the inputs.

t0yv0 commented 3 months ago

I'd like to request some clarification here if possible. I understand this bug is in Resource Get functions support for bridged providers. This area is distinct from import, could we remove area/import tag perhaps?

In my understanding the bridge never supported this functionality well at least not with explicit tests. There was an expectation that any implementation of Read would somehow be usable with the get functions. So making this more robust could be a good item to pull into a completeness effort.

Before we proceed with trying to decide which fixes to take, would it be possible to get a written reference for what the getter functions are supposed to be doing.

For example:

From aws.ec2.Vpc.get:

     /**
     * Get an existing Vpc resource's state with the given name, ID, and optional extra
     * properties used to qualify the lookup.
     *
     * @param name The _unique_ name of the resulting resource.
     * @param id The _unique_ provider ID of the resource to lookup.
     * @param state Any extra arguments used during the lookup.
     * @param opts Optional settings to control the behavior of the CustomResource.
     */
    static get(name: string, id: pulumi.Input<pulumi.ID>, state?: VpcState, opts?: pulumi.CustomResourceOptions): Vpc;

There is a notion here that the user may be specifying VpcState to qualify the lookup or perform some filtering on available results. Yet there is no mention in Resource Get

Nor this is covered in https://pulumi-developer-docs.readthedocs.io

What is the expectation of filtering on state? Which part is provider vs engine responsibility ?

VenelinMartinov commented 3 months ago

My understanding of the feature is that the user is allowed to specify a partial state, which should guide or filter the read resource.

Given that the engine does not do anything to filter the result and it passes the partial state to the provider, I believe this to be the responsibility of the provider right now.

This feature was indeed never supported well in the bridge and it might have been broken from the beginning. Note that it is currently pretty broken, as for example in https://github.com/pulumi/pulumi-gcp/issues/2285, the TF provider ends up accidentally generating an unintended API call to the upstream API. This happens because we call Refresh with the unexpected state.

VenelinMartinov commented 3 months ago

This needs a change in the engine: Provide a way for providers to distinguish between Refresh and partial state resource .get pulumi/pulumi#16886

VenelinMartinov commented 3 weeks ago

Likely connected as well, although it might no longer be relevant: https://github.com/pulumi/pulumi-gcp/issues/886

pulumi / pulumi-terraform-bridge