From 42bb4a644cd4220bfbf5dd0a8ae1922d1ea79d0a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 17 Oct 2019 16:05:27 -0400 Subject: [PATCH] make use of the new state Dependencies Make use of the new Dependencies field in the instance state. The inter-instance dependencies will be determined from the complete reference graph, so that absolute addresses can be stored, rather than just references within a module. The Dependencies are added to the node in the same manner as state, i.e. via an "attacher" interface and transformer. This is because dependencies are calculated from the graph itself, and not from the config. --- terraform/context_apply_test.go | 152 ++++++++++++++++++---- terraform/context_refresh_test.go | 78 +++++++++++ terraform/eval_apply.go | 8 +- terraform/eval_read_data.go | 31 ++--- terraform/eval_state.go | 9 +- terraform/graph_builder_apply.go | 1 + terraform/graph_builder_refresh.go | 1 + terraform/node_data_refresh.go | 1 - terraform/node_resource_abstract.go | 110 +++++----------- terraform/node_resource_apply_instance.go | 22 ++-- terraform/node_resource_plan_instance.go | 5 +- terraform/node_resource_refresh.go | 11 ++ terraform/terraform_test.go | 21 +-- terraform/transform_destroy_edge.go | 45 +++++-- terraform/transform_reference.go | 77 +++++++++++ 15 files changed, 406 insertions(+), 166 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 4e64109bb..317369bcf 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1328,19 +1328,7 @@ func testContext2Apply_destroyDependsOn(t *testing.T) { // Test that destroy ordering is correct with dependencies only // in the state. func TestContext2Apply_destroyDependsOnStateOnly(t *testing.T) { - // It is possible for this to be racy, so we loop a number of times - // just to check. - for i := 0; i < 10; i++ { - testContext2Apply_destroyDependsOnStateOnly(t) - } -} - -func testContext2Apply_destroyDependsOnStateOnly(t *testing.T) { - m := testModule(t, "empty") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ + legacyState := MustShimLegacyState(&State{ Modules: []*ModuleState{ &ModuleState{ Path: rootModulePath, @@ -1368,6 +1356,65 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T) { }, }) + newState := states.NewState() + root := newState.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.AbsResource{}, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "bar", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }, + Module: root.Addr, + }, + }, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + // It is possible for this to be racy, so we loop a number of times + // just to check. + for i := 0; i < 10; i++ { + t.Run("legacy", func(t *testing.T) { + testContext2Apply_destroyDependsOnStateOnly(t, legacyState) + }) + t.Run("new", func(t *testing.T) { + testContext2Apply_destroyDependsOnStateOnly(t, newState) + }) + } +} + +func testContext2Apply_destroyDependsOnStateOnly(t *testing.T, state *states.State) { + m := testModule(t, "empty") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn // Record the order we see Apply var actual []string var actualLock sync.Mutex @@ -1408,19 +1455,7 @@ func testContext2Apply_destroyDependsOnStateOnly(t *testing.T) { // Test that destroy ordering is correct with dependencies only // in the state within a module (GH-11749) func TestContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { - // It is possible for this to be racy, so we loop a number of times - // just to check. - for i := 0; i < 10; i++ { - testContext2Apply_destroyDependsOnStateOnlyModule(t) - } -} - -func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { - m := testModule(t, "empty") - p := testProvider("aws") - p.ApplyFn = testApplyFn - p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ + legacyState := MustShimLegacyState(&State{ Modules: []*ModuleState{ &ModuleState{ Path: []string{"root", "child"}, @@ -1448,6 +1483,66 @@ func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T) { }, }) + newState := states.NewState() + child := newState.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey)) + child.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.AbsResource{}, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + child.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "bar", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }, + Module: child.Addr, + }, + }, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + // It is possible for this to be racy, so we loop a number of times + // just to check. + for i := 0; i < 10; i++ { + t.Run("legacy", func(t *testing.T) { + testContext2Apply_destroyDependsOnStateOnlyModule(t, legacyState) + }) + t.Run("new", func(t *testing.T) { + testContext2Apply_destroyDependsOnStateOnlyModule(t, newState) + }) + } +} + +func testContext2Apply_destroyDependsOnStateOnlyModule(t *testing.T, state *states.State) { + m := testModule(t, "empty") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + // Record the order we see Apply var actual []string var actualLock sync.Mutex @@ -3466,6 +3561,9 @@ module.B: provider = provider.aws foo = foo type = aws_instance + + Dependencies: + module.A.aws_instance.foo `) } @@ -8706,7 +8804,7 @@ aws_instance.foo: type = aws_instance Dependencies: - module.child + module.child.aws_instance.mod module.child: aws_instance.mod: diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index dbd57eb53..e956e7791 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1964,3 +1964,81 @@ func TestContext2Refresh_dataResourceDependsOn(t *testing.T) { t.Fatalf("unexpected errors: %s", diags.Err()) } } + +// verify that dependencies are updated in the state during refresh +func TestRefresh_updateDependencies(t *testing.T) { + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "bar", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar","foo":"foo"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "aws_instance" "bar" { + foo = aws_instance.foo.id +} + +resource "aws_instance" "foo" { +}`, + }) + + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: state, + }) + + result, diags := ctx.Refresh() + if diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + expect := strings.TrimSpace(` +aws_instance.bar: + ID = bar + provider = provider.aws + foo = foo + + Dependencies: + aws_instance.foo +aws_instance.foo: + ID = foo + provider = provider.aws +`) + + checkStateString(t, result, expect) +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 6b839fcaf..cbabaefb1 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -24,7 +24,6 @@ import ( type EvalApply struct { Addr addrs.ResourceInstance Config *configs.Resource - Dependencies []addrs.Referenceable State **states.ResourceInstanceObject Change **plans.ResourceInstanceChange ProviderAddr addrs.AbsProviderConfig @@ -271,10 +270,9 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { var newState *states.ResourceInstanceObject if !newVal.IsNull() { // null value indicates that the object is deleted, so we won't set a new state in that case newState = &states.ResourceInstanceObject{ - Status: states.ObjectReady, - Value: newVal, - Private: resp.Private, - Dependencies: n.Dependencies, // Should be populated by the caller from the StateDependencies method on the resource instance node + Status: states.ObjectReady, + Value: newVal, + Private: resp.Private, } } diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 4999480f5..e58ec7c6e 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -21,7 +21,6 @@ import ( type EvalReadData struct { Addr addrs.ResourceInstance Config *configs.Resource - Dependencies []addrs.Referenceable Provider *providers.Interface ProviderAddr addrs.AbsProviderConfig ProviderSchema **ProviderSchema @@ -161,9 +160,8 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { } 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 - Dependencies: n.Dependencies, + Value: change.After, + Status: states.ObjectPlanned, // because the partial value in the plan must be used for now } *n.OutputState = state } @@ -275,9 +273,8 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { }, } state := &states.ResourceInstanceObject{ - Value: change.After, - Status: states.ObjectReady, // because we completed the read from the provider - Dependencies: n.Dependencies, + Value: change.After, + Status: states.ObjectReady, // because we completed the read from the provider } err = ctx.Hook(func(h Hook) (HookAction, error) { @@ -306,14 +303,13 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) { // 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 - ProviderSchema **ProviderSchema - Output **states.ResourceInstanceObject - Config *configs.Resource - Change **plans.ResourceInstanceChange - StateReferences []addrs.Referenceable + Addr addrs.ResourceInstance + Provider *providers.Interface + ProviderAddr addrs.AbsProviderConfig + ProviderSchema **ProviderSchema + Output **states.ResourceInstanceObject + Config *configs.Resource + Change **plans.ResourceInstanceChange } func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { @@ -385,9 +381,8 @@ func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { if n.Output != nil { *n.Output = &states.ResourceInstanceObject{ - Value: newVal, - Status: states.ObjectReady, - Dependencies: n.StateReferences, + Value: newVal, + Status: states.ObjectReady, } } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index b611113e3..270507051 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -200,6 +200,10 @@ type EvalWriteState struct { // ProviderAddr is the address of the provider configuration that // produced the given object. ProviderAddr addrs.AbsProviderConfig + + // Dependencies are the inter-resource dependencies to be stored in the + // state. + Dependencies []addrs.AbsResource } func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { @@ -215,7 +219,6 @@ func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { if n.ProviderAddr.ProviderConfig.Type == "" { return nil, fmt.Errorf("failed to write state for %s, missing provider type", absAddr) } - obj := *n.State if obj == nil || obj.Value.IsNull() { // No need to encode anything: we'll just write it directly. @@ -223,6 +226,10 @@ func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[TRACE] EvalWriteState: removing state object for %s", absAddr) return nil, nil } + + // store the new deps in the state + obj.Dependencies = n.Dependencies + if n.ProviderSchema == nil || *n.ProviderSchema == nil { // Should never happen, unless our state object is nil panic("EvalWriteState used with pointer to nil ProviderSchema object") diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 73b6b9a56..615731328 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -155,6 +155,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Connect references so ordering is correct &ReferenceTransformer{}, + &AttachDependenciesTransformer{}, // Destruction ordering &DestroyEdgeTransformer{ diff --git a/terraform/graph_builder_refresh.go b/terraform/graph_builder_refresh.go index 0342cdbe8..1c7ae4898 100644 --- a/terraform/graph_builder_refresh.go +++ b/terraform/graph_builder_refresh.go @@ -165,6 +165,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer { // Connect so that the references are ready for targeting. We'll // have to connect again later for providers and so on. &ReferenceTransformer{}, + &AttachDependenciesTransformer{}, // Target &TargetsTransformer{ diff --git a/terraform/node_data_refresh.go b/terraform/node_data_refresh.go index 2a4327f91..7133d42bf 100644 --- a/terraform/node_data_refresh.go +++ b/terraform/node_data_refresh.go @@ -169,7 +169,6 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode { &EvalReadData{ Addr: addr.Resource, Config: n.Config, - Dependencies: n.StateReferences(), Provider: &provider, ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index d147b42e4..a1a4fc0b5 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -3,7 +3,6 @@ package terraform import ( "fmt" "log" - "sort" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/configs" @@ -93,8 +92,8 @@ type NodeAbstractResourceInstance struct { // The fields below will be automatically set using the Attach // interfaces if you're running those transforms, but also be explicitly // set if you already have that information. - ResourceState *states.Resource + Dependencies []addrs.AbsResource } var ( @@ -220,7 +219,8 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { // If we have a configuration attached then we'll delegate to our // embedded abstract resource, which knows how to extract dependencies - // from configuration. + // from configuration. If there is no config, then the dependencies will + // be connected during destroy from those stored in the state. if n.Config != nil { if n.Schema == nil { // We'll produce a log message about this out here so that @@ -232,8 +232,10 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { return n.NodeAbstractResource.References() } - // Otherwise, if we have state then we'll use the values stored in state - // as a fallback. + // FIXME: remove once the deprecated DependsOn values have been removed from state + // The state dependencies are now connected in a separate transformation as + // absolute addresses, but we need to keep this here until we can be sure + // that no state will need to use the old depends_on references. if rs := n.ResourceState; rs != nil { if s := rs.Instance(n.InstanceKey); s != nil { // State is still storing dependencies as old-style strings, so we'll @@ -248,23 +250,23 @@ func (n *NodeAbstractResourceInstance) References() []*addrs.Reference { // https://github.com/hashicorp/terraform/issues/21407 if s.Current == nil { log.Printf("[WARN] no current state found for %s", n.Name()) - } else { - for _, addr := range s.Current.Dependencies { - if addr == nil { - // Should never happen; indicates a bug in the state loader - panic(fmt.Sprintf("dependencies for current object on %s contains nil address", n.ResourceInstanceAddr())) - } - - // This is a little weird: we need to manufacture an addrs.Reference - // with a fake range here because the state isn't something we can - // make source references into. - result = append(result, &addrs.Reference{ - Subject: addr, - SourceRange: tfdiags.SourceRange{ - Filename: "(state file)", - }, - }) + return nil + } + for _, addr := range s.Current.DependsOn { + if addr == nil { + // Should never happen; indicates a bug in the state loader + panic(fmt.Sprintf("dependencies for current object on %s contains nil address", n.ResourceInstanceAddr())) } + + // This is a little weird: we need to manufacture an addrs.Reference + // with a fake range here because the state isn't something we can + // make source references into. + result = append(result, &addrs.Reference{ + Subject: addr, + SourceRange: tfdiags.SourceRange{ + Filename: "(state file)", + }, + }) } return result } @@ -288,67 +290,17 @@ func dottedInstanceAddr(tr addrs.ResourceInstance) string { return tr.Resource.String() + suffix } -// StateReferences returns the dependencies to put into the state for -// this resource. -func (n *NodeAbstractResourceInstance) StateReferences() []addrs.Referenceable { - selfAddrs := n.ReferenceableAddrs() - - // Since we don't include the source location references in our - // results from this method, we'll also filter out duplicates: - // there's no point in listing the same object twice without - // that additional context. - seen := map[string]struct{}{} - - // Pretend that we've already "seen" all of our own addresses so that we - // won't record self-references in the state. This can arise if, for - // example, a provisioner for a resource refers to the resource itself, - // which is valid (since provisioners always run after apply) but should - // not create an explicit dependency edge. - for _, selfAddr := range selfAddrs { - seen[selfAddr.String()] = struct{}{} - if riAddr, ok := selfAddr.(addrs.ResourceInstance); ok { - seen[riAddr.ContainingResource().String()] = struct{}{} +// StateDependencies returns the dependencies saved in the state. +func (n *NodeAbstractResourceInstance) StateDependencies() []addrs.AbsResource { + if rs := n.ResourceState; rs != nil { + if s := rs.Instance(n.InstanceKey); s != nil { + if s.Current != nil { + return s.Current.Dependencies + } } } - depsRaw := n.References() - deps := make([]addrs.Referenceable, 0, len(depsRaw)) - for _, d := range depsRaw { - subj := d.Subject - if mco, isOutput := subj.(addrs.ModuleCallOutput); isOutput { - // For state dependencies, we simplify outputs to just refer - // to the module as a whole. It's not really clear why we do this, - // but this logic is preserved from before the 0.12 rewrite of - // this function. - subj = mco.Call - } - - k := subj.String() - if _, exists := seen[k]; exists { - continue - } - seen[k] = struct{}{} - switch tr := subj.(type) { - case addrs.ResourceInstance: - deps = append(deps, tr) - case addrs.Resource: - deps = append(deps, tr) - case addrs.ModuleCallInstance: - deps = append(deps, tr) - default: - // No other reference types are recorded in the state. - } - } - - // We'll also sort them, since that'll avoid creating changes in the - // serialized state that make no semantic difference. - sort.Slice(deps, func(i, j int) bool { - // Simple string-based sort because we just care about consistency, - // not user-friendliness. - return deps[i].String() < deps[j].String() - }) - - return deps + return nil } func (n *NodeAbstractResource) SetProvider(p addrs.AbsProviderConfig) { diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index d79532467..2bd25174a 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -28,12 +28,13 @@ type NodeApplyableResourceInstance struct { } var ( - _ GraphNodeResource = (*NodeApplyableResourceInstance)(nil) - _ GraphNodeResourceInstance = (*NodeApplyableResourceInstance)(nil) - _ GraphNodeCreator = (*NodeApplyableResourceInstance)(nil) - _ GraphNodeReferencer = (*NodeApplyableResourceInstance)(nil) - _ GraphNodeDeposer = (*NodeApplyableResourceInstance)(nil) - _ GraphNodeEvalable = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeResource = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeResourceInstance = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeCreator = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeReferencer = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeDeposer = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeEvalable = (*NodeApplyableResourceInstance)(nil) + _ GraphNodeAttachDependencies = (*NodeApplyableResourceInstance)(nil) ) // GraphNodeAttachDestroyer @@ -97,6 +98,11 @@ func (n *NodeApplyableResourceInstance) References() []*addrs.Reference { return ret } +// GraphNodeAttachDependencies +func (n *NodeApplyableResourceInstance) AttachDependencies(deps []addrs.AbsResource) { + n.Dependencies = deps +} + // GraphNodeEvalable func (n *NodeApplyableResourceInstance) EvalTree() EvalNode { addr := n.ResourceInstanceAddr() @@ -171,7 +177,6 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou &EvalReadData{ Addr: addr.Resource, Config: n.Config, - Dependencies: n.StateReferences(), Planned: &change, // setting this indicates that the result must be complete Provider: &provider, ProviderAddr: n.ResolvedProvider, @@ -341,7 +346,6 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe &EvalApply{ Addr: addr.Resource, Config: n.Config, - Dependencies: n.StateReferences(), State: &state, Change: &diffApply, Provider: &provider, @@ -363,6 +367,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, State: &state, + Dependencies: n.Dependencies, }, &EvalApplyProvisioners{ Addr: addr.Resource, @@ -384,6 +389,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, State: &state, + Dependencies: n.Dependencies, }, &EvalIf{ If: func(ctx EvalContext) (bool, error) { diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 0f74bbe61..05ccefc34 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -78,8 +78,8 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou // Check and see if any of our dependencies have changes. changes := ctx.Changes() - for _, d := range n.StateReferences() { - ri, ok := d.(addrs.ResourceInstance) + for _, d := range n.References() { + ri, ok := d.Subject.(addrs.ResourceInstance) if !ok { continue } @@ -114,7 +114,6 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou &EvalReadData{ Addr: addr.Resource, Config: n.Config, - Dependencies: n.StateReferences(), Provider: &provider, ProviderAddr: n.ResolvedProvider, ProviderSchema: &providerSchema, diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 9daeabfa6..7a1ebc94a 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -18,6 +18,10 @@ import ( // NodeRefreshableManagedResourceInstance. Resource count orphans are also added. type NodeRefreshableManagedResource struct { *NodeAbstractResource + + // We attach dependencies to the Resource during refresh, since the + // instances are instantiated during DynamicExpand. + Dependencies []addrs.AbsResource } var ( @@ -27,8 +31,14 @@ var ( _ GraphNodeReferencer = (*NodeRefreshableManagedResource)(nil) _ GraphNodeResource = (*NodeRefreshableManagedResource)(nil) _ GraphNodeAttachResourceConfig = (*NodeRefreshableManagedResource)(nil) + _ GraphNodeAttachDependencies = (*NodeRefreshableManagedResource)(nil) ) +// GraphNodeAttachDependencies +func (n *NodeRefreshableManagedResource) AttachDependencies(deps []addrs.AbsResource) { + n.Dependencies = deps +} + // GraphNodeDynamicExpandable func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, error) { var diags tfdiags.Diagnostics @@ -58,6 +68,7 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, // Add the config and state since we don't do that via transforms a.Config = n.Config a.ResolvedProvider = n.ResolvedProvider + a.Dependencies = n.Dependencies return &NodeRefreshableManagedResourceInstance{ NodeAbstractResourceInstance: a, diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 0d8cc0900..0baa2a204 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -608,9 +608,6 @@ aws_instance.bar: foo = true type = aws_instance - Dependencies: - module.child - module.child: Outputs: @@ -666,6 +663,9 @@ module.child: provider = provider.aws type = aws_instance value = bar + + Dependencies: + aws_instance.foo ` const testTerraformApplyOutputOrphanStr = ` @@ -784,17 +784,11 @@ aws_instance.foo.1: provider = provider.aws foo = number 1 type = aws_instance - - Dependencies: - aws_instance.foo[0] aws_instance.foo.2: ID = foo provider = provider.aws foo = number 2 type = aws_instance - - Dependencies: - aws_instance.foo[0] ` const testTerraformApplyProvisionerDiffStr = ` @@ -859,7 +853,7 @@ aws_instance.a: type = aws_instance Dependencies: - module.child + module.child.aws_instance.child module.child: aws_instance.child: @@ -877,7 +871,7 @@ aws_instance.a: type = aws_instance Dependencies: - module.child + module.child.module.grandchild.aws_instance.c module.child.grandchild: aws_instance.c: @@ -897,7 +891,7 @@ module.child: type = aws_instance Dependencies: - module.grandchild + module.child.module.grandchild.aws_instance.c module.child.grandchild: aws_instance.c: ID = foo @@ -1305,9 +1299,6 @@ data.null_data_source.bar: ID = foo provider = provider.null bar = yes - - Dependencies: - data.null_data_source.foo data.null_data_source.foo: ID = foo provider = provider.null diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index be1540ffe..0ad66a713 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -54,10 +54,14 @@ type DestroyEdgeTransformer struct { func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Build a map of what is being destroyed (by address string) to - // the list of destroyers. Usually there will be at most one destroyer - // per node, but we allow multiple if present for completeness. + // the list of destroyers. destroyers := make(map[string][]GraphNodeDestroyer) destroyerAddrs := make(map[string]addrs.AbsResourceInstance) + + // destroyersByResource records each destroyer by the AbsResourceAddress. + // We use this because dependencies are only referenced as resources, but we + // will want to connect all the individual instances for correct ordering. + destroyersByResource := make(map[string][]GraphNodeDestroyer) for _, v := range g.Vertices() { dn, ok := v.(GraphNodeDestroyer) if !ok { @@ -66,6 +70,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { addrP := dn.DestroyAddr() if addrP == nil { + log.Printf("[WARN] DestroyEdgeTransformer: %q (%T) has no destroy address", dag.VertexName(dn), v) continue } addr := *addrP @@ -74,6 +79,9 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(dn), v, key) destroyers[key] = append(destroyers[key], dn) destroyerAddrs[key] = addr + + resAddr := addr.Resource.Absolute(addr.Module).String() + destroyersByResource[resAddr] = append(destroyersByResource[resAddr], dn) } // If we aren't destroying anything, there will be no edges to make @@ -82,6 +90,29 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { return nil } + type withDeps interface { + StateDependencies() []addrs.AbsResource + } + + // Connect destroy dependencies directly as stored in the state in case + // there's no configuration to create the edges otherwise. + for _, ds := range destroyers { + for _, des := range ds { + ri, ok := des.(withDeps) + if !ok { + continue + } + + for _, resAddr := range ri.StateDependencies() { + for _, desDep := range destroyersByResource[resAddr.String()] { + log.Printf("[TRACE] DestroyEdgeTransformer: %s depends on %s\n", dag.VertexName(des), dag.VertexName(desDep)) + g.Connect(dag.BasicEdge(desDep, des)) + + } + } + } + } + // Go through and connect creators to destroyers. Going along with // our example, this makes: A_d => A for _, v := range g.Vertices() { @@ -95,13 +126,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { continue } - key := addr.String() - ds := destroyers[key] - if len(ds) == 0 { - continue - } - - for _, d := range ds { + for _, d := range destroyers[addr.String()] { // For illustrating our example a_d := d.(dag.Vertex) a := v @@ -124,6 +149,8 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } + // FIXME: connect StateDependencies in here somewhere + // This is strange but is the easiest way to get the dependencies // of a node that is being destroyed. We use another graph to make sure // the resource is in the graph and ask for references. We have to do this diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index f9199be0e..11d261e33 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -6,9 +6,11 @@ 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 @@ -37,6 +39,11 @@ type GraphNodeReferencer interface { References() []*addrs.Reference } +type GraphNodeAttachDependencies interface { + GraphNodeResource + AttachDependencies([]addrs.AbsResource) +} + // 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 @@ -84,11 +91,81 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { for _, parent := range parents { g.Connect(dag.BasicEdge(v, parent)) } + + if len(parents) > 0 { + continue + } } 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 { + for _, v := range g.Vertices() { + attacher, ok := v.(GraphNodeAttachDependencies) + if !ok { + continue + } + selfAddr := attacher.ResourceAddr() + + // Data sources don't need to track destroy dependencies + if selfAddr.Resource.Mode == addrs.DataResourceMode { + continue + } + + ans, err := g.Ancestors(v) + if err != nil { + return err + } + + // dedupe addrs when there's multiple instances involved, or + // multiple paths in the un-reduced graph + depMap := map[string]addrs.AbsResource{} + for _, d := range ans.List() { + var addr addrs.AbsResource + + switch d := d.(type) { + case GraphNodeResourceInstance: + instAddr := d.ResourceInstanceAddr() + addr = instAddr.Resource.Resource.Absolute(instAddr.Module) + case GraphNodeResource: + addr = d.ResourceAddr() + default: + continue + } + + // Data sources don't need to track destroy dependencies + if addr.Resource.Mode == addrs.DataResourceMode { + continue + } + + if addr.Equal(selfAddr) { + continue + } + depMap[addr.String()] = addr + } + + deps := make([]addrs.AbsResource, 0, len(depMap)) + for _, d := range depMap { + deps = append(deps, d) + } + + log.Printf("[TRACE] AttachDependenciesTransformer: %s depends on %s", attacher.ResourceAddr(), deps) + attacher.AttachDependencies(deps) + + } + return nil +} + // DestroyReferenceTransformer is a GraphTransformer that reverses the edges // for locals and outputs that depend on other nodes which will be // removed during destroy. If a destroy node is evaluated before the local or