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

Computed attrs are not set to unknown after a AttributePlanModifier causes a non-empty plan #456

Closed
goncalo-rodrigues opened this issue Aug 19, 2022 · 8 comments
Labels
bug Something isn't working
Milestone

Comments

@goncalo-rodrigues
Copy link

Module version

v0.11.1

Relevant provider source code

Resource has a computed field (e.g. timestamp) that might be changed upon any update. Terraform normally sets it to unknown on any plan (correctly) if there's anything to update.

tfsdk.Schema{
    "timestamp": {
        ....
	Computed:    true,
    },
    "name": {
        ....
	Required:    true,
    },
    "status": {
       	Computed:      true,
	PlanModifiers: []tfsdk.AttributePlanModifier{modifiers.ResourceStatusModifier{}},
    }
}

However, if the plan is changed from an empty plan to a required update due to an AttributePlanModifier, it is no longer set to unknown.

func (m ResourceStatusModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, resp *tfsdk.ModifyAttributePlanResponse) {
	resp.AttributePlan = types.String{Value: "Running"}
}

Inconsistent planning

If update is triggered by a user change to a config, we get this plan:

resource test "test" {
    timestamp = "2022-08-19T10:29:57+00:00" -> (known after apply)
    name = "test1" -> "test2"
    status = "STOPPED" -> "RUNNING"
}

If instead the user changed nothing, but a plan is still created due to the AttributePlanModifier, we get:

resource test "test" {
    status = "STOPPED" -> "RUNNING"
}

which generates the error after the apply is successful:


│ Error: Provider produced inconsistent result after apply

│ When applying changes to test.test, provider "provider["hashicorp.com/dev/multy"]" produced an unexpected new value: timestamp: was cty.StringVal("2022-08-19T10:29:57+00:00"), but now cty.StringVal("2022-08-19T10:33:51+00:00").

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

So, my question is: why does this differ in these 2 cases? Is this a known bug? Is there a workaround?

@goncalo-rodrigues goncalo-rodrigues added the bug Something isn't working label Aug 19, 2022
@bflad
Copy link
Member

bflad commented Aug 25, 2022

Hi @goncalo-rodrigues 👋 Thank you for raising this and sorry this caused you some trouble.

The behavior you mention is an artifact of the existing internal implementation of resource planning in the framework, where the following steps are executed if the resource is not planned for destruction:

  • If the entire plan does not match the prior state, mark Computed attributes with null configuration as unknown to prevent Terraform errors similar to the one you mention
  • Run schema-based plan modifiers
  • Run resource-level plan modification

Since you have an attribute plan modifier which always marks the planned attribute value as changed to a new value and since schema-based plan modification happens after the unknown marking, its expected other Computed attributes would be missing the unknown handling, given the current framework implementation. There have been some debates whether the schema-based plan modifiers and resource-level plan modification should also run before the unknown marking, but the use cases that might require that behavior have not been clear, so it has been left in its current state for awhile now.

This is where it would be great to understand your use case a little more and expected behavior of Terraform/the provider in this case. Are you wanting every Terraform execution to show a planned/drift difference? If not, you should be able to protect the attribute plan modifier from changing the value every execution with if req.Plan.Raw.Equal(req.State.Raw) { return } beforehand right now.

The details are a little hazy at the moment (I apologize) but I thought Terraform would only check a resource plan if a resource had a configuration change. If that's not the case, then I'm guessing Terraform would check a no-configuration-change resource plan to support how the prior provider development SDK was implemented so diagnostics could be raised in certain validation scenarios as those prior validators did not have access to the entire configuration. As mentioned above, the framework does not protect schema-based plan modification or resource-level plan modification from a no-operation plan currently. The schema-based validation in this framework might have potentially offset the need for this, so we could potentially change this behavior. There may be use cases that I'm forgetting though, so I would be hesitant to rush to change this framework planning behavior.

Hope this makes sense and thanks again for raising this.

@bflad
Copy link
Member

bflad commented Aug 25, 2022

Just to cross-reference an existing issue for this: #183

@goncalo-rodrigues
Copy link
Author

  • If the entire plan does not match the prior state, mark Computed attributes with null configuration as unknown to prevent Terraform errors similar to the one you mention
  • Run schema-based plan modifiers
  • Run resource-level plan modification

Then it makes perfect sense on why it is happening.

I don't want to do this logic if req.Plan.Raw.Equal(req.State.Raw) { return } as I'm explicitly looking for things that have changed in the state against a planned attribute that is always fixed to null.

What I'm trying to implement is something like drift detection for attributes outside what's declared in Terraform schema.

