hashicorp / terraform-plugin-framework

A next-generation framework for building Terraform providers.
https://developer.hashicorp.com/terraform/plugin/framework
Mozilla Public License 2.0
298 stars 92 forks source link

Removing a block results in planned for existence but config wants absence. #603

Open mvantellingen opened 1 year ago

mvantellingen commented 1 year ago

Module version

github.com/hashicorp/terraform-plugin-framework v1.0.0
github.com/hashicorp/terraform-plugin-go v0.14.2
github.com/hashicorp/terraform-plugin-log v0.7.0
github.com/hashicorp/terraform-plugin-sdk/v2 v2.23.0

Relevant provider source code

I reproduced the issue i'm running into at https://github.com/mvantellingen/terraform-pf-testcase The actual implementation is at https://github.com/labd/terraform-provider-commercetools/pull/328

// Schema defines the schema for the data source.
func (r *orderResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) {
    resp.Schema = schema.Schema{
        Description: "Manages an order.",
        Attributes: map[string]schema.Attribute{
            "id": schema.StringAttribute{
                Description: "Numeric identifier of the order.",
                Computed:    true,
                PlanModifiers: []planmodifier.String{
                    stringplanmodifier.UseStateForUnknown(),
                },
            },
        },
        Blocks: map[string]schema.Block{
            "myblock": schema.SingleNestedBlock{
                Attributes: map[string]schema.Attribute{
                    "optional": schema.BoolAttribute{
                        Optional: true,
                    },
                    "optional_int": schema.Int64Attribute{
                        Optional: true,
                    },
                },
            },
        },
    }
}

Terraform Configuration Files

When I have the following config applied:

resource "hashicups_order" "test" {
    myblock {
        optional = false
        optional_int = 10
    }
}               

and then want to remove the myblock

resource "hashicups_order" "test" {
}   

Actual Behavior

Error: Provider produced invalid plan

Provider "registry.terraform.io/hashicorp/hashicups" planned an invalid value
for hashicups_order.test.myblock: planned for existence but config wants
absence.

This is a bug in the provider, which should be reported in the provider's own
issue tracker

Steps to Reproduce

See https://github.com/mvantellingen/terraform-pf-testcase (run make testacc)

mvantellingen commented 1 year ago

A workaround I tried is to add a custom planmodifier with the following code:

func (m removeBlockModifier) PlanModifyObject(ctx context.Context, req planmodifier.ObjectRequest, resp *planmodifier.ObjectResponse) {
    attrs := req.ConfigValue.Attributes()
    if len(attrs) == 0 {
        resp.PlanValue = req.ConfigValue
    }
}
bflad commented 1 year ago

Hi @mvantellingen 👋 Thank you for raising this and sorry you ran into this unexpected behavior. Could you let us know which version(s) of Terraform CLI by chance? I'm guessing it might be all of them including the latest 1.3.x, but just trying to confirm. It'd also be good to know if the workaround you tried worked or not.

Single nested block support in terraform-plugin-framework is net-new over what was available to providers using terraform-plugin-sdk and specifically was added for the use case of bridging sdk's timeouts block support for migrating from sdk to framework via https://github.com/hashicorp/terraform-plugin-framework-timeouts. There may be some rough edges with its support within Terraform CLI or framework since it was not previously available to providers so it could take a few Terraform CLI or framework releases to get working for all use cases. We can work with the CLI team to assess whether these changes are feasible, relatively straightforward, and can be prioritized for implementation.

In the meantime and if possible for your use case, it'd be recommended to use a single nested attribute in this case rather than a single nested block. This does require protocol version 6 (Terraform 1.x) though. If Terraform 0.12+ support on protocol version 5 is necessary, then implementing a list nested block with validation for more than 1 element (e.g. listvalidator.SizeAtMost()) similar to a terraform-plugin-sdk based provider implementation would be the recommendation.

bflad commented 1 year ago

Hi again. After some further triage, this was determined to be a Terraform CLI bug https://github.com/hashicorp/terraform/issues/32460. The plan (proposed new state) value should match the configuration value in this case and should be coming across the protocol as null. It looks like your schema-based plan modifier should do the right thing as a workaround, although I'd recommend checking via IsNull() for clarity:

func (m removeBlockModifier) PlanModifyObject(ctx context.Context, req planmodifier.ObjectRequest, resp *planmodifier.ObjectResponse) {
    // Reference: https://github.com/hashicorp/terraform/issues/32460
    if req.ConfigValue.IsNull() {
        resp.PlanValue = req.ConfigValue
    }
}

This provider code workaround is necessary until that issue is resolved in Terraform, although unfortunately it may need to be permanently in provider code unless the framework implements similar logic, since the provider can be used by any protocol-compatible Terraform versions (0.12+ or 1.0+). The above comment has better recommendations (e.g. using a single nested attribute instead).

mvantellingen commented 1 year ago

Hi @bflad,

I was using Terraform 1.3.x. I wanted to use the blocks to keep it backwards compatible with the current provider developed in SDK v2. For v1 we used the attributes syntax (block = {}) and we switch to the block syntax for v2.

What I finally ended up doing was using a ListNestedBlock with a validator to make sure there is at most one item. This is basically the same approach as used before with SDK v2 and also removed the need for a state migration.

bflad commented 1 year ago

Super helpful followup, thank you, @mvantellingen. 😄

https://github.com/hashicorp/terraform/pull/32463 contains the Terraform CLI fix and if it was merged right now would be included in Terraform 1.3.8+ and 1.4+. While it should be relatively safe to implement a configuration versus plan value check for this in the framework to cover providers being called by prior Terraform versions, I think it might be best to see if other folks run into this situation before committing to that effort. The implementation would require some additional internal data versus schema heuristics that are not in place at the moment since they are not needed elsewhere.

patrickcping commented 1 year ago

I've also hit this problem, we're doing a gradual migration of SDKv2 to Framework and we're maintaining the block structure until the next major release of the provider. I've also used the ListNestedBlock as a workaround (thanks @mvantellingen )