From a0d3245ee309ead42969d6d366d27d8fe091c292 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 7 Jan 2016 14:43:43 -0600 Subject: [PATCH] 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 | 54 +++++++ terraform/context_plan_test.go | 150 +++++++++++++++++- terraform/graph_config_node_resource.go | 11 +- terraform/resource_address_test.go | 24 +++ terraform/state.go | 60 +++++++ terraform/state_test.go | 54 +++++++ .../plan-targeted-module-orphan/main.tf | 6 + .../plan-targeted-over-ten/main.tf | 3 + terraform/transform_orphan.go | 78 ++++----- terraform/transform_orphan_test.go | 5 +- terraform/transform_targets.go | 13 +- 11 files changed, 405 insertions(+), 53 deletions(-) create mode 100644 terraform/test-fixtures/plan-targeted-module-orphan/main.tf create mode 100644 terraform/test-fixtures/plan-targeted-over-ten/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index b6fea26fc..d88be4c7d 100644 --- a/terraform/context_apply_test.go +++ b/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") diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index e91fc7747..0008f913c 100644 --- a/terraform/context_plan_test.go +++ b/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) } diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 9fc696c2a..1d3627428 100644 --- a/terraform/graph_config_node_resource.go +++ b/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{}) diff --git a/terraform/resource_address_test.go b/terraform/resource_address_test.go index 0b10a24fd..03e762a74 100644 --- a/terraform/resource_address_test.go +++ b/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", diff --git a/terraform/state.go b/terraform/state.go index 8734cfc17..a20fd7ba4 100644 --- a/terraform/state.go +++ b/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 diff --git a/terraform/state_test.go b/terraform/state_test.go index 8d24a8e75..c3bfb18df 100644 --- a/terraform/state_test.go +++ b/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) + } + } +} diff --git a/terraform/test-fixtures/plan-targeted-module-orphan/main.tf b/terraform/test-fixtures/plan-targeted-module-orphan/main.tf new file mode 100644 index 000000000..2b33fedae --- /dev/null +++ b/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" +} +*/ diff --git a/terraform/test-fixtures/plan-targeted-over-ten/main.tf b/terraform/test-fixtures/plan-targeted-over-ten/main.tf new file mode 100644 index 000000000..1c7bc8769 --- /dev/null +++ b/terraform/test-fixtures/plan-targeted-over-ten/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "foo" { + count = 13 +} diff --git a/terraform/transform_orphan.go b/terraform/transform_orphan.go index 13e8fbf94..d221e57db 100644 --- a/terraform/transform_orphan.go +++ b/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. diff --git a/terraform/transform_orphan_test.go b/terraform/transform_orphan_test.go index 85bb4637b..76fbaa6a1 100644 --- a/terraform/transform_orphan_test.go +++ b/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) } diff --git a/terraform/transform_targets.go b/terraform/transform_targets.go index cab8c8b1e..db577b361 100644 --- a/terraform/transform_targets.go +++ b/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 }