From c6d0333dc0869060eaf05155394f4491644212c0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 23 Feb 2017 10:03:59 -0800 Subject: [PATCH] flatmap: mark computed list as a computed value in Expand Fixes #12183 The fix is in flatmap for this but the entire issue is a bit more complex. Given a schema with a computed set, if you reference it like this: lookup(attr[0], "field") And "attr" contains a computed set within it, it would panic even though "field" is available. There were a couple avenues I could've taken to fix this: 1.) Any complex value containing any unknown value at any point is entirely unknown. 2.) Only the specific part of the complex value is unknown. I took route 2 so that the above works without any computed (since "name" is not computed but something else is). This may actually have an effect on other parts of Terraform configs, however those similar configs would've simply crashed previously so it shouldn't break any pre-existing configs. --- builtin/providers/test/provider.go | 3 +- builtin/providers/test/resource_gh12183.go | 64 +++++++++++++++++++ .../providers/test/resource_gh12183_test.go | 37 +++++++++++ flatmap/expand.go | 11 +++- flatmap/expand_test.go | 17 +++++ helper/schema/schema_test.go | 59 +++++++++++++++++ terraform/state_test.go | 39 +++++++++++ 7 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 builtin/providers/test/resource_gh12183.go create mode 100644 builtin/providers/test/resource_gh12183_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index fc85ad4a4..a11e3f1cd 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -8,7 +8,8 @@ import ( func Provider() terraform.ResourceProvider { return &schema.Provider{ ResourcesMap: map[string]*schema.Resource{ - "test_resource": testResource(), + "test_resource": testResource(), + "test_resource_gh12183": testResourceGH12183(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_gh12183.go b/builtin/providers/test/resource_gh12183.go new file mode 100644 index 000000000..07f164de1 --- /dev/null +++ b/builtin/providers/test/resource_gh12183.go @@ -0,0 +1,64 @@ +package test + +import ( + "github.com/hashicorp/terraform/helper/schema" +) + +// This is a test resource to help reproduce GH-12183. This issue came up +// as a complex mixing of core + helper/schema and while we added core tests +// to cover some of the cases, this test helps top it off with an end-to-end +// test. +func testResourceGH12183() *schema.Resource { + return &schema.Resource{ + Create: testResourceCreate_gh12183, + Read: testResourceRead_gh12183, + Update: testResourceUpdate_gh12183, + Delete: testResourceDelete_gh12183, + Schema: map[string]*schema.Schema{ + "key": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + }, + + "config": &schema.Schema{ + Type: schema.TypeList, + Optional: true, + ForceNew: true, + MinItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "name": { + Type: schema.TypeString, + Required: true, + }, + + "rules": { + Type: schema.TypeSet, + Computed: true, + Elem: &schema.Schema{Type: schema.TypeString}, + Set: schema.HashString, + }, + }, + }, + }, + }, + } +} + +func testResourceCreate_gh12183(d *schema.ResourceData, meta interface{}) error { + d.SetId("testId") + return testResourceRead(d, meta) +} + +func testResourceRead_gh12183(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceUpdate_gh12183(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceDelete_gh12183(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_gh12183_test.go b/builtin/providers/test/resource_gh12183_test.go new file mode 100644 index 000000000..6b03b8027 --- /dev/null +++ b/builtin/providers/test/resource_gh12183_test.go @@ -0,0 +1,37 @@ +package test + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" +) + +// Tests GH-12183. This would previously cause a crash. More granular +// unit tests are scattered through helper/schema and terraform core for +// this. +func TestResourceGH12183_basic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_gh12183" "a" { + config { + name = "hello" + } +} + +resource "test_resource_gh12183" "b" { + key = "${lookup(test_resource_gh12183.a.config[0], "name")}" +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} diff --git a/flatmap/expand.go b/flatmap/expand.go index 331357ff3..422b1ffcc 100644 --- a/flatmap/expand.go +++ b/flatmap/expand.go @@ -5,6 +5,8 @@ import ( "sort" "strconv" "strings" + + "github.com/hashicorp/hil" ) // Expand takes a map and a key (prefix) and expands that value into @@ -22,7 +24,14 @@ func Expand(m map[string]string, key string) interface{} { } // Check if the key is an array, and if so, expand the array - if _, ok := m[key+".#"]; ok { + if v, ok := m[key+".#"]; ok { + // If the count of the key is unknown, then just put the unknown + // value in the value itself. This will be detected by Terraform + // core later. + if v == hil.UnknownValue { + return v + } + return expandArray(m, key) } diff --git a/flatmap/expand_test.go b/flatmap/expand_test.go index b5da3197b..53963a1ba 100644 --- a/flatmap/expand_test.go +++ b/flatmap/expand_test.go @@ -3,6 +3,8 @@ package flatmap import ( "reflect" "testing" + + "github.com/hashicorp/hil" ) func TestExpand(t *testing.T) { @@ -117,6 +119,21 @@ func TestExpand(t *testing.T) { Key: "set", Output: []interface{}{"a", "b", "c"}, }, + + { + Map: map[string]string{ + "struct.#": "1", + "struct.0.name": "hello", + "struct.0.rules.#": hil.UnknownValue, + }, + Key: "struct", + Output: []interface{}{ + map[string]interface{}{ + "name": "hello", + "rules": hil.UnknownValue, + }, + }, + }, } for _, tc := range cases { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 0a3b7721f..f335c8aea 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -861,6 +861,65 @@ func TestSchemaMap_Diff(t *testing.T) { Err: false, }, + { + Name: "List with computed set", + Schema: map[string]*Schema{ + "config": &Schema{ + Type: TypeList, + Optional: true, + ForceNew: true, + MinItems: 1, + Elem: &Resource{ + Schema: map[string]*Schema{ + "name": { + Type: TypeString, + Required: true, + }, + + "rules": { + Type: TypeSet, + Computed: true, + Elem: &Schema{Type: TypeString}, + Set: HashString, + }, + }, + }, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "config": []interface{}{ + map[string]interface{}{ + "name": "hello", + }, + }, + }, + + Diff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "config.#": &terraform.ResourceAttrDiff{ + Old: "0", + New: "1", + RequiresNew: true, + }, + + "config.0.name": &terraform.ResourceAttrDiff{ + Old: "", + New: "hello", + }, + + "config.0.rules.#": &terraform.ResourceAttrDiff{ + Old: "", + NewComputed: true, + }, + }, + }, + + Err: false, + }, + { Name: "Set", Schema: map[string]*Schema{ diff --git a/terraform/state_test.go b/terraform/state_test.go index 2d57dafbb..694c9f751 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -1456,6 +1456,45 @@ func TestInstanceState_MergeDiffRemoveCounts(t *testing.T) { } } +// GH-12183. This tests that a list with a computed set generates the +// right partial state. This never failed but is put here for completion +// of the test case for GH-12183. +func TestInstanceState_MergeDiff_computedSet(t *testing.T) { + is := InstanceState{} + + diff := &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "config.#": &ResourceAttrDiff{ + Old: "0", + New: "1", + RequiresNew: true, + }, + + "config.0.name": &ResourceAttrDiff{ + Old: "", + New: "hello", + }, + + "config.0.rules.#": &ResourceAttrDiff{ + Old: "", + NewComputed: true, + }, + }, + } + + is2 := is.MergeDiff(diff) + + expected := map[string]string{ + "config.#": "1", + "config.0.name": "hello", + "config.0.rules.#": config.UnknownVariableValue, + } + + if !reflect.DeepEqual(expected, is2.Attributes) { + t.Fatalf("bad: %#v", is2.Attributes) + } +} + func TestInstanceState_MergeDiff_nil(t *testing.T) { var is *InstanceState