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.

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

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

Open
mvantellingen opened this issue Dec 27, 2022 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@mvantellingen
Copy link

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 labd/terraform-provider-commercetools#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 mvantellingen added the bug Something isn't working label Dec 27, 2022
@mvantellingen
Copy link
Author

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

bflad commented Jan 3, 2023

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

bflad commented Jan 4, 2023

Hi again. After some further triage, this was determined to be a Terraform CLI bug hashicorp/terraform#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 added a commit to labd/terraform-provider-commercetools that referenced this issue Jan 5, 2023
The step to using a SingleNestedBlock failed due to some issues with terraform, described at hashicorp/terraform-plugin-framework#603

Instead use a ListNestedBlock with a maximum size of 1. This basically mirrors the approach used in the terraform-plugin-sdk and seems to work stable. It also removes the need to migrate existing state from a list to a object.
mvantellingen added a commit to labd/terraform-provider-commercetools that referenced this issue Jan 5, 2023
The step to using a SingleNestedBlock failed due to some issues with terraform, described at hashicorp/terraform-plugin-framework#603

Instead use a ListNestedBlock with a maximum size of 1. This basically mirrors the approach used in the terraform-plugin-sdk and seems to work stable. It also removes the need to migrate existing state from a list to a object.
mvantellingen added a commit to labd/terraform-provider-commercetools that referenced this issue Jan 5, 2023
The step to using a SingleNestedBlock failed due to some issues with terraform, described at hashicorp/terraform-plugin-framework#603

Instead use a ListNestedBlock with a maximum size of 1. This basically mirrors the approach used in the terraform-plugin-sdk and seems to work stable. It also removes the need to migrate existing state from a list to a object.
@mvantellingen
Copy link
Author

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

bflad commented Jan 5, 2023

Super helpful followup, thank you, @mvantellingen. 😄

hashicorp/terraform#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
Copy link

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 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants