add a flag to determine whether `ReadContext` is called via `schema.ImportStatePassthroughContext`

[Serpentiel]

SDK version

github.com/hashicorp/terraform-plugin-sdk/v2 v2.18.0dk/...

Use-cases

currently, there is no way to determine whether we are encountering an on-going import operation in schema.Resource.ReadContext if it is called via schema.ImportStatePassthroughContext

Attempted Solutions

there is, currently, no clean solution to solve this

there is, however, a way to achieve it by implementing custom schema.Resource.Importer.StateContext, however we would like to be using schema.ImportStatePassthroughContext

Proposal

add a method to schema.ResourceData, similar to IsNewResource, which would return true if there is an on-going import operation

[bflad]

Hi @Serpentiel 👋 Thank you for raising this feature request, as this type of information may be helpful in certain more advanced use cases. Could you expand a little more on what you're trying to do so we can ensure we are meeting your needs in the right manner?

Just for some additional context upfront, importing a resource into Terraform triggers two wholly separate RPCs against providers:

• ImportResourceState
• ReadResource (called after above is successful; also used for refreshing resources "normally")

In general, we recommend treating Read as self-contained, since it is generally supposed to be associated with entirely refreshing a resource no matter when the call was made, however sometimes pieces of information are only available in a configuration (e.g. not in an API response) or during resource creation. We do have a separate feature request, https://github.com/hashicorp/terraform/issues/28536, which is where that type of feature would have to be driven first, to potentially have Terraform give the provider whatever information it can from the configuration. If implemented in Terraform and the protocol, then the SDK could theoretically also provide that to providers during import. Since that's not available though, most providers today will denote which attributes are not supported after import, recommending that configurations are written to use the lifecycle configuration block ignore_changes attribute to prevent Terraform from showing differences on the unavailable data after import.

Outside of that, there are two other ways we could potentially handle this across the two separate RPCs given to providers:

• Have Terraform provide some sort of ReadResource request flag that denotes that this request was triggered specifically after ImportResourceState. The SDK could then read this request flag and do something like you propose, setting ResourceData to also denote something special.
• Implement something similar to #836, where a provider could set some private state information in the ImportResourceState response, which would be automatically included in the ReadResource request as separate data.

In any event, I think we'd need to talk through this to understand more about whether this is a detail that should be exposed to providers or not. Thanks again for raising this.

[Serpentiel]

hi @bflad, thanks for your quick response!

the API underlying our Terraform provider has a specific response that we need to consider erroneous then and only then when schema.Resource.ReadContext was called during import operation

I understand that it might be possible by implementing custom schema.Resource.Importer.StateContext, however we would like to be using schema.ImportStatePassthroughContext

when using schema.ImportStatePassthroughContext, there is no way to differentiate between normal read and import operations in schema.Resource.ReadContext

I believe the following solution that you suggested would work for us:

Have Terraform provide some sort of ReadResource request flag that denotes that this request was triggered specifically after ImportResourceState. The SDK could then read this request flag and do something like you propose, setting ResourceData to also denote something special.

I hope this made things a bit more clear!

[bflad]

the API underlying our Terraform provider has a specific response that we need to consider erroneous then and only then when schema.Resource.ReadContext was called during import operation

Would you be able to elaborate a little bit more on this class of API response? How would refreshing an imported resource differ from refreshing a created resource? What would be the intended provider and practitioner behavior if this error was encountered during resource read?

Before a protocol-level change like this is considered, where protocol changes have a very long maintenance tail, we would like to ensure we understand the use case fully so that we are not missing any potential design considerations that would offer a more preferable solution.

[Serpentiel]

Would you be able to elaborate a little bit more on this class of API response?

a simple HTTP GET operation to read some remote resource :)

How would refreshing an imported resource differ from refreshing a created resource? What would be the intended provider and practitioner behavior if this error was encountered during resource read?

we need to output the 404 Not Found error to the user when the resource is being refreshed during an on-going import operation, otherwise we need to call d.SetId("") and return nil, see the code example below—read the comments, too:

// schema.ReadContextFunc
func ResourceServiceRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
    client := m.(*aiven.Client)

    projectName, serviceName, err := SplitResourceID2(d.Id())
    if err != nil {
        return diag.FromErr(err)
    }

    s, err := client.Services.Get(projectName, serviceName)
    if err != nil {
        if err = ResourceReadHandleNotFound(err, d); err != nil {
            return diag.FromErr(fmt.Errorf("unable to refresh service %s: %s", d.Id(), err))
        }

        return nil
    }

    // code to refresh the resource goes here

    return nil
}

