Dirty plan when schema contains a nested computed attribute that also has a computed attribute.

[dimuon]

Terraform Version

1.3.7

Terraform Configuration Files

resource "hashicups_order" "test" {
  items = [
    {
      coffee = {
        id = 2
      }
      quantity = 2
    },
  ]
}

Debug Output

The issue can be reproduced on the TF hashicups example with slight modifications of the schema. Please refer to
https://github.com/dimuon/terraform-provider-hashicups-pf/tree/nested-attributes-issue

Expected Behavior

The plan output should be empty after apply.

Actual Behavior

The plan is always dirty, even if the TF configuration hasn't changed.
The schema of hashicups_order resource defines coffee as a nested Computed and Optional attribute that has Computed attributes (e.g. name):

"coffee": schema.SingleNestedAttribute{
	Description: "Coffee item in the order.",
	Optional:    true,
	// Computed triggers the error
	Computed: true,
	Attributes: map[string]schema.Attribute{
		"id": schema.Int64Attribute{
			Description: "Numeric identifier of the coffee.",
			Required:    true,
		},
		"name": schema.StringAttribute{
			Description: "Product name of the coffee.",
			Computed:    true,
		},
		"teaser": schema.StringAttribute{
			Description: "Fun tagline for the coffee.",
			Computed:    true,
		},
		"description": schema.StringAttribute{
			Description: "Product description of the coffee.",
			Computed:    true,
		},
		"price": schema.Float64Attribute{
			Description: "Suggested cost of the coffee.",
			Computed:    true,
		},
		"image": schema.StringAttribute{
			Description: "URI for an image of the coffee.",
			Computed:    true,
		},
	},
},

Steps to Reproduce

Run make error in https://github.com/dimuon/terraform-provider-hashicups-pf/tree/nested-attributes-issue.
The script starts docker image with hashicups server and runs tests against it.
The TestAccOrderResource fails with order_resource_test.go:10: Step 1/3 error: After applying this test step, the plan was not empty.

Additional Context

No response

References

https://github.com/dimuon/terraform-provider-hashicups-pf/tree/nested-attributes-issue

[jbardin]

Hi @dimuon,

Thanks for filing the issue. The proposed plan here is not generated by Terraform, but by the provider, so there's nothing we can do in this repo to fix the problem. I think rather than going through the provider issue tracker first, you can probably file this directly with the Terraform Plugin Framework, since that is likely the source of the recomputed attributes during the plan.

Thanks

[jbardin]

Hi @dimuon,

I'm actually going to re-open as a core issue for now. The fact that the framework allows a nested attribute to be required in the configuration while inside of a computed value was a bit surprising, but not really related to the behavior here. The current implementation requires some extra work by the provider to handle nested objects inside an optional+computed attribute, but we might be able to precompute those during the plan so that this "just works" in most cases.

[dimuon]

Hi @jbardin,

Thank you for the prompt response.

It looks like the issue is caused by the fact that TF client doesn't include the current state values in ProposedNewState for attributes that are Computed in that call - github.com/hashicorp/[email protected]/internal/proto6server/server_planresourcechange.go#L52. I didn't debug what actually comes in MsgPack so it could be that the Framework decode it incorrectly but if the attribute isn't Computed, it works as expected.

Please compare 2 attached screenshots.

The non-computed attribute behavior (the correct one):

The computed attribute behavior:

