core: Orphan addressing / targeting

Instead of trying to skip non-targeted orphans as they are added to
the graph in OrphanTransformer, remove knowledge of targeting from
OrphanTransformer and instead make the orphan resource nodes properly
addressable.

That allows us to use existing logic in TargetTransformer to filter out
the nodes appropriately. This does require adding TargetTransformer to the
list of transforms that run during DynamicExpand so that targeting can
be applied to nodes with expanded counts.

Fixes #4515
Fixes #2538
Fixes #4462

terraform/context_apply_test.go

@@ -3359,6 +3359,60 @@ aws_instance.bar:
 	`)
 }
 
+// https://github.com/hashicorp/terraform/issues/4462
+func TestContext2Apply_targetedDestroyModule(t *testing.T) {
+	m := testModule(t, "apply-targeted-module")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: &State{
+			Modules: []*ModuleState{
+				&ModuleState{
+					Path: rootModulePath,
+					Resources: map[string]*ResourceState{
+						"aws_instance.foo": resourceState("aws_instance", "i-bcd345"),
+						"aws_instance.bar": resourceState("aws_instance", "i-abc123"),
+					},
+				},
+				&ModuleState{
+					Path: []string{"root", "child"},
+					Resources: map[string]*ResourceState{
+						"aws_instance.foo": resourceState("aws_instance", "i-bcd345"),
+						"aws_instance.bar": resourceState("aws_instance", "i-abc123"),
+					},
+				},
+			},
+		},
+		Targets: []string{"module.child.aws_instance.foo"},
+		Destroy: true,
+	})
+
+	if _, err := ctx.Plan(); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	state, err := ctx.Apply()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	checkStateString(t, state, `
+aws_instance.bar:
+  ID = i-abc123
+aws_instance.foo:
+  ID = i-bcd345
+
+module.child:
+  aws_instance.bar:
+    ID = i-abc123
+	`)
+}
+
 func TestContext2Apply_targetedDestroyCountIndex(t *testing.T) {
 	m := testModule(t, "apply-targeted-count")
 	p := testProvider("aws")

terraform/context_plan_test.go

@@ -1647,6 +1647,12 @@ func TestContext2Plan_targetedOrphan(t *testing.T) {
 								ID: "i-789xyz",
 							},
 						},
+						"aws_instance.nottargeted": &ResourceState{
+							Type: "aws_instance",
+							Primary: &InstanceState{
+								ID: "i-abc123",
+							},
+						},
 					},
 				},
 			},
@@ -1667,8 +1673,150 @@ DESTROY: aws_instance.orphan
 
 STATE:
 
+aws_instance.nottargeted:
+  ID = i-abc123
 aws_instance.orphan:
-  ID = i-789xyz`)
+  ID = i-789xyz
+`)
+	if actual != expected {
+		t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
+	}
+}
+
+// https://github.com/hashicorp/terraform/issues/2538
+func TestContext2Plan_targetedModuleOrphan(t *testing.T) {
+	m := testModule(t, "plan-targeted-module-orphan")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: &State{
+			Modules: []*ModuleState{
+				&ModuleState{
+					Path: []string{"root", "child"},
+					Resources: map[string]*ResourceState{
+						"aws_instance.orphan": &ResourceState{
+							Type: "aws_instance",
+							Primary: &InstanceState{
+								ID: "i-789xyz",
+							},
+						},
+						"aws_instance.nottargeted": &ResourceState{
+							Type: "aws_instance",
+							Primary: &InstanceState{
+								ID: "i-abc123",
+							},
+						},
+					},
+				},
+			},
+		},
+		Destroy: true,
+		Targets: []string{"module.child.aws_instance.orphan"},
+	})
+
+	plan, err := ctx.Plan()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(plan.String())
+	expected := strings.TrimSpace(`DIFF:
+
+module.child:
+  DESTROY: aws_instance.orphan
+
+STATE:
+
+module.child:
+  aws_instance.nottargeted:
+    ID = i-abc123
+  aws_instance.orphan:
+    ID = i-789xyz
+`)
+	if actual != expected {
+		t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
+	}
+}
+
+// https://github.com/hashicorp/terraform/issues/4515
+func TestContext2Plan_targetedOverTen(t *testing.T) {
+	m := testModule(t, "plan-targeted-over-ten")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	resources := make(map[string]*ResourceState)
+	var expectedState []string
+	for i := 0; i < 13; i++ {
+		key := fmt.Sprintf("aws_instance.foo.%d", i)
+		id := fmt.Sprintf("i-abc%d", i)
+		resources[key] = &ResourceState{
+			Type:    "aws_instance",
+			Primary: &InstanceState{ID: id},
+		}
+		expectedState = append(expectedState,
+			fmt.Sprintf("%s:\n  ID = %s\n", key, id))
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: &State{
+			Modules: []*ModuleState{
+				&ModuleState{
+					Path:      rootModulePath,
+					Resources: resources,
+				},
+			},
+		},
+		Targets: []string{"aws_instance.foo[1]"},
+	})
+
+	plan, err := ctx.Plan()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(plan.String())
+	sort.Strings(expectedState)
+	expected := strings.TrimSpace(`
+DIFF:
+
+
+
+STATE:
+
+aws_instance.foo.0:
+  ID = i-abc0
+aws_instance.foo.1:
+  ID = i-abc1
+aws_instance.foo.10:
+  ID = i-abc10
+aws_instance.foo.11:
+  ID = i-abc11
+aws_instance.foo.12:
+  ID = i-abc12
+aws_instance.foo.2:
+  ID = i-abc2
+aws_instance.foo.3:
+  ID = i-abc3
+aws_instance.foo.4:
+  ID = i-abc4
+aws_instance.foo.5:
+  ID = i-abc5
+aws_instance.foo.6:
+  ID = i-abc6
+aws_instance.foo.7:
+  ID = i-abc7
+aws_instance.foo.8:
+  ID = i-abc8
+aws_instance.foo.9:
+  ID = i-abc9
+	`)
 	if actual != expected {
 		t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
 	}

terraform/graph_config_node_resource.go

@@ -163,9 +163,8 @@ func (n *GraphNodeConfigResource) DynamicExpand(ctx EvalContext) (*Graph, error)
 		// expand orphans, which have all the same semantics in a destroy
 		// as a primary.
 		steps = append(steps, &OrphanTransformer{
-			State:   state,
-			View:    n.Resource.Id(),
-			Targets: n.Targets,
+			State: state,
+			View:  n.Resource.Id(),
 		})
 
 		steps = append(steps, &DeposedTransformer{
@@ -181,6 +180,12 @@ func (n *GraphNodeConfigResource) DynamicExpand(ctx EvalContext) (*Graph, error)
 		})
 	}
 
+	// We always want to apply targeting
+	steps = append(steps, &TargetsTransformer{
+		ParsedTargets: n.Targets,
+		Destroy:       n.DestroyMode != DestroyNone,
+	})
+
 	// Always end with the root being added
 	steps = append(steps, &RootTransformer{})
 

terraform/resource_address_test.go

@@ -28,6 +28,15 @@ func TestParseResourceAddress(t *testing.T) {
 				Index:        2,
 			},
 		},
+		"implicit primary, explicit index over ten": {
+			Input: "aws_instance.foo[12]",
+			Expected: &ResourceAddress{
+				Type:         "aws_instance",
+				Name:         "foo",
+				InstanceType: TypePrimary,
+				Index:        12,
+			},
+		},
 		"explicit primary, explicit index": {
 			Input: "aws_instance.foo.primary[2]",
 			Expected: &ResourceAddress{
@@ -184,6 +193,21 @@ func TestResourceAddressEquals(t *testing.T) {
 			},
 			Expect: true,
 		},
+		"index over ten": {
+			Address: &ResourceAddress{
+				Type:         "aws_instance",
+				Name:         "foo",
+				InstanceType: TypePrimary,
+				Index:        1,
+			},
+			Other: &ResourceAddress{
+				Type:         "aws_instance",
+				Name:         "foo",
+				InstanceType: TypePrimary,
+				Index:        13,
+			},
+			Expect: false,
+		},
 		"different type": {
 			Address: &ResourceAddress{
 				Type:         "aws_instance",

terraform/state.go

@@ -9,6 +9,7 @@ import (
 	"log"
 	"reflect"
 	"sort"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/terraform/config"
@@ -661,6 +662,65 @@ func (m *ModuleState) String() string {
 	return buf.String()
 }
 
+// ResourceStateKey is a structured representation of the key used for the
+// ModuleState.Resources mapping
+type ResourceStateKey struct {
+	Name  string
+	Type  string
+	Index int
+}
+
+// Equal determines whether two ResourceStateKeys are the same
+func (rsk *ResourceStateKey) Equal(other *ResourceStateKey) bool {
+	if rsk == nil || other == nil {
+		return false
+	}
+	if rsk.Type != other.Type {
+		return false
+	}
+	if rsk.Name != other.Name {
+		return false
+	}
+	if rsk.Index != other.Index {
+		return false
+	}
+	return true
+}
+
+func (rsk *ResourceStateKey) String() string {
+	if rsk == nil {
+		return ""
+	}
+	if rsk.Index == -1 {
+		return fmt.Sprintf("%s.%s", rsk.Type, rsk.Name)
+	}
+	return fmt.Sprintf("%s.%s.%d", rsk.Type, rsk.Name, rsk.Index)
+}
+
+// ParseResourceStateKey accepts a key in the format used by
+// ModuleState.Resources and returns a resource name and resource index. In the
+// state, a resource has the format "type.name.index" or "type.name". In the
+// latter case, the index is returned as -1.
+func ParseResourceStateKey(k string) (*ResourceStateKey, error) {
+	parts := strings.Split(k, ".")
+	if len(parts) < 2 || len(parts) > 3 {
+		return nil, fmt.Errorf("Malformed resource state key: %s", k)
+	}
+	rsk := &ResourceStateKey{
+		Type:  parts[0],
+		Name:  parts[1],
+		Index: -1,
+	}
+	if len(parts) == 3 {
+		index, err := strconv.Atoi(parts[2])
+		if err != nil {
+			return nil, fmt.Errorf("Malformed resource state key index: %s", k)
+		}
+		rsk.Index = index
+	}
+	return rsk, nil
+}
+
 // ResourceState holds the state of a resource that is used so that
 // a provider can find and manage an existing resource as well as for
 // storing attributes that are used to populate variables of child

terraform/state_test.go

@@ -895,3 +895,57 @@ func TestUpgradeV1State(t *testing.T) {
 		t.Fatalf("bad: %#v", bt)
 	}
 }
+
+func TestParseResourceStateKey(t *testing.T) {
+	cases := []struct {
+		Input       string
+		Expected    *ResourceStateKey
+		ExpectedErr bool
+	}{
+		{
+			Input: "aws_instance.foo.3",
+			Expected: &ResourceStateKey{
+				Type:  "aws_instance",
+				Name:  "foo",
+				Index: 3,
+			},
+		},
+		{
+			Input: "aws_instance.foo.0",
+			Expected: &ResourceStateKey{
+				Type:  "aws_instance",
+				Name:  "foo",
+				Index: 0,
+			},
+		},
+		{
+			Input: "aws_instance.foo",
+			Expected: &ResourceStateKey{
+				Type:  "aws_instance",
+				Name:  "foo",
+				Index: -1,
+			},
+		},
+		{
+			Input:       "aws_instance.foo.malformed",
+			ExpectedErr: true,
+		},
+		{
+			Input:       "aws_instance.foo.malformedwithnumber.123",
+			ExpectedErr: true,
+		},
+		{
+			Input:       "malformed",
+			ExpectedErr: true,
+		},
+	}
+	for _, tc := range cases {
+		rsk, err := ParseResourceStateKey(tc.Input)
+		if rsk != nil && tc.Expected != nil && !rsk.Equal(tc.Expected) {
+			t.Fatalf("%s: expected %s, got %s", tc.Input, tc.Expected, rsk)
+		}
+		if (err != nil) != tc.ExpectedErr {
+			t.Fatalf("%s: expected err: %t, got %s", tc.Input, tc.ExpectedErr, err)
+		}
+	}
+}

terraform/test-fixtures/plan-targeted-module-orphan/main.tf

@@ -0,0 +1,6 @@
+# Once opon a time, there was a child module here
+/*
+module "child" {
+  source = "./child"
+}
+*/

terraform/test-fixtures/plan-targeted-over-ten/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "foo" {
+  count    = 13
+}

terraform/transform_orphan.go

@@ -2,7 +2,6 @@ package terraform
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/module"
@@ -26,11 +25,6 @@ type OrphanTransformer struct {
 	// using the graph path.
 	Module *module.Tree
 
-	// Targets are user-specified resources to target. We need to be aware of
-	// these so we don't improperly identify orphans when they've just been
-	// filtered out of the graph via targeting.
-	Targets []ResourceAddress
-
 	// View, if non-nil will set a view on the module state.
 	View string
 }
@@ -68,22 +62,6 @@ func (t *OrphanTransformer) Transform(g *Graph) error {
 		}
 
 		resourceOrphans := state.Orphans(config)
-		if len(t.Targets) > 0 {
-			var targetedOrphans []string
-			for _, o := range resourceOrphans {
-				targeted := false
-				for _, t := range t.Targets {
-					prefix := fmt.Sprintf("%s.%s.%d", t.Type, t.Name, t.Index)
-					if strings.HasPrefix(o, prefix) {
-						targeted = true
-					}
-				}
-				if targeted {
-					targetedOrphans = append(targetedOrphans, o)
-				}
-			}
-			resourceOrphans = targetedOrphans
-		}
 
 		resourceVertexes = make([]dag.Vertex, len(resourceOrphans))
 		for i, k := range resourceOrphans {
@@ -95,11 +73,15 @@ func (t *OrphanTransformer) Transform(g *Graph) error {
 
 			rs := state.Resources[k]
 
+			rsk, err := ParseResourceStateKey(k)
+			if err != nil {
+				return err
+			}
 			resourceVertexes[i] = g.Add(&graphNodeOrphanResource{
-				ResourceName: k,
-				ResourceType: rs.Type,
-				Provider:     rs.Provider,
-				dependentOn:  rs.Dependencies,
+				Path:        g.Path,
+				ResourceKey: rsk,
+				Provider:    rs.Provider,
+				dependentOn: rs.Dependencies,
 			})
 		}
 	}
@@ -175,15 +157,25 @@ func (n *graphNodeOrphanModule) Expand(b GraphBuilder) (GraphNodeSubgraph, error
 
 // graphNodeOrphanResource is the graph vertex representing an orphan resource..
 type graphNodeOrphanResource struct {
-	ResourceName string
-	ResourceType string
-	Provider     string
+	Path        []string
+	ResourceKey *ResourceStateKey
+	Provider    string
 
 	dependentOn []string
 }
 
+func (n *graphNodeOrphanResource) ConfigType() GraphNodeConfigType {
+	return GraphNodeConfigTypeResource
+}
+
 func (n *graphNodeOrphanResource) ResourceAddress() *ResourceAddress {
-	return n.ResourceAddress()
+	return &ResourceAddress{
+		Index:        n.ResourceKey.Index,
+		InstanceType: TypePrimary,
+		Name:         n.ResourceKey.Name,
+		Path:         n.Path[1:],
+		Type:         n.ResourceKey.Type,
+	}
 }
 
 func (n *graphNodeOrphanResource) DependableName() []string {
@@ -202,11 +194,11 @@ func (n *graphNodeOrphanResource) Flatten(p []string) (dag.Vertex, error) {
 }
 
 func (n *graphNodeOrphanResource) Name() string {
-	return fmt.Sprintf("%s (orphan)", n.ResourceName)
+	return fmt.Sprintf("%s (orphan)", n.ResourceKey)
 }
 
 func (n *graphNodeOrphanResource) ProvidedBy() []string {
-	return []string{resourceProvider(n.ResourceName, n.Provider)}
+	return []string{resourceProvider(n.ResourceKey.Type, n.Provider)}
 }
 
 // GraphNodeEvalable impl.
@@ -217,7 +209,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 	seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)}
 
 	// Build instance info
-	info := &InstanceInfo{Id: n.ResourceName, Type: n.ResourceType}
+	info := &InstanceInfo{Id: n.ResourceKey.String(), Type: n.ResourceKey.Type}
 	seq.Nodes = append(seq.Nodes, &EvalInstanceInfo{Info: info})
 
 	// Refresh the resource
@@ -230,7 +222,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 					Output: &provider,
 				},
 				&EvalReadState{
-					Name:   n.ResourceName,
+					Name:   n.ResourceKey.String(),
 					Output: &state,
 				},
 				&EvalRefresh{
@@ -240,8 +232,8 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 					Output:   &state,
 				},
 				&EvalWriteState{
-					Name:         n.ResourceName,
-					ResourceType: n.ResourceType,
+					Name:         n.ResourceKey.String(),
+					ResourceType: n.ResourceKey.Type,
 					Provider:     n.Provider,
 					Dependencies: n.DependentOn(),
 					State:        &state,
@@ -257,7 +249,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 		Node: &EvalSequence{
 			Nodes: []EvalNode{
 				&EvalReadState{
-					Name:   n.ResourceName,
+					Name:   n.ResourceKey.String(),
 					Output: &state,
 				},
 				&EvalDiffDestroy{
@@ -266,7 +258,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 					Output: &diff,
 				},
 				&EvalWriteDiff{
-					Name: n.ResourceName,
+					Name: n.ResourceKey.String(),
 					Diff: &diff,
 				},
 			},
@@ -280,7 +272,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 		Node: &EvalSequence{
 			Nodes: []EvalNode{
 				&EvalReadDiff{
-					Name: n.ResourceName,
+					Name: n.ResourceKey.String(),
 					Diff: &diff,
 				},
 				&EvalGetProvider{
@@ -288,7 +280,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 					Output: &provider,
 				},
 				&EvalReadState{
-					Name:   n.ResourceName,
+					Name:   n.ResourceKey.String(),
 					Output: &state,
 				},
 				&EvalApply{
@@ -300,8 +292,8 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 					Error:    &err,
 				},
 				&EvalWriteState{
-					Name:         n.ResourceName,
-					ResourceType: n.ResourceType,
+					Name:         n.ResourceKey.String(),
+					ResourceType: n.ResourceKey.Type,
 					Provider:     n.Provider,
 					Dependencies: n.DependentOn(),
 					State:        &state,
@@ -320,7 +312,7 @@ func (n *graphNodeOrphanResource) EvalTree() EvalNode {
 }
 
 func (n *graphNodeOrphanResource) dependableName() string {
-	return n.ResourceName
+	return n.ResourceKey.String()
 }
 
 // GraphNodeDestroyable impl.

terraform/transform_orphan_test.go

@@ -333,17 +333,18 @@ func TestGraphNodeOrphanResource_impl(t *testing.T) {
 	var _ dag.Vertex = new(graphNodeOrphanResource)
 	var _ dag.NamedVertex = new(graphNodeOrphanResource)
 	var _ GraphNodeProviderConsumer = new(graphNodeOrphanResource)
+	var _ GraphNodeAddressable = new(graphNodeOrphanResource)
 }
 
 func TestGraphNodeOrphanResource_ProvidedBy(t *testing.T) {
-	n := &graphNodeOrphanResource{ResourceName: "aws_instance.foo"}
+	n := &graphNodeOrphanResource{ResourceKey: &ResourceStateKey{Type: "aws_instance"}}
 	if v := n.ProvidedBy(); v[0] != "aws" {
 		t.Fatalf("bad: %#v", v)
 	}
 }
 
 func TestGraphNodeOrphanResource_ProvidedBy_alias(t *testing.T) {
-	n := &graphNodeOrphanResource{ResourceName: "aws_instance.foo", Provider: "aws.bar"}
+	n := &graphNodeOrphanResource{ResourceKey: &ResourceStateKey{Type: "aws_instance"}, Provider: "aws.bar"}
 	if v := n.ProvidedBy(); v[0] != "aws.bar" {
 		t.Fatalf("bad: %#v", v)
 	}

terraform/transform_targets.go

@@ -13,20 +13,25 @@ type TargetsTransformer struct {
 	// List of targeted resource names specified by the user
 	Targets []string
 
+	// List of parsed targets, provided by callers like ResourceCountTransform
+	// that already have the targets parsed
+	ParsedTargets []ResourceAddress
+
 	// Set to true when we're in a `terraform destroy` or a
 	// `terraform plan -destroy`
 	Destroy bool
 }
 
 func (t *TargetsTransformer) Transform(g *Graph) error {
-	if len(t.Targets) > 0 {
-		// TODO: duplicated in OrphanTransformer; pull up parsing earlier
+	if len(t.Targets) > 0 && len(t.ParsedTargets) == 0 {
 		addrs, err := t.parseTargetAddresses()
 		if err != nil {
 			return err
 		}
-
-		targetedNodes, err := t.selectTargetedNodes(g, addrs)
+		t.ParsedTargets = addrs
+	}
+	if len(t.ParsedTargets) > 0 {
+		targetedNodes, err := t.selectTargetedNodes(g, t.ParsedTargets)
 		if err != nil {
 			return err
 		}