From 944ffdb95b343c12a6ad076e8526c25cd6a612ae Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 11:07:01 -0700 Subject: [PATCH] terraform: multi-var ordering is by count Fixes #9444 This appears to be a regression from 0.7.0, but there were no tests covering it so we missed it and changed the behavior at some point! Oh no. This PR make the ordering of multi-var access: `resource.name.*.attr` consistent: it is the ordering of the count, not the lexical ordering of the value. This allows behavior where two lists are indexed by count index and can be assumed to be related (for example user data for an aws instance, as shown in the above referenced issue). Two new context tests added to cover this case. --- terraform/context_apply_test.go | 68 +++++++++++++++++++ terraform/interpolate.go | 68 +++++++++++++------ .../apply-multi-var-order-interp/main.tf | 15 ++++ .../apply-multi-var-order/main.tf | 10 +++ 4 files changed, 142 insertions(+), 19 deletions(-) create mode 100644 terraform/test-fixtures/apply-multi-var-order-interp/main.tf create mode 100644 terraform/test-fixtures/apply-multi-var-order/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 337261616..6daee8f5a 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2039,6 +2039,74 @@ func TestContext2Apply_multiVar(t *testing.T) { } } +// Test that multi-var (splat) access is ordered by count, not by +// value. +func TestContext2Apply_multiVarOrder(t *testing.T) { + m := testModule(t, "apply-multi-var-order") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + // First, apply with a count of 3 + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + t.Logf("State: %s", state.String()) + + actual := state.RootModule().Outputs["should-be-11"] + expected := "index-11" + if actual == nil || actual.Value != expected { + t.Fatalf("bad: \n%s", actual) + } +} + +// Test that multi-var (splat) access is ordered by count, not by +// value, through interpolations. +func TestContext2Apply_multiVarOrderInterp(t *testing.T) { + m := testModule(t, "apply-multi-var-order-interp") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + // First, apply with a count of 3 + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + t.Logf("State: %s", state.String()) + + actual := state.RootModule().Outputs["should-be-11"] + expected := "baz-index-11" + if actual == nil || actual.Value != expected { + t.Fatalf("bad: \n%s", actual) + } +} + func TestContext2Apply_nilDiff(t *testing.T) { m := testModule(t, "apply-good") p := testProvider("aws") diff --git a/terraform/interpolate.go b/terraform/interpolate.go index 7038c1bc8..57aa61962 100644 --- a/terraform/interpolate.go +++ b/terraform/interpolate.go @@ -6,6 +6,7 @@ import ( "os" "regexp" "sort" + "strconv" "strings" "sync" @@ -491,13 +492,13 @@ func (i *Interpolater) computeResourceMultiVariable( } // Get the keys for all the resources that are created for this resource - resourceKeys, err := i.resourceCountKeys(module, cr, v) + countMax, err := i.resourceCountMax(module, cr, v) if err != nil { return nil, err } // If count is zero, we return an empty list - if len(resourceKeys) == 0 { + if countMax == 0 { return &ast.Variable{Type: ast.TypeList, Value: []ast.Variable{}}, nil } @@ -507,7 +508,9 @@ func (i *Interpolater) computeResourceMultiVariable( } var values []interface{} - for _, id := range resourceKeys { + for idx := 0; idx < countMax; idx++ { + id := fmt.Sprintf("%s.%d", v.ResourceId(), idx) + // ID doesn't have a trailing index. We try both here, but if a value // without a trailing index is found we prefer that. This choice // is for legacy reasons: older versions of TF preferred it. @@ -678,10 +681,10 @@ func (i *Interpolater) resourceVariableInfo( return module, cr, nil } -func (i *Interpolater) resourceCountKeys( +func (i *Interpolater) resourceCountMax( ms *ModuleState, cr *config.Resource, - v *config.ResourceVariable) ([]string, error) { + v *config.ResourceVariable) (int, error) { id := v.ResourceId() // If we're NOT applying, then we assume we can read the count @@ -690,31 +693,58 @@ func (i *Interpolater) resourceCountKeys( if i.Operation != walkApply { count, err := cr.Count() if err != nil { - return nil, err + return 0, err } - result := make([]string, count) - for i := 0; i < count; i++ { - result[i] = fmt.Sprintf("%s.%d", id, i) - } - - return result, nil + return count, nil } // We need to determine the list of resource keys to get values from. // This needs to be sorted so the order is deterministic. We used to // use "cr.Count()" but that doesn't work if the count is interpolated // and we can't guarantee that so we instead depend on the state. - var resourceKeys []string + max := -1 for k, _ := range ms.Resources { - // If we don't have the right prefix then ignore it - if k != id && !strings.HasPrefix(k, id+".") { + // Get the index number for this resource + index := "" + if k == id { + // If the key is the id, then its just 0 (no explicit index) + index = "0" + } else if strings.HasPrefix(k, id+".") { + // Grab the index number out of the state + index = k[len(id+"."):] + if idx := strings.IndexRune(index, '.'); idx >= 0 { + index = index[:idx] + } + } + + // If there was no index then this resource didn't match + // the one we're looking for, exit. + if index == "" { continue } - // Add it to the list - resourceKeys = append(resourceKeys, k) + // Turn the index into an int + raw, err := strconv.ParseInt(index, 0, 0) + if err != nil { + return 0, fmt.Errorf( + "%s: error parsing index %q as int: %s", + id, index, err) + } + + // Keep track of this index if its the max + if new := int(raw); new > max { + max = new + } } - sort.Strings(resourceKeys) - return resourceKeys, nil + + // If we never found any matching resources in the state, we + // have zero. + if max == -1 { + return 0, nil + } + + // The result value is "max+1" because we're returning the + // max COUNT, not the max INDEX, and we zero-index. + return max + 1, nil } diff --git a/terraform/test-fixtures/apply-multi-var-order-interp/main.tf b/terraform/test-fixtures/apply-multi-var-order-interp/main.tf new file mode 100644 index 000000000..d6e6ae236 --- /dev/null +++ b/terraform/test-fixtures/apply-multi-var-order-interp/main.tf @@ -0,0 +1,15 @@ +variable "count" { default = 15 } + +resource "aws_instance" "bar" { + count = "${var.count}" + foo = "index-${count.index}" +} + +resource "aws_instance" "baz" { + count = "${var.count}" + foo = "baz-${element(aws_instance.bar.*.foo, count.index)}" +} + +output "should-be-11" { + value = "${element(aws_instance.baz.*.foo, 11)}" +} diff --git a/terraform/test-fixtures/apply-multi-var-order/main.tf b/terraform/test-fixtures/apply-multi-var-order/main.tf new file mode 100644 index 000000000..53e6cf01d --- /dev/null +++ b/terraform/test-fixtures/apply-multi-var-order/main.tf @@ -0,0 +1,10 @@ +variable "count" { default = 15 } + +resource "aws_instance" "bar" { + count = "${var.count}" + foo = "index-${count.index}" +} + +output "should-be-11" { + value = "${element(aws_instance.bar.*.foo, 11)}" +}