core: Clean up resource states when they are orphaned

We previously had mechanisms to clean up only individual instance states,
leaving behind empty resource husks in the state after they were all
destroyed.

This takes care of it in the "orphan" case. It does not yet do it in the
"terraform destroy" or "terraform plan -destroy" cases because we don't
have anywhere to record in the plan that we're actually destroying and so
the resource configurations should be ignored and _everything_ should be
cleaned. We'll let the state be not-quite-empty in that case for now,
since it doesn't really hurt; cleaning up orphans is the main case because
the state will live on afterwards and so leftover cruft will accumulate
over the course of many changes.

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.

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(`
-<no state>
-module.middle:
-  <no state>
-module.middle.bottom:
-  <no state>
-		`)
-	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))
 	}
 }
 

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
+}

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},
 

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,
+	}
+}

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

terraform/test-fixtures/apply-orphan-resource/main.tf

@@ -0,0 +1,7 @@
+resource "test_thing" "zero" {
+  count = 0
+}
+
+resource "test_thing" "one" {
+  count = 1
+}

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
+
+}