func ResourceReadHandleNotFound(err error, d *schema.ResourceData) error {
    // n.b. we currently use d.IsNewResource to determine an on-going import, 
    // but it doesn't works properly if there is a resource in Terraform state
    // with the same identifier already

    // IsUnknownResource basically checks if the provided error is a 404 Not Found error
    if err != nil && IsUnknownResource(err) && !d.IsNewResource() {
        d.SetId("")
        return nil
    }

    return err
}

[bflad]

Thank you for that additional information! Normally we would expect that API call and error return to happen in the Importer function, if desired, but it sounds like you would prefer some way to reduce your code so it can be all contained in the Read* function. Makes sense.

Is this preference because the 404 Not Found API error is more indicative of a problem (or otherwise helpful) to the practitioner than Terraform's error in this case? I believe Terraform should complain if you import a non-existent resource, e.g. called with d.SetId("") in the Read* function. If you have the Terraform import output of what happens when d.SetId("") is called versus your desired error output, that would be very helpful. Maybe Terraform's error messaging can be improved in this case, or the provider can use a warning diagnostic to signal the missing resource on that particular API error (regardless of whether its during import or normal refresh).

[bflad]

For additional context, I believe this is the Terraform code that should be triggered if there is no state for an imported resource:

https://github.com/hashicorp/terraform/blob/5da30c2b65265c9c7ac7580a1295e87715ebb568/internal/terraform/node_resource_import.go#L235-L251

Which should generate an error diagnostic to practitioners:

Error: Cannot import non-existent remote object

While attempting to import an existing object to {ADDRESS},
the provider detected that no object exists with the given id.
Only pre-existing objects can be imported; check that the id
is correct and that it is associated with the provider's
configured region or endpoint, or use "terraform apply" to
create a new remote object for this resource.

[Serpentiel]

hey, @bflad, sorry for long response!

we do get this output, yes, thanks to !d.IsNewResource() sanity check that we do—however, I just noticed that this issue only persists in acceptance tests since acceptance tests allow you to import resources that are already in the state, which Terraform doesn't allows you to do

I dug through the code and I noticed that it appears to be setting up some kind of a virtual state, but d.IsNewResource call actually returns false inside the virtual state, so it appears to be a bug

also, acceptance tests are missing the ability to fully simulate user's experience, i.e. the way that it would work via terraform import, and are not comparable at the moment

feel free to edit this issue accordingly, if needed, or let me know if creating another issue is required

[Serpentiel]

hey 👋

looks like we were wrong and currently there is no way to use d.IsNewResource to determine an on-going import operation

we've introduced a fix on our end, but it would be nice to see this in the SDK itself: https://github.com/aiven/terraform-provider-aiven/pull/1234

thank you!

[bflad]

@Serpentiel there is no requirement that "import identifiers" (the last argument given to the terraform import command; d.Id() during import) are unique, even with the same resource type, so therefore trying to rely on something like that is technically unreliable. If it works for your use case though, that's great, but it also means that it might not be acceptable for this SDK which should shy away from features which can potentially be misunderstood or misused.

Since this issue was raised though, terraform-plugin-framework has been made generally available which does support the notion of private state which is opaque to Terraform and hidden from plans, but accessible to provider code. That is likely a more reliable methodology in these cases, since the import code can set private state data directly associated with the exact resource state of the resource being imported rather than relying on an external detail by convention. If you are unfamiliar with terraform-plugin-framework, there is a benefits page that goes into some of the numerous improvements over sdk-based development. There's still https://github.com/hashicorp/terraform-plugin-sdk/issues/836 for tracking private state management in this SDK, but given that the framework is fairly stable and a lot of the provider ecosystem is moving in that direction, it's going to become less likely over time that the enhancement might land here.

[Serpentiel]

hey, @bflad! 👋

thanks for your quick reply, I wasn't saying that our "fix" was ideal and didn't mean to suggest reusing it as-is in the Plugin SDK

as for the Plugin Framework, we are aware of its existence and currently are working in the direction to switch to it completely in the near future

nonetheless, we believe that a crucial part of the SDK is missing—an ability to properly identify an ongoing import in the ReadContext