From 74885b1108090461879b0e0aeadb13dabafe0f46 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 11 Apr 2022 11:55:53 -0400 Subject: [PATCH] remove data sources from state read and upgrade Data sources should not require reading the previous versions. While we previously skipped the decoding if it were to fail, this removes the need for any prior state at all. The only place where the prior state was functionally used was in the destroy path. Because a data source destroy is only for cleanup purposes to clean out the state using the same code paths as a managed resource, we can substitute the prior state in the change change with a null value to maintain the same behavior. --- internal/terraform/node_resource_abstract.go | 8 ----- .../terraform/node_resource_plan_destroy.go | 35 +++++++++++++++++++ internal/terraform/upgrade_resource_state.go | 13 ++++--- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index 73e8c4859b..4903cdbe37 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/internal/terraform/node_resource_abstract.go @@ -394,14 +394,6 @@ func (n *NodeAbstractResource) readResourceInstanceState(ctx EvalContext, addr a obj, err := src.Decode(schema.ImpliedType()) if err != nil { - // In the case of a data source which contains incompatible state - // migrations, we can just ignore decoding errors and skip comparing - // the prior state. - if addr.Resource.Resource.Mode == addrs.DataResourceMode { - log.Printf("[DEBUG] readResourceInstanceState: data source schema change for %s prevents decoding: %s", addr, err) - return nil, diags - } - diags = diags.Append(err) } diff --git a/internal/terraform/node_resource_plan_destroy.go b/internal/terraform/node_resource_plan_destroy.go index 907f20519c..dd8216445d 100644 --- a/internal/terraform/node_resource_plan_destroy.go +++ b/internal/terraform/node_resource_plan_destroy.go @@ -1,10 +1,13 @@ package terraform import ( + "fmt" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" + "github.com/zclconf/go-cty/cty" ) // NodePlanDestroyableResourceInstance represents a resource that is ready @@ -39,6 +42,19 @@ func (n *NodePlanDestroyableResourceInstance) DestroyAddr() *addrs.AbsResourceIn func (n *NodePlanDestroyableResourceInstance) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { addr := n.ResourceInstanceAddr() + switch addr.Resource.Resource.Mode { + case addrs.ManagedResourceMode: + return n.managedResourceExecute(ctx, op) + case addrs.DataResourceMode: + return n.dataResourceExecute(ctx, op) + default: + panic(fmt.Errorf("unsupported resource mode %s", n.Config.Mode)) + } +} + +func (n *NodePlanDestroyableResourceInstance) managedResourceExecute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { + addr := n.ResourceInstanceAddr() + // Declare a bunch of variables that are used for state during // evaluation. These are written to by address in the EvalNodes we // declare below. @@ -85,3 +101,22 @@ func (n *NodePlanDestroyableResourceInstance) Execute(ctx EvalContext, op walkOp diags = diags.Append(n.writeChange(ctx, change, "")) return diags } + +func (n *NodePlanDestroyableResourceInstance) dataResourceExecute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { + + // We may not be able to read a prior data source from the state if the + // schema was upgraded and we are destroying before ever refreshing that + // data source. Regardless, a data source "destroy" is simply writing a + // null state, which we can do with a null prior state too. + change := &plans.ResourceInstanceChange{ + Addr: n.ResourceInstanceAddr(), + PrevRunAddr: n.prevRunAddr(ctx), + Change: plans.Change{ + Action: plans.Delete, + Before: cty.NullVal(cty.DynamicPseudoType), + After: cty.NullVal(cty.DynamicPseudoType), + }, + ProviderAddr: n.ResolvedProvider, + } + return diags.Append(n.writeChange(ctx, change, "")) +} diff --git a/internal/terraform/upgrade_resource_state.go b/internal/terraform/upgrade_resource_state.go index 647db361cc..906898e281 100644 --- a/internal/terraform/upgrade_resource_state.go +++ b/internal/terraform/upgrade_resource_state.go @@ -21,6 +21,14 @@ import ( // If any errors occur during upgrade, error diagnostics are returned. In that // case it is not safe to proceed with using the original state object. func upgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Interface, src *states.ResourceInstanceObjectSrc, currentSchema *configschema.Block, currentVersion uint64) (*states.ResourceInstanceObjectSrc, tfdiags.Diagnostics) { + if addr.Resource.Resource.Mode != addrs.ManagedResourceMode { + // We only do state upgrading for managed resources. + // This was a part of the normal workflow in older versions and + // returned early, so we are only going to log the error for now. + log.Printf("[ERROR] data resource %s should not require state upgrade", addr) + return src, nil + } + // Remove any attributes from state that are not present in the schema. // This was previously taken care of by the provider, but data sources do // not go through the UpgradeResourceState process. @@ -32,11 +40,6 @@ func upgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Int src.AttrsJSON = stripRemovedStateAttributes(src.AttrsJSON, currentSchema.ImpliedType()) } - if addr.Resource.Resource.Mode != addrs.ManagedResourceMode { - // We only do state upgrading for managed resources. - return src, nil - } - stateIsFlatmap := len(src.AttrsJSON) == 0 // TODO: This should eventually use a proper FQN.