From 3e5b8d4aaa9c624bbaa6d45b93c777e1da8b1896 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 May 2020 10:22:50 -0400 Subject: [PATCH 01/21] add GraphNodeAttachDependsOn GraphNodeAttachDependsOn give us a method for adding all transitive resource dependencies found through depends_on references, so that data source can determine if they can be read during plan. This will be done by inspecting the changes of all dependency resources, and delaying read until apply if any changes are planned. --- terraform/node_resource_abstract.go | 45 ++++++++++++++++++++--------- terraform/node_resource_plan.go | 1 + terraform/transform_reference.go | 23 +++++++++++++-- 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 8c2573745..4614daf45 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -58,7 +58,11 @@ type NodeAbstractResource struct { ProvisionerSchemas map[string]*configschema.Block - Targets []addrs.Targetable // Set from GraphNodeTargetable + // Set from GraphNodeTargetable + Targets []addrs.Targetable + + // Set from GraphNodeDependsOn + dependsOn []addrs.ConfigResource // The address of the provider this resource will use ResolvedProvider addrs.AbsProviderConfig @@ -75,6 +79,7 @@ var ( _ GraphNodeAttachProvisionerSchema = (*NodeAbstractResource)(nil) _ GraphNodeAttachProviderMetaConfigs = (*NodeAbstractResource)(nil) _ GraphNodeTargetable = (*NodeAbstractResource)(nil) + _ GraphNodeAttachDependsOn = (*NodeAbstractResource)(nil) _ dag.GraphNodeDotter = (*NodeAbstractResource)(nil) ) @@ -175,18 +180,7 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { if c := n.Config; c != nil { var result []*addrs.Reference - for _, traversal := range c.DependsOn { - ref, diags := addrs.ParseRef(traversal) - if diags.HasErrors() { - // We ignore this here, because this isn't a suitable place to return - // errors. This situation should be caught and rejected during - // validation. - log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err()) - continue - } - - result = append(result, ref) - } + result = append(result, n.DependsOn()...) if n.Schema == nil { // Should never happens, but we'll log if it does so that we can @@ -230,6 +224,26 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { return nil } +func (n *NodeAbstractResource) DependsOn() []*addrs.Reference { + var result []*addrs.Reference + if c := n.Config; c != nil { + + for _, traversal := range c.DependsOn { + ref, diags := addrs.ParseRef(traversal) + if diags.HasErrors() { + // We ignore this here, because this isn't a suitable place to return + // errors. This situation should be caught and rejected during + // validation. + log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err()) + continue + } + + result = append(result, ref) + } + } + return result +} + // GraphNodeReferencer func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { // If we have a configuration attached then we'll delegate to our @@ -382,6 +396,11 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) { n.Targets = targets } +// GraphNodeAttachDependsOn +func (n *NodeAbstractResource) AttachDependsOn(deps []addrs.ConfigResource) { + n.dependsOn = deps +} + // GraphNodeAttachResourceState func (n *NodeAbstractResourceInstance) AttachResourceState(s *states.Resource) { n.ResourceState = s diff --git a/terraform/node_resource_plan.go b/terraform/node_resource_plan.go index 897e0dc63..2517a133d 100644 --- a/terraform/node_resource_plan.go +++ b/terraform/node_resource_plan.go @@ -214,6 +214,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { a.Schema = n.Schema a.ProvisionerSchemas = n.ProvisionerSchemas a.ProviderMetas = n.ProviderMetas + a.dependsOn = n.dependsOn return &NodePlannableResourceInstance{ NodeAbstractResourceInstance: a, diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index ef64eb2c1..600ec9d5c 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -45,6 +45,22 @@ type GraphNodeAttachDependencies interface { AttachDependencies([]addrs.ConfigResource) } +// GraphNodeAttachDependsOn records all resources that are transitively +// referenced through depends_on in the configuration. This is used by data +// resources to determine if they can be read during the plan, or if they need +// to be further delayed until apply. +// We can only use an addrs.ConfigResource address here, because modules are +// not yet expended in the graph. While this will cause some extra data +// resources to show in the plan when their depends_on references may be in +// unrelated module instances, the fact that it only happens when there are any +// resource updates pending means we ca still avoid the problem of the +// "perpetual diff" +type GraphNodeAttachDependsOn interface { + GraphNodeConfigResource + AttachDependsOn([]addrs.ConfigResource) + DependsOn() []*addrs.Reference +} + // GraphNodeReferenceOutside is an interface that can optionally be implemented. // A node that implements it can specify that its own referenceable addresses // and/or the addresses it references are in a different module than the @@ -69,7 +85,7 @@ type GraphNodeReferenceOutside interface { ReferenceOutside() (selfPath, referencePath addrs.Module) } -// ReferenceTransformer is a GraphTransformer that connects all the +// Referenceeransformer is a GraphTransformer that connects all the // nodes that reference each other in order to form the proper ordering. type ReferenceTransformer struct{} @@ -116,7 +132,10 @@ type AttachDependenciesTransformer struct { } func (t AttachDependenciesTransformer) Transform(g *Graph) error { - // FIXME: this is only working with ResourceConfigAddr for now + // TODO: create a list of depends_on resources, and attach these to + // resources during plan (the destroy deps will just be ignored here). + // Data sources can then use the depens_on deps to determine if they can be + // read during plan. for _, v := range g.Vertices() { attacher, ok := v.(GraphNodeAttachDependencies) From f8907b92136dde3f2cb894262f1a4509560c6123 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 May 2020 17:52:45 -0400 Subject: [PATCH 02/21] add AttachDependsOnTransformer Adding a transformer to attach any transitive DependsOn references to data sources during plan. Refactored the ReferenceMap from the ReferenceTransformer so it can be reused for both. --- terraform/transform_reference.go | 138 ++++++++++++++++++++++++------- 1 file changed, 109 insertions(+), 29 deletions(-) diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 600ec9d5c..6d92f6969 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -7,11 +7,9 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/lang" - "github.com/hashicorp/terraform/states" ) // GraphNodeReferenceable must be implemented by any node that represents @@ -122,21 +120,81 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { return nil } +type depMap map[string]addrs.ConfigResource + +// addDep adds the vertex if it represents a resource in the +// graph. +func (m depMap) add(v dag.Vertex) { + // we're only concerned with resources which may have changes that + // need to be applied. + switch v := v.(type) { + case GraphNodeResourceInstance: + instAddr := v.ResourceInstanceAddr() + addr := instAddr.ContainingResource().Config() + m[addr.String()] = addr + case GraphNodeConfigResource: + addr := v.ResourceAddr() + m[addr.String()] = addr + } +} + +// AttachDependsOnTransformer records all resources transitively referenced +// through a configuration depends_on. +type AttachDependsOnTransformer struct { +} + +func (t AttachDependsOnTransformer) Transform(g *Graph) error { + // First we need to make a map of referenceable addresses to their vertices. + // This is very similar to what's done in ReferenceTransformer, but we keep + // implementation separate as they may need to change independently. + vertices := g.Vertices() + refMap := NewReferenceMap(vertices) + + for _, v := range vertices { + depender, ok := v.(GraphNodeAttachDependsOn) + if !ok { + continue + } + selfAddr := depender.ResourceAddr() + + // Only data need to attach depends_on, so they can determine if they + // are eligible to be read during plan. + if selfAddr.Resource.Mode != addrs.DataResourceMode { + continue + } + + // depMap will dedupe and only add resource references + m := make(depMap) + + for _, dep := range refMap.DependsOn(v) { + // any the dependency + m.add(dep) + // and check any ancestors + ans, _ := g.Ancestors(dep) + for _, v := range ans { + m.add(v) + } + } + + deps := make([]addrs.ConfigResource, 0, len(m)) + for _, d := range m { + deps = append(deps, d) + } + + log.Printf("[TRACE] AttachDependsOnTransformer: %s depends on %s", depender.ResourceAddr(), deps) + depender.AttachDependsOn(deps) + } + + return nil +} + // AttachDependenciesTransformer records all resource dependencies for each // instance, and attaches the addresses to the node itself. Managed resource // will record these in the state for proper ordering of destroy operations. type AttachDependenciesTransformer struct { - Config *configs.Config - State *states.State - Schemas *Schemas } func (t AttachDependenciesTransformer) Transform(g *Graph) error { - // TODO: create a list of depends_on resources, and attach these to - // resources during plan (the destroy deps will just be ignored here). - // Data sources can then use the depens_on deps to determine if they can be - // read during plan. - for _, v := range g.Vertices() { attacher, ok := v.(GraphNodeAttachDependencies) if !ok { @@ -247,19 +305,13 @@ func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error { } // ReferenceMap is a structure that can be used to efficiently check -// for references on a graph. -type ReferenceMap struct { - // vertices is a map from internal reference keys (as produced by the - // mapKey method) to one or more vertices that are identified by each key. - // - // A particular reference key might actually identify multiple vertices, - // e.g. in situations where one object is contained inside another. - vertices map[string][]dag.Vertex -} +// for references on a graph, mapping internal reference keys (as produced by +// the mapKey method) to one or more vertices that are identified by each key. +type ReferenceMap map[string][]dag.Vertex // References returns the set of vertices that the given vertex refers to, // and any referenced addresses that do not have corresponding vertices. -func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex { +func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex { rn, ok := v.(GraphNodeReferencer) if !ok { return nil @@ -271,7 +323,7 @@ func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex { subject := ref.Subject key := m.referenceMapKey(v, subject) - if _, exists := m.vertices[key]; !exists { + if _, exists := m[key]; !exists { // If what we were looking for was a ResourceInstance then we // might be in a resource-oriented graph rather than an // instance-oriented graph, and so we'll see if we have the @@ -289,7 +341,38 @@ func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex { } key = m.referenceMapKey(v, subject) } - vertices := m.vertices[key] + vertices := m[key] + for _, rv := range vertices { + // don't include self-references + if rv == v { + continue + } + matches = append(matches, rv) + } + } + + return matches +} + +// DependsOn returns the set of vertices that the given vertex refers to from +// the configured depends_on. +func (m ReferenceMap) DependsOn(v dag.Vertex) []dag.Vertex { + depender, ok := v.(GraphNodeAttachDependsOn) + if !ok { + return nil + } + + var matches []dag.Vertex + + for _, ref := range depender.DependsOn() { + subject := ref.Subject + + key := m.referenceMapKey(v, subject) + vertices, ok := m[key] + if !ok { + log.Printf("[WARN] DependOn: reference not found: %q", subject) + continue + } for _, rv := range vertices { // don't include self-references if rv == v { @@ -370,11 +453,9 @@ func (m *ReferenceMap) referenceMapKey(referrer dag.Vertex, addr addrs.Reference // NewReferenceMap is used to create a new reference map for the // given set of vertices. -func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { - var m ReferenceMap - +func NewReferenceMap(vs []dag.Vertex) ReferenceMap { // Build the lookup table - vertices := make(map[string][]dag.Vertex) + m := make(ReferenceMap) for _, v := range vs { // We're only looking for referenceable nodes rn, ok := v.(GraphNodeReferenceable) @@ -387,12 +468,11 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { // Go through and cache them for _, addr := range rn.ReferenceableAddrs() { key := m.mapKey(path, addr) - vertices[key] = append(vertices[key], v) + m[key] = append(m[key], v) } } - m.vertices = vertices - return &m + return m } // ReferencesFromConfig returns the references that a configuration has From 18ca98a064fec2b5d194dd98aa67bd29d57d8eb4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 May 2020 20:42:45 -0400 Subject: [PATCH 03/21] GetConfigResourceChanges from plans In order to find any changes related to a particular configuration address, we need a new method to get changes to all possible instances. --- plans/changes.go | 16 ++++++++++++++++ plans/changes_sync.go | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/plans/changes.go b/plans/changes.go index d7c86e6c2..04d070410 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -52,6 +52,22 @@ func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInst } return nil + +} + +// ConfigResourceInstances returns the planned change for the current objects +// of the resource instances of the given address, if any. Returns nil if no +// changes are planned. +func (c *Changes) ConfigResourceInstances(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { + var changes []*ResourceInstanceChangeSrc + for _, rc := range c.Resources { + resAddr := rc.Addr.ContainingResource().Config() + if resAddr.Equal(addr) && rc.DeposedKey == states.NotDeposed { + changes = append(changes, rc) + } + } + + return changes } // ResourceInstanceDeposed returns the plan change of a deposed object of diff --git a/plans/changes_sync.go b/plans/changes_sync.go index 56323e0e9..97cac918e 100644 --- a/plans/changes_sync.go +++ b/plans/changes_sync.go @@ -62,6 +62,29 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance, panic(fmt.Sprintf("unsupported generation value %#v", gen)) } +// GetConfigResourceChanges searched the set of resource instance +// changes and returns all changes related to a given configuration address. +// This is be used to find possible changes related to a configuration +// reference. +// +// If no such changes exist, nil is returned. +// +// The returned objects are a deep copy of the change recorded in the plan, so +// callers may mutate them although it's generally better (less confusing) to +// treat planned changes as immutable after they've been initially constructed. +func (cs *ChangesSync) GetConfigResourceChanges(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { + if cs == nil { + panic("GetConfigResourceChanges on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + var changes []*ResourceInstanceChangeSrc + for _, c := range cs.changes.ConfigResourceInstances(addr) { + changes = append(changes, c.DeepCopy()) + } + return changes +} + // RemoveResourceInstanceChange searches the set of resource instance changes // for one matching the given address and generation, and removes it from the // set if it exists. From 23e259a68c227cdc36c1e6e6c3f709285d0eac61 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 May 2020 21:45:17 -0400 Subject: [PATCH 04/21] add AttachDependsOnTransformer to plan This transformer is what will provider the data sources with the transitive dependencies needed to determine if they can read during plan or must be deferred. --- terraform/graph_builder_plan.go | 4 ++++ terraform/transform_reference.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 82974e949..eb5d3a437 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -150,6 +150,10 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // have to connect again later for providers and so on. &ReferenceTransformer{}, + // Make sure data sources are aware of any depends_on from the + // configuration + &AttachDependsOnTransformer{}, + // Add the node to fix the state count boundaries &CountBoundaryTransformer{ Config: b.Config, diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 6d92f6969..d246a56ce 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -163,7 +163,7 @@ func (t AttachDependsOnTransformer) Transform(g *Graph) error { continue } - // depMap will dedupe and only add resource references + // depMap will only add resource references and dedupe m := make(depMap) for _, dep := range refMap.DependsOn(v) { From 0f5dab48389ef67674292aae64ee2e1b1aa0f761 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 May 2020 21:49:05 -0400 Subject: [PATCH 05/21] always load data source state during refresh We need to load the state during refresh, so that even if the data source can't be read due to `depends_on`, the state can be saved back again to prevent it from being lost altogether. This is a step towards having data sources refresh like resources, which will be from their saved state value. --- terraform/node_data_refresh.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 8e1b9d5f0..57b3a91b1 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -205,15 +205,11 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { Schema: &providerSchema, }, - // Always destroy the existing state first, since we must - // make sure that values from a previous read will not - // get interpolated if we end up needing to defer our - // loading until apply time. - &EvalWriteState{ + &EvalReadState{ Addr: addr.Resource, - ProviderAddr: n.ResolvedProvider, - State: &state, // a pointer to nil, here + Provider: &provider, ProviderSchema: &providerSchema, + Output: &state, }, // EvalReadData will _attempt_ to read the data source, but may @@ -229,12 +225,6 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { OutputChange: &change, OutputConfigValue: &configVal, OutputState: &state, - // If the config explicitly has a depends_on for this data - // source, assume the intention is to prevent refreshing ahead - // of that dependency, and therefore we need to deal with this - // resource during the apply phase. We do that by forcing this - // read to result in a plan. - ForcePlanRead: len(n.Config.DependsOn) > 0, }, &EvalIf{ From 7df0f6c1fc2b4e450ce82c6f5be8b6d351ff1073 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 May 2020 21:53:43 -0400 Subject: [PATCH 06/21] read data sources during plan In order to udpate data sources correctly when their configuration changes, they need to be evaluated during plan. Since the plan working state isn't saved, store any data source reads as plan changes to be applied later. This is currently abusing the Update plan action to indicate that the data source was read and needs to be applied to state. We can possibly add a Store action for data sources if this approach works out. The Read action still indicates that the data source was deferred to the Apply phase. We also fully handle any data source depends_on changes. Now that all the transitive resource dependencies are known at the time of evaluation, we can check the plan to determine if there are any changes in the dependencies and selectively defer reading the data source. --- terraform/eval_read_data.go | 115 +++++++++++++++++------ terraform/node_resource_plan_instance.go | 38 ++++---- 2 files changed, 104 insertions(+), 49 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index b3f7fdbe5..d11c90110 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -39,7 +39,7 @@ type EvalReadData struct { // _always_ generate a plan. This is used during the plan walk, since we // mustn't actually apply anything there. (The resulting state doesn't // get persisted) - ForcePlanRead bool + ForcePlanRead *bool // The result from this EvalNode has a few different possibilities // depending on the input: @@ -57,6 +57,10 @@ type EvalReadData struct { // non-error outcome is to set Output.Action (if non-nil) to a plans.NoOp // change and put the complete resulting state in OutputState, ready to // be saved in the overall state and used for expression evaluation. + // + // FIXME: these fields are a mess. OutputValue is getting the config passed + // in, OutputState is passed in as well, and OuputValue is replaced with + // the state value which goes in OutputState. OutputChange **plans.ResourceInstanceChange OutputValue *cty.Value OutputConfigValue *cty.Value @@ -64,15 +68,24 @@ type EvalReadData struct { } func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { + state := *n.OutputState absAddr := n.Addr.Absolute(ctx.Path()) - log.Printf("[TRACE] EvalReadData: working on %s", absAddr) + + var planned *plans.ResourceInstanceChange + if n.Planned != nil { + planned = *n.Planned + } + + forcePlanRead := false + if n.ForcePlanRead != nil { + forcePlanRead = *n.ForcePlanRead + } 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 // TODO: Do we need to handle Delete changes here? EvalReadDataDiff and @@ -90,11 +103,17 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) } - // We'll always start by evaluating the configuration. What we do after - // that will depend on the evaluation result along with what other inputs - // we were given. + // While data source are read-only, and don't necessarily use the prior + // state, we record it here and use it to determine if we have a change or + // not. If we needed to read a new value, but it still matches the + // previous state, then we can record a NoNop change. If the states don't + // match then we record a Read change so that the new value is applied to + // the state. objTy := schema.ImpliedType() - priorVal := cty.NullVal(objTy) // for data resources, prior is always null because we start fresh every time + priorVal := cty.NullVal(objTy) + if state != nil { + priorVal = state.Value + } forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) @@ -130,20 +149,21 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + configKnown := configVal.IsWhollyKnown() // If our configuration contains any unknown values then we must defer the // read to the apply phase by producing a "Read" change for this resource, // and a placeholder value for it in the state. - if n.ForcePlanRead || !configVal.IsWhollyKnown() { + if forcePlanRead || !configKnown { // If the configuration is still unknown when we're applying a planned // change then that indicates a bug in Terraform, since we should have // everything resolved by now. - if n.Planned != nil && *n.Planned != nil { + if planned != nil { return nil, fmt.Errorf( "configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)", absAddr, ) } - if n.ForcePlanRead { + if configKnown { log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) @@ -156,7 +176,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - change = &plans.ResourceInstanceChange{ + change := &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ @@ -182,10 +202,11 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { if n.OutputConfigValue != nil { *n.OutputConfigValue = configVal } + if n.OutputState != nil { state := &states.ResourceInstanceObject{ - Value: change.After, - Status: states.ObjectPlanned, // because the partial value in the plan must be used for now + Value: cty.NullVal(objTy), + Status: states.ObjectPlanned, } *n.OutputState = state } @@ -193,15 +214,49 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } - if n.Planned != nil && *n.Planned != nil && (*n.Planned).Action != plans.Read { - // If any other action gets in here then that's always a bug; this - // EvalNode only deals with reading. - return nil, fmt.Errorf( - "invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)", - (*n.Planned).Action, absAddr, - ) + if planned != nil { + if !(planned.Action == plans.Read || planned.Action == plans.Update) { + // If any other action gets in here then that's always a bug; this + // EvalNode only deals with reading. + return nil, fmt.Errorf( + "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", + planned.Action, absAddr, + ) + } + + // we have a change and it is complete, which means we read the data + // source during plan. + if planned.Action == plans.Update { + state = &states.ResourceInstanceObject{ + Value: planned.After, + Status: states.ObjectReady, + } + + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostRefresh(absAddr, states.CurrentGen, planned.Before, planned.After) + }) + if err != nil { + return nil, err + } + + if n.OutputChange != nil { + *n.OutputChange = planned + } + if n.OutputValue != nil { + *n.OutputValue = planned.After + } + if n.OutputConfigValue != nil { + *n.OutputConfigValue = configVal + } + if n.OutputState != nil { + *n.OutputState = state + } + return nil, diags.ErrWithWarnings() + } } + var change *plans.ResourceInstanceChange + log.Printf("[TRACE] Re-validating config for %s", absAddr) validateResp := provider.ValidateDataSourceConfig( providers.ValidateDataSourceConfigRequest{ @@ -266,7 +321,8 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { ), )) } - if !newVal.IsWhollyKnown() { + + if !newVal.IsNull() && !newVal.IsWhollyKnown() { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Provider produced invalid object", @@ -285,19 +341,24 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { newVal = cty.UnknownAsNull(newVal) } - // Since we've completed the read, we actually have no change to make, but - // we'll produce a NoOp one anyway to preserve the usual flow of the - // plan phase and allow it to produce a complete plan. + action := plans.NoOp + if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { + // FIXME: for now we are abusing Update to mean "apply this new value" + action = plans.Update + } + + // Produce a change regardless of the outcome. change = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ - Action: plans.NoOp, - Before: newVal, + Action: action, + Before: priorVal, After: newVal, }, } - state := &states.ResourceInstanceObject{ + + state = &states.ResourceInstanceObject{ Value: change.After, Status: states.ObjectReady, // because we completed the read from the provider } diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 38c115842..7102ded3c 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -53,6 +53,8 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou var state *states.ResourceInstanceObject var configVal cty.Value + forcePlanRead := new(bool) + return &EvalSequence{ Nodes: []EvalNode{ &EvalGetProvider{ @@ -76,30 +78,22 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou If: func(ctx EvalContext) (bool, error) { depChanges := false - // Check and see if any of our dependencies have changes. + // Check and see if any depends_on dependencies have + // changes, since they won't show up as changes in the + // configuration. changes := ctx.Changes() - for _, d := range n.References() { - ri, ok := d.Subject.(addrs.ResourceInstance) - if !ok { - continue + depChanges = func() bool { + for _, d := range n.dependsOn { + for _, change := range changes.GetConfigResourceChanges(d) { + if change != nil && change.Action != plans.NoOp { + return true + } + } } - change := changes.GetResourceInstanceChange(ri.Absolute(ctx.Path()), states.CurrentGen) - if change != nil && change.Action != plans.NoOp { - depChanges = true - break - } - } + return false + }() - refreshed := state != nil && state.Status != states.ObjectPlanned - - // If there are no dependency changes, and it's not a forced - // read because we there was no Refresh, then we don't need - // to re-read. If any dependencies have changes, it means - // our config may also have changes and we need to Read the - // data source again. - if !depChanges && refreshed { - return false, EvalEarlyExitError{} - } + *forcePlanRead = depChanges return true, nil }, Then: EvalNoop{}, @@ -118,7 +112,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderAddr: n.ResolvedProvider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - ForcePlanRead: true, // _always_ produce a Read change, even if the config seems ready + ForcePlanRead: forcePlanRead, OutputChange: &change, OutputValue: &configVal, OutputState: &state, From 924162923af33973d335a855a7e75501aed46859 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 5 May 2020 11:45:33 -0400 Subject: [PATCH 07/21] Fix some plan tests with planned data Start fixing plan tests that don't expect data sources to be in the plan. A few were just checking that Read was never called, and some expected the data source to be nil. --- terraform/context_plan_test.go | 36 ++++++++++++---------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 7a97f7b23..1ecc306ca 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1892,10 +1892,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) { _, diags = ctx.Plan() // should do nothing with data resource in this step, since it was already read assertNoErrors(t, diags) - if p.ReadDataSourceCalled { - t.Fatalf("ReadDataSource was called on provider during plan; should not have been called") + if !p.ReadDataSourceCalled { + t.Fatalf("ReadDataSource should have been called") } - } func TestContext2Plan_computedDataCountResource(t *testing.T) { @@ -4992,8 +4991,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { } } p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + cfg := req.Config.AsValueMap() + cfg["id"] = cty.StringVal("data_id") return providers.ReadDataSourceResponse{ - Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")), + State: cty.ObjectVal(cfg), } } @@ -5010,9 +5011,6 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { // thus the plan call below is forced to produce a deferred read action. plan, diags := ctx.Plan() - if p.ReadDataSourceCalled { - t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh") - } if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -5042,7 +5040,7 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { } checkVals(t, objectVal(t, schema, map[string]cty.Value{ "num": cty.StringVal("2"), - "computed": cty.UnknownVal(cty.String), + "computed": cty.StringVal("data_id"), }), ric.After) case "aws_instance.foo[1]": if res.Action != plans.Create { @@ -5050,30 +5048,22 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { } checkVals(t, objectVal(t, schema, map[string]cty.Value{ "num": cty.StringVal("2"), - "computed": cty.UnknownVal(cty.String), + "computed": cty.StringVal("data_id"), }), ric.After) case "data.aws_vpc.bar[0]": - if res.Action != plans.Read { - t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) + if res.Action != plans.Update { + t.Fatalf("resource %s should be update, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ - // In a normal flow we would've read an exact value in - // ReadDataSource, but because this test doesn't run - // cty.Refresh we have no opportunity to do that lookup - // and a deferred read is forced. - "id": cty.UnknownVal(cty.String), + "id": cty.StringVal("data_id"), "foo": cty.StringVal("0"), }), ric.After) case "data.aws_vpc.bar[1]": - if res.Action != plans.Read { - t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) + if res.Action != plans.Update { + t.Fatalf("resource %s should be update, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ - // In a normal flow we would've read an exact value in - // ReadDataSource, but because this test doesn't run - // cty.Refresh we have no opportunity to do that lookup - // and a deferred read is forced. - "id": cty.UnknownVal(cty.String), + "id": cty.StringVal("data_id"), "foo": cty.StringVal("1"), }), ric.After) default: From 047c9b3cc6e894c6a48f34181bca6cdd451215c2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 5 May 2020 21:23:27 -0400 Subject: [PATCH 08/21] missing id in schema in plan test The data source was never read, so the schema was never verified. --- terraform/context_plan_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 1ecc306ca..4ff1dc5f2 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1992,6 +1992,7 @@ func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) { DataSources: map[string]*configschema.Block{ "aws_data_source": { Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, "foo": {Type: cty.String, Optional: true}, }, }, From cf2dd43f301a7eea751de0f5e7e444ce7ededb98 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 5 May 2020 21:34:31 -0400 Subject: [PATCH 09/21] add data responses where they were missing A few test with had data sources that were never read before, and needed to get valid responses for the tests. --- terraform/context_plan_test.go | 16 +++++++++++++++- terraform/context_refresh_test.go | 5 +++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 4ff1dc5f2..282f07f1b 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5504,11 +5504,18 @@ func TestContext2Plan_invalidOutput(t *testing.T) { data "aws_data_source" "name" {} output "out" { - value = "${data.aws_data_source.name.missing}" + value = data.aws_data_source.name.missing }`, }) p := testProvider("aws") + p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data_id"), + "foo": cty.StringVal("foo"), + }), + } + ctx := testContext2(t, &ContextOpts{ Config: m, Providers: map[addrs.Provider]providers.Factory{ @@ -5549,6 +5556,13 @@ resource "aws_instance" "foo" { }) p := testProvider("aws") + p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data_id"), + "foo": cty.StringVal("foo"), + }), + } + ctx := testContext2(t, &ContextOpts{ Config: m, Providers: map[addrs.Provider]providers.Factory{ diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 4f739e129..9ae8c55cc 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1612,6 +1612,11 @@ func TestContext2Refresh_dataResourceDependsOn(t *testing.T) { }, } p.DiffFn = testDiffFn + p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "compute": cty.StringVal("value"), + }), + } state := states.NewState() root := state.EnsureModule(addrs.RootModuleInstance) From be20a7941d74df59f3eaad84704b36e9e47b861c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 6 May 2020 15:02:52 -0400 Subject: [PATCH 10/21] remove EvalReadDataApply EvalReadDataApply was all dead code, as it was only using during delete and simply set the state to nil. --- terraform/eval_read_data.go | 111 ----------------------------- terraform/node_resource_destroy.go | 17 ++--- 2 files changed, 6 insertions(+), 122 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index d11c90110..0d852b6f1 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -385,114 +385,3 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } - -// EvalReadDataApply is an EvalNode implementation that executes a data -// resource's ReadDataApply method to read data from the data source. -type EvalReadDataApply struct { - Addr addrs.ResourceInstance - Provider *providers.Interface - ProviderAddr addrs.AbsProviderConfig - ProviderMeta *configs.ProviderMeta - ProviderSchema **ProviderSchema - Output **states.ResourceInstanceObject - Config *configs.Resource - Change **plans.ResourceInstanceChange -} - -func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { - provider := *n.Provider - change := *n.Change - providerSchema := *n.ProviderSchema - absAddr := n.Addr.Absolute(ctx.Path()) - - var diags tfdiags.Diagnostics - - // If the diff is for *destroying* this resource then we'll - // just drop its state and move on, since data resources don't - // support an actual "destroy" action. - if change != nil && change.Action == plans.Delete { - if n.Output != nil { - *n.Output = nil - } - return nil, nil - } - - metaConfigVal := cty.NullVal(cty.DynamicPseudoType) - if n.ProviderMeta != nil { - // if the provider doesn't support this feature, throw an error - if (*n.ProviderSchema).ProviderMeta == nil { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: fmt.Sprintf("Provider %s doesn't support provider_meta", n.ProviderAddr.Provider.String()), - Detail: fmt.Sprintf("The resource %s belongs to a provider that doesn't support provider_meta blocks", n.Addr), - Subject: &n.ProviderMeta.ProviderRange, - }) - } else { - var configDiags tfdiags.Diagnostics - metaConfigVal, _, configDiags = ctx.EvaluateBlock(n.ProviderMeta.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return nil, diags.Err() - } - } - } - - // For the purpose of external hooks we present a data apply as a - // "Refresh" rather than an "Apply" because creating a data source - // is presented to users/callers as a "read" operation. - err := ctx.Hook(func(h Hook) (HookAction, error) { - // We don't have a state yet, so we'll just give the hook an - // empty one to work with. - return h.PreRefresh(absAddr, states.CurrentGen, cty.NullVal(cty.DynamicPseudoType)) - }) - if err != nil { - return nil, err - } - - resp := provider.ReadDataSource(providers.ReadDataSourceRequest{ - TypeName: n.Addr.Resource.Type, - Config: change.After, - ProviderMeta: metaConfigVal, - }) - diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config)) - if diags.HasErrors() { - return nil, diags.Err() - } - - 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 data source %q", n.Addr.Resource.Type) - } - - newVal := resp.State - for _, err := range newVal.Type().TestConformance(schema.ImpliedType()) { - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Provider produced invalid object", - fmt.Sprintf( - "Provider %q planned an invalid value for %s. The result could not be saved.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.", - n.ProviderAddr.Provider.String(), tfdiags.FormatErrorPrefixed(err, absAddr.String()), - ), - )) - } - if diags.HasErrors() { - return nil, diags.Err() - } - - err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostRefresh(absAddr, states.CurrentGen, change.Before, newVal) - }) - if err != nil { - return nil, err - } - - if n.Output != nil { - *n.Output = &states.ResourceInstanceObject{ - Value: newVal, - Status: states.ObjectReady, - } - } - - return nil, diags.ErrWithWarnings() -} diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 0c848fc5c..da154257d 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -237,19 +237,14 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { // Make sure we handle data sources properly. &EvalIf{ If: func(ctx EvalContext) (bool, error) { - return addr.Resource.Resource.Mode == addrs.DataResourceMode, nil + if addr.Resource.Resource.Mode == addrs.DataResourceMode { + // deleting a data source is simply removing the state + state = nil + } + return addr.Resource.Resource.Mode == addrs.ManagedResourceMode, nil }, - Then: &EvalReadDataApply{ - Addr: addr.Resource, - Config: n.Config, - Change: &changeApply, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - Output: &state, - }, - Else: &EvalApply{ + Then: &EvalApply{ Addr: addr.Resource, Config: nil, // No configuration because we are destroying State: &state, From 4a92b7888f486245fa892fddf830a7995ee39a4a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 6 May 2020 18:04:09 -0400 Subject: [PATCH 11/21] start to refactor EvalReadData Remove extra fields, remove the depends_on logic from NodePlannableResourceInstnace, and start breaking up the massive Eval method. --- terraform/eval_read_data.go | 282 +++++++++++----------- terraform/node_data_refresh.go | 20 +- terraform/node_resource_apply_instance.go | 6 +- terraform/node_resource_plan_instance.go | 40 +-- 4 files changed, 149 insertions(+), 199 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 0d852b6f1..6219f695b 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -34,66 +34,60 @@ type EvalReadData struct { // in this planned change. Planned **plans.ResourceInstanceChange - // ForcePlanRead, if true, overrides the usual behavior of immediately - // reading from the data source where possible, instead forcing us to - // _always_ generate a plan. This is used during the plan walk, since we - // mustn't actually apply anything there. (The resulting state doesn't - // get persisted) - ForcePlanRead *bool + // State is the current state for the data source, and is updated once the + // new state has need read. + // While data source are read-only, we need to start with the prior state + // to determine if we have a change or not. If we needed to read a new + // value, but it still matches the previous state, then we can record a + // NoNop change. If the states don't match then we record a Read change so + // that the new value is applied to the state. + State **states.ResourceInstanceObject // The result from this EvalNode has a few different possibilities // depending on the input: - // - If Planned is nil then we assume we're aiming to _produce_ the plan, - // and so the following two outcomes are possible: - // - OutputChange.Action is plans.NoOp and OutputState is the complete - // result of reading from the data source. This is the easy path. - // - OutputChange.Action is plans.Read and OutputState is a planned + // - If Planned is nil then we assume we're aiming to either read the + // resource or produce a plan, and so the following two outcomes are + // possible: + // - OutputChange.Action is plans.NoOp and the + // result of reading from the data source is stored in state. This is + // the easy path, and only happens during refresh. + // - OutputChange.Action is plans.Read and State is a planned // object placeholder (states.ObjectPlanned). In this case, the - // returned change must be recorded in the overral changeset and - // eventually passed to another instance of this struct during the - // apply walk. + // returned change must be recorded in the overall changeset and this + // resource will be read during apply. + // - OutputChange.Action is plans.Update, in which case the change + // contains the complete state of this resource, and only needs to be + // stored into the final state during apply. // - If Planned is non-nil then we assume we're aiming to complete a - // planned read from an earlier plan walk. In this case the only possible - // non-error outcome is to set Output.Action (if non-nil) to a plans.NoOp - // change and put the complete resulting state in OutputState, ready to - // be saved in the overall state and used for expression evaluation. - // - // FIXME: these fields are a mess. OutputValue is getting the config passed - // in, OutputState is passed in as well, and OuputValue is replaced with - // the state value which goes in OutputState. - OutputChange **plans.ResourceInstanceChange - OutputValue *cty.Value - OutputConfigValue *cty.Value - OutputState **states.ResourceInstanceObject + // planned read from an earlier plan walk, from one of the options above. + OutputChange **plans.ResourceInstanceChange + + // dependsOn stores the list of transitive resource addresses that any + // configuration depends_on references may resolve to. This is used to + // determine if there are any changes that will force this data sources to + // be deferred to apply. + dependsOn []addrs.ConfigResource + + // refresh indicates this is being called from a refresh node, and we can't + // resolve any depends_on dependencies. + refresh bool } func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { - state := *n.OutputState absAddr := n.Addr.Absolute(ctx.Path()) + var diags tfdiags.Diagnostics + var configVal cty.Value + var planned *plans.ResourceInstanceChange if n.Planned != nil { planned = *n.Planned } - forcePlanRead := false - if n.ForcePlanRead != nil { - forcePlanRead = *n.ForcePlanRead - } - if n.ProviderSchema == nil || *n.ProviderSchema == nil { return nil, fmt.Errorf("provider schema not available for %s", n.Addr) } - var diags tfdiags.Diagnostics - var configVal cty.Value - - // TODO: Do we need to handle Delete changes here? EvalReadDataDiff and - // EvalReadDataApply did, but it seems like we should handle that via a - // separate mechanism since it boils down to just deleting the object from - // the state... and we do that on every plan anyway, forcing the data - // resource to re-read. - config := *n.Config provider := *n.Provider providerSchema := *n.ProviderSchema @@ -103,19 +97,13 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) } - // While data source are read-only, and don't necessarily use the prior - // state, we record it here and use it to determine if we have a change or - // not. If we needed to read a new value, but it still matches the - // previous state, then we can record a NoNop change. If the states don't - // match then we record a Read change so that the new value is applied to - // the state. objTy := schema.ImpliedType() priorVal := cty.NullVal(objTy) - if state != nil { - priorVal = state.Value + if n.State != nil && *n.State != nil { + priorVal = (*n.State).Value } - forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx) + forEach, _ := evaluateForEachExpression(config.ForEach, ctx) keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) var configDiags tfdiags.Diagnostics @@ -125,50 +113,26 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.Err() } - metaConfigVal := cty.NullVal(cty.DynamicPseudoType) - if n.ProviderMetas != nil { - if m, ok := n.ProviderMetas[n.ProviderAddr.Provider]; ok && m != nil { - // if the provider doesn't support this feature, throw an error - if (*n.ProviderSchema).ProviderMeta == nil { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: fmt.Sprintf("Provider %s doesn't support provider_meta", n.ProviderAddr.Provider.String()), - Detail: fmt.Sprintf("The resource %s belongs to a provider that doesn't support provider_meta blocks", n.Addr), - Subject: &m.ProviderRange, - }) - } else { - var configDiags tfdiags.Diagnostics - metaConfigVal, _, configDiags = ctx.EvaluateBlock(m.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return nil, diags.Err() - } - } - } + metaConfigVal, metaDiags := n.providerMetas(ctx) + diags = diags.Append(metaDiags) + if diags.HasErrors() { + return nil, diags.Err() } - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) - configKnown := configVal.IsWhollyKnown() - // If our configuration contains any unknown values then we must defer the - // read to the apply phase by producing a "Read" change for this resource, - // and a placeholder value for it in the state. - if forcePlanRead || !configKnown { - // If the configuration is still unknown when we're applying a planned - // change then that indicates a bug in Terraform, since we should have - // everything resolved by now. - if planned != nil { - return nil, fmt.Errorf( - "configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)", - absAddr, - ) - } + // If our configuration contains any unknown values, or we depend on any + // unknown values then we must defer the read to the apply phase by + // producing a "Read" change for this resource, and a placeholder value for + // it in the state. + if n.forcePlanRead(ctx) || !configKnown { if configKnown { log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) } + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) }) @@ -196,63 +160,47 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { if n.OutputChange != nil { *n.OutputChange = change } - if n.OutputValue != nil { - *n.OutputValue = change.After - } - if n.OutputConfigValue != nil { - *n.OutputConfigValue = configVal - } - - if n.OutputState != nil { - state := &states.ResourceInstanceObject{ + if n.State != nil { + *n.State = &states.ResourceInstanceObject{ Value: cty.NullVal(objTy), Status: states.ObjectPlanned, } - *n.OutputState = state } return nil, diags.ErrWithWarnings() } - if planned != nil { - if !(planned.Action == plans.Read || planned.Action == plans.Update) { - // If any other action gets in here then that's always a bug; this - // EvalNode only deals with reading. - return nil, fmt.Errorf( - "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", - planned.Action, absAddr, - ) + if planned != nil && !(planned.Action == plans.Read || planned.Action == plans.Update) { + // If any other action gets in here then that's always a bug; this + // EvalNode only deals with reading. + return nil, fmt.Errorf( + "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", + planned.Action, absAddr, + ) + } + + // we have a change and it is complete, which means we read the data + // source during plan and only need to store it in state. + if planned != nil && planned.Action == plans.Update { + outputState := &states.ResourceInstanceObject{ + Value: planned.After, + Status: states.ObjectReady, } - // we have a change and it is complete, which means we read the data - // source during plan. - if planned.Action == plans.Update { - state = &states.ResourceInstanceObject{ - Value: planned.After, - Status: states.ObjectReady, - } - - err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostRefresh(absAddr, states.CurrentGen, planned.Before, planned.After) - }) - if err != nil { - return nil, err - } - - if n.OutputChange != nil { - *n.OutputChange = planned - } - if n.OutputValue != nil { - *n.OutputValue = planned.After - } - if n.OutputConfigValue != nil { - *n.OutputConfigValue = configVal - } - if n.OutputState != nil { - *n.OutputState = state - } - return nil, diags.ErrWithWarnings() + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) + }) + if err != nil { + return nil, err } + + if n.OutputChange != nil { + *n.OutputChange = planned + } + if n.State != nil { + *n.State = outputState + } + return nil, diags.ErrWithWarnings() } var change *plans.ResourceInstanceChange @@ -265,7 +213,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { }, ) if validateResp.Diagnostics.HasErrors() { - return nil, validateResp.Diagnostics.InConfigBody(n.Config.Config).Err() + return nil, validateResp.Diagnostics.InConfigBody(config.Config).Err() } // If we get down here then our configuration is complete and we're read @@ -275,7 +223,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { err := ctx.Hook(func(h Hook) (HookAction, error) { // We don't have a state yet, so we'll just give the hook an // empty one to work with. - return h.PreRefresh(absAddr, states.CurrentGen, cty.NullVal(cty.DynamicPseudoType)) + return h.PreRefresh(absAddr, states.CurrentGen, priorVal) }) if err != nil { return nil, err @@ -286,7 +234,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { Config: configVal, ProviderMeta: metaConfigVal, }) - diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config)) if diags.HasErrors() { return nil, diags.Err() } @@ -343,7 +291,8 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { action := plans.NoOp if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { - // FIXME: for now we are abusing Update to mean "apply this new value" + // since a data source is read-only, update here only means that we + // need to update the state. action = plans.Update } @@ -358,13 +307,13 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { }, } - state = &states.ResourceInstanceObject{ - Value: change.After, + outputState := &states.ResourceInstanceObject{ + Value: newVal, Status: states.ObjectReady, // because we completed the read from the provider } err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostRefresh(absAddr, states.CurrentGen, change.Before, newVal) + return h.PostRefresh(absAddr, states.CurrentGen, priorVal, newVal) }) if err != nil { return nil, err @@ -373,15 +322,56 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { if n.OutputChange != nil { *n.OutputChange = change } - if n.OutputValue != nil { - *n.OutputValue = change.After - } - if n.OutputConfigValue != nil { - *n.OutputConfigValue = configVal - } - if n.OutputState != nil { - *n.OutputState = state + if n.State != nil { + *n.State = outputState } return nil, diags.ErrWithWarnings() } + +func (n *EvalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + metaConfigVal := cty.NullVal(cty.DynamicPseudoType) + if n.ProviderMetas != nil { + if m, ok := n.ProviderMetas[n.ProviderAddr.Provider]; ok && m != nil { + // if the provider doesn't support this feature, throw an error + if (*n.ProviderSchema).ProviderMeta == nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Provider %s doesn't support provider_meta", n.ProviderAddr.Provider.String()), + Detail: fmt.Sprintf("The resource %s belongs to a provider that doesn't support provider_meta blocks", n.Addr), + Subject: &m.ProviderRange, + }) + } else { + var configDiags tfdiags.Diagnostics + metaConfigVal, _, configDiags = ctx.EvaluateBlock(m.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey) + diags = diags.Append(configDiags) + } + } + } + return metaConfigVal, diags +} + +// ForcePlanRead, if true, overrides the usual behavior of immediately +// reading from the data source where possible, instead forcing us to +// _always_ generate a plan. This is used during the plan walk, since we +// mustn't actually apply anything there. (The resulting state doesn't +// get persisted) +func (n *EvalReadData) forcePlanRead(ctx EvalContext) bool { + if n.refresh && len(n.Config.DependsOn) > 0 { + return true + } + + // Check and see if any depends_on dependencies have + // changes, since they won't show up as changes in the + // configuration. + changes := ctx.Changes() + for _, d := range n.dependsOn { + for _, change := range changes.GetConfigResourceChanges(d) { + if change != nil && change.Action != plans.NoOp { + return true + } + } + } + return false +} diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 57b3a91b1..a3e37bf3e 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -7,7 +7,6 @@ import ( "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/tfdiags" - "github.com/zclconf/go-cty/cty" ) type nodeExpandRefreshableDataResource struct { @@ -195,7 +194,6 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { var providerSchema *ProviderSchema var change *plans.ResourceInstanceChange var state *states.ResourceInstanceObject - var configVal cty.Value return &EvalSequence{ Nodes: []EvalNode{ @@ -216,15 +214,15 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { // generate an incomplete planned object if the configuration // includes values that won't be known until apply. &EvalReadData{ - Addr: addr.Resource, - Config: n.Config, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - OutputChange: &change, - OutputConfigValue: &configVal, - OutputState: &state, + Addr: addr.Resource, + Config: n.Config, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + OutputChange: &change, + State: &state, + refresh: true, }, &EvalIf{ diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 2a55f23a7..9c2dfbb27 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -3,8 +3,6 @@ package terraform import ( "fmt" - "github.com/zclconf/go-cty/cty" - "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/plans" @@ -182,7 +180,7 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderAddr: n.ResolvedProvider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - OutputState: &state, + State: &state, }, &EvalWriteState{ @@ -215,7 +213,6 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe var err error var createNew bool var createBeforeDestroyEnabled bool - var configVal cty.Value var deposedKey states.DeposedKey return &EvalSequence{ @@ -293,7 +290,6 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe State: &state, PreviousDiff: &diff, OutputChange: &diffApply, - OutputValue: &configVal, OutputState: &state, }, diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 7102ded3c..c12de1eec 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/addrs" - "github.com/zclconf/go-cty/cty" ) // NodePlannableResourceInstance represents a _single_ resource @@ -51,9 +50,6 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou var providerSchema *ProviderSchema var change *plans.ResourceInstanceChange var state *states.ResourceInstanceObject - var configVal cty.Value - - forcePlanRead := new(bool) return &EvalSequence{ Nodes: []EvalNode{ @@ -71,34 +67,6 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou Output: &state, }, - // If we already have a non-planned state then we already dealt - // with this during the refresh walk and so we have nothing to do - // here. - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - depChanges := false - - // Check and see if any depends_on dependencies have - // changes, since they won't show up as changes in the - // configuration. - changes := ctx.Changes() - depChanges = func() bool { - for _, d := range n.dependsOn { - for _, change := range changes.GetConfigResourceChanges(d) { - if change != nil && change.Action != plans.NoOp { - return true - } - } - } - return false - }() - - *forcePlanRead = depChanges - return true, nil - }, - Then: EvalNoop{}, - }, - &EvalValidateSelfRef{ Addr: addr.Resource, Config: config.Config, @@ -112,10 +80,9 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderAddr: n.ResolvedProvider, ProviderMetas: n.ProviderMetas, ProviderSchema: &providerSchema, - ForcePlanRead: forcePlanRead, OutputChange: &change, - OutputValue: &configVal, - OutputState: &state, + State: &state, + dependsOn: n.dependsOn, }, &EvalWriteState{ @@ -153,8 +120,7 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe Addr: addr.Resource, Provider: &provider, ProviderSchema: &providerSchema, - - Output: &state, + Output: &state, }, &EvalValidateSelfRef{ From 96be76effb8f15c94ec4f118923083cb80124131 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 May 2020 16:10:03 -0400 Subject: [PATCH 12/21] cleanup refresh test --- terraform/context_refresh_test.go | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 9ae8c55cc..64be9098f 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -949,32 +949,11 @@ func TestContext2Refresh_dataState(t *testing.T) { p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { m := req.Config.AsValueMap() - m["inputs"] = cty.MapVal(map[string]cty.Value{"test": cty.StringVal("yes")}) readStateVal = cty.ObjectVal(m) return providers.ReadDataSourceResponse{ State: readStateVal, } - - // FIXME: should the "outputs" value here be added to the reutnred state? - // Attributes: map[string]*ResourceAttrDiff{ - // "inputs.#": { - // Old: "0", - // New: "1", - // Type: DiffAttrInput, - // }, - // "inputs.test": { - // Old: "", - // New: "yes", - // Type: DiffAttrInput, - // }, - // "outputs.#": { - // Old: "", - // New: "", - // NewComputed: true, - // Type: DiffAttrOutput, - // }, - // }, } s, diags := ctx.Refresh() @@ -986,14 +965,6 @@ func TestContext2Refresh_dataState(t *testing.T) { t.Fatal("ReadDataSource should have been called") } - // mod := s.RootModule() - // if got := mod.Resources["data.null_data_source.testing"].Primary.ID; got != "-" { - // t.Fatalf("resource id is %q; want %s", got, "-") - // } - // if !reflect.DeepEqual(mod.Resources["data.null_data_source.testing"].Primary, p.ReadDataApplyReturn) { - // t.Fatalf("bad: %#v", mod.Resources) - // } - mod := s.RootModule() newState, err := mod.Resources["data.null_data_source.testing"].Instances[addrs.NoKey].Current.Decode(schema.ImpliedType()) From 6ca252faab5623c8fd7ec8a0e8d4394233e23253 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 May 2020 21:41:57 -0400 Subject: [PATCH 13/21] refactor EvalReadData The logic for refresh, plan and apply are all subtly different, so rather than trying to manage that complex flow through a giant 300 line method, break it up somewhat into 3 different types that can share the types and a few helpers. --- terraform/eval_read_data.go | 377 +++++++++------------- terraform/eval_read_data_apply.go | 94 ++++++ terraform/eval_read_data_plan.go | 166 ++++++++++ terraform/node_data_refresh.go | 28 +- terraform/node_resource_apply_instance.go | 20 +- terraform/node_resource_plan_instance.go | 22 +- 6 files changed, 456 insertions(+), 251 deletions(-) create mode 100644 terraform/eval_read_data_apply.go create mode 100644 terraform/eval_read_data_plan.go diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 6219f695b..001afd325 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -16,10 +16,9 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) -// EvalReadData is an EvalNode implementation that deals with the main part -// of the data resource lifecycle: either actually reading from the data source -// or generating a plan to do so. -type EvalReadData struct { +// evalReadData implements shared methods and data for the individual data +// source eval nodes. +type evalReadData struct { Addr addrs.ResourceInstance Config *configs.Resource Provider *providers.Interface @@ -35,177 +34,70 @@ type EvalReadData struct { Planned **plans.ResourceInstanceChange // State is the current state for the data source, and is updated once the - // new state has need read. - // While data source are read-only, we need to start with the prior state + // new state has been read. + // While data sources are read-only, we need to start with the prior state // to determine if we have a change or not. If we needed to read a new // value, but it still matches the previous state, then we can record a // NoNop change. If the states don't match then we record a Read change so // that the new value is applied to the state. State **states.ResourceInstanceObject - // The result from this EvalNode has a few different possibilities - // depending on the input: - // - If Planned is nil then we assume we're aiming to either read the - // resource or produce a plan, and so the following two outcomes are - // possible: - // - OutputChange.Action is plans.NoOp and the - // result of reading from the data source is stored in state. This is - // the easy path, and only happens during refresh. - // - OutputChange.Action is plans.Read and State is a planned - // object placeholder (states.ObjectPlanned). In this case, the - // returned change must be recorded in the overall changeset and this - // resource will be read during apply. - // - OutputChange.Action is plans.Update, in which case the change - // contains the complete state of this resource, and only needs to be - // stored into the final state during apply. - // - If Planned is non-nil then we assume we're aiming to complete a - // planned read from an earlier plan walk, from one of the options above. + // Output change records any change for this data source, which is + // interpreted differently than changes for managed resources. + // - During Refresh, this change is only used to correctly evaluate + // references to the data source, but it is not saved. + // - If a planned change has the action of plans.Read, it indicates that the + // data source could not be evaluated yet, and reading is being deferred to + // apply. + // - If planned action is plans.Update, it indicates that the data source + // was read, and the result needs to be stored in state during apply. OutputChange **plans.ResourceInstanceChange - - // dependsOn stores the list of transitive resource addresses that any - // configuration depends_on references may resolve to. This is used to - // determine if there are any changes that will force this data sources to - // be deferred to apply. - dependsOn []addrs.ConfigResource - - // refresh indicates this is being called from a refresh node, and we can't - // resolve any depends_on dependencies. - refresh bool } -func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { - absAddr := n.Addr.Absolute(ctx.Path()) - +// readDataSource handles everything needed to call ReadDataSource on the provider. +// A previously evaluated configVal can be passed in, or a new one is generated +// from the resource configuration. +func (n *evalReadData) readDataSource(ctx EvalContext, configVal cty.Value) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - var configVal cty.Value - - var planned *plans.ResourceInstanceChange - if n.Planned != nil { - planned = *n.Planned - } - - if n.ProviderSchema == nil || *n.ProviderSchema == nil { - return nil, fmt.Errorf("provider schema not available for %s", n.Addr) - } + var newVal cty.Value config := *n.Config - provider := *n.Provider - providerSchema := *n.ProviderSchema - 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 %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) + absAddr := n.Addr.Absolute(ctx.Path()) + + if n.ProviderSchema == nil || *n.ProviderSchema == nil { + diags = diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr)) + return newVal, diags } - objTy := schema.ImpliedType() - priorVal := cty.NullVal(objTy) - if n.State != nil && *n.State != nil { - priorVal = (*n.State).Value - } + provider := *n.Provider + providerSchema := *n.ProviderSchema forEach, _ := evaluateForEachExpression(config.ForEach, ctx) keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) - var configDiags tfdiags.Diagnostics - configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return nil, diags.Err() + schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource()) + if schema == nil { + // Should be caught during validation, so we don't bother with a pretty error here + diags = diags.Append(fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)) + return newVal, diags + } + + if configVal == cty.NilVal { + val, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return newVal, diags + } + configVal = val } metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { - return nil, diags.Err() + return newVal, diags } - configKnown := configVal.IsWhollyKnown() - // If our configuration contains any unknown values, or we depend on any - // unknown values then we must defer the read to the apply phase by - // producing a "Read" change for this resource, and a placeholder value for - // it in the state. - if n.forcePlanRead(ctx) || !configKnown { - if configKnown { - log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) - } else { - log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr) - } - - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) - - err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) - }) - if err != nil { - return nil, err - } - - 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(absAddr, states.CurrentGen, change.Action, priorVal, proposedNewVal) - }) - if err != nil { - return nil, err - } - - if n.OutputChange != nil { - *n.OutputChange = change - } - if n.State != nil { - *n.State = &states.ResourceInstanceObject{ - Value: cty.NullVal(objTy), - Status: states.ObjectPlanned, - } - } - - return nil, diags.ErrWithWarnings() - } - - if planned != nil && !(planned.Action == plans.Read || planned.Action == plans.Update) { - // If any other action gets in here then that's always a bug; this - // EvalNode only deals with reading. - return nil, fmt.Errorf( - "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", - planned.Action, absAddr, - ) - } - - // we have a change and it is complete, which means we read the data - // source during plan and only need to store it in state. - if planned != nil && planned.Action == plans.Update { - outputState := &states.ResourceInstanceObject{ - Value: planned.After, - Status: states.ObjectReady, - } - - err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) - }) - if err != nil { - return nil, err - } - - if n.OutputChange != nil { - *n.OutputChange = planned - } - if n.State != nil { - *n.State = outputState - } - return nil, diags.ErrWithWarnings() - } - - var change *plans.ResourceInstanceChange - - log.Printf("[TRACE] Re-validating config for %s", absAddr) + log.Printf("[TRACE] EvalReadData: Re-validating config for %s", absAddr) validateResp := provider.ValidateDataSourceConfig( providers.ValidateDataSourceConfigRequest{ TypeName: n.Addr.Resource.Type, @@ -213,22 +105,13 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { }, ) if validateResp.Diagnostics.HasErrors() { - return nil, validateResp.Diagnostics.InConfigBody(config.Config).Err() + return newVal, validateResp.Diagnostics.InConfigBody(config.Config) } // If we get down here then our configuration is complete and we're read // to actually call the provider to read the data. log.Printf("[TRACE] EvalReadData: %s configuration is complete, so reading from provider", absAddr) - err := ctx.Hook(func(h Hook) (HookAction, error) { - // We don't have a state yet, so we'll just give the hook an - // empty one to work with. - return h.PreRefresh(absAddr, states.CurrentGen, priorVal) - }) - if err != nil { - return nil, err - } - resp := provider.ReadDataSource(providers.ReadDataSourceRequest{ TypeName: n.Addr.Resource.Type, Config: configVal, @@ -236,9 +119,9 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { }) diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config)) if diags.HasErrors() { - return nil, diags.Err() + return newVal, diags } - newVal := resp.State + newVal = resp.State if newVal == cty.NilVal { // This can happen with incompletely-configured mocks. We'll allow it // and treat it as an alias for a properly-typed null value. @@ -256,7 +139,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { )) } if diags.HasErrors() { - return nil, diags.Err() + return newVal, diags } if newVal.IsNull() { @@ -289,47 +172,10 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { newVal = cty.UnknownAsNull(newVal) } - action := plans.NoOp - if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { - // since a data source is read-only, update here only means that we - // need to update the state. - action = plans.Update - } - - // Produce a change regardless of the outcome. - change = &plans.ResourceInstanceChange{ - Addr: absAddr, - ProviderAddr: n.ProviderAddr, - Change: plans.Change{ - Action: action, - Before: priorVal, - After: newVal, - }, - } - - outputState := &states.ResourceInstanceObject{ - Value: newVal, - Status: states.ObjectReady, // because we completed the read from the provider - } - - err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostRefresh(absAddr, states.CurrentGen, priorVal, newVal) - }) - if err != nil { - return nil, err - } - - if n.OutputChange != nil { - *n.OutputChange = change - } - if n.State != nil { - *n.State = outputState - } - - return nil, diags.ErrWithWarnings() + return newVal, diags } -func (n *EvalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { +func (n *evalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics metaConfigVal := cty.NullVal(cty.DynamicPseudoType) if n.ProviderMetas != nil { @@ -352,26 +198,119 @@ func (n *EvalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagno return metaConfigVal, diags } -// ForcePlanRead, if true, overrides the usual behavior of immediately -// reading from the data source where possible, instead forcing us to -// _always_ generate a plan. This is used during the plan walk, since we -// mustn't actually apply anything there. (The resulting state doesn't -// get persisted) -func (n *EvalReadData) forcePlanRead(ctx EvalContext) bool { - if n.refresh && len(n.Config.DependsOn) > 0 { - return true +// EvalReadDataRefresh is an EvalNode implementation that handled the data +// resource lifecycle during refresh +type EvalReadDataRefresh struct { + evalReadData +} + +func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { + var diags tfdiags.Diagnostics + + if n.ProviderSchema == nil || *n.ProviderSchema == nil { + return nil, fmt.Errorf("provider schema not available for %s", n.Addr) } - // Check and see if any depends_on dependencies have - // changes, since they won't show up as changes in the - // configuration. - changes := ctx.Changes() - for _, d := range n.dependsOn { - for _, change := range changes.GetConfigResourceChanges(d) { - if change != nil && change.Action != plans.NoOp { - return true + absAddr := n.Addr.Absolute(ctx.Path()) + config := *n.Config + providerSchema := *n.ProviderSchema + 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 %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) + } + + objTy := schema.ImpliedType() + priorVal := cty.NullVal(objTy) + if n.State != nil && *n.State != nil { + priorVal = (*n.State).Value + } + + forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) + + configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return nil, diags.Err() + } + + configKnown := configVal.IsWhollyKnown() + // If our configuration contains any unknown values, or we depend on any + // unknown values then we must defer the read to the apply phase by + // producing a "Read" change for this resource, and a placeholder value for + // it in the state. + if len(n.Config.DependsOn) > 0 || !configKnown { + if configKnown { + log.Printf("[TRACE] EvalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + } else { + log.Printf("[TRACE] EvalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr) + } + + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + + // We need to store a change so tat other references to this data + // source can resolve correctly, since the state is not going to be up + // to date. + change := &plans.ResourceInstanceChange{ + Addr: absAddr, + ProviderAddr: n.ProviderAddr, + Change: plans.Change{ + Action: plans.Read, + Before: priorVal, + After: proposedNewVal, + }, + } + + if n.OutputChange != nil { + *n.OutputChange = change + } + if n.State != nil { + *n.State = &states.ResourceInstanceObject{ + // We need to keep the prior value in the state so that plan + // has something to diff against. + Value: priorVal, + // TODO: this needs to be ObjectPlanned to trigger a plan, but + // the prior value is lost preventing plan from resulting in a + // NoOp + Status: states.ObjectPlanned, } } + + return nil, diags.ErrWithWarnings() } - return false + + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreRefresh(absAddr, states.CurrentGen, priorVal) + }); err != nil { + diags = diags.Append(err) + return nil, diags.ErrWithWarnings() + } + + newVal, readDiags := n.readDataSource(ctx, configVal) + diags = diags.Append(readDiags) + if diags.HasErrors() { + return nil, diags.ErrWithWarnings() + } + + // TODO: Need to signal to plan that this may have changed. We may be able + // to use ObjectPlanned for that, but that currently causes the state to be + // dropped altogether + outputState := &states.ResourceInstanceObject{ + Value: newVal, + Status: states.ObjectReady, + } + + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostRefresh(absAddr, states.CurrentGen, priorVal, newVal) + }) + if err != nil { + return nil, err + } + + if n.State != nil { + *n.State = outputState + } + + return nil, diags.ErrWithWarnings() } diff --git a/terraform/eval_read_data_apply.go b/terraform/eval_read_data_apply.go new file mode 100644 index 000000000..ff9b4fb94 --- /dev/null +++ b/terraform/eval_read_data_apply.go @@ -0,0 +1,94 @@ +package terraform + +import ( + "fmt" + + "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" +) + +// EvalReadDataApply is an EvalNode implementation that deals with the main part +// of the data resource lifecycle: either actually reading from the data source +// or generating a plan to do so. +type EvalReadDataApply struct { + evalReadData +} + +func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { + absAddr := n.Addr.Absolute(ctx.Path()) + + var diags tfdiags.Diagnostics + + var planned *plans.ResourceInstanceChange + if n.Planned != nil { + planned = *n.Planned + } + + if n.ProviderSchema == nil || *n.ProviderSchema == nil { + return nil, fmt.Errorf("provider schema not available for %s", n.Addr) + } + + if planned != nil && !(planned.Action == plans.Read || planned.Action == plans.Update) { + // If any other action gets in here then that's always a bug; this + // EvalNode only deals with reading. + return nil, fmt.Errorf( + "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", + planned.Action, absAddr, + ) + } + + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreApply(absAddr, states.CurrentGen, planned.Action, planned.Before, planned.After) + }); err != nil { + return nil, err + } + + // we have a change and it is complete, which means we read the data + // source during plan and only need to store it in state. + if planned.Action == plans.Update { + outputState := &states.ResourceInstanceObject{ + Value: planned.After, + Status: states.ObjectReady, + } + + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) + }) + if err != nil { + return nil, err + } + + if n.OutputChange != nil { + *n.OutputChange = planned + } + if n.State != nil { + *n.State = outputState + } + return nil, diags.ErrWithWarnings() + } + + newVal, readDiags := n.readDataSource(ctx, cty.NilVal) + diags = diags.Append(readDiags) + if diags.HasErrors() { + return nil, diags.ErrWithWarnings() + } + + outputState := &states.ResourceInstanceObject{ + Value: newVal, + Status: states.ObjectReady, + } + + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostApply(absAddr, states.CurrentGen, newVal, diags.Err()) + }); err != nil { + return nil, err + } + + if n.State != nil { + *n.State = outputState + } + + return nil, diags.ErrWithWarnings() +} diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go new file mode 100644 index 000000000..63051cbd1 --- /dev/null +++ b/terraform/eval_read_data_plan.go @@ -0,0 +1,166 @@ +package terraform + +import ( + "fmt" + "log" + + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/plans/objchange" + "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/tfdiags" +) + +// EvalReadDataPlan is an EvalNode implementation that deals with the main part +// of the data resource lifecycle: either actually reading from the data source +// or generating a plan to do so. +type EvalReadDataPlan struct { + evalReadData + + // dependsOn stores the list of transitive resource addresses that any + // configuration depends_on references may resolve to. This is used to + // determine if there are any changes that will force this data sources to + // be deferred to apply. + dependsOn []addrs.ConfigResource +} + +func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { + absAddr := n.Addr.Absolute(ctx.Path()) + + var diags tfdiags.Diagnostics + var configVal cty.Value + + if n.ProviderSchema == nil || *n.ProviderSchema == nil { + return nil, fmt.Errorf("provider schema not available for %s", n.Addr) + } + + config := *n.Config + providerSchema := *n.ProviderSchema + 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 %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) + } + + objTy := schema.ImpliedType() + priorVal := cty.NullVal(objTy) + if n.State != nil && *n.State != nil { + priorVal = (*n.State).Value + } + + forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) + + var configDiags tfdiags.Diagnostics + configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData) + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return nil, diags.Err() + } + + configKnown := configVal.IsWhollyKnown() + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + // If our configuration contains any unknown values, or we depend on any + // unknown values then we must defer the read to the apply phase by + // producing a "Read" change for this resource, and a placeholder value for + // it in the state. + if n.forcePlanRead(ctx) || !configKnown { + if configKnown { + log.Printf("[TRACE] EvalReadDataPlan: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + } else { + log.Printf("[TRACE] EvalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr) + } + + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) + }) + if err != nil { + return nil, err + } + + 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(absAddr, states.CurrentGen, change.Action, priorVal, proposedNewVal) + }) + if err != nil { + return nil, err + } + + *n.OutputChange = change + return nil, diags.ErrWithWarnings() + } + + newVal, readDiags := n.readDataSource(ctx, configVal) + diags = diags.Append(readDiags) + if diags.HasErrors() { + return nil, diags.ErrWithWarnings() + } + + action := plans.NoOp + if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { + // since a data source is read-only, update here only means that we + // need to update the state. + action = plans.Update + } + + // Produce a change regardless of the outcome. + change := &plans.ResourceInstanceChange{ + Addr: absAddr, + ProviderAddr: n.ProviderAddr, + Change: plans.Change{ + Action: action, + Before: priorVal, + After: newVal, + }, + } + + status := states.ObjectReady + if action == plans.Update { + status = states.ObjectPlanned + } + + outputState := &states.ResourceInstanceObject{ + Value: newVal, + Status: status, + } + + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostDiff(absAddr, states.CurrentGen, action, priorVal, newVal) + }); err != nil { + return nil, err + } + + *n.OutputChange = change + *n.State = outputState + + return nil, diags.ErrWithWarnings() +} + +// forcePlanRead determines if we need to override the usual behavior of +// immediately reading from the data source where possible, instead forcing us +// to generate a plan. +func (n *EvalReadDataPlan) forcePlanRead(ctx EvalContext) bool { + // Check and see if any depends_on dependencies have + // changes, since they won't show up as changes in the + // configuration. + changes := ctx.Changes() + for _, d := range n.dependsOn { + for _, change := range changes.GetConfigResourceChanges(d) { + if change != nil && change.Action != plans.NoOp { + return true + } + } + } + return false +} diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index a3e37bf3e..6bbc5d689 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -210,24 +210,26 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { Output: &state, }, - // EvalReadData will _attempt_ to read the data source, but may - // generate an incomplete planned object if the configuration + // EvalReadDataRefresh will _attempt_ to read the data source, but + // may generate an incomplete planned object if the configuration // includes values that won't be known until apply. - &EvalReadData{ - Addr: addr.Resource, - Config: n.Config, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - OutputChange: &change, - State: &state, - refresh: true, + &EvalReadDataRefresh{ + evalReadData{ + Addr: addr.Resource, + Config: n.Config, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + OutputChange: &change, + State: &state, + }, }, &EvalIf{ If: func(ctx EvalContext) (bool, error) { - return (*state).Status != states.ObjectPlanned, nil + return change == nil, nil + }, Then: &EvalSequence{ Nodes: []EvalNode{ diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 9c2dfbb27..8f1970554 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -172,15 +172,17 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou // In this particular call to EvalReadData we include our planned // change, which signals that we expect this read to complete fully // with no unknown values; it'll produce an error if not. - &EvalReadData{ - Addr: addr.Resource, - Config: n.Config, - Planned: &change, // setting this indicates that the result must be complete - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - State: &state, + &EvalReadDataApply{ + evalReadData{ + Addr: addr.Resource, + Config: n.Config, + Planned: &change, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + State: &state, + }, }, &EvalWriteState{ diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index c12de1eec..7e5073480 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -73,16 +73,18 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderSchema: &providerSchema, }, - &EvalReadData{ - Addr: addr.Resource, - Config: n.Config, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - OutputChange: &change, - State: &state, - dependsOn: n.dependsOn, + &EvalReadDataPlan{ + evalReadData: evalReadData{ + Addr: addr.Resource, + Config: n.Config, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + OutputChange: &change, + State: &state, + }, + dependsOn: n.dependsOn, }, &EvalWriteState{ From 05575a863c82bcbac038a593f7fca0077acfb34d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 8 May 2020 18:46:32 -0400 Subject: [PATCH 14/21] check for data source changed during plan Rather than re-read the data source during every plan cycle, apply the config to the prior state, and skip reading if there is no change. Remove the TODOs, as we're going to accept that data-only changes will still not be plan-able for the time being. Fix the null data source test resource, as it had no computed fields at all, even the id. --- terraform/context_apply_test.go | 10 ++++++ terraform/context_plan_test.go | 5 +-- terraform/context_test.go | 3 +- terraform/eval_read_data.go | 34 +++++++++---------- terraform/eval_read_data_plan.go | 34 ++++++++++--------- .../testdata/apply-data-depends-on/main.tf | 1 - terraform/transform_reference.go | 4 +-- 7 files changed, 52 insertions(+), 39 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f87bb720c..b26d2c481 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8782,6 +8782,16 @@ func TestContext2Apply_dataDependsOn(t *testing.T) { if actual != expected { t.Fatalf("bad:\n%s", strings.TrimSpace(state.String())) } + + // run another plan to make sure the data source doesn't show as a change + plan, diags := ctx.Plan() + assertNoErrors(t, diags) + + for _, c := range plan.Changes.Resources { + if c.Action != plans.NoOp { + t.Fatalf("unexpected change for %s", c.Addr) + } + } } func TestContext2Apply_terraformWorkspace(t *testing.T) { diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 282f07f1b..d83ab465b 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1892,8 +1892,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) { _, diags = ctx.Plan() // should do nothing with data resource in this step, since it was already read assertNoErrors(t, diags) - if !p.ReadDataSourceCalled { - t.Fatalf("ReadDataSource should have been called") + if p.ReadDataSourceCalled { + // there was no config change to read during plan + t.Fatalf("ReadDataSource should not have been called") } } diff --git a/terraform/context_test.go b/terraform/context_test.go index 58b896284..f071a2afa 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -693,11 +693,12 @@ func testProviderSchema(name string) *ProviderSchema { Attributes: map[string]*configschema.Attribute{ "id": { Type: cty.String, - Optional: true, + Computed: true, }, "foo": { Type: cty.String, Optional: true, + Computed: true, }, }, }, diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 001afd325..c85e60f73 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -236,19 +236,24 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { } configKnown := configVal.IsWhollyKnown() - // If our configuration contains any unknown values, or we depend on any - // unknown values then we must defer the read to the apply phase by - // producing a "Read" change for this resource, and a placeholder value for - // it in the state. - if len(n.Config.DependsOn) > 0 || !configKnown { + // If our configuration contains any unknown values, then we must defer the + // read until plan or apply. If we've never read this data source and we + // have any depends_on, we will have to defer reading until plan to resolve + // the dependency changes. + // Assuming we can read the data source with depends_on if we have + // existing state is a compromise to prevent data sources from continually + // showing a diff. We have to make the assumption that if we have a prior + // state, since there are no prior dependency changes happening during + // refresh, that we can read this resource. If there are dependency updates + // in the config, they we be discovered in plan and the data source will be + // read again. + if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) { if configKnown { log.Printf("[TRACE] EvalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { log.Printf("[TRACE] EvalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr) } - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) - // We need to store a change so tat other references to this data // source can resolve correctly, since the state is not going to be up // to date. @@ -258,21 +263,17 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { Change: plans.Change{ Action: plans.Read, Before: priorVal, - After: proposedNewVal, + After: objchange.PlannedDataResourceObject(schema, configVal), }, } if n.OutputChange != nil { *n.OutputChange = change } + if n.State != nil { *n.State = &states.ResourceInstanceObject{ - // We need to keep the prior value in the state so that plan - // has something to diff against. - Value: priorVal, - // TODO: this needs to be ObjectPlanned to trigger a plan, but - // the prior value is lost preventing plan from resulting in a - // NoOp + Value: cty.NullVal(objTy), Status: states.ObjectPlanned, } } @@ -293,9 +294,8 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } - // TODO: Need to signal to plan that this may have changed. We may be able - // to use ObjectPlanned for that, but that currently causes the state to be - // dropped altogether + // This may still have been refreshed with references to resources that + // will be updated, but that will be caught as a change during plan. outputState := &states.ResourceInstanceObject{ Value: newVal, Status: states.ObjectReady, diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index 63051cbd1..fe0761056 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -61,7 +61,6 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { } configKnown := configVal.IsWhollyKnown() - proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) // If our configuration contains any unknown values, or we depend on any // unknown values then we must defer the read to the apply phase by // producing a "Read" change for this resource, and a placeholder value for @@ -73,6 +72,8 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[TRACE] EvalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr) } + proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) + err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) }) @@ -101,42 +102,43 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } + // If we have a stored state we may not need to re-read the data source. + // Check the config against the state to see if there are any difference. + if !priorVal.IsNull() { + // Applying the configuration to the prior state lets us see if there + // are any differences. + proposed := objchange.ProposedNewObject(schema, priorVal, configVal) + if proposed.Equals(priorVal).True() { + log.Printf("[TRACE] EvalReadDataPlan: %s no change detected, using existing state", absAddr) + // state looks up to date, and must have been read during refresh + return nil, diags.ErrWithWarnings() + } + } + newVal, readDiags := n.readDataSource(ctx, configVal) diags = diags.Append(readDiags) if diags.HasErrors() { return nil, diags.ErrWithWarnings() } - action := plans.NoOp - if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() { - // since a data source is read-only, update here only means that we - // need to update the state. - action = plans.Update - } - // Produce a change regardless of the outcome. change := &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ - Action: action, + Action: plans.Update, Before: priorVal, After: newVal, }, } - status := states.ObjectReady - if action == plans.Update { - status = states.ObjectPlanned - } - outputState := &states.ResourceInstanceObject{ Value: newVal, - Status: status, + Status: states.ObjectPlanned, } if err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(absAddr, states.CurrentGen, action, priorVal, newVal) + return h.PostDiff(absAddr, states.CurrentGen, plans.Update, priorVal, newVal) }); err != nil { return nil, err } diff --git a/terraform/testdata/apply-data-depends-on/main.tf b/terraform/testdata/apply-data-depends-on/main.tf index 0371413cd..f48b48e40 100644 --- a/terraform/testdata/apply-data-depends-on/main.tf +++ b/terraform/testdata/apply-data-depends-on/main.tf @@ -3,6 +3,5 @@ resource "null_instance" "write" { } data "null_data_source" "read" { - foo = "" depends_on = ["null_instance.write"] } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index d246a56ce..5280c67d2 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -51,7 +51,7 @@ type GraphNodeAttachDependencies interface { // not yet expended in the graph. While this will cause some extra data // resources to show in the plan when their depends_on references may be in // unrelated module instances, the fact that it only happens when there are any -// resource updates pending means we ca still avoid the problem of the +// resource updates pending means we can still avoid the problem of the // "perpetual diff" type GraphNodeAttachDependsOn interface { GraphNodeConfigResource @@ -83,7 +83,7 @@ type GraphNodeReferenceOutside interface { ReferenceOutside() (selfPath, referencePath addrs.Module) } -// Referenceeransformer is a GraphTransformer that connects all the +// ReferenceTransformer is a GraphTransformer that connects all the // nodes that reference each other in order to form the proper ordering. type ReferenceTransformer struct{} From 44919641ef47b1d10986178347bde3089b78971e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 9 May 2020 10:23:46 -0400 Subject: [PATCH 15/21] set state in deferred data read The state was not being set, so the change was not evaluated correctly for dependant resources. Remove use of cty.NilVal in readDataSource, only one place was using it, so the code could just be moved out. Fix a bunch of places where warnings would be lost. --- terraform/eval_read_data.go | 43 +++++++------------------- terraform/eval_read_data_apply.go | 50 +++++++++++++++++-------------- terraform/eval_read_data_plan.go | 34 ++++++++++----------- 3 files changed, 54 insertions(+), 73 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index c85e60f73..474e82f93 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -70,11 +70,8 @@ func (n *evalReadData) readDataSource(ctx EvalContext, configVal cty.Value) (cty } provider := *n.Provider + providerSchema := *n.ProviderSchema - - forEach, _ := evaluateForEachExpression(config.ForEach, ctx) - keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) - schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource()) if schema == nil { // Should be caught during validation, so we don't bother with a pretty error here @@ -82,15 +79,6 @@ func (n *evalReadData) readDataSource(ctx EvalContext, configVal cty.Value) (cty return newVal, diags } - if configVal == cty.NilVal { - val, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) - diags = diags.Append(configDiags) - if configDiags.HasErrors() { - return newVal, diags - } - configVal = val - } - metaConfigVal, metaDiags := n.providerMetas(ctx) diags = diags.Append(metaDiags) if diags.HasErrors() { @@ -232,7 +220,7 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, diags.Err() + return nil, diags.ErrWithWarnings() } configKnown := configVal.IsWhollyKnown() @@ -257,7 +245,7 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { // We need to store a change so tat other references to this data // source can resolve correctly, since the state is not going to be up // to date. - change := &plans.ResourceInstanceChange{ + *n.OutputChange = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ @@ -267,15 +255,9 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { }, } - if n.OutputChange != nil { - *n.OutputChange = change - } - - if n.State != nil { - *n.State = &states.ResourceInstanceObject{ - Value: cty.NullVal(objTy), - Status: states.ObjectPlanned, - } + *n.State = &states.ResourceInstanceObject{ + Value: cty.NullVal(objTy), + Status: states.ObjectPlanned, } return nil, diags.ErrWithWarnings() @@ -296,20 +278,15 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { // This may still have been refreshed with references to resources that // will be updated, but that will be caught as a change during plan. - outputState := &states.ResourceInstanceObject{ + *n.State = &states.ResourceInstanceObject{ Value: newVal, Status: states.ObjectReady, } - err := ctx.Hook(func(h Hook) (HookAction, error) { + if err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PostRefresh(absAddr, states.CurrentGen, priorVal, newVal) - }) - if err != nil { - return nil, err - } - - if n.State != nil { - *n.State = outputState + }); err != nil { + diags = diags.Append(err) } return nil, diags.ErrWithWarnings() diff --git a/terraform/eval_read_data_apply.go b/terraform/eval_read_data_apply.go index ff9b4fb94..5d70fc9f3 100644 --- a/terraform/eval_read_data_apply.go +++ b/terraform/eval_read_data_apply.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/tfdiags" - "github.com/zclconf/go-cty/cty" ) // EvalReadDataApply is an EvalNode implementation that deals with the main part @@ -48,34 +47,43 @@ func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { // we have a change and it is complete, which means we read the data // source during plan and only need to store it in state. if planned.Action == plans.Update { - outputState := &states.ResourceInstanceObject{ + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) + }); err != nil { + diags = diags.Append(err) + } + + *n.State = &states.ResourceInstanceObject{ Value: planned.After, Status: states.ObjectReady, } - - err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) - }) - if err != nil { - return nil, err - } - - if n.OutputChange != nil { - *n.OutputChange = planned - } - if n.State != nil { - *n.State = outputState - } return nil, diags.ErrWithWarnings() } - newVal, readDiags := n.readDataSource(ctx, cty.NilVal) + config := *n.Config + providerSchema := *n.ProviderSchema + 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 %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type) + } + + forEach, _ := evaluateForEachExpression(config.ForEach, ctx) + keyData := EvalDataForInstanceKey(n.Addr.Key, forEach) + + configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData) + diags = diags.Append(configDiags) + if configDiags.HasErrors() { + return nil, diags.ErrWithWarnings() + } + + newVal, readDiags := n.readDataSource(ctx, configVal) diags = diags.Append(readDiags) if diags.HasErrors() { return nil, diags.ErrWithWarnings() } - outputState := &states.ResourceInstanceObject{ + *n.State = &states.ResourceInstanceObject{ Value: newVal, Status: states.ObjectReady, } @@ -83,11 +91,7 @@ func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { if err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PostApply(absAddr, states.CurrentGen, newVal, diags.Err()) }); err != nil { - return nil, err - } - - if n.State != nil { - *n.State = outputState + diags = diags.Append(err) } return nil, diags.ErrWithWarnings() diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index fe0761056..96e79cdb1 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -57,7 +57,7 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData) diags = diags.Append(configDiags) if configDiags.HasErrors() { - return nil, diags.Err() + return nil, diags.ErrWithWarnings() } configKnown := configVal.IsWhollyKnown() @@ -74,14 +74,14 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) - err := ctx.Hook(func(h Hook) (HookAction, error) { + if err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal) - }) - if err != nil { - return nil, err + }); err != nil { + diags = diags.Append(err) + return nil, diags.ErrWithWarnings() } - change := &plans.ResourceInstanceChange{ + *n.OutputChange = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ @@ -91,14 +91,17 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { }, } - err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(absAddr, states.CurrentGen, change.Action, priorVal, proposedNewVal) - }) - if err != nil { - return nil, err + *n.State = &states.ResourceInstanceObject{ + Value: cty.NullVal(objTy), + Status: states.ObjectPlanned, + } + + if err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostDiff(absAddr, states.CurrentGen, plans.Read, priorVal, proposedNewVal) + }); err != nil { + diags = diags.Append(err) } - *n.OutputChange = change return nil, diags.ErrWithWarnings() } @@ -122,7 +125,7 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { } // Produce a change regardless of the outcome. - change := &plans.ResourceInstanceChange{ + *n.OutputChange = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ @@ -132,7 +135,7 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { }, } - outputState := &states.ResourceInstanceObject{ + *n.State = &states.ResourceInstanceObject{ Value: newVal, Status: states.ObjectPlanned, } @@ -143,9 +146,6 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - *n.OutputChange = change - *n.State = outputState - return nil, diags.ErrWithWarnings() } From c6c851eb3f9835bccd311063122a702ae3ad03d8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 9 May 2020 10:41:24 -0400 Subject: [PATCH 16/21] add test for using a data source with depends_on Ensure that a data source with depends_on not only plans to update during refresh, but evaluates correctly in the plan ensuring dependencies are planned accordingly. --- terraform/context_apply_test.go | 62 ++++++++++++++++++- .../testdata/apply-data-depends-on/main.tf | 7 --- 2 files changed, 61 insertions(+), 8 deletions(-) delete mode 100644 terraform/testdata/apply-data-depends-on/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index b26d2c481..6e910a69b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -8724,7 +8724,20 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin // that resource to be applied first. func TestContext2Apply_dataDependsOn(t *testing.T) { p := testProvider("null") - m := testModule(t, "apply-data-depends-on") + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "null_instance" "write" { + foo = "attribute" +} + +data "null_data_source" "read" { + depends_on = ["null_instance.write"] +} + +resource "null_instance" "depends" { + foo = data.null_data_source.read.foo +} +`}) ctx := testContext2(t, &ContextOpts{ Config: m, @@ -8792,6 +8805,53 @@ func TestContext2Apply_dataDependsOn(t *testing.T) { t.Fatalf("unexpected change for %s", c.Addr) } } + + // now we cause a change in the first resource, which should trigger a plan + // in the data source, and the resource that depends on the data source + // must plan a change as well. + m = testModuleInline(t, map[string]string{ + "main.tf": ` +resource "null_instance" "write" { + foo = "new" +} + +data "null_data_source" "read" { + depends_on = ["null_instance.write"] +} + +resource "null_instance" "depends" { + foo = data.null_data_source.read.foo +} +`}) + + p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) { + // the side effect of the resource being applied + provisionerOutput = "APPLIED_AGAIN" + return testApplyFn(info, s, d) + } + + ctx = testContext2(t, &ContextOpts{ + Config: m, + State: state, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("null"): testProviderFuncFixed(p), + }, + }) + + plan, diags = ctx.Plan() + assertNoErrors(t, diags) + + expectedChanges := map[string]plans.Action{ + "null_instance.write": plans.Update, + "data.null_data_source.read": plans.Read, + "null_instance.depends": plans.Update, + } + + for _, c := range plan.Changes.Resources { + if c.Action != expectedChanges[c.Addr.String()] { + t.Errorf("unexpected %s for %s", c.Action, c.Addr) + } + } } func TestContext2Apply_terraformWorkspace(t *testing.T) { diff --git a/terraform/testdata/apply-data-depends-on/main.tf b/terraform/testdata/apply-data-depends-on/main.tf deleted file mode 100644 index f48b48e40..000000000 --- a/terraform/testdata/apply-data-depends-on/main.tf +++ /dev/null @@ -1,7 +0,0 @@ -resource "null_instance" "write" { - foo = "attribute" -} - -data "null_data_source" "read" { - depends_on = ["null_instance.write"] -} From 8e3728af5401c0334bd6397f5a0779a700f4ad2e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 May 2020 08:59:34 -0400 Subject: [PATCH 17/21] rename methods for ConfigResource changes --- plans/changes.go | 4 ++-- plans/changes_sync.go | 8 ++++---- terraform/eval_read_data_plan.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plans/changes.go b/plans/changes.go index 04d070410..9e8f25bae 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -55,10 +55,10 @@ func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInst } -// ConfigResourceInstances returns the planned change for the current objects +// InstancesForConfigResource returns the planned change for the current objects // of the resource instances of the given address, if any. Returns nil if no // changes are planned. -func (c *Changes) ConfigResourceInstances(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { +func (c *Changes) InstancesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { var changes []*ResourceInstanceChangeSrc for _, rc := range c.Resources { resAddr := rc.Addr.ContainingResource().Config() diff --git a/plans/changes_sync.go b/plans/changes_sync.go index 97cac918e..615f85392 100644 --- a/plans/changes_sync.go +++ b/plans/changes_sync.go @@ -62,7 +62,7 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance, panic(fmt.Sprintf("unsupported generation value %#v", gen)) } -// GetConfigResourceChanges searched the set of resource instance +// GetChangesForConfigResource searched the set of resource instance // changes and returns all changes related to a given configuration address. // This is be used to find possible changes related to a configuration // reference. @@ -72,14 +72,14 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance, // The returned objects are a deep copy of the change recorded in the plan, so // callers may mutate them although it's generally better (less confusing) to // treat planned changes as immutable after they've been initially constructed. -func (cs *ChangesSync) GetConfigResourceChanges(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { +func (cs *ChangesSync) GetChangesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc { if cs == nil { - panic("GetConfigResourceChanges on nil ChangesSync") + panic("GetChangesForConfigResource on nil ChangesSync") } cs.lock.Lock() defer cs.lock.Unlock() var changes []*ResourceInstanceChangeSrc - for _, c := range cs.changes.ConfigResourceInstances(addr) { + for _, c := range cs.changes.InstancesForConfigResource(addr) { changes = append(changes, c.DeepCopy()) } return changes diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index 96e79cdb1..c2ec7b2fb 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -158,7 +158,7 @@ func (n *EvalReadDataPlan) forcePlanRead(ctx EvalContext) bool { // configuration. changes := ctx.Changes() for _, d := range n.dependsOn { - for _, change := range changes.GetConfigResourceChanges(d) { + for _, change := range changes.GetChangesForConfigResource(d) { if change != nil && change.Action != plans.NoOp { return true } From 7b8f13862c145deffb6a5b102713a40ff7ef909b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 May 2020 09:06:39 -0400 Subject: [PATCH 18/21] un-export new data eval nodes --- terraform/eval_read_data.go | 10 +++++----- terraform/eval_read_data_apply.go | 6 +++--- terraform/eval_read_data_plan.go | 14 +++++++------- terraform/node_data_refresh.go | 2 +- terraform/node_resource_apply_instance.go | 2 +- terraform/node_resource_plan_instance.go | 2 +- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 474e82f93..b00f031a3 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -186,13 +186,13 @@ func (n *evalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagno return metaConfigVal, diags } -// EvalReadDataRefresh is an EvalNode implementation that handled the data +// evalReadDataRefresh is an EvalNode implementation that handled the data // resource lifecycle during refresh -type EvalReadDataRefresh struct { +type evalReadDataRefresh struct { evalReadData } -func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { +func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { var diags tfdiags.Diagnostics if n.ProviderSchema == nil || *n.ProviderSchema == nil { @@ -237,9 +237,9 @@ func (n *EvalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { // read again. if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) { if configKnown { - log.Printf("[TRACE] EvalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + log.Printf("[TRACE] evalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { - log.Printf("[TRACE] EvalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr) + log.Printf("[TRACE] evalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr) } // We need to store a change so tat other references to this data diff --git a/terraform/eval_read_data_apply.go b/terraform/eval_read_data_apply.go index 5d70fc9f3..ecdd440e6 100644 --- a/terraform/eval_read_data_apply.go +++ b/terraform/eval_read_data_apply.go @@ -8,14 +8,14 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) -// EvalReadDataApply is an EvalNode implementation that deals with the main part +// evalReadDataApply is an EvalNode implementation that deals with the main part // of the data resource lifecycle: either actually reading from the data source // or generating a plan to do so. -type EvalReadDataApply struct { +type evalReadDataApply struct { evalReadData } -func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { +func (n *evalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { absAddr := n.Addr.Absolute(ctx.Path()) var diags tfdiags.Diagnostics diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index c2ec7b2fb..0f86b9f4d 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -13,10 +13,10 @@ import ( "github.com/hashicorp/terraform/tfdiags" ) -// EvalReadDataPlan is an EvalNode implementation that deals with the main part +// evalReadDataPlan is an EvalNode implementation that deals with the main part // of the data resource lifecycle: either actually reading from the data source // or generating a plan to do so. -type EvalReadDataPlan struct { +type evalReadDataPlan struct { evalReadData // dependsOn stores the list of transitive resource addresses that any @@ -26,7 +26,7 @@ type EvalReadDataPlan struct { dependsOn []addrs.ConfigResource } -func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { +func (n *evalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { absAddr := n.Addr.Absolute(ctx.Path()) var diags tfdiags.Diagnostics @@ -67,9 +67,9 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { // it in the state. if n.forcePlanRead(ctx) || !configKnown { if configKnown { - log.Printf("[TRACE] EvalReadDataPlan: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) + log.Printf("[TRACE] evalReadDataPlan: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { - log.Printf("[TRACE] EvalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr) + log.Printf("[TRACE] evalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr) } proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal) @@ -112,7 +112,7 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { // are any differences. proposed := objchange.ProposedNewObject(schema, priorVal, configVal) if proposed.Equals(priorVal).True() { - log.Printf("[TRACE] EvalReadDataPlan: %s no change detected, using existing state", absAddr) + log.Printf("[TRACE] evalReadDataPlan: %s no change detected, using existing state", absAddr) // state looks up to date, and must have been read during refresh return nil, diags.ErrWithWarnings() } @@ -152,7 +152,7 @@ func (n *EvalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { // forcePlanRead determines if we need to override the usual behavior of // immediately reading from the data source where possible, instead forcing us // to generate a plan. -func (n *EvalReadDataPlan) forcePlanRead(ctx EvalContext) bool { +func (n *evalReadDataPlan) forcePlanRead(ctx EvalContext) bool { // Check and see if any depends_on dependencies have // changes, since they won't show up as changes in the // configuration. diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 6bbc5d689..3a601e372 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -213,7 +213,7 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { // EvalReadDataRefresh will _attempt_ to read the data source, but // may generate an incomplete planned object if the configuration // includes values that won't be known until apply. - &EvalReadDataRefresh{ + &evalReadDataRefresh{ evalReadData{ Addr: addr.Resource, Config: n.Config, diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 8f1970554..6b8206b2c 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -172,7 +172,7 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou // In this particular call to EvalReadData we include our planned // change, which signals that we expect this read to complete fully // with no unknown values; it'll produce an error if not. - &EvalReadDataApply{ + &evalReadDataApply{ evalReadData{ Addr: addr.Resource, Config: n.Config, diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 7e5073480..19f2b633f 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -73,7 +73,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderSchema: &providerSchema, }, - &EvalReadDataPlan{ + &evalReadDataPlan{ evalReadData: evalReadData{ Addr: addr.Resource, Config: n.Config, From 8850d787f475aba4522828d77050e13d4d1e6e72 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 May 2020 09:40:13 -0400 Subject: [PATCH 19/21] add evalWriteEmptyState for data source removal --- terraform/eval_state.go | 12 +++++++ terraform/node_resource_destroy.go | 53 +++++++++++++++++------------- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/terraform/eval_state.go b/terraform/eval_state.go index c19d37801..eca680569 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -187,6 +187,18 @@ func (n *EvalUpdateStateHook) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } +// evalWriteEmptyState wraps EvalWriteState to specifically record an empty +// state for a particular object. +type evalWriteEmptyState struct { + EvalWriteState +} + +func (n *evalWriteEmptyState) Eval(ctx EvalContext) (interface{}, error) { + var state *states.ResourceInstanceObject + n.State = &state + return n.EvalWriteState.Eval(ctx) +} + // EvalWriteState is an EvalNode implementation that saves the given object // as the current object for the selected resource instance. type EvalWriteState struct { diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index da154257d..6fc24fee2 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -234,35 +234,44 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { }, }, - // Make sure we handle data sources properly. + // Managed resources need to be destroyed, while data sources + // are only removed from state. &EvalIf{ If: func(ctx EvalContext) (bool, error) { - if addr.Resource.Resource.Mode == addrs.DataResourceMode { - // deleting a data source is simply removing the state - state = nil - } return addr.Resource.Resource.Mode == addrs.ManagedResourceMode, nil }, - Then: &EvalApply{ - Addr: addr.Resource, - Config: nil, // No configuration because we are destroying - State: &state, - Change: &changeApply, - Provider: &provider, - ProviderAddr: n.ResolvedProvider, - ProviderMetas: n.ProviderMetas, - ProviderSchema: &providerSchema, - Output: &state, - Error: &err, + Then: &EvalSequence{ + Nodes: []EvalNode{ + &EvalApply{ + Addr: addr.Resource, + Config: nil, // No configuration because we are destroying + State: &state, + Change: &changeApply, + Provider: &provider, + ProviderAddr: n.ResolvedProvider, + ProviderMetas: n.ProviderMetas, + ProviderSchema: &providerSchema, + Output: &state, + Error: &err, + }, + &EvalWriteState{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + State: &state, + }, + }, + }, + Else: &evalWriteEmptyState{ + EvalWriteState{ + Addr: addr.Resource, + ProviderAddr: n.ResolvedProvider, + ProviderSchema: &providerSchema, + }, }, }, - &EvalWriteState{ - Addr: addr.Resource, - ProviderAddr: n.ResolvedProvider, - ProviderSchema: &providerSchema, - State: &state, - }, + &EvalApplyPost{ Addr: addr.Resource, State: &state, From 291110fe785bfbaf1c5f022228f6bfb262de2546 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 13 May 2020 13:49:05 -0400 Subject: [PATCH 20/21] Don't use plans.Update for data sources The new data source planning logic no longer needs a separate action, and the apply status can be determined from whether the After value is complete or not. --- terraform/context_plan_test.go | 8 ++++---- terraform/eval_read_data_apply.go | 8 ++++---- terraform/eval_read_data_plan.go | 9 +++++++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index d83ab465b..b7eb83619 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -5053,16 +5053,16 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) { "computed": cty.StringVal("data_id"), }), ric.After) case "data.aws_vpc.bar[0]": - if res.Action != plans.Update { - t.Fatalf("resource %s should be update, got %s", ric.Addr, ric.Action) + if res.Action != plans.Read { + t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ "id": cty.StringVal("data_id"), "foo": cty.StringVal("0"), }), ric.After) case "data.aws_vpc.bar[1]": - if res.Action != plans.Update { - t.Fatalf("resource %s should be update, got %s", ric.Addr, ric.Action) + if res.Action != plans.Read { + t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action) } checkVals(t, objectVal(t, schema, map[string]cty.Value{ "id": cty.StringVal("data_id"), diff --git a/terraform/eval_read_data_apply.go b/terraform/eval_read_data_apply.go index ecdd440e6..888d1618d 100644 --- a/terraform/eval_read_data_apply.go +++ b/terraform/eval_read_data_apply.go @@ -29,11 +29,11 @@ func (n *evalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { return nil, fmt.Errorf("provider schema not available for %s", n.Addr) } - if planned != nil && !(planned.Action == plans.Read || planned.Action == plans.Update) { + if planned != nil && planned.Action != plans.Read { // If any other action gets in here then that's always a bug; this // EvalNode only deals with reading. return nil, fmt.Errorf( - "invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)", + "invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)", planned.Action, absAddr, ) } @@ -44,9 +44,9 @@ func (n *evalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - // we have a change and it is complete, which means we read the data + // We have a change and it is complete, which means we read the data // source during plan and only need to store it in state. - if planned.Action == plans.Update { + if planned.After.IsWhollyKnown() { if err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PostApply(absAddr, states.CurrentGen, planned.After, nil) }); err != nil { diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index 0f86b9f4d..0a6d4c431 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -81,6 +81,8 @@ func (n *evalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } + // Apply detects that the data source will need to be read by the After + // value containing unknowns from PlanDataResourceObject. *n.OutputChange = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, @@ -124,12 +126,15 @@ func (n *evalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) { return nil, diags.ErrWithWarnings() } - // Produce a change regardless of the outcome. + // The returned value from ReadDataSource must be non-nil and known, + // which we store in the change. Apply will use the fact that the After + // value is wholly kown to save the state directly, rather than reading the + // data source again. *n.OutputChange = &plans.ResourceInstanceChange{ Addr: absAddr, ProviderAddr: n.ProviderAddr, Change: plans.Change{ - Action: plans.Update, + Action: plans.Read, Before: priorVal, After: newVal, }, From a8e0914d7fb617b243614db34a7102dbd3e1ba07 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 18 May 2020 21:35:37 -0400 Subject: [PATCH 21/21] attach dependency type name changes --- terraform/graph_builder_plan.go | 2 +- terraform/node_resource_abstract.go | 28 ++++++++++++++-------------- terraform/transform_reference.go | 18 +++++++++--------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index eb5d3a437..9c3ffed84 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -152,7 +152,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // Make sure data sources are aware of any depends_on from the // configuration - &AttachDependsOnTransformer{}, + &attachDataResourceDependenciesTransformer{}, // Add the node to fix the state count boundaries &CountBoundaryTransformer{ diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 4614daf45..2945dbedf 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -69,18 +69,18 @@ type NodeAbstractResource struct { } var ( - _ GraphNodeReferenceable = (*NodeAbstractResource)(nil) - _ GraphNodeReferencer = (*NodeAbstractResource)(nil) - _ GraphNodeProviderConsumer = (*NodeAbstractResource)(nil) - _ GraphNodeProvisionerConsumer = (*NodeAbstractResource)(nil) - _ GraphNodeConfigResource = (*NodeAbstractResource)(nil) - _ GraphNodeAttachResourceConfig = (*NodeAbstractResource)(nil) - _ GraphNodeAttachResourceSchema = (*NodeAbstractResource)(nil) - _ GraphNodeAttachProvisionerSchema = (*NodeAbstractResource)(nil) - _ GraphNodeAttachProviderMetaConfigs = (*NodeAbstractResource)(nil) - _ GraphNodeTargetable = (*NodeAbstractResource)(nil) - _ GraphNodeAttachDependsOn = (*NodeAbstractResource)(nil) - _ dag.GraphNodeDotter = (*NodeAbstractResource)(nil) + _ GraphNodeReferenceable = (*NodeAbstractResource)(nil) + _ GraphNodeReferencer = (*NodeAbstractResource)(nil) + _ GraphNodeProviderConsumer = (*NodeAbstractResource)(nil) + _ GraphNodeProvisionerConsumer = (*NodeAbstractResource)(nil) + _ GraphNodeConfigResource = (*NodeAbstractResource)(nil) + _ GraphNodeAttachResourceConfig = (*NodeAbstractResource)(nil) + _ GraphNodeAttachResourceSchema = (*NodeAbstractResource)(nil) + _ GraphNodeAttachProvisionerSchema = (*NodeAbstractResource)(nil) + _ GraphNodeAttachProviderMetaConfigs = (*NodeAbstractResource)(nil) + _ GraphNodeTargetable = (*NodeAbstractResource)(nil) + _ graphNodeAttachResourceDependencies = (*NodeAbstractResource)(nil) + _ dag.GraphNodeDotter = (*NodeAbstractResource)(nil) ) // NewNodeAbstractResource creates an abstract resource graph node for @@ -396,8 +396,8 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) { n.Targets = targets } -// GraphNodeAttachDependsOn -func (n *NodeAbstractResource) AttachDependsOn(deps []addrs.ConfigResource) { +// graphNodeAttachResourceDependencies +func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource) { n.dependsOn = deps } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 5280c67d2..13f02b818 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -43,7 +43,7 @@ type GraphNodeAttachDependencies interface { AttachDependencies([]addrs.ConfigResource) } -// GraphNodeAttachDependsOn records all resources that are transitively +// graphNodeAttachResourceDependencies records all resources that are transitively // referenced through depends_on in the configuration. This is used by data // resources to determine if they can be read during the plan, or if they need // to be further delayed until apply. @@ -53,9 +53,9 @@ type GraphNodeAttachDependencies interface { // unrelated module instances, the fact that it only happens when there are any // resource updates pending means we can still avoid the problem of the // "perpetual diff" -type GraphNodeAttachDependsOn interface { +type graphNodeAttachResourceDependencies interface { GraphNodeConfigResource - AttachDependsOn([]addrs.ConfigResource) + AttachResourceDependencies([]addrs.ConfigResource) DependsOn() []*addrs.Reference } @@ -138,12 +138,12 @@ func (m depMap) add(v dag.Vertex) { } } -// AttachDependsOnTransformer records all resources transitively referenced +// attachDataResourceDependenciesTransformer records all resources transitively referenced // through a configuration depends_on. -type AttachDependsOnTransformer struct { +type attachDataResourceDependenciesTransformer struct { } -func (t AttachDependsOnTransformer) Transform(g *Graph) error { +func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error { // First we need to make a map of referenceable addresses to their vertices. // This is very similar to what's done in ReferenceTransformer, but we keep // implementation separate as they may need to change independently. @@ -151,7 +151,7 @@ func (t AttachDependsOnTransformer) Transform(g *Graph) error { refMap := NewReferenceMap(vertices) for _, v := range vertices { - depender, ok := v.(GraphNodeAttachDependsOn) + depender, ok := v.(graphNodeAttachResourceDependencies) if !ok { continue } @@ -182,7 +182,7 @@ func (t AttachDependsOnTransformer) Transform(g *Graph) error { } log.Printf("[TRACE] AttachDependsOnTransformer: %s depends on %s", depender.ResourceAddr(), deps) - depender.AttachDependsOn(deps) + depender.AttachResourceDependencies(deps) } return nil @@ -357,7 +357,7 @@ func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex { // DependsOn returns the set of vertices that the given vertex refers to from // the configured depends_on. func (m ReferenceMap) DependsOn(v dag.Vertex) []dag.Vertex { - depender, ok := v.(GraphNodeAttachDependsOn) + depender, ok := v.(graphNodeAttachResourceDependencies) if !ok { return nil }