From 3662dbc03d53747cb8a94b9bd428c7afba60b44f Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 13 Aug 2019 10:10:36 -0400 Subject: [PATCH 1/5] Ensure no key added to graph first --- terraform/transform_orphan_count.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/terraform/transform_orphan_count.go b/terraform/transform_orphan_count.go index 4f323a7a0ed2..ab268ff38756 100644 --- a/terraform/transform_orphan_count.go +++ b/terraform/transform_orphan_count.go @@ -50,6 +50,16 @@ func (t *OrphanResourceCountTransformer) Transform(g *Graph) error { } func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.InstanceKey]struct{}, g *Graph) error { + // If there is a no-key node, add this to the graph first, + // because the last item determines the resource mode for the whole resource, + // so if this (non-deterministically) happens to end up as the last one, + // that will change the resource's EachMode and our addressing for our instances + // will not work as expected + noKeyNode, hasNoKeyNode := haveKeys[addrs.NoKey] + if hasNoKeyNode { + g.Add(noKeyNode) + } + for key := range haveKeys { s, _ := key.(addrs.StringKey) // If the key is present in our current for_each, carry on @@ -57,6 +67,11 @@ func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.Ins continue } + // If the key is no-key, we have already added it, so skip + if key == addrs.NoKey { + continue + } + abstract := NewNodeAbstractResourceInstance(t.Addr.Instance(key)) var node dag.Vertex = abstract if f := t.Concrete; f != nil { From b9fa724273d4627c29d0c41850c6c1b558b30864 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 13 Aug 2019 10:24:26 -0400 Subject: [PATCH 2/5] Add edge --- terraform/transform_orphan_count.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/terraform/transform_orphan_count.go b/terraform/transform_orphan_count.go index ab268ff38756..029fa9a927b3 100644 --- a/terraform/transform_orphan_count.go +++ b/terraform/transform_orphan_count.go @@ -51,7 +51,8 @@ func (t *OrphanResourceCountTransformer) Transform(g *Graph) error { func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.InstanceKey]struct{}, g *Graph) error { // If there is a no-key node, add this to the graph first, - // because the last item determines the resource mode for the whole resource, + // so that we can create edges to it in subsequent (StringKey) nodes. + // This is because the last item determines the resource mode for the whole resource, // so if this (non-deterministically) happens to end up as the last one, // that will change the resource's EachMode and our addressing for our instances // will not work as expected @@ -79,6 +80,11 @@ func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.Ins } log.Printf("[TRACE] OrphanResourceCount(non-zero): adding %s as %T", t.Addr, node) g.Add(node) + + // Add edge to noKeyNode if it exists + if hasNoKeyNode { + g.Connect(dag.BasicEdge(node, noKeyNode)) + } } return nil } From 6fae69f07bc1a3a7e3fec70436c9b227be072d4a Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 13 Aug 2019 10:38:52 -0400 Subject: [PATCH 3/5] Creating the node would be nice --- terraform/transform_orphan_count.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/terraform/transform_orphan_count.go b/terraform/transform_orphan_count.go index 029fa9a927b3..6ad1d02ac95d 100644 --- a/terraform/transform_orphan_count.go +++ b/terraform/transform_orphan_count.go @@ -56,8 +56,14 @@ func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.Ins // so if this (non-deterministically) happens to end up as the last one, // that will change the resource's EachMode and our addressing for our instances // will not work as expected - noKeyNode, hasNoKeyNode := haveKeys[addrs.NoKey] + _, hasNoKeyNode := haveKeys[addrs.NoKey] + var noKeyNode dag.Vertex if hasNoKeyNode { + abstract := NewNodeAbstractResourceInstance(t.Addr.Instance(addrs.NoKey)) + noKeyNode = abstract + if f := t.Concrete; f != nil { + noKeyNode = f(abstract) + } g.Add(noKeyNode) } From 06e72c693fa0d5ff164ea03f0920e733c7678137 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 13 Aug 2019 16:31:07 -0400 Subject: [PATCH 4/5] Add test --- terraform/transform_orphan_count_test.go | 76 ++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/terraform/transform_orphan_count_test.go b/terraform/transform_orphan_count_test.go index 5cf2b895cc37..4853ce831b99 100644 --- a/terraform/transform_orphan_count_test.go +++ b/terraform/transform_orphan_count_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/states" + "github.com/zclconf/go-cty/cty" ) func TestOrphanResourceCountTransformer(t *testing.T) { @@ -331,6 +333,74 @@ func TestOrphanResourceCountTransformer_zeroAndNoneCount(t *testing.T) { } } +// When converting from a NoEach mode to an EachMap via a switch to for_each, +// an edge is necessary to ensure that the map-key'd instances +// are evaluated after the NoKey resource, because the final instance evaluated +// sets the whole resource's EachMode. +func TestOrphanResourceCountTransformer_ForEachEdgesAdded(t *testing.T) { + state := states.BuildState(func(s *states.SyncState) { + // "bar" key'd resource + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.StringKey("bar")).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsFlat: map[string]string{ + "id": "foo", + }, + Status: states.ObjectReady, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + // NoKey'd resource + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsFlat: map[string]string{ + "id": "foo", + }, + Status: states.ObjectReady, + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + }) + + g := Graph{Path: addrs.RootModuleInstance} + + { + tf := &OrphanResourceCountTransformer{ + Concrete: testOrphanResourceConcreteFunc, + // No keys in this ForEach ensure both our resources end + // up orphaned in this test + ForEach: map[string]cty.Value{}, + Addr: addrs.RootModuleInstance.Resource( + addrs.ManagedResourceMode, "aws_instance", "foo", + ), + State: state, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformOrphanResourceForEachStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + const testTransformOrphanResourceCountBasicStr = ` aws_instance.foo[2] (orphan) ` @@ -355,3 +425,9 @@ aws_instance.foo[0] (orphan) const testTransformOrphanResourceCountZeroAndNoneCountStr = ` aws_instance.foo (orphan) ` + +const testTransformOrphanResourceForEachStr = ` +aws_instance.foo (orphan) +aws_instance.foo["bar"] (orphan) + aws_instance.foo (orphan) +` From e6817f6319103691624d5de5df89dfa6a97622b7 Mon Sep 17 00:00:00 2001 From: Pam Selle <204372+pselle@users.noreply.github.com> Date: Tue, 13 Aug 2019 17:22:14 -0400 Subject: [PATCH 5/5] Reordering, comment update --- states/sync.go | 2 +- terraform/transform_orphan_count.go | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/states/sync.go b/states/sync.go index a37744673e62..47fb16d6eaca 100644 --- a/states/sync.go +++ b/states/sync.go @@ -283,7 +283,7 @@ func (s *SyncState) MaybeFixUpResourceInstanceAddressForCount(addr addrs.AbsReso } // SetResourceInstanceCurrent saves the given instance object as the current -// generation of the resource instance with the given address, simulataneously +// generation of the resource instance with the given address, simultaneously // updating the recorded provider configuration address, dependencies, and // resource EachMode. // diff --git a/terraform/transform_orphan_count.go b/terraform/transform_orphan_count.go index 6ad1d02ac95d..40163cf913a7 100644 --- a/terraform/transform_orphan_count.go +++ b/terraform/transform_orphan_count.go @@ -50,12 +50,12 @@ func (t *OrphanResourceCountTransformer) Transform(g *Graph) error { } func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.InstanceKey]struct{}, g *Graph) error { - // If there is a no-key node, add this to the graph first, + // If there is a NoKey node, add this to the graph first, // so that we can create edges to it in subsequent (StringKey) nodes. // This is because the last item determines the resource mode for the whole resource, - // so if this (non-deterministically) happens to end up as the last one, - // that will change the resource's EachMode and our addressing for our instances - // will not work as expected + // (see SetResourceInstanceCurrent for more information) and we need to evaluate + // an orphaned (NoKey) resource before the in-memory state is updated + // to deal with a new for_each resource _, hasNoKeyNode := haveKeys[addrs.NoKey] var noKeyNode dag.Vertex if hasNoKeyNode { @@ -68,14 +68,14 @@ func (t *OrphanResourceCountTransformer) transformForEach(haveKeys map[addrs.Ins } for key := range haveKeys { - s, _ := key.(addrs.StringKey) - // If the key is present in our current for_each, carry on - if _, ok := t.ForEach[string(s)]; ok { + // If the key is no-key, we have already added it, so skip + if key == addrs.NoKey { continue } - // If the key is no-key, we have already added it, so skip - if key == addrs.NoKey { + s, _ := key.(addrs.StringKey) + // If the key is present in our current for_each, carry on + if _, ok := t.ForEach[string(s)]; ok { continue }