all: Add automatic deferred action support for unknown provider configuration #1002
Conversation
…d `DeferredResponse` respectively to match protobuf definition
# Conflicts: # go.sum
Co-authored-by: Austin Valle <austinvalle@gmail.com>
Co-authored-by: Brian Flad <bflad417@gmail.com>
…s/automatic-deferred-action-support # Conflicts: # internal/fromproto5/client_capabilities.go # internal/fromproto6/client_capabilities.go
…n-support' into SBGoods/automatic-deferred-action-support
SBGoods
changed the title
| // Skip resource-level ModifyPlan for automatic deferrals | ||
| // unless ProviderDeferredBehavior.EnablePlanModification is true | ||
| if s.deferred != nil && !req.ResourceBehavior.ProviderDeferred.EnablePlanModification { | ||
| logging.FrameworkDebug(ctx, "Provider has deferred response configured, automatically returning deferred response.") | ||
| resp.Deferred = &resource.Deferred{ | ||
| Reason: resource.DeferredReason(s.deferred.Reason), | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
I'm interested what the potential impact might be with introducing the automatic deferral after all of these have run:
- Resource
Configuremethod - Provider-defined schema default plan modifiers
- Null computed attributes marked as unknown
- Provider-defined schema plan modifiers
I think this should be okay since we don't explicitly pass API client information here, but some of our documentation refers to plan modification as "online", like the blurb about validation: https://developer.hashicorp.com/terraform/plugin/framework/resources/validate-configuration
If we do go with this route, I think we should update the naming/documentation of ResourceBehavior.EnablePlanModification to be more explicit about "what" plan modification logic is being enabled/disabled. Might need to bikeshed a name like EnableModifyPlan, EnableResourcePlanModification, etc 🤔
Any thoughts on this @bflad ?
There was a problem hiding this comment.
That is a fair point. Another option is doing similar as the RPCs and returning without doing anything and would technically be the safest choice. I had hoped that since most of the list up there shouldn't be doing anything with using APIs, etc. we could improve the plan output for future rounds, but that could be changed later as an enhancement, rather than the reverse of taking functionality away being a breaking change.
I guess I'll concede we should be safer here and return super early before anything runs. We can always change that decision later. I do worry about the null to unknown logic not running although maybe core will already do something like that in the plan output based on the configuration (something also worth checking with ReadDataSource).
There was a problem hiding this comment.
For safe routes, would it be a bad idea to just duplicate the "marked computed nils as unknown" logic after checking for a deferral at the top of PlanResourceChange?
modifiedPlan, err := tftypes.Transform(resp.PlannedState.Raw, MarkComputedNilsAsUnknown(ctx, req.Config.Raw, req.ResourceSchema))I'm also cool with just coming back later and enhancing it once we have more info, using ProposedNewState like SDKv2: https://github.com/hashicorp/terraform-plugin-sdk/blob/5fee97c1b78c352a1195194a12a9ad514508e3c6/helper/schema/grpc_provider.go#L777-L792
There was a problem hiding this comment.
Probably fine but I guess it might be good to evaluate the "current" behavior if the provider side just reflects that data without adding the extra logic as maybe its not needed. That evaluation does not need to happen immediately though, which is nice.
There was a problem hiding this comment.
I've added a flag in 31f33f8 to essentially skip everything except "Null computed attributes marked as unknown" logic. Unfortunately, there's a lot of prerequisites to calling MarkComputedNilsAsUnknown() that I felt was bloating the deferral check block, but if the flag logic is too complicated, I can go back to duplicating the logic in the deferral check.
There was a problem hiding this comment.
My only reservation in adding a flag to all the other pieces of plan logic is just future maintenance/changes to PlanResourceChange, i.e. it's easier to describe just the operations that automatic deferred plans needs to run, rather than describe all the operations (and future operations) that shouldn't run.
If there are a ton of operations and such that need to run for the computed marking, maybe we should just return early with ProposedNewState (like SDKv2 is doing) for now and treat it as an enhancement once deferred actions/stacks is more in-use.
Plans with deferred actions aren't held to the consistency rules by Terraform between rounds, so this is mostly a UX problem
There was a problem hiding this comment.
This makes sense, I've decided to return early with the ProposedNewState and we can circle back on this issue later.
Co-authored-by: Austin Valle <austinvalle@gmail.com>
There was a problem hiding this comment.
Great work on this, @SBGoods 👍 A few notes, but overall pretty close. Will let @austinvalle give the final ✅
| @@ -378,6 +424,11 @@ func TestServerImportResourceState(t *testing.T) { | |||
| t.Run(name, func(t *testing.T) { | |||
| t.Parallel() | |||
|
|
|||
| if testCase.configureProviderReq != nil { | |||
There was a problem hiding this comment.
Similar to terraform-plugin-sdk, we should include a changelog note talking about deferral being experimental 👍
https://github.com/hashicorp/terraform-plugin-sdk/releases/tag/v2.34.0 for inspiration
There was a problem hiding this comment.
Since we are planning on releasing this with the manual deferred action work, this should be covered by: https://github.com/hashicorp/terraform-plugin-framework/blob/main/.changes/unreleased/NOTES-20240510-143136.yaml
| // Skip resource-level ModifyPlan for automatic deferrals | ||
| // unless ProviderDeferredBehavior.EnablePlanModification is true | ||
| if s.deferred != nil && !req.ResourceBehavior.ProviderDeferred.EnablePlanModification { | ||
| logging.FrameworkDebug(ctx, "Provider has deferred response configured, automatically returning deferred response.") | ||
| resp.Deferred = &resource.Deferred{ | ||
| Reason: resource.DeferredReason(s.deferred.Reason), | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
That is a fair point. Another option is doing similar as the RPCs and returning without doing anything and would technically be the safest choice. I had hoped that since most of the list up there shouldn't be doing anything with using APIs, etc. we could improve the plan output for future rounds, but that could be changed later as an enhancement, rather than the reverse of taking functionality away being a breaking change.
I guess I'll concede we should be safer here and return super early before anything runs. We can always change that decision later. I do worry about the null to unknown logic not running although maybe core will already do something like that in the plan output based on the configuration (something also worth checking with ReadDataSource).
…n based on PR feedback.
…values with `Unknown`
…blePlanModification` is false.
Co-authored-by: Brian Flad <bflad417@gmail.com>
…esource deferred reason.
Ref: hashicorp/terraform-plugin-go#403
This PR implements deferred action support for
PROVIDER_CONFIG_UNKNOWNduring theConfigureProviderRPC. This functionality will allow a provider to defer all resources and data sources associated with the framework provider during theReadResource,ReadDataSource,ImportResourceState, andPlanResourceChangeRPCs.Deferred actions is an experimental feature introduced in
Terraform v1.9.0-alpha20240404and is only available in experimental Terraform builds.