From 89c1ba099f9d3a7bb1c15e199f85395ed7a0f9f6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Feb 2019 17:07:56 -0500 Subject: [PATCH 1/8] add computed set test with CustomizeDiff --- builtin/providers/test/provider.go | 1 + .../providers/test/resource_computed_set.go | 74 +++++++++++++++++++ .../test/resource_computed_set_test.go | 52 +++++++++++++ 3 files changed, 127 insertions(+) create mode 100644 builtin/providers/test/resource_computed_set.go create mode 100644 builtin/providers/test/resource_computed_set_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index 73d347d71..f2c9d376d 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -31,6 +31,7 @@ func Provider() terraform.ResourceProvider { "test_resource_defaults": testResourceDefaults(), "test_resource_list": testResourceList(), "test_resource_map": testResourceMap(), + "test_resource_computed_set": testResourceComputedSet(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_computed_set.go b/builtin/providers/test/resource_computed_set.go new file mode 100644 index 000000000..5adf1bd99 --- /dev/null +++ b/builtin/providers/test/resource_computed_set.go @@ -0,0 +1,74 @@ +package test + +import ( + "fmt" + "math/rand" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceComputedSet() *schema.Resource { + return &schema.Resource{ + Create: testResourceComputedSetCreate, + Read: testResourceComputedSetRead, + Delete: testResourceComputedSetDelete, + Update: testResourceComputedSetUpdate, + + CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error { + o, n := d.GetChange("set_count") + if o != n { + d.SetNewComputed("string_set") + } + return nil + }, + + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "set_count": { + Type: schema.TypeInt, + Optional: true, + }, + "string_set": { + Type: schema.TypeSet, + Computed: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + Set: schema.HashString, + }, + }, + } +} + +func testResourceComputedSetCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId(fmt.Sprintf("%x", rand.Int63())) + return testResourceComputedSetRead(d, meta) +} + +func testResourceComputedSetRead(d *schema.ResourceData, meta interface{}) error { + count := 3 + v, ok := d.GetOk("set_count") + if ok { + count = v.(int) + } + + var set []interface{} + for i := 0; i < count; i++ { + set = append(set, fmt.Sprintf("%d", i)) + } + + d.Set("string_set", schema.NewSet(schema.HashString, set)) + return nil +} + +func testResourceComputedSetUpdate(d *schema.ResourceData, meta interface{}) error { + return testResourceComputedSetRead(d, meta) +} + +func testResourceComputedSetDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_computed_set_test.go b/builtin/providers/test/resource_computed_set_test.go new file mode 100644 index 000000000..806795c1d --- /dev/null +++ b/builtin/providers/test/resource_computed_set_test.go @@ -0,0 +1,52 @@ +package test + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestResourceComputedSet_update(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_computed_set" "foo" { +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_computed_set.foo", "string_set.#", "3", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_computed_set" "foo" { + set_count = 5 +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_computed_set.foo", "string_set.#", "5", + ), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_computed_set" "foo" { + set_count = 2 +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_computed_set.foo", "string_set.#", "2", + ), + ), + }, + }, + }) +} From 4a603011c540f54a2feb6b828f75dc6b5cd4f384 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Feb 2019 17:09:04 -0500 Subject: [PATCH 2/8] don't normalizeNullValues in ReadResource The required normalization now happens in PlanResourceChange, and this function is no longer appropriate for ReadResource. --- helper/plugin/grpc_provider.go | 1 - 1 file changed, 1 deletion(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index da048489c..ad1a95d73 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -454,7 +454,6 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso } newStateVal = copyTimeoutValues(newStateVal, stateVal) - newStateVal = normalizeNullValues(newStateVal, stateVal, false) newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType()) if err != nil { From 2e2374cfcb11a33f65b424db105614ea7ee5405c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Feb 2019 21:34:12 -0500 Subject: [PATCH 3/8] add failing test for set elements with custom diff Adding a DiffSuppressFunc for set elements can cause them to be missed in the set diff entirely. --- builtin/providers/test/provider.go | 1 + builtin/providers/test/resource_list_set.go | 73 +++++++++++++++++++ .../providers/test/resource_list_set_test.go | 55 ++++++++++++++ 3 files changed, 129 insertions(+) create mode 100644 builtin/providers/test/resource_list_set.go create mode 100644 builtin/providers/test/resource_list_set_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index f2c9d376d..5f7f8c787 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -30,6 +30,7 @@ func Provider() terraform.ResourceProvider { "test_resource_deprecated": testResourceDeprecated(), "test_resource_defaults": testResourceDefaults(), "test_resource_list": testResourceList(), + "test_resource_list_set": testResourceListSet(), "test_resource_map": testResourceMap(), "test_resource_computed_set": testResourceComputedSet(), }, diff --git a/builtin/providers/test/resource_list_set.go b/builtin/providers/test/resource_list_set.go new file mode 100644 index 000000000..6e8390364 --- /dev/null +++ b/builtin/providers/test/resource_list_set.go @@ -0,0 +1,73 @@ +package test + +import ( + "fmt" + "math/rand" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceListSet() *schema.Resource { + return &schema.Resource{ + Create: testResourceListSetCreate, + Read: testResourceListSetRead, + Delete: testResourceListSetDelete, + Update: testResourceListSetUpdate, + + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "list": { + Type: schema.TypeList, + Optional: true, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "set": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "elem": { + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: func(_, o, n string, _ *schema.ResourceData) bool { + return o == n + }, + }, + }, + }, + Set: func(v interface{}) int { + raw := v.(map[string]interface{}) + if el, ok := raw["elem"]; ok { + return schema.HashString(el) + } + return 42 + }, + }, + }, + }, + }, + }, + } +} + +func testResourceListSetCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId(fmt.Sprintf("%x", rand.Int63())) + return testResourceListSetRead(d, meta) +} + +func testResourceListSetUpdate(d *schema.ResourceData, meta interface{}) error { + return testResourceListSetRead(d, meta) +} + +func testResourceListSetRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceListSetDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_list_set_test.go b/builtin/providers/test/resource_list_set_test.go new file mode 100644 index 000000000..11cb742e8 --- /dev/null +++ b/builtin/providers/test/resource_list_set_test.go @@ -0,0 +1,55 @@ +package test + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestResourceListSet_basic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list_set" "foo" { + list { + set { + elem = "A" + } + set { + elem = "B" + } + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.1255198513.elem", "B"), + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.3554254475.elem", "A"), + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.#", "2"), + ), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list_set" "foo" { + list { + set { + elem = "B" + } + set { + elem = "C" + } + } +} + `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.1255198513.elem", "B"), + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.1037565863.elem", "C"), + resource.TestCheckResourceAttr("test_resource_list_set.foo", "list.0.set.#", "2"), + ), + }, + }, + }) +} From 81a4e705b1ab01573edd39b367b600974b397117 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 2 Feb 2019 08:41:14 -0500 Subject: [PATCH 4/8] DiffSuppressFunc should noop diffs in sets Sets rely on diffs being complete for all elements, even when they are unchanged. When encountering a DiffSuppressFunc inside a set the diffs were being dropped entirely, possible causing set elements to be lost. --- helper/schema/schema.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 77c50de2b..ac1c1eb42 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -799,10 +799,19 @@ func (m schemaMap) diff( for attrK, attrV := range unsupressedDiff.Attributes { switch rd := d.(type) { case *ResourceData: - if schema.DiffSuppressFunc != nil && - attrV != nil && + if schema.DiffSuppressFunc != nil && attrV != nil && schema.DiffSuppressFunc(attrK, attrV.Old, attrV.New, rd) { - continue + // If this attr diff is suppressed, we may still need it in the + // overall diff if it's contained within a set. Rather than + // dropping the diff, make it a NOOP. + if !all { + continue + } + + attrV = &terraform.ResourceAttrDiff{ + Old: attrV.Old, + New: attrV.Old, + } } } diff.Attributes[attrK] = attrV From 55b43077673de272f6aeae99afee1c2dbf5d8050 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Feb 2019 19:01:28 -0500 Subject: [PATCH 5/8] add proto5 feature flag Add feature flag to allow special proto 5 behavior in helper/schema. This is Meant to be used as a last resort for shim-related bugs. --- helper/schema/schema.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index ac1c1eb42..31b236072 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -19,6 +19,7 @@ import ( "sort" "strconv" "strings" + "sync" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/terraform" @@ -32,6 +33,27 @@ const PanicOnErr = "TF_SCHEMA_PANIC_ON_ERROR" // type used for schema package context keys type contextKey string +var ( + protoVersionMu sync.Mutex + protoVersion5 = false +) + +func isProto5() bool { + protoVersionMu.Lock() + defer protoVersionMu.Unlock() + return protoVersion5 + +} + +// SetProto5 enables a feature flag for any internal changes required required +// to work with the new plugin protocol. This should not be called by +// provider. +func SetProto5() { + protoVersionMu.Lock() + defer protoVersionMu.Unlock() + protoVersion5 = true +} + // Schema is used to describe the structure of a value. // // Read the documentation of the struct elements for important details. From 58c9c2311a24ef30082ca922337a3f51aa78eea5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Feb 2019 19:12:20 -0500 Subject: [PATCH 6/8] Turn on helper/schema proto5 flag in GetSchema This turns it on at the last moment, and in one place for all uses of helper/schema. There's no way to use the new protocol without calling GetSchema, so we can be sure that any subsequent api calls have this set when required. --- helper/plugin/grpc_provider.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index ad1a95d73..68c3b3316 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -44,6 +44,10 @@ type GRPCProviderServer struct { } func (s *GRPCProviderServer) GetSchema(_ context.Context, req *proto.GetProviderSchema_Request) (*proto.GetProviderSchema_Response, error) { + // Here we are certain that the provider is being called through grpc, so + // make sure the feature flag for helper/schema is set + schema.SetProto5() + resp := &proto.GetProviderSchema_Response{ ResourceSchemas: make(map[string]*proto.Schema), DataSourceSchemas: make(map[string]*proto.Schema), From 79d1e0d7cfa037d93aca1af442e9d3eaf0d4be2a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Feb 2019 19:15:15 -0500 Subject: [PATCH 7/8] add failing test for multiple computed set elems --- .../test/resource_nested_set_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/builtin/providers/test/resource_nested_set_test.go b/builtin/providers/test/resource_nested_set_test.go index 744e4c708..e47f1ac58 100644 --- a/builtin/providers/test/resource_nested_set_test.go +++ b/builtin/providers/test/resource_nested_set_test.go @@ -496,3 +496,34 @@ resource "test_resource_nested_set" "foo" { }, }) } + +func TestResourceNestedSet_multipleUnknownSetElements(t *testing.T) { + checkFunc := func(s *terraform.State) error { + return nil + } + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_nested_set" "a" { +} + +resource "test_resource_nested_set" "b" { +} + +resource "test_resource_nested_set" "c" { + multi { + optional = test_resource_nested_set.a.id + } + multi { + optional = test_resource_nested_set.b.id + } +} + `), + Check: checkFunc, + }, + }, + }) +} From 8be864c1c7aabf97d94f81e5588555f6ff49e2da Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 4 Feb 2019 19:15:43 -0500 Subject: [PATCH 8/8] don't allow computed set elems to be equal If set elements are computed, we can't be certain that they are actually equal. Catch identical computed set hashes when they are added to the set, and alter the set key slightly to keep the set counts correct. In previous versions the interpolation string would be included in the set, and different string values would cause the set to hash differently, so this is change is only activated for the new protocol. --- helper/schema/set.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/helper/schema/set.go b/helper/schema/set.go index cba289035..8ee89e475 100644 --- a/helper/schema/set.go +++ b/helper/schema/set.go @@ -198,6 +198,16 @@ func (s *Set) add(item interface{}, computed bool) string { code := s.hash(item) if computed { code = "~" + code + + if isProto5() { + tmpCode := code + count := 0 + for _, exists := s.m[tmpCode]; exists; _, exists = s.m[tmpCode] { + count++ + tmpCode = fmt.Sprintf("%s%d", code, count) + } + code = tmpCode + } } if _, ok := s.m[code]; !ok {