The only diff between these two cases is the following change in the schema:

						"coffee": schema.SingleNestedAttribute{
							Description: "Coffee item in the order.",
							Required:    true,
							// Optional: true,
							// Computed triggers the error
							// Computed: true,

The error happens when Computed is enabled for coffee attribute.

Also please mind the fact that non-computed attributes, e.g. id behaves as expected (you can see the correct value of id attribute on the second screenshot).

[bflad]

@dimuon if you'd like to extract the MessagePack data from the protocol, you can use the TF_LOG_SDK_PROTO_DATA_DIR environment variable during a Terraform execution or provider acceptance testing. This will instruct the lower level terraform-plugin-go Go module on the provider side to write files for each of the RPC request and response data structures, e.g.

1673965915816_ValidateProviderConfig_Request_Config.msgpack
1673965915817_ValidateResourceConfig_Request_Config.msgpack
1673965915847_ValidateProviderConfig_Request_Config.msgpack
1673965915848_ConfigureProvider_Request_Config.msgpack
1673965915881_UpgradeResourceState_Response_UpgradedState.msgpack
1673965915882_ValidateResourceConfig_Request_Config.msgpack
1673965915883_PlanResourceChange_Request_Config.msgpack
1673965915883_PlanResourceChange_Request_PriorPrivate.empty
1673965915883_PlanResourceChange_Request_PriorState.msgpack
1673965915883_PlanResourceChange_Request_ProposedNewState.msgpack
1673965915883_PlanResourceChange_Request_ProviderMeta.empty
1673965915884_PlanResourceChange_Response_PlannedPrivate.empty
1673965915884_PlanResourceChange_Response_PlannedState.msgpack

You can then use tooling such as https://github.com/wader/fq to inspect the data:

$ fq -d msgpack tovalue 1673965915883_PlanResourceChange_Request_ProposedNewState.msgpack
{
  "gap0": "-23 09:27:50 EST",
  "length": 3,
  "pairs": [
    {
      "key": {
        "length": 2,
        "type": "fixstr",
        "value": "id"
      },
      "value": {
        "length": 1,
        "type": "fixstr",
        "value": "5"
      }
    },
    {
      "key": {
        "length": 5,
        "type": "fixstr",
        "value": "items"
      },
      "value": {
        "elements": [
          {
            "length": 2,
            "pairs": [
              {
                "key": {
                  "length": 6,
                  "type": "fixstr",
                  "value": "coffee"
                },
                "value": {
                  "length": 6,
                  "pairs": [
                    {
                      "key": {
                        "length": 11,
                        "type": "fixstr",
                        "value": "description"
                      },
                      "value": {
                        "type": "nil",
                        "value": null
                      }
                    },
                    {
                      "key": {
                        "length": 2,
                        "type": "fixstr",
                        "value": "id"
                      },
                      "value": {
                        "type": "positive_fixint",
                        "value": 1
                      }
                    },
                    {
                      "key": {
                        "length": 5,
                        "type": "fixstr",
                        "value": "image"
                      },
                      "value": {
                        "type": "nil",
                        "value": null
                      }
                    },
                    {
                      "key": {
                        "length": 4,
                        "type": "fixstr",
                        "value": "name"
                      },
                      "value": {
                        "type": "nil",
                        "value": null
                      }
                    },
                    {
                      "key": {
                        "length": 5,
                        "type": "fixstr",
                        "value": "price"
                      },
                      "value": {
                        "type": "nil",
                        "value": null
                      }
                    },
                    {
                      "key": {
                        "length": 6,
                        "type": "fixstr",
                        "value": "teaser"
                      },
                      "value": {
                        "type": "nil",
                        "value": null
                      }
                    }
                  ],
                  "type": "fixmap"
                }
              },
              {
                "key": {
                  "length": 8,
                  "type": "fixstr",
                  "value": "quantity"
                },
                "value": {
                  "type": "positive_fixint",
                  "value": 2
                }
              }
            ],
            "type": "fixmap"
          }
        ],
        "length": 1,
        "type": "fixarray"
      }
    },
    {
      "key": {
        "length": 12,
        "type": "fixstr",
        "value": "last_updated"
      },
      "value": {
        "length": 15,
        "type": "fixstr",
        "value": "Tuesday, 17-Jan"
      }
    }
  ],
  "type": "fixmap"
}

Using this we are able to see from the protocol data that core is copying the configuration null values (not prior state) into the proposed new state for the underlying Computed only attributes.

Given the current core logic, this is expected, but as @jbardin notes there may be some potential to change that in this specific scenario: an Optional + Computed nested attribute that is configured with Computed-only underlying attributes. Depending on the issue prevalence, we can also potentially consider a change on the framework side so it can match the core behavior to cover Terraform versions prior to the potential change.

[dimuon]

@bflad , @jbardin , thank you for the prompt responses - much appreciated. Also thank you for the tip regarding MessagePack decoding.

Does the answer mean that, e.g. we cannot use Computed and Optional types.Set as a nested attribute with Computed underlying attributes (like the schema below) due to the current core logic?

"topology": tfsdk.Attribute{
		Optional:    true,
		Computed:    true,
		Attributes: tfsdk.SetNestedAttributes(map[string]tfsdk.Attribute{
			"id": {
				Type:        types.StringType,
				Required:    true,
			},
			"instance_configuration_id": {
				Type:        types.StringType,
				Computed:    true,
			},
			"size": {
				Type:        types.StringType,
				Computed:    true,
				Optional:    true,
			},
		}),
}

[dimuon]

OK, so from what I see on my local, the mentioned schema for topology attribute doesn't work for the same reason - if TF configuration doesn't explicitly set size, TF client doesn't provide the current state value for size in fwReq.ProposedNewState so the Framework "concludes" that the planned new state differs from the prior state and the plan will be dirty even if there is no change in the TF configuration.

It means, e.g., that the provider cannot return set with another cardinality after apply.

[dimuon]

Actually, it looks like in case of SetNestedAttributes, the TF client doesn't use the current state value in fwReq.ProposedNewState even if topology attribute is Required.

[dimuon]

Just a note - it looks like the proposed fix also works for nested maps if a map is computed and optional and contains computed and optional underlying attributes.

[dimuon]

@jbardin , thanks for the fix. How do you think when we can expect it to be released? I guess it won't be in v1.4.0, correct?

[crw]

@dimuon if that PR referenced above fixes the issue, then it should appear in v1.4.x and alpha releases for 1.4.

[github-actions]

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.