search

hashicorp / terraform-plugin-framework

A next-generation framework for building Terraform providers.
https://developer.hashicorp.com/terraform/plugin/framework
Mozilla Public License 2.0
298 stars 92 forks source link

Consider Identifying Attributes on Nested Attribute/Block Objects #720

Open bflad opened 1 year ago

bflad commented 1 year ago

Module version

v1.2.0

Use-cases

After #711, the UseStateForUnknown plan modifier will begin raising an implementation error if it discovers during runtime that it is implemented under a list or set. This behavior was necessary to prevent cases where there could be incorrect data proposed during certain updates and to prevent Terraform errors. This catch all behavior will likely be a surprise for developers without another methodology for preserving at least some prior state values during planning.

717 and #718 took an initial look at implementing a separate plan modifier which accepted identifying attributes for the list/set nested object, e.g.

schema.SetNestedAttribute{
    NestedObject: schema.NestedAttributeObject{
        Attributes: map[string]schema.Attribute{
            "computed_attr": schema.StringAttribute{
                Computed: true,
                PlanModifiers: []planmodifier.String{
                    // Preserve this computed value during updates.
                    stringplanmodifier.MatchElementStateForUnknown(
                        // Identify matching prior state value based on configurable_attr
                        path.MatchRelative().AtParent().AtName("configurable_attr"),
                        // ... potentially others ...
                    ),
                },
            },
            "configurable_attr": schema.StringAttribute{
                Required: true,
            },
        },
    },
    Optional: true,
}

This enables developers to better plan list/set element rearrangement, but understandably, this would be a little cumbersome for developers to understand when to implement the differing plan modifier as well as constructing the appropriate relative path expressions (since it needed to be very explicitly implemented and validated by the framework). Other custom plan modifiers needing prior state under a list/set would also need to reconstruct the internal logic of the new plan modifier, which would also mean that developers would need to duplicate the information when necessary, e.g.

schema.SetNestedAttribute{
    NestedObject: schema.NestedAttributeObject{
        Attributes: map[string]schema.Attribute{
            "computed_attr": schema.StringAttribute{
                Computed: true,
                PlanModifiers: []planmodifier.String{
                    stringplanmodifier.MatchElementStateForUnknown(
                        path.MatchRelative().AtParent().AtName("configurable_attr"),
                        // ... potentially others ...
                    ),
                    myCustomPlanModifier(
                        path.MatchRelative().AtParent().AtName("configurable_attr"),
                        // ... potentially others ...
                    ),
                    // ... potentially others ...
                },
            },
            "configurable_attr": schema.StringAttribute{
                Required: true,
            },
        },
    },
    Optional: true,
}

There's also the case where there may be many identifying attributes or many attributes implementing plan modifiers that need this information. The proposed solution for #70 would also require this information to ensure that semantic equality prior state data is correct (rather than falling back to null values and potentially noisier plans).

Having a more baked-in canonical representation of this data and automatically realigning prior state before passing data to plan modifiers would simplify provider implementations. Depending on how this information is defined, it could also be used in the case of https://github.com/hashicorp/terraform/issues/30753 being exposed across the provider protocol, where Terraform could better render list/set plans where it currently needs to rely on "conventional" identifying attribute names. A #70 solution could also interrogate this information.

It is however important to ensure that provider developers still are only opting into preserving prior state values for certain attributes same as today, where the choice of UseStateForUnknown should be dependent on API behavior. The framework should not impose any sort of automatic behavior in that case for Computed-only attributes.

Proposal

Implement new resource/schema.NestedAttributeObject and resource/schema.NestedBlockObject field for identifying attributes:

type NestedAttributeObject struct {
  // ... existing fields ...

  // IdentifyingAttributes should define a list of attribute names which
  // are used to align prior state elements with proposed state elements.
  // If defined, the framework will automatically realign prior state data
  // for attribute plan modifiers.
  //
  // This definition is necessary underneath lists and sets where elements
  // do not have a stable, identifying key to ensure any prior state is aligned
  // with proposed elements. This is not required underneath maps, however,
  // this information may be used in the future for other framework and
  // Terraform enhancements.
  IdentifyingAttributes []string
}

This information is only required today for resource schemas, since they are the only location where there is proposed new state data on top of prior state. This should not be present on data source or provider schemas.

Given that NestedAttributeObject and NestedBlockObject are already structure types instead of interface types, this implementation cannot limit the defining the new field to only list and set nested attributes and blocks. This information may be applicable to maps and single nested objects for other enhancements in the future, so having it exposed and potentially filled in by provider developers in currently extraneous cases may still serve beneficial.

The choice of []string instead of using a path package based solution is because the implementation will purposefully be limited to a single nesting level (the string in Attributes: map[string]schema.Attribute). Unlike certain implementation details in the prior terraform-plugin-sdk, it is not expected to introduce a synthetic syntax for constructing string-based paths in this location. If more complex schema/logic is necessary, a different solution would likely be required anyways.

In internal/fwschema, expose new NestedAttributeObject and NestedBlockObject interface methods:

type NestedAttributeObject interface {
  // ... existing methods ...

  // GetIdentifyingAttributes should return a list of attribute named that are identifying.
  GetIdentifyingAttributes() []string
}

During the PlanResourceChange RPC logic, the framework can then interrogate for this information to automatically realign prior state for list/set elements in the attribute plan modification logic or return null values instead. That realignment logic can implement an algorithm similar to #718 and ensure attribute plan modifiers receive the result of that realignment. This type of framework change is safe because it is opt-in for provider developers by nature of newly implementing the identifying attributes.

The UseStateForUnknown plan modifier should be able to introduce additional validation logic where if a parent path in the request path has non-zero identifying attributes, to not raise the implementation error. The implementation error itself should be updated to mention the new IdentifyingAttributes field to provide a helpful hint for provider developers.

After all is said and done, this should then be a valid resource schema definition:

schema.SetNestedAttribute{
    NestedObject: schema.NestedAttributeObject{
        Attributes: map[string]schema.Attribute{
            "computed_attr": schema.StringAttribute{
                Computed: true,
                PlanModifiers: []planmodifier.String{
                    stringplanmodifier.UseStateForUnknown(),
                },
            },
            "configurable_attr": schema.StringAttribute{
                Required: true,
            },
        },
        IdentifyingAttributes: []string{"configurable_attr"},
    },
    Optional: true,
}

References

bflad commented 1 year ago

After discussions with the core team and reviewing some of the core planning logic, the framework may be able to re-implement some of the same automatic configuration <-> prior state element matching behavior on this side of the provider protocol. That may be an initial step that can be taken in this space to reduce the need for this type of developer burden.