For example, let's say a VM has a status that can be one of [crashed, running]. By default, the status is running and this cannot be set by the user. If the VM ends up being in the "crashed" status, I want to notify the user when they try to do terraform plan and allow them to call terraform apply to make sure the VM is running.

My specific use case is a bit more nuanced. We have a bunch of attributes that we don't track (in aws, azure and gcp resources) but they have to be set in a certain way. If they get changed by whatever reason then we have an attribute that tracks this drift called "resource_status". Resource status is null if everything is as expected, but if there's any drift then it gets changed to "needs_update".

bflad added a commit that referenced this issue Aug 30, 2022
…ch has modified and unmodified Computed attributes (#463)

Reference: #183
Reference: #456
@bflad bflad added this to the v1.2.0 milestone Jan 13, 2023
@MrFishFinger
Copy link

Hi!

I have a similar situation to @goncalo-rodrigues :

  • I have a last_updated Computed attribute, that stores a timestamp, which is updated to the current time, whenever the resource Update function is run (like in the tutorial)
  • I have a disabled Bool Optional+Computed attribute, that uses a planmodifier to set a default value.

The issue I am facing:

if the disabled Optional attribute is removed from the resource TF config file, then I get this error for the last_updated attribute when doing an Update:

│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to redacted.resource_example2, provider "provider[\"redacted\"]" produced an unexpected new value: .last_updated: was cty.StringVal("2023-02-22_18-02-58"), but now cty.StringVal("2023-02-22_18-09-38").

This appears to be related to using a planmodifier to set a default value on the disabled attribute.


If i understand what has been discussed already, the reason this error occurs, is because:

  • a computed attribute needs to be marked as "unknown" for it to be updated during an Update
  • when using a planmodifier for an Optional attribute, it effectively breaks this "unknown-marking" behaviour for all other Computed attributes
  • therefore, any computed attributes that change their value as part of an Update, will cause the "inconsistent result" error

Is that correct?


afaik, the workaround if req.Plan.Raw.Equal(req.State.Raw) { return } you suggested @bflad will not help us here.

Is there something else that I could try?

thanks! 🙂

@MrFishFinger
Copy link

MrFishFinger commented Feb 22, 2023

for my usecase, i was able to workaround this to some extent by implementing the ModifyPlan interface, and manually set the LastUpdated attribute to unknown, whenever the attribute Disabled is unset. (and not use a planmodifier in the schema at all)

but it seems pretty cumbersome.... is there a better way?

func (r *iamRoleResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
	handleOptionalBoolAttribute(ctx, req, resp, "disabled", false)
	handleOptionalBoolAttribute(ctx, req, resp, "blah", false)
	handleOptionalBoolAttribute(ctx, req, resp, "blob", false)
}

func handleOptionalBoolAttribute(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse, attribute string, defaultValue bool) {
	var valFromConfig, valFromState types.Bool
	req.Config.GetAttribute(ctx, path.Root(attribute), &valFromConfig)
	req.State.GetAttribute(ctx, path.Root(attribute), &valFromState)

	if valFromConfig.IsNull() && valFromState != types.BoolValue(defaultValue) {
		resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root(attribute), types.BoolValue(defaultValue))...)
		resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, path.Root("last_updated"), types.StringUnknown())...)
	}
}

@bflad
Copy link
Member

bflad commented Mar 20, 2023

This should be covered by #668, which introduces new resource default value handling before the unknown marking, which will release with terraform-plugin-framework version 1.2.0 🔜 . If there are further bug reports, feature requests, or documentation suggestions with resource default value handling, please raise a new issue. 👍

@MrFishFinger you may also be looking for something like #605 if the default value handling does not cover your use case completely

@bflad bflad closed this as completed Mar 20, 2023
@MrFishFinger
Copy link

thanks @bflad , the switch to terraform-plugin-framework version 1.2.0 and then using the new Default attribute field resolved this issue for us.

more concretely, i removed our custom PlanModifier, and replaced it with Default: booldefault.StaticBool(false).

See below for detail:

// BEFORE
Attributes: map[string]schema.Attribute{
	"disabled": schema.BoolAttribute{
		Description: "Flag to disable object.",
		Optional:    true,
		Computed:    true,
		PlanModifiers: []planmodifier.Bool{
			attribute_plan_modifier_bool.DefaultValue(types.BoolValue(false)),
		},
	},
	...
}


// AFTER
Attributes: map[string]schema.Attribute{
	"disabled": schema.BoolAttribute{
		Description: "Flag to disable object.",
		Optional:    true,
		Computed:    true,
		Default:     booldefault.StaticBool(false),
	},
	...
}

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants