terraform-plugin-framework icon indicating copy to clipboard operation
terraform-plugin-framework copied to clipboard

Issues with Nested Set Attributes with PlanModifiers

Open megan07 opened this issue 3 years ago • 6 comments

Module version

github.com/hashicorp/terraform-plugin-framework v0.6.1

Relevant provider source code

			"members": {
				Description: "The members of the group",
				Optional:    true,
				Computed:    true,
				Attributes: tfsdk.SetNestedAttributes(map[string]tfsdk.Attribute{
					"email": {
						Description: "...",
						Type:     types.StringType,
						Required: true,
					},
					"role": {
						Description: "...",
						Type:     types.StringType,
						Optional: true,
						Computed: true,
						PlanModifiers: []tfsdk.AttributePlanModifier{
							DefaultModifier{
								DefaultVal: "MEMBER",
								ValType:    types.StringType,
							},
						},
						Validators: []tfsdk.AttributeValidator{
							stringInSliceValidator{
								stringOptions: []string{"MANAGER", "MEMBER", "OWNER"},
							},
						},
					},
					"type": {
						Description: "...",
						Type:     types.StringType,
						Optional: true,
						Computed: true,
						PlanModifiers: []tfsdk.AttributePlanModifier{
							DefaultModifier{
								DefaultVal: "USER",
								ValType:    types.StringType,
							},
						},
						Validators: []tfsdk.AttributeValidator{
							stringInSliceValidator{
								stringOptions: []string{"CUSTOMER", "GROUP", "USER"},
							},
						},
					},
					"delivery_settings": {
						Description: "...",
						Type:     types.StringType,
						Optional: true,
						Computed: true,
						PlanModifiers: []tfsdk.AttributePlanModifier{
							DefaultModifier{
								DefaultVal: deliverySettingsDefault,
								ValType:    types.StringType,
							},
						},
						Validators: []tfsdk.AttributeValidator{
							stringInSliceValidator{
								stringOptions: []string{"ALL_MAIL", "DAILY", "DIGEST", "DISABLED", "NONE"},
							},
						},
					},
					"status": {
						Description: "Status of member.",
						Type:        types.StringType,
						Computed:    true,
					},
					"member_id": {
						Description: "...",
						Type:        types.StringType,
						Computed:    true,
					},
				}, tfsdk.SetNestedAttributesOptions{}),
			},

Terraform Configuration Files

resource "googleworkspace_group_members" "my-group-members" {
 group_id = googleworkspace_group.my-group.id

  members = [{
    email = googleworkspace_user.my-new-user.primary_email
  }]
}

Debug Output

Expected Behavior

When the Default Modifier runs it checks if the attribute is Unknown and all the nested attributes should be Unknown in this case, since they aren't set in the config.

Actual Behavior

All the attributes are coming into the plan modifier like so: {Value: , Unknown: false, Null: true} so the default doesn't get set.

Steps to Reproduce

terraform apply

References

Digging into it I see this is where it's happening: When planning each attribute elem > Get the attribute plan > Get the terraform value at path > Walk the path > Apply Terraform 5 Path Step

That's where I got stuck, I know the len(diffs) should equal 0 but it equals 1

megan07 avatar Apr 18 '22 20:04 megan07

Hey @megan07 👋 Are you able to provide a trace log? The markComputedNilsAsUnknown transformation function that gets called during plan modification might provide some clues here.

bflad avatar Apr 22 '22 20:04 bflad

For what it is worth, checking for a null configuration value might be safest to determine whether a default value should be applied to an attribute -- only checking for a plan value of unknown may come from a configuration reference to another unknown value.

bflad avatar Apr 22 '22 20:04 bflad

Well, I made a bunch of changes last week - and I hardly remember what I was doing here - which goes to show I was not near as thorough as I thought I was when creating this ticket. Sorry about that! I don't think I'm running into the same error as before? - But I am running into this now:

        Error: Provider produced invalid plan

        Provider "registry.terraform.io/hashicorp/googleworkspace" planned an invalid
        value for googleworkspace_group_members.my-group-members.members: count in
        plan (4) disagrees with count in config (1).

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

