diff --git a/internal/states/sync.go b/internal/states/sync.go index c70714f9df49..f21984279509 100644 --- a/internal/states/sync.go +++ b/internal/states/sync.go @@ -248,60 +248,6 @@ func (s *SyncState) RemoveResourceIfEmpty(addr addrs.AbsResource) bool { 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. -// -// Set countEnabled to true if the resource has count set in its new -// configuration, or false if it does not. -// -// The state is modified in-place if necessary, moving a resource instance -// between the two addresses. The return value is true if a change was made, -// and false otherwise. -func (s *SyncState) MaybeFixUpResourceInstanceAddressForCount(addr addrs.ConfigResource, countEnabled bool) bool { - s.lock.Lock() - defer s.lock.Unlock() - - // get all modules instances that may match this state - modules := s.state.ModuleInstances(addr.Module) - if len(modules) == 0 { - return false - } - - changed := false - - for _, ms := range modules { - relAddr := addr.Resource - rs := ms.Resource(relAddr) - if rs == nil { - continue - } - - huntKey := addrs.NoKey - replaceKey := addrs.InstanceKey(addrs.IntKey(0)) - if !countEnabled { - huntKey, replaceKey = replaceKey, huntKey - } - - is, exists := rs.Instances[huntKey] - if !exists { - continue - } - - if _, exists := rs.Instances[replaceKey]; exists { - // If the replacement key also exists then we'll do nothing and keep both. - continue - } - - // If we get here then we need to "rename" from hunt to replace - rs.Instances[replaceKey] = is - delete(rs.Instances, huntKey) - changed = true - } - - return changed -} - // SetResourceInstanceCurrent saves the given instance object as the current // generation of the resource instance with the given address, simultaneously // updating the recorded provider configuration address, dependencies, and diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 9d155e73e865..babf067a5e69 100644 --- a/internal/terraform/context_apply_test.go +++ b/internal/terraform/context_apply_test.go @@ -2073,9 +2073,22 @@ func TestContext2Apply_countDecreaseToOneX(t *testing.T) { // https://github.com/PeoplePerHour/terraform/pull/11 // -// This tests a case where both a "resource" and "resource.0" are in -// the state file, which apparently is a reasonable backwards compatibility -// concern found in the above 3rd party repo. +// This tests a rare but possible situation where we have both a no-key and +// a zero-key instance of the same resource in the configuration when we +// disable count. +// +// The main way to get here is for a provider to fail to destroy the zero-key +// instance but succeed in creating the no-key instance, since those two +// can typically happen concurrently. There are various other ways to get here +// that might be considered user error, such as using "terraform state mv" +// to create a strange combination of different key types on the same resource. +// +// This test indirectly exercises an intentional interaction between +// refactoring.ImpliedMoveStatements and refactoring.ApplyMoves: we'll first +// generate an implied move statement from aws_instance.foo[0] to +// aws_instance.foo, but then refactoring.ApplyMoves should notice that and +// ignore the statement, in the same way as it would if an explicit move +// statement specified the same situation. func TestContext2Apply_countDecreaseToOneCorrupted(t *testing.T) { m := testModule(t, "apply-count-dec-one") p := testProvider("aws") diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 7d1353c6a5df..e07aab895444 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -297,7 +297,14 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State } func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, map[addrs.UniqueKey]refactoring.MoveResult) { - moveStmts := refactoring.FindMoveStatements(config) + explicitMoveStmts := refactoring.FindMoveStatements(config) + implicitMoveStmts := refactoring.ImpliedMoveStatements(config, prevRunState, explicitMoveStmts) + var moveStmts []refactoring.MoveStatement + if stmtsLen := len(explicitMoveStmts) + len(implicitMoveStmts); stmtsLen > 0 { + moveStmts = make([]refactoring.MoveStatement, 0, stmtsLen) + moveStmts = append(moveStmts, explicitMoveStmts...) + moveStmts = append(moveStmts, implicitMoveStmts...) + } moveResults := refactoring.ApplyMoves(moveStmts, prevRunState) return moveStmts, moveResults } diff --git a/internal/terraform/eval_count.go b/internal/terraform/eval_count.go index a7f3c25ab35d..c74a051b301f 100644 --- a/internal/terraform/eval_count.go +++ b/internal/terraform/eval_count.go @@ -2,10 +2,8 @@ package terraform import ( "fmt" - "log" "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/gocty" @@ -101,32 +99,3 @@ func evaluateCountExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Val return countVal, diags } - -// fixResourceCountSetTransition is a helper function to fix up the state when a -// resource transitions its "count" from being set to unset or vice-versa, -// treating a 0-key and a no-key instance as aliases for one another across -// the transition. -// -// The correct time to call this function is in the DynamicExpand method for -// a node representing a resource, just after evaluating the count with -// evaluateCountExpression, and before any other analysis of the -// state such as orphan detection. -// -// This function calls methods on the given EvalContext to update the current -// state in-place, if necessary. It is a no-op if there is no count transition -// taking place. -// -// Since the state is modified in-place, this function must take a writer lock -// on the state. The caller must therefore not also be holding a state lock, -// or this function will block forever awaiting the lock. -func fixResourceCountSetTransition(ctx EvalContext, addr addrs.ConfigResource, countEnabled bool) { - state := ctx.State() - if state.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) { - log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr) - } - - refreshState := ctx.RefreshState() - if refreshState != nil && refreshState.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) { - log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr) - } -} diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 94f1e7699fb5..75f9d3d4ad25 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -152,11 +152,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Target &TargetsTransformer{Targets: b.Targets}, - // Add the node to fix the state count boundaries - &CountBoundaryTransformer{ - Config: b.Config, - }, - // Close opened plugin connections &CloseProviderTransformer{}, diff --git a/internal/terraform/graph_builder_apply_test.go b/internal/terraform/graph_builder_apply_test.go index e1aba8d2c136..88ebcfefd1f3 100644 --- a/internal/terraform/graph_builder_apply_test.go +++ b/internal/terraform/graph_builder_apply_test.go @@ -5,10 +5,12 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" - "github.com/zclconf/go-cty/cty" ) func TestApplyGraphBuilder_impl(t *testing.T) { @@ -60,11 +62,10 @@ func TestApplyGraphBuilder(t *testing.T) { t.Fatalf("wrong path %q", g.Path.String()) } - actual := strings.TrimSpace(g.String()) - - expected := strings.TrimSpace(testApplyGraphBuilderStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(testApplyGraphBuilderStr) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -352,10 +353,10 @@ func TestApplyGraphBuilder_destroyCount(t *testing.T) { t.Fatalf("wrong module path %q", g.Path) } - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -699,9 +700,6 @@ func TestApplyGraphBuilder_orphanedWithProvider(t *testing.T) { } const testApplyGraphBuilderStr = ` -meta.count-boundary (EachMode fixup) - module.child (close) - test_object.other module.child (close) module.child.test_object.other module.child (expand) @@ -721,7 +719,7 @@ provider["registry.terraform.io/hashicorp/test"] (close) module.child.test_object.other test_object.other root - meta.count-boundary (EachMode fixup) + module.child (close) provider["registry.terraform.io/hashicorp/test"] (close) test_object.create test_object.create (expand) @@ -735,13 +733,10 @@ test_object.other (expand) ` const testApplyGraphBuilderDestroyCountStr = ` -meta.count-boundary (EachMode fixup) - test_object.B provider["registry.terraform.io/hashicorp/test"] provider["registry.terraform.io/hashicorp/test"] (close) test_object.B root - meta.count-boundary (EachMode fixup) provider["registry.terraform.io/hashicorp/test"] (close) test_object.A (expand) provider["registry.terraform.io/hashicorp/test"] diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index b267f9c428c7..709b917b6733 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -156,11 +156,6 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // node due to dependency edges, to avoid graph cycles during apply. &ForcedCBDTransformer{}, - // Add the node to fix the state count boundaries - &CountBoundaryTransformer{ - Config: b.Config, - }, - // Close opened plugin connections &CloseProviderTransformer{}, diff --git a/internal/terraform/graph_builder_plan_test.go b/internal/terraform/graph_builder_plan_test.go index 689f9faff3db..9ec16c6ed79f 100644 --- a/internal/terraform/graph_builder_plan_test.go +++ b/internal/terraform/graph_builder_plan_test.go @@ -4,10 +4,12 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/providers" - "github.com/zclconf/go-cty/cty" ) func TestPlanGraphBuilder_impl(t *testing.T) { @@ -45,10 +47,10 @@ func TestPlanGraphBuilder(t *testing.T) { t.Fatalf("wrong module path %q", g.Path) } - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testPlanGraphBuilderStr) - if actual != expected { - t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(testPlanGraphBuilderStr) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -92,15 +94,12 @@ func TestPlanGraphBuilder_dynamicBlock(t *testing.T) { // is that at the end test_thing.c depends on both test_thing.a and // test_thing.b. Other details might shift over time as other logic in // the graph builders changes. - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -meta.count-boundary (EachMode fixup) - test_thing.c (expand) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(` provider["registry.terraform.io/hashicorp/test"] provider["registry.terraform.io/hashicorp/test"] (close) test_thing.c (expand) root - meta.count-boundary (EachMode fixup) provider["registry.terraform.io/hashicorp/test"] (close) test_thing.a (expand) provider["registry.terraform.io/hashicorp/test"] @@ -110,8 +109,8 @@ test_thing.c (expand) test_thing.a (expand) test_thing.b (expand) `) - if actual != expected { - t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -150,23 +149,20 @@ func TestPlanGraphBuilder_attrAsBlocks(t *testing.T) { // list-of-objects attribute. This requires some special effort // inside lang.ReferencesInBlock to make sure it searches blocks of // type "nested" along with an attribute named "nested". - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -meta.count-boundary (EachMode fixup) - test_thing.b (expand) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(` provider["registry.terraform.io/hashicorp/test"] provider["registry.terraform.io/hashicorp/test"] (close) test_thing.b (expand) root - meta.count-boundary (EachMode fixup) provider["registry.terraform.io/hashicorp/test"] (close) test_thing.a (expand) provider["registry.terraform.io/hashicorp/test"] test_thing.b (expand) test_thing.a (expand) `) - if actual != expected { - t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -211,12 +207,12 @@ func TestPlanGraphBuilder_forEach(t *testing.T) { t.Fatalf("wrong module path %q", g.Path) } - actual := strings.TrimSpace(g.String()) + got := strings.TrimSpace(g.String()) // We're especially looking for the edge here, where aws_instance.bat // has a dependency on aws_instance.boo - expected := strings.TrimSpace(testPlanGraphBuilderForEachStr) - if actual != expected { - t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual) + want := strings.TrimSpace(testPlanGraphBuilderForEachStr) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("wrong result\n%s", diff) } } @@ -230,9 +226,6 @@ aws_security_group.firewall (expand) provider["registry.terraform.io/hashicorp/aws"] local.instance_id (expand) aws_instance.web (expand) -meta.count-boundary (EachMode fixup) - aws_load_balancer.weblb (expand) - output.instance_id openstack_floating_ip.random (expand) provider["registry.terraform.io/hashicorp/openstack"] output.instance_id @@ -245,7 +238,7 @@ provider["registry.terraform.io/hashicorp/openstack"] provider["registry.terraform.io/hashicorp/openstack"] (close) openstack_floating_ip.random (expand) root - meta.count-boundary (EachMode fixup) + output.instance_id provider["registry.terraform.io/hashicorp/aws"] (close) provider["registry.terraform.io/hashicorp/openstack"] (close) var.foo @@ -263,12 +256,6 @@ aws_instance.boo (expand) provider["registry.terraform.io/hashicorp/aws"] aws_instance.foo (expand) provider["registry.terraform.io/hashicorp/aws"] -meta.count-boundary (EachMode fixup) - aws_instance.bar (expand) - aws_instance.bar2 (expand) - aws_instance.bat (expand) - aws_instance.baz (expand) - aws_instance.foo (expand) provider["registry.terraform.io/hashicorp/aws"] provider["registry.terraform.io/hashicorp/aws"] (close) aws_instance.bar (expand) @@ -277,6 +264,5 @@ provider["registry.terraform.io/hashicorp/aws"] (close) aws_instance.baz (expand) aws_instance.foo (expand) root - meta.count-boundary (EachMode fixup) provider["registry.terraform.io/hashicorp/aws"] (close) ` diff --git a/internal/terraform/node_count_boundary.go b/internal/terraform/node_count_boundary.go deleted file mode 100644 index 26968b11937d..000000000000 --- a/internal/terraform/node_count_boundary.go +++ /dev/null @@ -1,80 +0,0 @@ -package terraform - -import ( - "fmt" - "log" - - "github.com/hashicorp/terraform/internal/addrs" - "github.com/hashicorp/terraform/internal/configs" - "github.com/hashicorp/terraform/internal/tfdiags" -) - -// NodeCountBoundary fixes up any transitions between "each modes" in objects -// saved in state, such as switching from NoEach to EachInt. -type NodeCountBoundary struct { - Config *configs.Config -} - -var _ GraphNodeExecutable = (*NodeCountBoundary)(nil) - -func (n *NodeCountBoundary) Name() string { - return "meta.count-boundary (EachMode fixup)" -} - -// GraphNodeExecutable -func (n *NodeCountBoundary) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - // We'll temporarily lock the state to grab the modules, then work on each - // one separately while taking a lock again for each separate resource. - // This means that if another caller concurrently adds a module here while - // we're working then we won't update it, but that's no worse than the - // concurrent writer blocking for our entire fixup process and _then_ - // adding a new module, and in practice the graph node associated with - // this eval depends on everything else in the graph anyway, so there - // should not be concurrent writers. - state := ctx.State().Lock() - moduleAddrs := make([]addrs.ModuleInstance, 0, len(state.Modules)) - for _, m := range state.Modules { - moduleAddrs = append(moduleAddrs, m.Addr) - } - ctx.State().Unlock() - - for _, addr := range moduleAddrs { - cfg := n.Config.DescendentForInstance(addr) - if cfg == nil { - log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", addr) - continue - } - if err := n.fixModule(ctx, addr); err != nil { - diags = diags.Append(err) - return diags - } - } - return diags -} - -func (n *NodeCountBoundary) fixModule(ctx EvalContext, moduleAddr addrs.ModuleInstance) error { - ms := ctx.State().Module(moduleAddr) - cfg := n.Config.DescendentForInstance(moduleAddr) - if ms == nil { - // Theoretically possible for a concurrent writer to delete a module - // while we're running, but in practice the graph node that called us - // depends on everything else in the graph and so there can never - // be a concurrent writer. - return fmt.Errorf("[WARN] no state found for %s while trying to fix up EachModes", moduleAddr) - } - if cfg == nil { - return fmt.Errorf("[WARN] no config found for %s while trying to fix up EachModes", moduleAddr) - } - - for _, r := range ms.Resources { - rCfg := cfg.Module.ResourceByAddr(r.Addr.Resource) - if rCfg == nil { - log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", r.Addr) - continue - } - hasCount := rCfg.Count != nil - fixResourceCountSetTransition(ctx, r.Addr.Config(), hasCount) - } - - return nil -} diff --git a/internal/terraform/node_count_boundary_test.go b/internal/terraform/node_count_boundary_test.go deleted file mode 100644 index 096a980ad773..000000000000 --- a/internal/terraform/node_count_boundary_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package terraform - -import ( - "testing" - - "github.com/hashicorp/hcl/v2/hcltest" - "github.com/hashicorp/terraform/internal/addrs" - "github.com/hashicorp/terraform/internal/configs" - "github.com/hashicorp/terraform/internal/states" - "github.com/zclconf/go-cty/cty" -) - -func TestNodeCountBoundaryExecute(t *testing.T) { - - // Create a state with a single instance (addrs.NoKey) of test_instance.foo - state := states.NewState() - state.Module(addrs.RootModuleInstance).SetResourceInstanceCurrent( - addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "test_instance", - Name: "foo", - }.Instance(addrs.NoKey), - &states.ResourceInstanceObjectSrc{ - Status: states.ObjectReady, - AttrsJSON: []byte(`{"type":"string","value":"hello"}`), - }, - addrs.AbsProviderConfig{ - Provider: addrs.NewDefaultProvider("test"), - Module: addrs.RootModule, - }, - ) - - // Create a config that uses count to create 2 instances of test_instance.foo - rc := &configs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "test_instance", - Name: "foo", - Count: hcltest.MockExprLiteral(cty.NumberIntVal(2)), - Config: configs.SynthBody("", map[string]cty.Value{ - "test_string": cty.StringVal("hello"), - }), - } - config := &configs.Config{ - Module: &configs.Module{ - ManagedResources: map[string]*configs.Resource{ - "test_instance.foo": rc, - }, - }, - } - - ctx := &MockEvalContext{ - StateState: state.SyncWrapper(), - } - node := NodeCountBoundary{Config: config} - - diags := node.Execute(ctx, walkApply) - if diags.HasErrors() { - t.Fatalf("unexpected error: %s", diags.Err()) - } - if !state.HasResources() { - t.Fatal("resources missing from state") - } - - // verify that the resource changed from test_instance.foo to - // test_instance.foo.0 in the state - actual := state.String() - expected := "test_instance.foo.0:\n ID = \n provider = provider[\"registry.terraform.io/hashicorp/test\"]\n type = string\n value = hello" - - if actual != expected { - t.Fatalf("wrong result: %s", actual) - } -} diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 64850c3b7a0f..c732575e5650 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -304,10 +304,6 @@ func (n *NodePlannableResource) ModifyCreateBeforeDestroy(v bool) error { func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) { var diags tfdiags.Diagnostics - // We need to potentially rename an instance address in the state - // if we're transitioning whether "count" is set at all. - fixResourceCountSetTransition(ctx, n.Addr.Config(), n.Config.Count != nil) - // Our instance expander should already have been informed about the // expansion of this resource and of all of its containing modules, so // it can tell us which instance addresses we need to process. diff --git a/internal/terraform/transform_count_boundary.go b/internal/terraform/transform_count_boundary.go deleted file mode 100644 index 9f944240edb9..000000000000 --- a/internal/terraform/transform_count_boundary.go +++ /dev/null @@ -1,33 +0,0 @@ -package terraform - -import ( - "github.com/hashicorp/terraform/internal/configs" - "github.com/hashicorp/terraform/internal/dag" -) - -// CountBoundaryTransformer adds a node that depends on everything else -// so that it runs last in order to clean up the state for nodes that -// are on the "count boundary": "foo.0" when only one exists becomes "foo" -type CountBoundaryTransformer struct { - Config *configs.Config -} - -func (t *CountBoundaryTransformer) Transform(g *Graph) error { - node := &NodeCountBoundary{ - Config: t.Config, - } - g.Add(node) - - // Depends on everything - for _, v := range g.Vertices() { - // Don't connect to ourselves - if v == node { - continue - } - - // Connect! - g.Connect(dag.BasicEdge(node, v)) - } - - return nil -}