Read-only computed attribute appears in every plan when adjacent to an optional+computed SetNestedAttribute or ListNestedAttribute with no default

[henryrecker-pingidentity]

Module version

1.4.2

Relevant provider source code

func (r *unexpectedPlanExampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"readonly": schema.StringAttribute{
				Optional: false,
				Computed: true,
			},
			"set": schema.SetNestedAttribute{
				Optional: true,
				Computed: true,
                                 // no default set in schema
				PlanModifiers: []planmodifier.Set{
					setplanmodifier.UseStateForUnknown(),
				},
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"name": schema.StringAttribute{
							Required: true,
						},
					},
				},
			},
		},
	}
}

func (r *unexpectedPlanExampleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
	var model unexpectedPlanExampleResourceModel
	req.Plan.Get(ctx, &model)

	// Set the same value every time for the string
	model.Readonly = types.StringValue("examplereadonly")

	// Set the same value every time for the set. {{"name": "examplename"}}
	setResult := []attr.Value{}
	setVal, _ := types.ObjectValue(map[string]attr.Type{
		"name": types.StringType,
	},
		map[string]attr.Value{
			"name": types.StringValue("examplename"),
		})
	setResult = append(setResult, setVal)
	model.Set, _ = types.SetValue(types.ObjectType{AttrTypes: map[string]attr.Type{
		"name": types.StringType,
	}}, setResult)

	resp.State.Set(ctx, model)
}

func (r *unexpectedPlanExampleResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
	resp.State.Raw = req.State.Raw
}

Terraform Configuration Files

resource "example_unexpected_plan_example" "myExamplePlan" {
  // both attributes are computed
}

Expected Behavior

Read-only computed attribute should not result in a plan if no other attributes are changed.

Actual Behavior

The following plan is always produced on a plan after the initial create.