Gist: https://gist.github.com/megan07/67f6c80765053908f52c781cb0998ad4

I think what is happening is that with each attribute change, an extra member is added to the list, rather than modifying the same member, does that make sense?

megan07 avatar Apr 25 '22 20:04 megan07

If you do the run with TF_LOG_SDK_PROTO_DATA_DIR set, you should be able to see the result of the PlanResourceChange response plan. This might help troubleshoot what's happening a little easier.

Offhandedly, markComputedNilsAsUnknown doesn't have a SetNestedAttributes unit test. We could also see if adding some assertions there does what we would expect with that type of schema setup. Set handling is weird because the entire value is considered part of the attribute path. It could be that when we encounter a set type there, we need to do some special logic to ensure we're transforming the value properly (e.g. deleting the old and saving the new, once).

bflad avatar Apr 27 '22 20:04 bflad

:wave: Greetings!

I've spent a bit of time debugging this issue in the same configuration, having a PlanModifier on an attribute in a SetNestedAttributes. I might be completely wrong but maybe it can be of help as well.

My feeling is that, after applying the PlanModifiers for _, planModifier := range a.PlanModifiers {, the plan is updated with the result:

https://github.com/hashicorp/terraform-plugin-framework/blob/f0051665855e2a32b3211f2daae814286e2d09ca/internal/fwserver/attribute_plan_modification.go#L85

But, to know which value to update, there is the req.AttributePath which contains the steps to the value. In this path, you will notice several AttributePathStep and the one pointing to the set element will be a ElementKeyValue. This represents the current set item and is a copy of the set item (source):

// WithElementKeyValue adds an ElementKeyValue to `a`, using `key` as the
// element's key. `a` is copied, not modified.
func (a *AttributePath) WithElementKeyValue(key Value) *AttributePath {
	steps := a.Steps()
	return &AttributePath{
		steps: append(steps, ElementKeyValue(key.Copy())),
	}
}

And while trying to figure out where is your parent in order to update the value with:

https://github.com/hashicorp/terraform-plugin-framework/blob/f0051665855e2a32b3211f2daae814286e2d09ca/tfsdk/plan.go#L278

parentValue appears to be a typeless value, and I think it's because it cannot find the same value with the KeyValue element in the plan, since this was modified.

As such we go in:

https://github.com/hashicorp/terraform-plugin-framework/blob/f0051665855e2a32b3211f2daae814286e2d09ca/tfsdk/plan.go#L289

Which create an empty parentValue with everything as empty/nullish:

https://github.com/hashicorp/terraform-plugin-framework/blob/f0051665855e2a32b3211f2daae814286e2d09ca/tfsdk/tftypes_value.go#L27-L34

And then are appended to the set:

https://github.com/hashicorp/terraform-plugin-framework/blob/f0051665855e2a32b3211f2daae814286e2d09ca/tfsdk/tftypes_value.go#L198-L200

And that's how you get the mismatch count!

Well, that's what I think is happening at least.

Regarding solutions... Maybe very naively, we could detect before running the PlanModifiers that the parent is a Set, keep a reference to the value the PlanModifier is updating, then after the modifiers, reset the req.AttributePath to replace the KeyValue by the new Set element?

adrien-f avatar Jun 02 '22 15:06 adrien-f

Thought I was experiencing something similar to the OP, now think it's more likely I misunderstood the mechanisms at work. This comment edited into a no-op.

chrismarget-j avatar Jul 25 '22 16:07 chrismarget-j

Hi folks 👋 v0.12.0 will take a new approach to the schema-based plan modification process by stepping through existing data and passing updates up through the call stack instead attempting to do potentially errant path-based lookups and updates at each level. Hopefully this will resolve most of the issues with attribute plan modifiers for set types. The v0.12.0 release should be going out in the coming days.

bflad avatar Sep 08 '22 13:09 bflad

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[bot] avatar Oct 09 '22 02:10 github-actions[bot]