Investigate Consolidation of Schema Required/Optional/Computed

[bflad]

Module version

v0.1.0

Use-cases

In Terraform Plugin SDK and v0.1.0 of this project, there are currently only a few meaningful valid configurations of Required, Optional, and Computed for schema attributes. Switching these to a single field that captures the meaningful valid configurations means there is no way to misconfigure them and it is not losing any configurability. Setting these as false is a perfectly valid implementation, just conventionally shied away from in most current schemas due to its unnecessary verbosity.

Mapping out the three possible boolean field declarations:

• Required: true: ✅
• Required: false: ❌
• Required: true + Optional: true: ❌
• Required: true + Optional: false: 👍 (just verbose)
• Required: false + Optional: true: 👍 (just verbose)
• Required: false + Optional: false: ❌
• Required: true + Computed: true: ❌
• Required: true + Computed: false: 👍 (just verbose)
• Required: false + Computed: true: 👍 (just verbose)
• Required: false + Computed: false: ❌
• Optional: true: ✅
• Optional: false: ❌
• Optional: true + Computed: true: ✅
• Optional: false + Computed: true: 👍 (just verbose)
• Optional: true + Computed: false: 👍 (just verbose)
• Optional: false + Computed: false: ❌
• Computed: true: ✅
• Computed: false: ❌

In terms of static analysis, it is much easier to deal with one required field than three optional and conflicting ones. For example, it is relatively easy to write go/analysis and semgrep tooling that checks for the existence of a field declaration with a struct. Tooling and ensuring consistency around that is harder if you need to check values, let alone multiple of them, to ensure validity and any stylistic conventions. Not the biggest deal, but preferring no false values for the current booleans requires additional static analysis, for example: https://github.com/bflad/tfproviderlint/tree/main/passes/S019.

Proposal

Quick sketch being:

const (
    // Requires configuration.
    AttributeConfigurationRequired AttributeConfiguration = 0

    // Configuration is not required, but value can be optionally set.
    // Shows difference if plugin returns value and there is no configuration.
    AttributeConfigurationOptional AttributeConfiguration = 1

    // Configuration is not required, but value can be optionally set.
    // Does not show difference if plugin returns value and there is no configuration.
    AttributeConfigurationOptionalComputed AttributeConfiguration = 2

    // Cannot be configured, however the plugin can set the value.
    AttributeConfigurationComputed AttributeConfiguration = 3
)

type AttributeConfiguration uint8

type Attribute struct {
    // ... other fields ...
    // The below would replace Required, Optional, and Computed
    Configuration AttributeConfiguration
}

Which the naming can be adjusted as necessary, e.g. can be further reduced with any of dropping Attribute, Attribute to Attr, Configuration to Config, moving to a separate package, etc.

References

• https://github.com/bflad/tfproviderlint#standard-schema-checks

[apparentlymart]

While pondering this, it could also be interesting to ponder whether Optional: true alone is really necessary as a separate state than Optional: true, Computed: true. We copied the old SDK model pretty directly when designing the protocol's schema structure, and hence we inherited this strange multi-flag design from there without critiquing it much.

Optional without Computed in practice only means that if the configuration doesn't set it then the value must be null. Optional and Computed together allows the provider to choose a default, and that default could itself be null if appropriate, and thus get the same effect.

Optional without Computed was perhaps incidentally useful in the old SDK so that the provider code could unambiguously recognize when the argument got "unset" in a later change (since Computed arguments have sticky values in the old SDK), but I believe the new framework makes it possible for provider code to examine the configuration directly and thus see when something's been unset regardless of whether there's a "sticky" computed value from the previous run included in the proposed new state.

I've not thought exhaustively about this so there might well be some other useful benefit to Optional without Computed that I'm not considering, but this proposal does seem like an opportunity to investigate that further and potentially end in an even less complicated final model, where we only need to include "Required", "Optional", and "Computed" in this enum, where "Optional" would represent what we currently model as Optional: true, Computed: true, instead of just Optional alone. 🤔

[bflad]

Just to capture the data point, core does not implement configurability validations, per hashicorp/terraform#30669. We did implement "Required" and "Read-Only" checks in v0.9.0 of the framework, per #370.

While looking at another issue involving validation, it reminded me that many of our "common use case" validators that will wind up in github.com/hashicorp/terraform-plugin-framework-validators will purposefully skip null values, because they are expecting something else to handle any "Required" checking. This makes me wonder if we could extend the simplification even further:

• Remove Required/Optional/Computed fields
• Without setting anything, attributes will be marked as Computed only
• Create validator(s) which represent Optional/Required
• Update the framework logic to sniff for those validators to handle Required/Optional and unset Computed when Required

This would allow provider developers to define all expected configuration value information in one place (attribute validators). I haven't really thought through whether this is desirable or would have other issues.

There's also another similar potential simplification:

• Remove Computed field
• Without setting anything, attributes will be marked as Computed only
• If Required, unset Computed (Optional will always set Computed as mentioned before)

I'm not sure when we'll really dive into this, but wanted to document the related issue, pull request, and ideas while thinking about it.

