diff --git a/states/sync.go b/states/sync.go index 20ef6d123..49ea259be 100644 --- a/states/sync.go +++ b/states/sync.go @@ -186,7 +186,8 @@ func (s *SyncState) SetResourceMeta(addr addrs.AbsResource, eachMode EachMode, p // RemoveResource removes the entire state for the given resource, taking with // it any instances associated with the resource. This should generally be // called only for resource objects whose instances have all been destroyed, -// but that is not enforced by this method. +// but that is not enforced by this method. (Use RemoveResourceIfEmpty instead +// to safely check first.) func (s *SyncState) RemoveResource(addr addrs.AbsResource) { s.lock.Lock() defer s.lock.Unlock() @@ -196,6 +197,35 @@ func (s *SyncState) RemoveResource(addr addrs.AbsResource) { s.maybePruneModule(addr.Module) } +// RemoveResourceIfEmpty is similar to RemoveResource but first checks to +// make sure there are no instances or objects left in the resource. +// +// Returns true if the resource was removed, or false if remaining child +// objects prevented its removal. Returns true also if the resource was +// already absent, and thus no action needed to be taken. +func (s *SyncState) RemoveResourceIfEmpty(addr addrs.AbsResource) bool { + s.lock.Lock() + defer s.lock.Unlock() + + ms := s.state.Module(addr.Module) + if ms == nil { + return true // nothing to do + } + rs := ms.Resource(addr.Resource) + if rs == nil { + return true // nothing to do + } + if len(rs.Instances) != 0 { + // We don't check here for the possibility of instances that exist + // but don't have any objects because it's the responsibility of the + // instance-mutation methods to prune those away automatically. + return false + } + ms.RemoveResource(addr.Resource) + s.maybePruneModule(addr.Module) + return true +} + // MaybeFixUpResourceInstanceAddressForCount deals with the situation where a // resource has changed from having "count" set to not set, or vice-versa, and // so we need to rename the zeroth instance key to no key at all, or vice-versa. diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 95b5b504f..b7681e62e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2694,6 +2694,89 @@ module.child: `) } +func TestContext2Apply_orphanResource(t *testing.T) { + // This is a two-step test: + // 1. Apply a configuration with resources that have count set. + // This should place the empty resource object in the state to record + // that each exists, and record any instances. + // 2. Apply an empty configuration against the same state, which should + // then clean up both the instances and the containing resource objects. + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_thing": {}, + }, + } + + // Step 1: create the resources and instances + m := testModule(t, "apply-orphan-resource") + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + _, diags := ctx.Plan() + assertNoErrors(t, diags) + state, diags := ctx.Apply() + assertNoErrors(t, diags) + + // At this point both resources should be recorded in the state, along + // with the single instance associated with test_thing.one. + want := states.BuildState(func (s *states.SyncState) { + providerAddr := addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance) + zeroAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "zero", + }.Absolute(addrs.RootModuleInstance) + oneAddr := addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_thing", + Name: "one", + }.Absolute(addrs.RootModuleInstance) + s.SetResourceMeta(zeroAddr, states.EachList, providerAddr) + s.SetResourceMeta(oneAddr, states.EachList, providerAddr) + s.SetResourceInstanceCurrent(oneAddr.Instance(addrs.IntKey(0)), &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, providerAddr) + }) + if !cmp.Equal(state, want) { + t.Fatalf("wrong state after step 1\n%s", cmp.Diff(want, state)) + } + + // Step 2: update with an empty config, to destroy everything + m = testModule(t, "empty") + ctx = testContext2(t, &ContextOpts{ + Config: m, + State: state, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ), + }) + _, diags = ctx.Plan() + assertNoErrors(t, diags) + state, diags = ctx.Apply() + assertNoErrors(t, diags) + + // The state should now be _totally_ empty, with just an empty root module + // (since that always exists) and no resources at all. + want = states.NewState() + if !cmp.Equal(state, want) { + t.Fatalf("wrong state after step 2\ngot: %swant: %s", spew.Sdump(state), spew.Sdump(want)) + } + +} + func TestContext2Apply_moduleOrphanInheritAlias(t *testing.T) { m := testModule(t, "apply-module-provider-inherit-alias-orphan") p := testProvider("aws") @@ -9137,17 +9220,8 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin } } - //Test that things were destroyed - actual := strings.TrimSpace(state.String()) - expected := strings.TrimSpace(` - -module.middle: - -module.middle.bottom: - - `) - if actual != expected { - t.Fatalf("expected: \n%s\n\nbad: \n%s", expected, actual) + if !state.Empty() { + t.Fatalf("state after apply: %s\nwant empty state", spew.Sdump(state)) } } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 5c08fb16a..09ad55190 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -425,3 +425,31 @@ func (n *EvalWriteResourceState) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } + +// EvalForgetResourceState is an EvalNode implementation that prunes out an +// empty resource-level state for a given resource address, or produces an +// error if it isn't empty after all. +// +// This should be the last action taken for a resource that has been removed +// from the configuration altogether, to clean up the leftover husk of the +// resource in the state after other EvalNodes have destroyed and removed +// all of the instances and instance objects beneath it. +type EvalForgetResourceState struct { + Addr addrs.Resource +} + +func (n *EvalForgetResourceState) Eval(ctx EvalContext) (interface{}, error) { + absAddr := n.Addr.Absolute(ctx.Path()) + state := ctx.State() + + pruned := state.RemoveResourceIfEmpty(absAddr) + if !pruned { + // If this produces an error, it indicates a bug elsewhere in Terraform + // -- probably missing graph nodes, graph edges, or + // incorrectly-implemented evaluation steps. + return nil, fmt.Errorf("orphan resource %s still has a non-empty state after apply; this is a bug in Terraform", absAddr) + } + log.Printf("[TRACE] EvalForgetResourceState: Pruned husk of %s from state", absAddr) + + return nil, nil +} diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index c34cb1a3e..7182dd7db 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -74,6 +74,12 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { } } + concreteOrphanResource := func(a *NodeAbstractResource) dag.Vertex { + return &NodeDestroyResource{ + NodeAbstractResource: a, + } + } + concreteResourceInstance := func(a *NodeAbstractResourceInstance) dag.Vertex { return &NodeApplyableResourceInstance{ NodeAbstractResourceInstance: a, @@ -99,6 +105,19 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { Changes: b.Changes, }, + // Creates extra cleanup nodes for any entire resources that are + // no longer present in config, so we can make sure we clean up the + // leftover empty resource states after the instances have been + // destroyed. + // (We don't track this particular type of change in the plan because + // it's just cleanup of our own state object, and so doesn't effect + // any real remote objects or consumable outputs.) + &OrphanResourceTransformer{ + Concrete: concreteOrphanResource, + Config: b.Config, + State: b.State, + }, + // Create orphan output nodes &OrphanOutputTransformer{Config: b.Config, State: b.State}, diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index ede2ba217..ca2267e47 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -12,7 +12,8 @@ import ( "github.com/hashicorp/terraform/states" ) -// NodeDestroyResource represents a resource that is to be destroyed. +// NodeDestroyResourceInstance represents a resource instance that is to be +// destroyed. type NodeDestroyResourceInstance struct { *NodeAbstractResourceInstance @@ -266,3 +267,55 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode { }, } } + +// NodeDestroyResourceInstance represents a resource that is to be destroyed. +// +// Destroying a resource is a state-only operation: it is the individual +// instances being destroyed that affects remote objects. During graph +// construction, NodeDestroyResource should always depend on any other node +// related to the given resource, since it's just a final cleanup to avoid +// leaving skeleton resource objects in state after their instances have +// all been destroyed. +type NodeDestroyResource struct { + *NodeAbstractResource +} + +var ( + _ GraphNodeResource = (*NodeDestroyResource)(nil) + _ GraphNodeReferenceable = (*NodeDestroyResource)(nil) + _ GraphNodeReferencer = (*NodeDestroyResource)(nil) + _ GraphNodeEvalable = (*NodeDestroyResource)(nil) +) + +func (n *NodeDestroyResource) Name() string { + return n.ResourceAddr().String() + " (clean up state)" +} + +// GraphNodeReferenceable, overriding NodeAbstractResource +func (n *NodeDestroyResource) ReferenceableAddrs() []addrs.Referenceable { + // NodeDestroyResource doesn't participate in references: the graph + // builder that created it should ensure directly that it already depends + // on every other node related to its resource, without relying on + // references. + return nil +} + +// GraphNodeReferencer, overriding NodeAbstractResource +func (n *NodeDestroyResource) References() []*addrs.Reference { + // NodeDestroyResource doesn't participate in references: the graph + // builder that created it should ensure directly that it already depends + // on every other node related to its resource, without relying on + // references. + return nil +} + +// GraphNodeEvalable +func (n *NodeDestroyResource) EvalTree() EvalNode { + // This EvalNode will produce an error if the resource isn't already + // empty by the time it is called, since it should just be pruning the + // leftover husk of a resource in state after all of the child instances + // and their objects were destroyed. + return &EvalForgetResourceState{ + Addr: n.ResourceAddr().Resource, + } +} diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 52cc15139..9e74abfea 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -153,7 +153,6 @@ func (n *NodePlanDeposedResourceInstanceObject) EvalTree() EvalNode { return seq } - // NodeDestroyDeposedResourceInstanceObject represents deposed resource // instance objects during apply. Nodes of this type are inserted by // DiffTransformer when the planned changeset contains "delete" changes for diff --git a/terraform/test-fixtures/apply-orphan-resource/main.tf b/terraform/test-fixtures/apply-orphan-resource/main.tf new file mode 100644 index 000000000..3e093ac83 --- /dev/null +++ b/terraform/test-fixtures/apply-orphan-resource/main.tf @@ -0,0 +1,7 @@ +resource "test_thing" "zero" { + count = 0 +} + +resource "test_thing" "one" { + count = 1 +} diff --git a/terraform/transform_orphan_resource.go b/terraform/transform_orphan_resource.go index 5ad9fe7b3..50df1781e 100644 --- a/terraform/transform_orphan_resource.go +++ b/terraform/transform_orphan_resource.go @@ -1,6 +1,8 @@ package terraform import ( + "log" + "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/dag" "github.com/hashicorp/terraform/states" @@ -84,9 +86,94 @@ func (t *OrphanResourceInstanceTransformer) transform(g *Graph, ms *states.Modul if f := t.Concrete; f != nil { node = f(abstract) } + log.Printf("[TRACE] OrphanResourceInstanceTransformer: adding single-instance orphan node for %s", addr) g.Add(node) } } return nil } + +// OrphanResourceTransformer is a GraphTransformer that adds orphaned +// resources to the graph. An "orphan" is a resource that is present in +// the state but no longer present in the config. +// +// This is separate to OrphanResourceInstanceTransformer in that it deals with +// whole resources, rather than individual instances of resources. Orphan +// resource nodes are only used during apply to clean up leftover empty +// resource state skeletons, after all of the instances inside have been +// removed. +// +// This transformer will also create edges in the graph to any pre-existing +// node that creates or destroys the entire orphaned resource or any of its +// instances, to ensure that the "orphan-ness" of a resource is always dealt +// with after all other aspects of it. +type OrphanResourceTransformer struct { + Concrete ConcreteResourceNodeFunc + + // State is the global state. + State *states.State + + // Config is the root node in the configuration tree. + Config *configs.Config +} + +func (t *OrphanResourceTransformer) Transform(g *Graph) error { + if t.State == nil { + // If the entire state is nil, there can't be any orphans + return nil + } + if t.Config == nil { + // Should never happen: we can't be doing any Terraform operations + // without at least an empty configuration. + panic("OrphanResourceTransformer used without setting Config") + } + + // We'll first collect up the existing nodes for each resource so we can + // create dependency edges for any new nodes we create. + deps := map[string][]dag.Vertex{} + for _, v := range g.Vertices() { + switch tv := v.(type) { + case GraphNodeResourceInstance: + k := tv.ResourceInstanceAddr().ContainingResource().String() + deps[k] = append(deps[k], v) + case GraphNodeResource: + k := tv.ResourceAddr().String() + deps[k] = append(deps[k], v) + case GraphNodeDestroyer: + k := tv.DestroyAddr().ContainingResource().String() + deps[k] = append(deps[k], v) + } + } + + for _, ms := range t.State.Modules { + moduleAddr := ms.Addr + + mc := t.Config.DescendentForInstance(moduleAddr) // might be nil if whole module has been removed + + for _, rs := range ms.Resources { + if mc != nil { + if r := mc.Module.ResourceByAddr(rs.Addr); r != nil { + // It's in the config, so nothing to do for this one. + continue + } + } + + addr := rs.Addr.Absolute(moduleAddr) + abstract := NewNodeAbstractResource(addr) + var node dag.Vertex = abstract + if f := t.Concrete; f != nil { + node = f(abstract) + } + log.Printf("[TRACE] OrphanResourceTransformer: adding whole-resource orphan node for %s", addr) + g.Add(node) + for _, dn := range deps[addr.String()] { + log.Printf("[TRACE] OrphanResourceTransformer: node %q depends on %q", dag.VertexName(node), dag.VertexName(dn)) + g.Connect(dag.BasicEdge(node, dn)) + } + } + } + + return nil + +}