From 1afdb055ca06dd6f38f2236daffd71d5ff528d2e Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 28 Aug 2018 14:55:28 -0700 Subject: [PATCH] core: Re-implement ReadDataDiff around our new approach This is no longer a call into the provider, since all of the data diff logic is standard for all data sources anyway. Instead, we just compute the planned new value and construct a planned change from that as-is. Previously the provider could, in principle, customize the read diff. In practice there is no real reason to do that and the existing SDK didn't pass that possibility through to provider code, so we can safely change this without impacting provider compatibility. --- terraform/eval_diff.go | 4 +- terraform/eval_read_data.go | 173 +++++++++++------------ terraform/eval_state.go | 4 +- terraform/node_data_refresh.go | 2 +- terraform/node_resource_apply.go | 2 +- terraform/node_resource_plan_instance.go | 2 +- terraform/schemas.go | 17 +++ 7 files changed, 110 insertions(+), 94 deletions(-) diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 79239adf8..89bb97e7e 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -716,7 +716,7 @@ func (n *EvalReadDiff) Eval(ctx EvalContext) (interface{}, error) { changes := ctx.Changes() addr := n.Addr.Absolute(ctx.Path()) - schema := providerSchema.ResourceTypes[n.Addr.Resource.Type] + schema := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource()) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here return nil, fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Type) @@ -777,7 +777,7 @@ func (n *EvalWriteDiff) Eval(ctx EvalContext) (interface{}, error) { panic("inconsistent address and/or deposed key in EvalWriteDiff") } - schema := providerSchema.ResourceTypes[n.Addr.Resource.Type] + schema := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource()) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here return nil, fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Type) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index be2aad5bc..f9924b29e 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -3,14 +3,15 @@ package terraform import ( "fmt" - "github.com/hashicorp/terraform/providers" - "github.com/hashicorp/terraform/states" - "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/plans/objchange" + "github.com/hashicorp/terraform/providers" + "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/tfdiags" ) // EvalReadDataDiff is an EvalNode implementation that executes a data @@ -18,7 +19,7 @@ import ( type EvalReadDataDiff struct { Addr addrs.ResourceInstance Config *configs.Resource - Provider *providers.Interface + ProviderAddr addrs.AbsProviderConfig ProviderSchema **ProviderSchema Output **plans.ResourceInstanceChange @@ -31,104 +32,102 @@ type EvalReadDataDiff struct { } func (n *EvalReadDataDiff) Eval(ctx EvalContext) (interface{}, error) { - return nil, fmt.Errorf("EvalReadDataDiff not yet updated for new state/plan/provider types") - /* - absAddr := n.Addr.Absolute(ctx.Path()) + absAddr := n.Addr.Absolute(ctx.Path()) - if n.ProviderSchema == nil || *n.ProviderSchema == nil { - return nil, fmt.Errorf("provider schema not available for %s", n.Addr) + if n.ProviderSchema == nil || *n.ProviderSchema == nil { + return nil, fmt.Errorf("provider schema not available for %s", n.Addr) + } + + var diags tfdiags.Diagnostics + var change *plans.ResourceInstanceChange + var configVal cty.Value + + if n.Previous != nil && *n.Previous != nil && (*n.Previous).Action == plans.Delete { + // If we're re-diffing for a diff that was already planning to + // destroy, then we'll just continue with that plan. + + nullVal := cty.NullVal(cty.DynamicPseudoType) + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreDiff(absAddr, states.CurrentGen, nullVal, nullVal) + }) + if err != nil { + return nil, err } - var diags tfdiags.Diagnostics + change = &plans.ResourceInstanceChange{ + Addr: absAddr, + ProviderAddr: n.ProviderAddr, + Change: plans.Change{ + Action: plans.Delete, + Before: nullVal, + After: nullVal, + }, + } + } else { + config := *n.Config + providerSchema := *n.ProviderSchema + schema := providerSchema.DataSources[n.Addr.Resource.Type] + if schema == nil { + // Should be caught during validation, so we don't bother with a pretty error here + return nil, fmt.Errorf("provider does not support data source %q", n.Addr.Resource.Type) + } - // The provider API still expects our legacy InstanceInfo type. - legacyInfo := NewInstanceInfo(n.Addr.Absolute(ctx.Path())) + objTy := schema.ImpliedType() + priorVal := cty.NullVal(objTy) // for data resources, prior is always null because we start fresh every time + + keyData := EvalDataForInstanceKey(n.Addr.Key) + + var configDiags tfdiags.Diagnostics + configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData) + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return nil, diags.Err() + } + + proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal) err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff(absAddr, cty.NullVal(cty.DynamicPseudoType), cty.NullVal(cty.DynamicPseudoType)) + return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) }) if err != nil { return nil, err } - var diff *InstanceDiff - var configVal cty.Value - - if n.Previous != nil && *n.Previous != nil && (*n.Previous).GetDestroy() { - // If we're re-diffing for a diff that was already planning to - // destroy, then we'll just continue with that plan. - diff = &InstanceDiff{Destroy: true} - } else { - provider := *n.Provider - config := *n.Config - providerSchema := *n.ProviderSchema - schema := providerSchema.DataSources[n.Addr.Resource.Type] - if schema == nil { - // Should be caught during validation, so we don't bother with a pretty error here - return nil, fmt.Errorf("provider does not support data source %q", n.Addr.Resource.Type) - } - - keyData := EvalDataForInstanceKey(n.Addr.Key) - - var configDiags tfdiags.Diagnostics - configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return nil, diags.Err() - } - - // The provider API still expects our legacy ResourceConfig type. - legacyRC := NewResourceConfigShimmed(configVal, schema) - - var err error - diff, err = provider.ReadDataDiff(legacyInfo, legacyRC) - if err != nil { - diags = diags.Append(err) - return nil, diags.Err() - } - if diff == nil { - diff = new(InstanceDiff) - } - - // if id isn't explicitly set then it's always computed, because we're - // always "creating a new resource". - diff.init() - if _, ok := diff.Attributes["id"]; !ok { - diff.SetAttribute("id", &ResourceAttrDiff{ - Old: "", - NewComputed: true, - RequiresNew: true, - Type: DiffAttrOutput, - }) - } + change = &plans.ResourceInstanceChange{ + Addr: absAddr, + ProviderAddr: n.ProviderAddr, + Change: plans.Change{ + Action: plans.Read, + Before: priorVal, + After: proposedNewVal, + }, } + } - err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(legacyInfo, diff) - }) - if err != nil { - return nil, err + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostDiff(absAddr, states.CurrentGen, change.Action, change.Before, change.After) + }) + if err != nil { + return nil, err + } + + if n.Output != nil { + *n.Output = change + } + + if n.OutputValue != nil { + *n.OutputValue = change.After + } + + if n.OutputState != nil { + state := &states.ResourceInstanceObject{ + Value: change.After, + Status: states.ObjectReady, } + *n.OutputState = state + } - *n.Output = diff - - if n.OutputValue != nil { - *n.OutputValue = configVal - } - - if n.OutputState != nil { - state := &InstanceState{} - *n.OutputState = state - - // Apply the diff to the returned state, so the state includes - // any attribute values that are not computed. - if !diff.Empty() && n.OutputState != nil { - *n.OutputState = state.MergeDiff(diff) - } - } - - return nil, diags.ErrWithWarnings() - */ + return nil, diags.ErrWithWarnings() } // EvalReadDataApply is an EvalNode implementation that executes a data diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 248ae691c..6069c2e79 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -198,7 +198,7 @@ func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { // TODO: Update this to use providers.Schema and populate the real // schema version in the second argument to Encode below. - schema := (*n.ProviderSchema).ResourceTypes[absAddr.Resource.Resource.Type] + schema := (*n.ProviderSchema).SchemaForResourceAddr(n.Addr.ContainingResource()) if schema == nil { // It shouldn't be possible to get this far in any real scenario // without a schema, but we might end up here in contrived tests that @@ -263,7 +263,7 @@ func (n *EvalWriteStateDeposed) Eval(ctx EvalContext) (interface{}, error) { // TODO: Update this to use providers.Schema and populate the real // schema version in the second argument to Encode below. - schema := (*n.ProviderSchema).ResourceTypes[absAddr.Resource.Resource.Type] + schema := (*n.ProviderSchema).SchemaForResourceAddr(n.Addr.ContainingResource()) if schema == nil { // It shouldn't be possible to get this far in any real scenario // without a schema, but we might end up here in contrived tests that diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index d1e72b48d..65da5dffb 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -147,7 +147,7 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { &EvalReadDataDiff{ Addr: addr.Resource, Config: n.Config, - Provider: &provider, + ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, Output: &change, OutputValue: &configVal, diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 4b97be52d..ecd40b873 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -158,7 +158,7 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou &EvalReadDataDiff{ Addr: addr.Resource, Config: n.Config, - Provider: &provider, + ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, Output: &change, OutputValue: &configVal, diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 31d35a333..d0a7865ac 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -84,7 +84,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou &EvalReadDataDiff{ Addr: addr.Resource, Config: n.Config, - Provider: &provider, + ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, Output: &change, OutputValue: &configVal, diff --git a/terraform/schemas.go b/terraform/schemas.go index 676fd0980..8423ce2b1 100644 --- a/terraform/schemas.go +++ b/terraform/schemas.go @@ -4,6 +4,7 @@ import ( "fmt" "log" + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/providers" @@ -242,6 +243,22 @@ type ProviderSchema struct { DataSources map[string]*configschema.Block } +// SchemaForResourceAddr attempts to find a schema for the mode and type from +// the given resource address. Returns nil if no such schema is available. +func (ps *ProviderSchema) SchemaForResourceAddr(addr addrs.Resource) *configschema.Block { + var m map[string]*configschema.Block + switch addr.Mode { + case addrs.ManagedResourceMode: + m = ps.ResourceTypes + case addrs.DataResourceMode: + m = ps.DataSources + default: + // Shouldn't happen, because the above cases are comprehensive. + return nil + } + return m[addr.Type] +} + // ProviderSchemaRequest is used to describe to a ResourceProvider which // aspects of schema are required, when calling the GetSchema method. type ProviderSchemaRequest struct {