[magodo]

@bflad One concern about changing these attributes from declarative to imperative manner is that makes the static analysis tools (e.g. tfproviderlint) hard to implement. What do you think about this?

[bflad]

@bflad One concern about changing these attributes from declarative to imperative manner is that makes the static analysis tools (e.g. tfproviderlint) hard to implement. What do you think about this?

@magodo it is certainly a concern, but the framework design has generally tried to prevent the need for static analysis tooling on framework code implementations in providers by making errant implementations not possible for developers to implement. This attribute definition triple flag situation is one of the remaining places that was not adjusted before the framework had a stable release. Did you have specific static analysis use cases in mind that replacing these flags with a single configuration would make problematic?

Honestly, I'm more concerned about other types of downstream tooling that relies on this information, should we decide to change parts of how its represented, since there is conflicting meanings to the flags between Terraform core and the provider side of the protocol.

[bflad]

Yet another drive-by comment 🙁, but I want to ensure this is captured in a form as it relates to this issue.

#898 highlights another interesting case in this space. Both the prior SDK (with blocks errantly being able to be defined as computed) and framework (with nested attributes being able to be defined as computed with non-computed underlying attributes) conflict with how Terraform core wants providers to define their schemas in comparison to how they might be perceived by provider developers. While the implementation of the framework prevents the block case by not exposing computed (or optional or required) as an option for provider developers, there is nothing preventing provider developers from implementing computed nested attributes with non-computed underlying attributes.

From a provider developer experience perspective, this seems like it should be valid and supported. "My system supports configuration of a collection of objects or will compute them, but when configured, inside those objects we want to require configuration of some parts." So you create a schema definition such as:

// comments from the provider developers perspective
schema.SetNestedAttribute{
	Optional: true, // this collection may be supplied via configuration
	Computed: true, // this collection may be supplied by the system when not configured
	PlanModifiers: []planmodifier.Set{ // system behavior specific
		setplanmodifier.UseStateForUnknown(), // preserve the system value on updates (it does not change)
	},
	NestedObject: schema.NestedAttributeObject{
		Attributes: map[string]schema.Attribute{
			"name": schema.StringAttribute{
				Required: true, // this must be configured, when the object is configured
			},
		},
	},
},

As of Terraform 1.6.x though, the core algorithm for generating the proposed new state won't act the same as a regular computed attribute in this case when the nested attribute configuration is null. Adjusting the core algorithm may be safe for this use case (ignore computability in underlying attributes if the nested attribute is computed, configuration of the nested attribute is null, and there is a prior state), but that sort of change would only apply to environments running versions of Terraform after the change. We've talked about potentially moving the proposed new state algorithm into the framework since it has all the provider-side context (e.g. default values) to enhance planning, which would resolve that sort of Terraform versioning issue, but its a quite risky change.

Another, not necessarily mutually exclusive, option we have is to prevent provider developers from getting into this awkward situation by raising a new "implementation" error when we detect non-computed attributes under a computed nested attribute. Luckily the attribute-based validation abstractions in the framework operate when "this nesting level is configured", so its possible to add a "not null" validator similar to the framework's internal Required: true implementation that behaves the same way (if the object is not configured, no error). We would just need to make those validators readily available to provider developers if we were to introduce this sort of implementation error.

There is also potential for the framework's type system to let developers say "this value should never be null" from a type-perspective as another, not mutually exclusive, option. The type-based validation would theoretically never trigger until the object for the value exists, giving the same behavior as the prior option. One wrinkle here is that type definitions can exist within other type definitions, meaning the framework might need to walk entire values in more places than it does today.

To somewhat prevent this particular implementation issue (it wouldn't fix the missing computed 😖 ), we could consider removing required as a flag given to provider developers. Instead, we would make "required" (not null) a validator or type behavior you choose to add, like you do for other configuration validation rules on attribute values. We would want to verify that switching from required to optional like this won't fundamentally break anything from core's perspective with practitioner configuration behaviors or planning internals if provider developers do this unilaterally. There are unfortunate consequences though, as things like documentation generation and editor integrations rely on that flag to immediately give practitioners that configuration expectation, which has somewhat higher urgency than other validations on the value itself.

If that is something we wanted to do, then we would mark the Required fields as deprecated and say to replace it with Optional: true and the additional configuration validator or type behavior. I hesitate to say that we should automatically do this with existing Required: true schema definitions though, as the framework's schema design/behavior has tried to be extremely transparent with respect to being 1:1 with the protocol where applicable. The prior SDK caused everyone enough confusion by doing "magic" like that and we're still paying for it as developers try to migrate to the more accurate schema representations. 🥲

If something like that was to happen, what we would be left with (barring the protocol changing) would be debating the merits of whether attributes would have a "default" configurability that provider developers could override if necessary or if we still want to require provider developers to explicitly state an intention every attribute. Either of those options feel like they could use a single attribute definition field as originally proposed here, just with slightly different naming thats tailored for provider developers, e.g. read-only/provider-only (computed), read-write/configuration-or-provider (computed and optional), and potentially write-only/configuration-only (optional).