Terraform will perform the following actions:

  # example_unexpected_plan_example.myExamplePlan will be updated in-place
  ~ resource "example_unexpected_plan_example" "myExamplePlan" {
      ~ readonly = "examplereadonly" -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

When I instead change the "set" attribute to be a SetAttribute of strings rather than a SetNestedAttribute, it works as expected, with no plan being produced after the create. I also tried a simple StringAttribute rather than a set and that worked as expected as well. Only SetNestedAttribute and ListNestedAttribute seems to cause this.

I also found that if a default is set in the schema for the set/list, then it stops generating this plan.

Steps to Reproduce

1. terraform apply to create the resource
2. terraform plan to see the generated plan

[bflad]

Hi @henryrecker-pingidentity 👋 Thank you for reporting this and sorry you are running into trouble here. Much appreciated for the clear reproduction case as I was able to observe the same behavior.

Enabling TF_LOG=TRACE, you can start to see some of the reasoning why this is happening during the PlanResourceChange RPC:

2023-12-28T13:54:38.317-0500 [DEBUG] sdk.framework: Detected value change between proposed new state and prior state: tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898 tf_attribute_path=set
2023-12-28T13:54:38.317-0500 [DEBUG] sdk.framework: Marking Computed attributes with null configuration values as unknown (known after apply) in the plan to prevent potential Terraform errors: tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898
2023-12-28T13:54:38.317-0500 [DEBUG] sdk.framework: marking computed attribute that is null in the config as unknown: tf_attribute_path="AttributeName(\"readonly\")" tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898
2023-12-28T13:54:38.317-0500 [DEBUG] sdk.framework: marking computed attribute that is null in the config as unknown: tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898 tf_attribute_path="AttributeName(\"set\")" tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8
2023-12-28T13:54:38.317-0500 [TRACE] sdk.framework: At least one Computed null Config value was changed to unknown: tf_resource_type=framework_898 tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange
// ...
2023-12-28T13:54:38.317-0500 [TRACE] sdk.framework: Calling provider defined planmodifier.Set: tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898 tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_attribute_path=set description="Once set, the value of this attribute in state will not change."
2023-12-28T13:54:38.317-0500 [TRACE] sdk.framework: Called provider defined planmodifier.Set: tf_provider_addr=registry.terraform.io/hashicorp/framework tf_rpc=PlanResourceChange tf_resource_type=framework_898 tf_req_id=87c88351-0cc6-f628-da88-a434a07d05b8 tf_attribute_path=set description="Once set, the value of this attribute in state will not change."

Using TF_LOG_SDK_PROTO_DATA_DIR and fq -d msgpack torepr FILE shows that in the protocol level data during the PlanResourceChange RPC, Terraform proposed a new state with a null value for the set SetNestedAttribute and the framework (according to the logs above) did actually preserve the prior state set value in the response, but marked readonly as unknown:

$ fq -d msgpack torepr 1703789678317_PlanResourceChange_Request_PriorState.msgpack
{
  "readonly": "examplereadonly",
  "set": [
    {
      "name": "examplename"
    }
  ]
}

$ fq -d msgpack torepr 1703789678317_PlanResourceChange_Request_ProposedNewState.msgpack
{
  "readonly": "examplereadonly",
  "set": null
}

$ fq -d msgpack torepr 1703789678317_PlanResourceChange_Response_PlannedState.msgpack
{
  "readonly": "\u0000", // torepr representation of unknown
  "set": [
    {
      "name": "examplename"
    }
  ]
}

This matches the human-readable plan output and all of these behaviors are currently expected in the latest framework implementation details. In particular the "if the plan differs from the current resource state" part of step 2 there, according to the data:

If the plan differs from the current resource state, the framework marks computed attributes that are null in the configuration as unknown in the plan. This is intended to prevent unexpected Terraform errors. Providers can later enter any values that may be known.

What is interesting and I'll double check is a SetAttribute. It should behave the same from Terraform's perspective in the protocol data, but if you are seeing differently and I can reproduce, I have a hunch that this is something that needs to be fixed in both Terraform and the framework. More on that after I try out SetAttribute though and confirm that hunch.

[bflad]

Okay, hunch confirmed, as suspected Terraform's data handling behavior is indeed different:

			"set": schema.SetAttribute{
				Optional: true,
				Computed: true,
				// no default set in schema
				PlanModifiers: []planmodifier.Set{
					setplanmodifier.UseStateForUnknown(),
				},
				ElementType: types.StringType,
			},

$ fq -d msgpack torepr 1703791686878_PlanResourceChange_Request_PriorState.msgpack
{
  "readonly": "examplereadonly",
  "set": [
    "examplename"
  ]
}

$ fq -d msgpack torepr 1703791686878_PlanResourceChange_Request_ProposedNewState.msgpack
{
  "readonly": "examplereadonly",
  "set": [ // not null!
    "examplename"
  ]
}

$ fq -d msgpack torepr 1703791686878_PlanResourceChange_Response_PlannedState.msgpack
{
  "readonly": "examplereadonly",
  "set": [
    "examplename"
  ]
}

Presumably, Terraform should behave the same whether its an attribute or a nested attribute. That feels like a Terraform bug, which I will submit if there is not already one.

This leaves us in an awkward position from the framework though. If/when that change occurs in Terraform, it will not apply to environments running an earlier version of Terraform. Presumably we need to catch the situation as it occurs today, to prevent the plan from showing unexpected changes. Back in #783 (comment) I had mentioned:

The longer term option is disregarding Terraform's ProposedNewState data completely and re-implementing core's logic for list/set type alignment to generate our own ProposedNewState data, which was the unspoken plan for #709.

It is probably time to actually write up that issue and prioritize it, but it is going to be quite difficult to implement, unfortunately.

[henryrecker-pingidentity]

Thanks @bflad . For now, I'm working around this issue by putting UseStateForUnknown on the read-only attribute to prevent the unnecessary plans, and then checking for other "real" changes in a ModifyPlan. If there are other changes, I mark the read-only attribute as unknown manually. The manual checking for other real changes can be somewhat painful if there are multiple readonly attributes like this.

[bflad]

@henryrecker-pingidentity I did some spelunking in the Terraform core code that handles this and found that it is detecting the Required: true part of the nested name attribute to prevent sending over the proposed new state data for the whole attribute. Terraform's heuristics in this area, for better or worse, expect to find Computed: true to "preserve" the prior state value in the proposed new state of the plan.

Changing the SetNestedAttribute to the following causes an empty plan after apply:

			"set": schema.SetNestedAttribute{
				Optional: true,
				Computed: true,
				// no default set in schema
				PlanModifiers: []planmodifier.Set{
					setplanmodifier.UseStateForUnknown(),
				},
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"name": schema.StringAttribute{
							Optional: true,
							Computed: true,
						},
					},
				},
			},

Since it was previously Required: true, but now cannot be as part of this workaround, I did verify that you can use a attribute-based validator to reimplement the behavior of the configuration of that attribute being required, but only when the object is not null:

"set": schema.SetNestedAttribute{
				Optional: true,
				Computed: true,
				// no default set in schema
				PlanModifiers: []planmodifier.Set{
					setplanmodifier.UseStateForUnknown(),
				},
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"name": schema.StringAttribute{
							// Required: true,
							Optional: true,
							Computed: true,
							Validators: []validator.String{
								StringNotNull(),
							},
						},
					},
				},
			},

