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

Improve Ergonomics of Setting Default Values #516

Closed
jaloren opened this issue Oct 18, 2022 · 4 comments
Closed

Improve Ergonomics of Setting Default Values #516

jaloren opened this issue Oct 18, 2022 · 4 comments
Labels
enhancement New feature or request
Milestone

Comments

@jaloren
Copy link

jaloren commented Oct 18, 2022

Note improving the ergonomics may just improving the documentation. If the docs already exist and I miss them, then this is user error on my part and apologies for wasting this team's time in advance. I spent several days trying to figure out how to do this and I am not sure my solution is "correct" though it seems to work. I did review the following before opening this ticket. I also tried to hunt through all open github issues to see if this was a known problem.

See list of open github issues I found relating to this in the References section at the bottom.

Module version

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

I am also testing this terraform 1.0.11. I can test against 1.3.2 if necessary.

Use-cases

I am provisioning infrastructure, where it has a integer property that defaults to 1 if not set on creation. In the infrastructure, this property is never null. An end user may want to override the default so I want this property to be optional in terraform while defaulting to 1 if not set.

I expected the following behavior:

  • if the field is not set in terraform during creation, then the provider supplies the value of 1 using tfsdk.
    AttributePlanModifier and terraform creates the infra where the property is set to one in the infra.
  • if the field is not in the terraform config during update, then the provider supplies the value of 1 using tfsdk.
    AttributePlanModifier and terraform should not flag the resource as needing to be updated.
  • if the user adds the field in the terraform config during either creation or update, the provider does not supply
    a value and terraform creates/updates the resource to have the user supplied value.
  • if the user adds the field in the terraform config, successfully performs a terraform apply and then removes
    the field from the config, then terraform will update the infrastructure to use the default value since the user-supplied
    value no longer exists.

Attempted Solutions

From the go docs for tfsdk.Attribute:

When defining an attribute that has Optional set to true,
and uses PlanModifiers to set a "default value" when none is provided,
Computed must also be set to true. This is necessary because default
values are, in effect, set by the provider (i.e. computed).

Based on this, I believe I needed to do the following:

  • in the schema, the attribute must have optional and computed set to true
  • i need to create an implementation of a plan modifier like what was described in this comment

Here is what the schema looks like:

		Attributes: map[string]tfsdk.Attribute{
			"name": {
				Type:     types.StringType,
				Required: true,
			}, 
			retention_time_in_days": {
				Type:     types.Int64Type,
				Optional: true,
				Computed: true,
				PlanModifiers: []tfsdk.AttributePlanModifier{
					planmodifiers.DefaultAttr(types.Int64{Value: 1}),
				},
			},
		},
	}

I doubt it matters so I am omitting the code for the AttributePlanModifier though I can include it if someone thinks
it would be helpful.

This does not work for this part of my use case:

If the user adds the field in the terraform config, successfully performs a terraform apply and then removes
the field from the config, then terraform will update the infrastructure to use the default value since the user-supplied
value no longer exists.

Instead what happens is that terraform does not detect any changes and never updates either terraform state file nor the
infrastructure. The infrastructure thus will have whatever setting was last set in the terraform config before that
setting was removed. This introduces state drift that terraform will never detect.

I believe this is behaving as designed according to the docs:

Proposed New State: Terraform Core uses some built-in logic to perform an initial basic merger of the Configuration and the Prior State which a provider may use as a starting point for its planning operation. The built-in logic primarily deals with the expected behavior for attributes marked in the schema as both "optional" and "computed", which means that the user may either set it or may leave it unset to allow the provider to choose a value instead. Terraform Core therefore constructs the proposed new state by taking the attribute value from Configuration if it is non-null, and then using the Prior State as a fallback otherwise, thereby helping a provider to preserve its previously-chosen value for the attribute where appropriate.

In other words, the property is no longer set so terraform falls back to the prior state which was the value set when the user had last set the property and did a terraform apply. This in turns mean that terraform does not detect a delta between
the state and what is in the config (since the config doesn't have the value). This means that using AttributePlanModifier isn't a good fit.

I then switched from the AttributePlanModifier to the ResourceWithModifyPlan interface. Here is the implementation

func (d *databaseResourceHandler) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) {
	if req.State.Raw.IsNull() || !req.State.Raw.IsKnown() {
		return
	}

	config := &databaseResource{}
	resp.Diagnostics.Append(req.Config.Get(ctx, config)...)

	if !config.DataRetentionTimeInDays.IsNull() {
		return
	}

	plan := &databaseResource{}
	resp.Diagnostics.Append(req.Plan.Get(ctx, plan)...)
	
	if resp.Diagnostics.HasError() {
		return
	}
	
	plan.DataRetentionTimeInDays = types.Int64{
		Value: 1,
	}
	resp.Diagnostics.Append(resp.Plan.Set(ctx, plan)...)
}

This works in that it addresses all the requirements in my use case. However, I am unsure if this is correct nor whether
this is the only or most ergonomic way to handle defaults given my requirements. In particular, the godoc for ModifyPlan
says this:

Any attribute with a known value must not have its value changed in subsequent calls to ModifyPlan or Create/Read/Update.

The attribute is null in the config but it is not unknown in the plan. The plan populated the attribute from the tf state file. I am intentionally changing a known value. Does this mean I am violating the API? Based on the docs, it might be yes. Or is this saying you may modify this known value one time during a modify plan but any subsequent calls you should not. If so, that seems subtle and easy to get wrong.

Proposal

When implementing a provider, convention over configuration is a major contributor to good ergonomics for end users. If you have a resource with 100 properties (looking at you s3 buckets) but you only need to specify two properties and the other 98 have sane defaults, then that reduces the burden on a user configuring a resource. Given that, I'd suggest detailed documentation that explicitly covers how to handle default settings with code examples.

I'd also suggest a single unified interface that handles all situations for handling default values and to significantly reduce boiler plate and footguns around setting those values. Right now it looks we have two interfaces that overlap in terms of their functionality for setting default values and they are general purpose and server a much broader set of use cases.

Given the importance of default values, I'd argue an interface targeted to this specific use case might be worth considering where it would be put on the attribute's schema.

References

@jaloren jaloren added the enhancement New feature or request label Oct 18, 2022
@pecigonzalo
Copy link

Adding a note to this, I found myself searching for a solution to this problem but for the provider settings. Unfortunately PlanModifiers don't seem to work for provider configuration as they execute in Configure, and their req does not have the required attributes.

@pksunkara
Copy link

I built an utility library just for this issue, https://github.com/terraform-community-providers/terraform-plugin-framework-utils and am using it in one of my providers. https://github.com/terraform-community-providers/terraform-provider-linear

@bflad
Copy link
Member

bflad commented Mar 20, 2023

This should be covered by #668 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. 👍

@bflad bflad closed this as completed Mar 20, 2023
@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 Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants