diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f7b1130a1..a99aae428 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11121,17 +11121,36 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) { m := testModule(t, "apply-module-depends-on") p := testProvider("test") - p.ReadDataSourceResponse = providers.ReadDataSourceResponse{ - State: cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("data"), - "foo": cty.NullVal(cty.String), - }), - } - p.DiffFn = testDiffFn // each instance being applied should happen in sequential order applied := int64(0) + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + cfg := req.Config.AsValueMap() + foo := cfg["foo"].AsString() + ord := atomic.LoadInt64(&applied) + + resp := providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data"), + "foo": cfg["foo"], + }), + } + + if foo == "a" && ord < 4 { + // due to data source "a"'s module depending on instance 4, this + // should not be less than 4 + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("data source a read too early")) + } + if foo == "b" && ord < 1 { + // due to data source "b"'s module depending on instance 1, this + // should not be less than 1 + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("data source b read too early")) + } + return resp + } + p.DiffFn = testDiffFn + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) { state := req.PlannedState.AsValueMap() num, _ := state["num"].AsBigFloat().Float64() @@ -11154,7 +11173,12 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) { }, }) - _, diags := ctx.Plan() + _, diags := ctx.Refresh() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } + + _, diags = ctx.Plan() if diags.HasErrors() { t.Fatal(diags.ErrWithWarnings()) } @@ -11165,6 +11189,10 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) { } // run the plan again to ensure that data sources are not going to be re-read + _, diags = ctx.Refresh() + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } plan, diags := ctx.Plan() if diags.HasErrors() { t.Fatal(diags.ErrWithWarnings()) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index b00f031a3..9235130bd 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -52,6 +52,15 @@ type evalReadData struct { // - 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 + // forceDependsOn indicates that resources may be missing from dependsOn, + // but the parent module may have depends_on configured. + forceDependsOn bool } // readDataSource handles everything needed to call ReadDataSource on the provider. @@ -235,7 +244,7 @@ func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { // 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 || (priorVal.IsNull() && n.forcePlanRead()) { if configKnown { log.Printf("[TRACE] evalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr) } else { @@ -291,3 +300,10 @@ func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) { 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 *evalReadDataRefresh) forcePlanRead() bool { + return len(n.Config.DependsOn) > 0 || len(n.dependsOn) > 0 || n.forceDependsOn +} diff --git a/terraform/eval_read_data_plan.go b/terraform/eval_read_data_plan.go index 0a6d4c431..fd7fd276f 100644 --- a/terraform/eval_read_data_plan.go +++ b/terraform/eval_read_data_plan.go @@ -6,7 +6,6 @@ import ( "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" @@ -18,12 +17,6 @@ import ( // 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) { diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index e34d05647..546fdff63 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -171,6 +171,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { // have to connect again later for providers and so on. &ReferenceTransformer{}, &AttachDependenciesTransformer{}, + &attachDataResourceDependenciesTransformer{}, // Target &TargetsTransformer{ diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 98eda631c..135bcb2a2 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -121,6 +121,8 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er a.Config = n.Config a.ResolvedProvider = n.ResolvedProvider a.ProviderMetas = n.ProviderMetas + a.dependsOn = n.dependsOn + a.forceDependsOn = n.forceDependsOn return &NodeRefreshableDataResourceInstance{ NodeAbstractResourceInstance: a, @@ -227,6 +229,8 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { ProviderSchema: &providerSchema, OutputChange: &change, State: &state, + dependsOn: n.dependsOn, + forceDependsOn: n.forceDependsOn, }, }, diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index cfc519a33..892b6f4bd 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -47,18 +47,7 @@ func (n *nodeExpandModule) References() []*addrs.Reference { return nil } - for _, traversal := range n.ModuleCall.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 - } - - refs = append(refs, ref) - } + refs = append(refs, n.DependsOn()...) // Expansion only uses the count and for_each expressions, so this // particular graph node only refers to those. @@ -82,6 +71,28 @@ func (n *nodeExpandModule) References() []*addrs.Reference { return appendResourceDestroyReferences(refs) } +func (n *nodeExpandModule) DependsOn() []*addrs.Reference { + if n.ModuleCall == nil { + return nil + } + + var refs []*addrs.Reference + for _, traversal := range n.ModuleCall.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 + } + + refs = append(refs, ref) + } + + return refs +} + // GraphNodeReferenceOutside func (n *nodeExpandModule) ReferenceOutside() (selfPath, referencePath addrs.Module) { return n.Addr, n.Addr.Parent() diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 11eca0d96..aada8d09c 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -61,8 +61,9 @@ type NodeAbstractResource struct { // Set from GraphNodeTargetable Targets []addrs.Targetable - // Set from GraphNodeDependsOn - dependsOn []addrs.ConfigResource + // Set from AttachResourceDependencies + dependsOn []addrs.ConfigResource + forceDependsOn bool // The address of the provider this resource will use ResolvedProvider addrs.AbsProviderConfig @@ -402,8 +403,9 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) { } // graphNodeAttachResourceDependencies -func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource) { +func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource, force bool) { n.dependsOn = deps + n.forceDependsOn = force } // GraphNodeAttachResourceState diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 19f2b633f..4b9c2bde4 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -83,8 +83,8 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou ProviderSchema: &providerSchema, OutputChange: &change, State: &state, + dependsOn: n.dependsOn, }, - dependsOn: n.dependsOn, }, &EvalWriteState{ diff --git a/terraform/testdata/apply-module-depends-on/moda/main.tf b/terraform/testdata/apply-module-depends-on/moda/main.tf index 914e545c8..e60d300ba 100644 --- a/terraform/testdata/apply-module-depends-on/moda/main.tf +++ b/terraform/testdata/apply-module-depends-on/moda/main.tf @@ -3,6 +3,7 @@ resource "test_instance" "a" { } data "test_data_source" "a" { + foo = "a" } output "out" { diff --git a/terraform/testdata/apply-module-depends-on/modb/main.tf b/terraform/testdata/apply-module-depends-on/modb/main.tf index 17192b039..961c5d560 100644 --- a/terraform/testdata/apply-module-depends-on/modb/main.tf +++ b/terraform/testdata/apply-module-depends-on/modb/main.tf @@ -3,6 +3,7 @@ resource "test_instance" "b" { } data "test_data_source" "b" { + foo = "b" } output "out" { diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 5ea5bd8e0..94659e380 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -43,6 +43,12 @@ type GraphNodeAttachDependencies interface { AttachDependencies([]addrs.ConfigResource) } +// graphNodeDependsOn is implemented by resources that need to expose any +// references set via DependsOn in their configuration. +type graphNodeDependsOn interface { + DependsOn() []*addrs.Reference +} + // 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 @@ -55,8 +61,16 @@ type GraphNodeAttachDependencies interface { // "perpetual diff" type graphNodeAttachResourceDependencies interface { GraphNodeConfigResource - AttachResourceDependencies([]addrs.ConfigResource) - DependsOn() []*addrs.Reference + graphNodeDependsOn + + // AttachResourceDependencies stored the discovered dependencies in the + // resource node for evaluation later. + // + // The force parameter indicates that even if there are no dependencies, + // force the data source to act as though there are for refresh purposes. + // This is needed because yet-to-be-created resources won't be in the + // initial refresh graph, but may still be referenced through depends_on. + AttachResourceDependencies(deps []addrs.ConfigResource, force bool) } // GraphNodeReferenceOutside is an interface that can optionally be implemented. @@ -122,7 +136,7 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { type depMap map[string]addrs.ConfigResource -// addDep adds the vertex if it represents a resource in the +// add stores 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 @@ -155,34 +169,28 @@ func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error { 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 { + if depender.ResourceAddr().Resource.Mode != addrs.DataResourceMode { continue } - // depMap will only add resource references and dedupe - m := make(depMap) - - for _, dep := range refMap.DependsOn(v) { + // depMap will only add resource references then dedupe + deps := make(depMap) + dependsOnDeps, fromModule := refMap.dependsOn(g, depender) + for _, dep := range dependsOnDeps { // any the dependency - m.add(dep) - // and check any ancestors - ans, _ := g.Ancestors(dep) - for _, v := range ans { - m.add(v) - } + deps.add(dep) } - deps := make([]addrs.ConfigResource, 0, len(m)) - for _, d := range m { - deps = append(deps, d) + res := make([]addrs.ConfigResource, 0, len(deps)) + for _, d := range deps { + res = append(res, d) } - log.Printf("[TRACE] AttachDependsOnTransformer: %s depends on %s", depender.ResourceAddr(), deps) - depender.AttachResourceDependencies(deps) + log.Printf("[TRACE] attachDataDependenciesTransformer: %s depends on %s", depender.ResourceAddr(), res) + depender.AttachResourceDependencies(res, fromModule) } return nil @@ -254,6 +262,16 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { return nil } +func isDependableResource(v dag.Vertex) bool { + switch v.(type) { + case GraphNodeResourceInstance: + return true + case GraphNodeConfigResource: + return true + } + return false +} + // ReferenceMap is a structure that can be used to efficiently check // 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. @@ -304,35 +322,87 @@ func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex { 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.(graphNodeAttachResourceDependencies) - if !ok { - return nil +// dependsOn returns the set of vertices that the given vertex refers to from +// the configured depends_on. The bool return value indicates if depends_on was +// found in a parent module configuration. +func (m ReferenceMap) dependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Vertex, bool) { + var res []dag.Vertex + fromModule := false + + refs := depender.DependsOn() + + // This is where we record that a module has depends_on configured. + if _, ok := depender.(*nodeExpandModule); ok && len(refs) > 0 { + fromModule = true } - var matches []dag.Vertex - - for _, ref := range depender.DependsOn() { + for _, ref := range refs { subject := ref.Subject - key := m.referenceMapKey(v, subject) + key := m.referenceMapKey(depender, subject) vertices, ok := m[key] if !ok { - log.Printf("[WARN] DependOn: reference not found: %q", subject) + // the ReferenceMap generates all possible keys, so any warning + // here is probably not useful for this implementation. continue } for _, rv := range vertices { // don't include self-references - if rv == v { + if rv == depender { continue } - matches = append(matches, rv) + res = append(res, rv) + + // and check any ancestors for transitive dependencies + ans, _ := g.Ancestors(rv) + for _, v := range ans { + if isDependableResource(v) { + res = append(res, v) + } + } } } - return matches + parentDeps, fromParentModule := m.parentModuleDependsOn(g, depender) + res = append(res, parentDeps...) + + return res, fromModule || fromParentModule +} + +// parentModuleDependsOn returns the set of vertices that a data sources parent +// module references through the module call's depends_on. The bool return +// value indicates if depends_on was found in a parent module configuration. +func (n ReferenceMap) parentModuleDependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Vertex, bool) { + var res []dag.Vertex + fromModule := false + + // Look for containing modules with DependsOn. + // This should be connected directly to the module node, so we only need to + // look one step away. + for _, v := range g.DownEdges(depender) { + // we're only concerned with module expansion nodes here. + mod, ok := v.(*nodeExpandModule) + if !ok { + continue + } + + deps, fromParentModule := n.dependsOn(g, mod) + for _, dep := range deps { + // add the dependency + res = append(res, dep) + + // and check any transitive resource dependencies for more resources + ans, _ := g.Ancestors(dep) + for _, v := range ans { + if isDependableResource(v) { + res = append(res, v) + } + } + } + fromModule = fromModule || fromParentModule + } + + return res, fromModule } func (m *ReferenceMap) mapKey(path addrs.Module, addr addrs.Referenceable) string {