Where that validator looks something like the below (sorry, this is not something immediately available in terraform-plugin-framework-validators today):

package provider

import (
	"context"

	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
)

var _ validator.String = StringNotNullValidator{}

// StringNotNullValidator validates that an attribute is not null. Most
// attributes should set Required: true instead, however in certain scenarios,
// such as a computed nested attribute, all underlying attributes must also be
// computed for planning to not show unexpected differences.
type StringNotNullValidator struct{}

// Description describes the validation in plain text formatting.
func (v StringNotNullValidator) Description(_ context.Context) string {
	return "value must be configured"
}

// MarkdownDescription describes the validation in Markdown formatting.
func (v StringNotNullValidator) MarkdownDescription(ctx context.Context) string {
	return v.Description(ctx)
}

// Validate performs the validation.
func (v StringNotNullValidator) ValidateString(ctx context.Context, req validator.StringRequest, resp *validator.StringResponse) {
	if !req.ConfigValue.IsNull() {
		return
	}

	resp.Diagnostics.AddAttributeError(
		req.Path,
		"Missing Attribute Value",
		req.Path.String()+": "+v.Description(ctx),
	)
}

// StringNotNull returns an validator which ensures that the string attribute is
// configured. Most attributes should set Required: true instead, however in
// certain scenarios, such as a computed nested attribute, all underlying
// attributes must also be computed for planning to not show unexpected
// differences.
func StringNotNull() validator.String {
	return StringNotNullValidator{}
}

Which does nothing when the entire attribute is computed, but triggers the following error when an object exists without the attribute:

resource "framework_898" "test" {
  set = [{}]
}

        Error: Missing Attribute Value
        
          with framework_898.test,
          on terraform_plugin_test.tf line 12, in resource "framework_898" "test":
          12: 					set = [{}]
        
        set[Value({"name":<null>})].name: value must be configured

Maybe that is a less painful workaround for you.

[ThomasRooney]

Thanks @bflad for the in depth analysis!

It appears that SingleNestedAttribute works in a similar way -- having an Optional: true+Computed: true parent but with a Required: true child within, or a Optional: true, Computed: false child causes an unnecessary plan when nothing has changed.

Thanks @bflad . For now, I'm working around this issue by putting UseStateForUnknown on the read-only attribute to prevent the unnecessary plans, and then checking for other "real" changes in a ModifyPlan. If there are other changes, I mark the read-only attribute as unknown manually. The manual checking for other real changes can be somewhat painful if there are multiple readonly attributes like this.

I also previously worked around this in the Speakeasy provider generator by generating a plan modifier that effectively checked "if there are no changes in the entire resource's state and plan (excluding Null <=> Unknown), then propagate prior state for this attribute into current plan state", however this had an error case when a defined element was removed from a plan.

E.g. the transition:

resource "my_resource" "my_example" {
  name = "test"
  paused = true
}

to:

resource "my_resource" "my_example" {
  name = "test"
}

Would not result in a change. I couldn't work out a way to write a plan modifier that would detect this. This was my attempt

However I can confirm that changing Required: true into a NotNull() validator works for all (bool, float64, int64, maplist, map, number, object, set, string) data types worked. I rewrote your example above into one for each object type, e.g. https://github.com/speakeasy-sdks/terraform-provider-hashicups/blob/chore/validators/internal/validators/boolvalidators/not_null.go

[bflad]

@ThomasRooney 👋 Thanks so much for confirming! Given the current situation that folks can unknowingly find themselves in, I think our best course of action for now might be to introduce out-of-the-box validators and then consider raising an implementation error in the framework. If you are interested in contributing those validators, I created hashicorp/terraform-plugin-framework-validators#185 and your example implementation there matches the implementation expectations for that Go module, just with some additional unit testing.

Aside: I'll also create a separate issue in the terraform-plugin-framework-validators Go module to consider migrating those validators into the framework Go module proper. We had separated out that Go module originally because we were not sure how the ecosystem would evolve with all the new framework validation concepts, since we really wanted developers to use custom types when possible, and wanted to leave the room for separate versioning of the declarative validators. Over a year after the framework v1.0.0 release now though, the validators Go module has been surprisingly stable. I hesitate to say that everything will move over as-is because another option we do have in some of those cases is introducing some of the very common use case validations into the base types themselves, but that sort of discussion can occur in that issue.

[akinross]

@bflad are there any updates regarding fixing this issue?

[bflad]

Hi @akinross 👋 Thanks for reaching out -- I'm no longer at HashiCorp, but the maintainer team might be able to provide any status update.

[akinross]

Hi @bflad, Thanks for the update and all the best outside of HashiCorp. Will wait on reply from the maintainer team.