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

Adding timeout attribute the the schema forces resource replacement instead of in-place update #802

Closed
wojciechwroblewski opened this issue Jul 14, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@wojciechwroblewski
Copy link

Module version

github.com/hashicorp/terraform-plugin-framework v1.3.2
github.com/hashicorp/terraform-plugin-framework-timeouts v0.4.1

Relevant provider source code

Slightly modified schema from terraform-provider-scaffolding, with an attribute configured to require replace if changed.

func (r *ExampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		MarkdownDescription: "Example resource",

		Attributes: map[string]schema.Attribute{
			"id": schema.StringAttribute{
				Computed:            true,
				MarkdownDescription: "Example identifier",
				PlanModifiers: []planmodifier.String{
					stringplanmodifier.UseStateForUnknown(),
				},
			},
			"replace": schema.StringAttribute{
				MarkdownDescription: "Example configurable attribute that forces replacement if changed",
				Optional:            true,
				Computed:            true,
				PlanModifiers: []planmodifier.String{
					stringplanmodifier.RequiresReplace(),
				},
			},
			"timeouts": timeouts.Attributes(ctx, timeouts.Opts{
				Create: true,
				Update: true,
				Delete: true,
			}),
		},
	}
}

func (r *ExampleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
	var data *ExampleResourceModel
	resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
	if resp.Diagnostics.HasError() {
		return
	}

	data.Id = types.StringValue("example-id")
	data.Replace = types.StringNull() // set the value to null, assume that backend did not provide a value

	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
}

func (r *ExampleResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
	var data *ExampleResourceModel
	resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...)
	if resp.Diagnostics.HasError() {
		return
	}

	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
}

Terraform Configuration Files

Initial configuration:

resource "scaffolding_example" "test" {}

Updated config:

resource "scaffolding_example" "test" {
  timeouts = {
    create = "60s"
  }
}

Debug Output

Expected Behavior

Adding a timeout section should trigger in-place upgrade

Actual Behavior

The resource was planned to be replaced

Steps to Reproduce

Code based on terraform-provider-scaffolding with test case demonstrating the behavior:
wojciechwroblewski/terraform-provider-scaffolding-framework@6eeeb98

References

Not sure if the issue is exclusive to the timeouts block, just discovered that while implementing them.
Using stringplanmodifier.RequiresReplaceIfConfigured() instead of RequiresReplace() for replace attribute solves the issue, but I'd rather avoid it as it may result in configuration drift.

@wojciechwroblewski wojciechwroblewski added the bug Something isn't working label Jul 14, 2023
@bflad
Copy link
Member

bflad commented Jul 14, 2023

Hi @wojciechwroblewski 👋 Thank you for raising this and sorry you ran into trouble here. I'm not sure if there would be anything specific to timeouts from terraform-plugin-framework-timeouts -- I think this behavior would be reproducible with any attribute change that causes a resource update.

Running with your example, we can see the following human readable plan:

  | Terraform used the selected providers to generate the following execution
  | plan. Resource actions are indicated with the following symbols:
  | -/+ destroy and then create replacement
  |
  | Terraform will perform the following actions:
  |
  |   # framework_timeouts.test must be replaced
  | -/+ resource "framework_timeouts" "test" {
  |       ~ id       = "test" -> (known after apply)
  |       + replace  = (known after apply) # forces replacement
  |       + timeouts = {
  |           + create = "120s"
  |         }
  |     }
  |
  | Plan: 1 to add, 0 to change, 1 to destroy.

Since the replace attribute is marked as Computed (potentially configured from the provider/API rather than the Terraform configuration) and since the Terraform configuration value is null, the framework's planning logic will automatically set its value as unknown in the plan. This is intentional to prevent practitioners and provider developers from constantly dealing with Terraform data consistency errors which would not be avoidable with released providers, instead leaving the burden on provider developers to "enhance"/"clean up" the plan by removing unknown values if they happen to be known.

Typically using the UseStateForUnknown plan modifier could be used for preserving a prior state value instead of being unknown, however (as coded in the example code given anyways) this attribute has the property that it might have a null value expected to be stored in state as well. The UseStateForUnknown plan modifier is explicitly coded to skip handling if the prior state is null so it does not force the value to null during resource creation. It would require a slight variation of that logic to also support preserving null values from prior state, while still being unknown during resource creation (I don't think it would be safe to change the existing logic without potentially causing a compatibility issue for existing providers).

Taking a step back though it might be helpful to understand more about your goals with this particular attribute and its expected behaviors. Is this intended to be a practitioner controlled "replace me" attribute, an API controlled "replace me" attribute, or a combination of the two?

@wojciechwroblewski
Copy link
Author

Thanks for quick reply.

I could have found a better name for this attribute, but "replace me" attribute in my scenario is a value that can be set by the user, but may also be set by the API if left unconfigured. The API returns an empty string if the request did not contain a value for replace attribute. Changing the value requires replace action, as the API has no update method.

I'm trying to add a timeouts attribute to the resource, but also avoid the need to replace existing resources that are already deployed using the provider. If I understand that correctly, the timeouts are only stored in terraform state, so it should not be a problem to do the in-place update. My plan is to implement Update method that will allow updating just the timeouts attribute and add timeout handling to the Create/Delete methods. However, as I tried to demonstrate in the example, currently the action results in replace rather than in-place update.

The UseStateForUnknown plan modifier seems to work around this, but I'm worried if it's safe to use in my scenario. What happens if the value of "replace me" attribute changes server side?

AFAIK in provider-sdk-v2 the same scenario worked without the modifiers, the replace action was triggered only if there was a change in one of the attributes that were marked with ForceNew flag, no matter if the state contained empty values.

@bflad
Copy link
Member

bflad commented Jan 30, 2024

Hi @wojciechwroblewski 👋 I apologize that I missed the question in your above reply, but it has also been quite a long time since this was discussed. Did you try anything out or do you need anything else here?

@bflad bflad added the waiting-response Issues or pull requests waiting for an external response label Jan 30, 2024
@bflad
Copy link
Member

bflad commented Apr 25, 2024

Closing due to lack of response.

@bflad bflad closed this as not planned Won't fix, can't repro, duplicate, stale Apr 25, 2024
@github-actions github-actions bot removed the waiting-response Issues or pull requests waiting for an external response label Apr 25, 2024
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

2 participants